Changeset View
Standalone View
sys/kern/kern_cpu.c
Show First 20 Lines • Show All 70 Lines • ▼ Show 20 Lines | struct cpufreq_softc { | ||||
struct sx lock; | struct sx lock; | ||||
struct cf_level curr_level; | struct cf_level curr_level; | ||||
int curr_priority; | int curr_priority; | ||||
SLIST_HEAD(, cf_saved_freq) saved_freq; | SLIST_HEAD(, cf_saved_freq) saved_freq; | ||||
struct cf_level_lst all_levels; | struct cf_level_lst all_levels; | ||||
int all_count; | int all_count; | ||||
int max_mhz; | int max_mhz; | ||||
device_t dev; | device_t dev; | ||||
device_t cf_drv_dev; | |||||
struct sysctl_ctx_list sysctl_ctx; | struct sysctl_ctx_list sysctl_ctx; | ||||
struct task startup_task; | struct task startup_task; | ||||
struct cf_level *levels_buf; | struct cf_level *levels_buf; | ||||
}; | }; | ||||
struct cf_setting_array { | struct cf_setting_array { | ||||
struct cf_setting sets[MAX_SETTINGS]; | struct cf_setting sets[MAX_SETTINGS]; | ||||
int count; | int count; | ||||
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines | |||||
static int cf_verbose; | static int cf_verbose; | ||||
static SYSCTL_NODE(_debug, OID_AUTO, cpufreq, CTLFLAG_RD, NULL, | static SYSCTL_NODE(_debug, OID_AUTO, cpufreq, CTLFLAG_RD, NULL, | ||||
"cpufreq debugging"); | "cpufreq debugging"); | ||||
SYSCTL_INT(_debug_cpufreq, OID_AUTO, lowest, CTLFLAG_RWTUN, &cf_lowest_freq, 1, | SYSCTL_INT(_debug_cpufreq, OID_AUTO, lowest, CTLFLAG_RWTUN, &cf_lowest_freq, 1, | ||||
"Don't provide levels below this frequency."); | "Don't provide levels below this frequency."); | ||||
SYSCTL_INT(_debug_cpufreq, OID_AUTO, verbose, CTLFLAG_RWTUN, &cf_verbose, 1, | SYSCTL_INT(_debug_cpufreq, OID_AUTO, verbose, CTLFLAG_RWTUN, &cf_verbose, 1, | ||||
"Print verbose debugging messages"); | "Print verbose debugging messages"); | ||||
/* | |||||
* This is called as the result of a hardware specific frequency control driver | |||||
* calling cpufreq_register. It provides a general interface for system wide | |||||
* frequency controls and operates on a per cpu basis. | |||||
cem: basic -> basis, I think | |||||
*/ | |||||
static int | static int | ||||
cpufreq_attach(device_t dev) | cpufreq_attach(device_t dev) | ||||
Not Done Inline ActionsThe 'dev' -> 'cf_dev' rename adds a lot of noise (and is a bit unusual in that almost all (if not actually all) foo_attach, etc. routines in the tree use 'dev' as the variable name. It is minor, but I probably would have left it as 'dev'. jhb: The 'dev' -> 'cf_dev' rename adds a lot of noise (and is a bit unusual in that almost all (if… | |||||
{ | { | ||||
struct cpufreq_softc *sc; | struct cpufreq_softc *sc; | ||||
struct pcpu *pc; | struct pcpu *pc; | ||||
device_t parent; | device_t parent; | ||||
uint64_t rate; | uint64_t rate; | ||||
int numdevs; | |||||
CF_DEBUG("initializing %s\n", device_get_nameunit(dev)); | CF_DEBUG("initializing %s\n", device_get_nameunit(dev)); | ||||
sc = device_get_softc(dev); | sc = device_get_softc(dev); | ||||
parent = device_get_parent(dev); | parent = device_get_parent(dev); | ||||
sc->dev = dev; | sc->dev = dev; | ||||
sysctl_ctx_init(&sc->sysctl_ctx); | sysctl_ctx_init(&sc->sysctl_ctx); | ||||
TAILQ_INIT(&sc->all_levels); | TAILQ_INIT(&sc->all_levels); | ||||
CF_MTX_INIT(&sc->lock); | CF_MTX_INIT(&sc->lock); | ||||
sc->curr_level.total_set.freq = CPUFREQ_VAL_UNKNOWN; | sc->curr_level.total_set.freq = CPUFREQ_VAL_UNKNOWN; | ||||
SLIST_INIT(&sc->saved_freq); | SLIST_INIT(&sc->saved_freq); | ||||
/* Try to get nominal CPU freq to use it as maximum later if needed */ | /* Try to get nominal CPU freq to use it as maximum later if needed */ | ||||
sc->max_mhz = cpu_get_nominal_mhz(dev); | sc->max_mhz = cpu_get_nominal_mhz(dev); | ||||
/* If that fails, try to measure the current rate */ | /* If that fails, try to measure the current rate */ | ||||
if (sc->max_mhz <= 0) { | if (sc->max_mhz <= 0) { | ||||
CF_DEBUG("Unable to obtain nominal frequency.\n"); | |||||
pc = cpu_get_pcpu(dev); | pc = cpu_get_pcpu(dev); | ||||
if (cpu_est_clockrate(pc->pc_cpuid, &rate) == 0) | if (cpu_est_clockrate(pc->pc_cpuid, &rate) == 0) | ||||
sc->max_mhz = rate / 1000000; | sc->max_mhz = rate / 1000000; | ||||
else | else | ||||
sc->max_mhz = CPUFREQ_VAL_UNKNOWN; | sc->max_mhz = CPUFREQ_VAL_UNKNOWN; | ||||
} | } | ||||
/* | |||||
* Only initialize one set of sysctls for all CPUs. In the future, | |||||
* if multiple CPUs can have different settings, we can move these | |||||
* sysctls to be under every CPU instead of just the first one. | |||||
*/ | |||||
numdevs = devclass_get_count(cpufreq_dc); | |||||
if (numdevs > 1) | |||||
return (0); | |||||
CF_DEBUG("initializing one-time data for %s\n", | CF_DEBUG("initializing one-time data for %s\n", | ||||
device_get_nameunit(dev)); | device_get_nameunit(dev)); | ||||
sc->levels_buf = malloc(CF_MAX_LEVELS * sizeof(*sc->levels_buf), | sc->levels_buf = malloc(CF_MAX_LEVELS * sizeof(*sc->levels_buf), | ||||
M_DEVBUF, M_WAITOK); | M_DEVBUF, M_WAITOK); | ||||
SYSCTL_ADD_PROC(&sc->sysctl_ctx, | SYSCTL_ADD_PROC(&sc->sysctl_ctx, | ||||
SYSCTL_CHILDREN(device_get_sysctl_tree(parent)), | SYSCTL_CHILDREN(device_get_sysctl_tree(parent)), | ||||
OID_AUTO, "freq", CTLTYPE_INT | CTLFLAG_RW, sc, 0, | OID_AUTO, "freq", CTLTYPE_INT | CTLFLAG_RW, sc, 0, | ||||
cpufreq_curr_sysctl, "I", "Current CPU frequency"); | cpufreq_curr_sysctl, "I", "Current CPU frequency"); | ||||
Show All 20 Lines | cpufreq_startup_task(void *ctx, int pending) | ||||
cpufreq_settings_changed((device_t)ctx); | cpufreq_settings_changed((device_t)ctx); | ||||
} | } | ||||
static int | static int | ||||
cpufreq_detach(device_t dev) | cpufreq_detach(device_t dev) | ||||
{ | { | ||||
struct cpufreq_softc *sc; | struct cpufreq_softc *sc; | ||||
struct cf_saved_freq *saved_freq; | struct cf_saved_freq *saved_freq; | ||||
int numdevs; | |||||
CF_DEBUG("shutdown %s\n", device_get_nameunit(dev)); | CF_DEBUG("shutdown %s\n", device_get_nameunit(dev)); | ||||
sc = device_get_softc(dev); | sc = device_get_softc(dev); | ||||
sysctl_ctx_free(&sc->sysctl_ctx); | sysctl_ctx_free(&sc->sysctl_ctx); | ||||
while ((saved_freq = SLIST_FIRST(&sc->saved_freq)) != NULL) { | while ((saved_freq = SLIST_FIRST(&sc->saved_freq)) != NULL) { | ||||
SLIST_REMOVE_HEAD(&sc->saved_freq, link); | SLIST_REMOVE_HEAD(&sc->saved_freq, link); | ||||
free(saved_freq, M_TEMP); | free(saved_freq, M_TEMP); | ||||
} | } | ||||
/* Only clean up these resources when the last device is detaching. */ | |||||
numdevs = devclass_get_count(cpufreq_dc); | |||||
if (numdevs == 1) { | |||||
CF_DEBUG("final shutdown for %s\n", device_get_nameunit(dev)); | |||||
free(sc->levels_buf, M_DEVBUF); | free(sc->levels_buf, M_DEVBUF); | ||||
} | |||||
return (0); | return (0); | ||||
} | } | ||||
static int | static int | ||||
cf_set_method(device_t dev, const struct cf_level *level, int priority) | cf_set_method(device_t dev, const struct cf_level *level, int priority) | ||||
{ | { | ||||
struct cpufreq_softc *sc; | struct cpufreq_softc *sc; | ||||
▲ Show 20 Lines • Show All 173 Lines • ▼ Show 20 Lines | out: | ||||
EVENTHANDLER_INVOKE(cpufreq_post_change, level, error); | EVENTHANDLER_INVOKE(cpufreq_post_change, level, error); | ||||
if (error && set) | if (error && set) | ||||
device_printf(set->dev, "set freq failed, err %d\n", error); | device_printf(set->dev, "set freq failed, err %d\n", error); | ||||
return (error); | return (error); | ||||
} | } | ||||
static int | static int | ||||
cpufreq_get_frequency(device_t dev) | |||||
Done Inline ActionsPlease do not use implementation-defined namespace without a reason (I do not see one). kib: Please do not use implementation-defined namespace without a reason (I do not see one). | |||||
Done Inline ActionsI don't mind changing it, but it's static, so I'm not sure what you mean about adding implementation-defined namespace. bwidawsk: I don't mind changing it, but it's static, so I'm not sure what you mean about adding… | |||||
Done Inline ActionsThe implementation-defined part is the __ prefix in the function name. cem: The implementation-defined part is the `__` prefix in the function name. | |||||
{ | |||||
struct cf_setting set; | |||||
if (CPUFREQ_DRV_GET(dev, &set) != 0) | |||||
return (-1); | |||||
Done Inline Actionsreturn (-1); kib: return (-1); | |||||
return (set.freq); | |||||
Done Inline Actionsreturn (set.freq); kib: return (set.freq); | |||||
} | |||||
/* Returns the index into *levels with the match */ | |||||
static int | |||||
cpufreq_get_level(device_t dev, struct cf_level *levels, int count) | |||||
Done Inline ActionsAnd there too. kib: And there too. | |||||
{ | |||||
int i, freq; | |||||
if ((freq = cpufreq_get_frequency(dev)) < 0) | |||||
return (-1); | |||||
Done Inline ActionsIs this case possible? It looks like this is only called with freq_dev, and I don't see a case where freq_dev isn't attached before it is initialized. But I may be mistaken. cem: Is this case possible? It looks like this is only called with `freq_dev`, and I don't see a… | |||||
for (i = 0; i < count; i++) | |||||
if (freq == levels[i].total_set.freq) | |||||
return (i); | |||||
return (-1); | |||||
Done Inline ActionsI don't see where set is initialized. Probably this is the result of refactoring CPUFREQ_DRV_GET into __get_frequency and you meant to save the result of __get_frequency and use it for this comparison? cem: I don't see where `set` is initialized. Probably this is the result of refactoring… | |||||
} | |||||
/* | |||||
Done Inline ActionsAre error values from this function used for anything ? If not, why not return -1 ? kib: Are error values from this function used for anything ? If not, why not return -1 ? | |||||
Done Inline ActionsMostly it was for debug to be able to differentiate error cases. I suppose someone debugging can always change it though, and I'll do -1 bwidawsk: Mostly it was for debug to be able to differentiate error cases. I suppose someone debugging… | |||||
Done Inline ActionsIt's mostly uncommon in FreeBSD to return negative error numbers and positive success values — that's more of a Linux (and imported Linux code) thing. Typically we'd return 0 or EFOO and set an output parameter for the level index. (Or sometimes, return the level index, or -1 as an out-of-range sentinel value.) (Also, the spelling would be return (-ENOENT); the idea being that return() could be defined as a macro.) cem: It's mostly uncommon in FreeBSD to return negative error numbers and positive success values —… | |||||
* Used by the cpufreq core, this function will populate *level with the current | |||||
* frequency as either determined by a cached value sc->curr_level, or in the | |||||
* case the lower level driver has set the CPUFREQ_FLAG_UNCACHED flag, it will | |||||
* obtain the frequency from the driver itself. | |||||
*/ | |||||
static int | |||||
cf_get_method(device_t dev, struct cf_level *level) | cf_get_method(device_t dev, struct cf_level *level) | ||||
Done Inline ActionsI appreciate adding comments documenting these routines, by the way. Thank you. cem: I appreciate adding comments documenting these routines, by the way. Thank you. | |||||
Not Done Inline Actions'dev' vs 'cf_drv_dev' would seem more consistent with existing code, though I see that this is always 'sc->cf_drv_dev' in the caller, so probably ok as-is. jhb: 'dev' vs 'cf_drv_dev' would seem more consistent with existing code, though I see that this is… | |||||
{ | { | ||||
struct cpufreq_softc *sc; | struct cpufreq_softc *sc; | ||||
struct cf_level *levels; | struct cf_level *levels; | ||||
struct cf_setting *curr_set, set; | struct cf_setting *curr_set; | ||||
struct pcpu *pc; | struct pcpu *pc; | ||||
device_t *devs; | int bdiff, count, diff, error, i, type; | ||||
int bdiff, count, diff, error, i, n, numdevs; | |||||
uint64_t rate; | uint64_t rate; | ||||
sc = device_get_softc(dev); | sc = device_get_softc(dev); | ||||
error = 0; | error = 0; | ||||
levels = NULL; | levels = NULL; | ||||
/* If we already know the current frequency, we're done. */ | /* | ||||
* If we already know the current frequency, and the driver didn't ask | |||||
* for uncached usage, we're done. | |||||
*/ | |||||
CF_MTX_LOCK(&sc->lock); | CF_MTX_LOCK(&sc->lock); | ||||
curr_set = &sc->curr_level.total_set; | curr_set = &sc->curr_level.total_set; | ||||
if (curr_set->freq != CPUFREQ_VAL_UNKNOWN) { | error = CPUFREQ_DRV_TYPE(sc->cf_drv_dev, &type); | ||||
if (error == 0 && (type & CPUFREQ_FLAG_UNCACHED)) { | |||||
struct cf_setting set; | |||||
/* | |||||
* If the driver wants to always report back the real frequency, | |||||
* first try the driver and if that fails, fall back to | |||||
* estimating. | |||||
Not Done Inline ActionsWould this ever be the case for Intel HWP, or is this just allowing the possibility in general? cem: Would this ever be the case for Intel HWP, or is this just allowing the possibility in general? | |||||
*/ | |||||
if (CPUFREQ_DRV_GET(sc->cf_drv_dev, &set) != 0) | |||||
goto estimate; | |||||
sc->curr_level.total_set = set; | |||||
CF_DEBUG("get returning immediate freq %d\n", curr_set->freq); | |||||
goto out; | |||||
} else if (curr_set->freq != CPUFREQ_VAL_UNKNOWN) { | |||||
CF_DEBUG("get returning known freq %d\n", curr_set->freq); | CF_DEBUG("get returning known freq %d\n", curr_set->freq); | ||||
error = 0; | |||||
Done Inline ActionsHm, the drawback of this approach seems to be we're always invoking CPUFREQ_DRV_TYPE() even for all drivers prior to this change that allow cached frequencies for a given level. Maybe I'm imagining things and that is a negligible cost? It seems like we could make an invariant that CPUFREQ_FLAG_UNCACHED => (curr_set->freq == CPUFREQ_VAL_UNKNOWN). Then we would only need to check for != VAL_UNKNOWN to bail early here (like the condition prior to this change). We would then only invoke DRV_TYPE() if the value was UNKNOWN, and only save freq if !UNCACHED. Again, I might just be overcomplicating this for no reason; feel free to tell me I'm wrong. cem: Hm, the drawback of this approach seems to be we're always invoking `CPUFREQ_DRV_TYPE()` even… | |||||
Done Inline ActionsI'm going to punt on this for now... TYPE should be a pretty cheap operation and I'm risk averse :-) bwidawsk: I'm going to punt on this for now... TYPE should be a pretty cheap operation and I'm risk… | |||||
Not Done Inline Actions+1 cem: +1 | |||||
Not Done Inline Actionserror = 0; cem: `error = 0;` | |||||
goto out; | goto out; | ||||
} | } | ||||
CF_MTX_UNLOCK(&sc->lock); | CF_MTX_UNLOCK(&sc->lock); | ||||
Done Inline Actionsstyle(9) nitpick: /* * foo (Multi-line comment blocks begin with a blank line. One thing to keep in mind with the style(9) manual page is: "Many of the style rules are implicit in the examples.") cem: style(9) nitpick:
```
/*
* foo
```
(Multi-line comment blocks begin with a blank line. One… | |||||
Not Done Inline ActionsThis seems a little gratuitous given it's just the De Morgan's transformation of the if error == 0 && .... UNCACHED expression just 19 lines earlier, and every path under that condition has an explicit goto past this condition. cem: This seems a little gratuitous given it's just the De Morgan's transformation of the `if error… | |||||
/* | /* | ||||
* We need to figure out the current level. Loop through every | * We need to figure out the current level. Loop through every | ||||
* driver, getting the current setting. Then, attempt to get a best | * driver, getting the current setting. Then, attempt to get a best | ||||
* match of settings against each level. | * match of settings against each level. | ||||
*/ | */ | ||||
count = CF_MAX_LEVELS; | count = CF_MAX_LEVELS; | ||||
levels = malloc(count * sizeof(*levels), M_TEMP, M_NOWAIT); | levels = malloc(count * sizeof(*levels), M_TEMP, M_NOWAIT); | ||||
if (levels == NULL) | if (levels == NULL) | ||||
return (ENOMEM); | return (ENOMEM); | ||||
error = CPUFREQ_LEVELS(sc->dev, levels, &count); | error = CPUFREQ_LEVELS(sc->dev, levels, &count); | ||||
if (error) { | if (error) { | ||||
if (error == E2BIG) | if (error == E2BIG) | ||||
printf("cpufreq: need to increase CF_MAX_LEVELS\n"); | printf("cpufreq: need to increase CF_MAX_LEVELS\n"); | ||||
free(levels, M_TEMP); | free(levels, M_TEMP); | ||||
return (error); | return (error); | ||||
} | } | ||||
error = device_get_children(device_get_parent(dev), &devs, &numdevs); | |||||
if (error) { | |||||
free(levels, M_TEMP); | |||||
return (error); | |||||
} | |||||
/* | /* | ||||
* Reacquire the lock and search for the given level. | * Reacquire the lock and search for the given level. | ||||
* | * | ||||
* XXX Note: this is not quite right since we really need to go | * XXX Note: this is not quite right since we really need to go | ||||
* through each level and compare both absolute and relative | * through each level and compare both absolute and relative | ||||
* settings for each driver in the system before making a match. | * settings for each driver in the system before making a match. | ||||
* The estimation code below catches this case though. | * The estimation code below catches this case though. | ||||
*/ | */ | ||||
CF_MTX_LOCK(&sc->lock); | CF_MTX_LOCK(&sc->lock); | ||||
for (n = 0; n < numdevs && curr_set->freq == CPUFREQ_VAL_UNKNOWN; n++) { | i = cpufreq_get_level(sc->cf_drv_dev, levels, count); | ||||
if (!device_is_attached(devs[n])) | if (i >= 0) | ||||
continue; | |||||
if (CPUFREQ_DRV_GET(devs[n], &set) != 0) | |||||
continue; | |||||
for (i = 0; i < count; i++) { | |||||
if (set.freq == levels[i].total_set.freq) { | |||||
sc->curr_level = levels[i]; | sc->curr_level = levels[i]; | ||||
break; | else | ||||
} | CF_DEBUG("Couldn't find supported level for %s\n", | ||||
} | device_get_nameunit(sc->cf_drv_dev)); | ||||
} | |||||
free(devs, M_TEMP); | |||||
if (curr_set->freq != CPUFREQ_VAL_UNKNOWN) { | if (curr_set->freq != CPUFREQ_VAL_UNKNOWN) { | ||||
CF_DEBUG("get matched freq %d from drivers\n", curr_set->freq); | CF_DEBUG("get matched freq %d from drivers\n", curr_set->freq); | ||||
goto out; | goto out; | ||||
} | } | ||||
Done Inline ActionsThe curr_set pointer into sc->curr_level is still valid, but it is a little confusing to me that the sc->curr_level assignment above is what affects this condition. I would probably replace curr_set->freq here with sc->curr_level.total_set.freq myself. Obviously, this isn't introduced in this change, and maybe it is clear to others. Feel free to ignore. cem: The `curr_set` pointer into `sc->curr_level` is still valid, but it is a little confusing to me… | |||||
estimate: | |||||
Done Inline ActionsMaybe assert the CF_MTX lock here for reader clarity. I only suggest this because the goto estimate above jumps over a region that unlocks and relocks this lock, which is a little error prone for future refactoring. cem: Maybe assert the CF_MTX lock here for reader clarity. I only suggest this because the `goto… | |||||
CF_MTX_ASSERT(&sc->lock); | |||||
/* | /* | ||||
* We couldn't find an exact match, so attempt to estimate and then | * We couldn't find an exact match, so attempt to estimate and then | ||||
* match against a level. | * match against a level. | ||||
*/ | */ | ||||
pc = cpu_get_pcpu(dev); | pc = cpu_get_pcpu(dev); | ||||
if (pc == NULL) { | if (pc == NULL) { | ||||
error = ENXIO; | error = ENXIO; | ||||
goto out; | goto out; | ||||
Show All 15 Lines | if (error == 0) | ||||
*level = sc->curr_level; | *level = sc->curr_level; | ||||
CF_MTX_UNLOCK(&sc->lock); | CF_MTX_UNLOCK(&sc->lock); | ||||
if (levels) | if (levels) | ||||
free(levels, M_TEMP); | free(levels, M_TEMP); | ||||
return (error); | return (error); | ||||
} | } | ||||
/* | |||||
* Either directly obtain settings from the cpufreq driver, or build a list of | |||||
* relative settings to be integrated later against an absolute max. | |||||
Done Inline ActionsThe grammar here is a bit funky cem: The grammar here is a bit funky | |||||
*/ | |||||
static int | static int | ||||
cf_levels_method(device_t dev, struct cf_level *levels, int *count) | cpufreq_add_levels(device_t cf_dev, struct cf_setting_lst *rel_sets) | ||||
{ | { | ||||
struct cf_setting_array *set_arr; | struct cf_setting_array *set_arr; | ||||
struct cf_setting_lst rel_sets; | |||||
struct cpufreq_softc *sc; | |||||
struct cf_level *lev; | |||||
struct cf_setting *sets; | struct cf_setting *sets; | ||||
Done Inline ActionsIt's a bit uncommon to use static variables in functions; are we sure it's safe in this context? In particular, we hold the CF_MTX for this softc, but do we hold any global lock preventing a race? (And I guess the flip side — is sizeof(sets) large enough to actually be a concern re: stack use? By my count, cf_setting is 40 bytes on amd64, and MAX_SETTINGS is 256. Yeah, ok, that's too large for the stack.) Is there a reason to avoid malloc here (which was used prior to this refactoring)? cem: It's a bit uncommon to use `static` variables in functions; are we sure it's safe in this… | |||||
Done Inline ActionsIn an earlier revision, the free paths were getting a little messy, and static seemed like an easy solution. Realistically, nothing uses anywhere near 256, but I agree it's problematic. I will rework it to use malloc. bwidawsk: In an earlier revision, the free paths were getting a little messy, and static seemed like an… | |||||
Not Done Inline ActionsThanks! cem: Thanks! | |||||
struct pcpu *pc; | device_t dev; | ||||
device_t *devs; | struct cpufreq_softc *sc; | ||||
int error, i, numdevs, set_count, type; | int type, set_count, error; | ||||
uint64_t rate; | |||||
if (levels == NULL || count == NULL) | sc = device_get_softc(cf_dev); | ||||
return (EINVAL); | dev = sc->cf_drv_dev; | ||||
TAILQ_INIT(&rel_sets); | |||||
sc = device_get_softc(dev); | |||||
error = device_get_children(device_get_parent(dev), &devs, &numdevs); | |||||
if (error) | |||||
return (error); | |||||
sets = malloc(MAX_SETTINGS * sizeof(*sets), M_TEMP, M_NOWAIT); | |||||
if (sets == NULL) { | |||||
free(devs, M_TEMP); | |||||
return (ENOMEM); | |||||
} | |||||
/* Get settings from all cpufreq drivers. */ | |||||
CF_MTX_LOCK(&sc->lock); | |||||
for (i = 0; i < numdevs; i++) { | |||||
/* Skip devices that aren't ready. */ | /* Skip devices that aren't ready. */ | ||||
if (!device_is_attached(devs[i])) | if (!device_is_attached(cf_dev)) | ||||
continue; | return (0); | ||||
/* | /* | ||||
* Get settings, skipping drivers that offer no settings or | * Get settings, skipping drivers that offer no settings or | ||||
* provide settings for informational purposes only. | * provide settings for informational purposes only. | ||||
*/ | */ | ||||
Done Inline Actionsstyle(9) nit: error != 0 (above) cem: style(9) nit: `error != 0` (above) | |||||
error = CPUFREQ_DRV_TYPE(devs[i], &type); | error = CPUFREQ_DRV_TYPE(dev, &type); | ||||
if (error || (type & CPUFREQ_FLAG_INFO_ONLY)) { | if (error != 0 || (type & CPUFREQ_FLAG_INFO_ONLY)) { | ||||
if (error == 0) { | if (error == 0) { | ||||
CF_DEBUG("skipping info-only driver %s\n", | CF_DEBUG("skipping info-only driver %s\n", | ||||
device_get_nameunit(devs[i])); | device_get_nameunit(cf_dev)); | ||||
} | } | ||||
continue; | return (error); | ||||
} | } | ||||
sets = malloc(MAX_SETTINGS * sizeof(*sets), M_TEMP, M_NOWAIT); | |||||
if (sets == NULL) | |||||
return (ENOMEM); | |||||
set_count = MAX_SETTINGS; | set_count = MAX_SETTINGS; | ||||
error = CPUFREQ_DRV_SETTINGS(devs[i], sets, &set_count); | error = CPUFREQ_DRV_SETTINGS(dev, sets, &set_count); | ||||
if (error || set_count == 0) | if (error != 0 || set_count == 0) | ||||
continue; | goto out; | ||||
/* Add the settings to our absolute/relative lists. */ | /* Add the settings to our absolute/relative lists. */ | ||||
switch (type & CPUFREQ_TYPE_MASK) { | switch (type & CPUFREQ_TYPE_MASK) { | ||||
case CPUFREQ_TYPE_ABSOLUTE: | case CPUFREQ_TYPE_ABSOLUTE: | ||||
error = cpufreq_insert_abs(sc, sets, set_count); | error = cpufreq_insert_abs(sc, sets, set_count); | ||||
break; | break; | ||||
case CPUFREQ_TYPE_RELATIVE: | case CPUFREQ_TYPE_RELATIVE: | ||||
CF_DEBUG("adding %d relative settings\n", set_count); | CF_DEBUG("adding %d relative settings\n", set_count); | ||||
set_arr = malloc(sizeof(*set_arr), M_TEMP, M_NOWAIT); | set_arr = malloc(sizeof(*set_arr), M_TEMP, M_NOWAIT); | ||||
if (set_arr == NULL) { | if (set_arr == NULL) { | ||||
error = ENOMEM; | error = ENOMEM; | ||||
goto out; | goto out; | ||||
} | } | ||||
bcopy(sets, set_arr->sets, set_count * sizeof(*sets)); | bcopy(sets, set_arr->sets, set_count * sizeof(*sets)); | ||||
set_arr->count = set_count; | set_arr->count = set_count; | ||||
TAILQ_INSERT_TAIL(&rel_sets, set_arr, link); | TAILQ_INSERT_TAIL(rel_sets, set_arr, link); | ||||
break; | break; | ||||
default: | default: | ||||
error = EINVAL; | error = EINVAL; | ||||
} | } | ||||
out: | |||||
free(sets, M_TEMP); | |||||
return (error); | |||||
} | |||||
static int | |||||
cf_levels_method(device_t dev, struct cf_level *levels, int *count) | |||||
{ | |||||
struct cf_setting_array *set_arr; | |||||
struct cf_setting_lst rel_sets; | |||||
struct cpufreq_softc *sc; | |||||
struct cf_level *lev; | |||||
struct pcpu *pc; | |||||
int error, i; | |||||
uint64_t rate; | |||||
if (levels == NULL || count == NULL) | |||||
return (EINVAL); | |||||
TAILQ_INIT(&rel_sets); | |||||
sc = device_get_softc(dev); | |||||
CF_MTX_LOCK(&sc->lock); | |||||
error = cpufreq_add_levels(sc->dev, &rel_sets); | |||||
if (error) | if (error) | ||||
goto out; | goto out; | ||||
} | |||||
/* | /* | ||||
* If there are no absolute levels, create a fake one at 100%. We | * If there are no absolute levels, create a fake one at 100%. We | ||||
* then cache the clockrate for later use as our base frequency. | * then cache the clockrate for later use as our base frequency. | ||||
Not Done Inline Actionswe invoke DRV_TYPE on cf_drv_dev but device_get_nameunit on cf_dev. Is that intentional? cem: we invoke `DRV_TYPE` on `cf_drv_dev` but `device_get_nameunit` on `cf_dev`. Is that… | |||||
*/ | */ | ||||
if (TAILQ_EMPTY(&sc->all_levels)) { | if (TAILQ_EMPTY(&sc->all_levels)) { | ||||
struct cf_setting set; | |||||
CF_DEBUG("No absolute levels returned by driver\n"); | |||||
if (sc->max_mhz == CPUFREQ_VAL_UNKNOWN) { | if (sc->max_mhz == CPUFREQ_VAL_UNKNOWN) { | ||||
sc->max_mhz = cpu_get_nominal_mhz(dev); | sc->max_mhz = cpu_get_nominal_mhz(dev); | ||||
/* | /* | ||||
* If the CPU can't report a rate for 100%, hope | * If the CPU can't report a rate for 100%, hope | ||||
* the CPU is running at its nominal rate right now, | * the CPU is running at its nominal rate right now, | ||||
Not Done Inline Actionserror != 0 cem: `error != 0` | |||||
Not Done Inline ActionsTo be fair, that is the existing line. For better or for worse a lot of the code in the tree uses 'if (error)' rather than 'if (error != 0)' jhb: To be fair, that is the existing line. For better or for worse a lot of the code in the tree… | |||||
Not Done Inline Actionsmea culpa. Scott, thanks for doing it anyway :) cem: mea culpa. Scott, thanks for doing it anyway :) | |||||
* and use that instead. | * and use that instead. | ||||
*/ | */ | ||||
if (sc->max_mhz <= 0) { | if (sc->max_mhz <= 0) { | ||||
pc = cpu_get_pcpu(dev); | pc = cpu_get_pcpu(dev); | ||||
cpu_est_clockrate(pc->pc_cpuid, &rate); | cpu_est_clockrate(pc->pc_cpuid, &rate); | ||||
sc->max_mhz = rate / 1000000; | sc->max_mhz = rate / 1000000; | ||||
} | } | ||||
} | } | ||||
memset(&sets[0], CPUFREQ_VAL_UNKNOWN, sizeof(*sets)); | memset(&set, CPUFREQ_VAL_UNKNOWN, sizeof(set)); | ||||
sets[0].freq = sc->max_mhz; | set.freq = sc->max_mhz; | ||||
sets[0].dev = NULL; | set.dev = NULL; | ||||
error = cpufreq_insert_abs(sc, sets, 1); | error = cpufreq_insert_abs(sc, &set, 1); | ||||
if (error) | if (error) | ||||
goto out; | goto out; | ||||
} | } | ||||
/* Create a combined list of absolute + relative levels. */ | /* Create a combined list of absolute + relative levels. */ | ||||
TAILQ_FOREACH(set_arr, &rel_sets, link) | TAILQ_FOREACH(set_arr, &rel_sets, link) | ||||
cpufreq_expand_set(sc, set_arr); | cpufreq_expand_set(sc, set_arr); | ||||
Show All 28 Lines | out: | ||||
} | } | ||||
sc->all_count = 0; | sc->all_count = 0; | ||||
CF_MTX_UNLOCK(&sc->lock); | CF_MTX_UNLOCK(&sc->lock); | ||||
while ((set_arr = TAILQ_FIRST(&rel_sets)) != NULL) { | while ((set_arr = TAILQ_FIRST(&rel_sets)) != NULL) { | ||||
TAILQ_REMOVE(&rel_sets, set_arr, link); | TAILQ_REMOVE(&rel_sets, set_arr, link); | ||||
free(set_arr, M_TEMP); | free(set_arr, M_TEMP); | ||||
} | } | ||||
free(devs, M_TEMP); | |||||
free(sets, M_TEMP); | |||||
return (error); | return (error); | ||||
} | } | ||||
/* | /* | ||||
* Create levels for an array of absolute settings and insert them in | * Create levels for an array of absolute settings and insert them in | ||||
* sorted order in the specified list. | * sorted order in the specified list. | ||||
*/ | */ | ||||
static int | static int | ||||
▲ Show 20 Lines • Show All 328 Lines • ▼ Show 20 Lines | cpufreq_settings_sysctl(SYSCTL_HANDLER_ARGS) | ||||
error = sysctl_handle_string(oidp, sbuf_data(&sb), sbuf_len(&sb), req); | error = sysctl_handle_string(oidp, sbuf_data(&sb), sbuf_len(&sb), req); | ||||
out: | out: | ||||
free(sets, M_TEMP); | free(sets, M_TEMP); | ||||
sbuf_delete(&sb); | sbuf_delete(&sb); | ||||
return (error); | return (error); | ||||
} | } | ||||
static void | |||||
cpufreq_add_freq_driver_sysctl(device_t cf_dev) | |||||
{ | |||||
struct cpufreq_softc *sc; | |||||
sc = device_get_softc(cf_dev); | |||||
SYSCTL_ADD_CONST_STRING(&sc->sysctl_ctx, | |||||
SYSCTL_CHILDREN(device_get_sysctl_tree(cf_dev)), OID_AUTO, | |||||
"freq_driver", CTLFLAG_RD, device_get_nameunit(sc->cf_drv_dev), | |||||
Not Done Inline ActionsWould device_get_nameunit() make sense, or is it a singleton? Or is that just not useful information for this sysctl? cem: Would `device_get_nameunit()` make sense, or is it a singleton? Or is that just not useful… | |||||
Done Inline ActionsI don't see any harm in including the unit number, and it might be informative if for some reasons cpus get initialized out of order. then you might notice that cpu2 has hwpstate_intel5 or whatever. scottph: I don't see any harm in including the unit number, and it might be informative if for some… | |||||
"cpufreq driver used by this cpu"); | |||||
Not Done Inline ActionsIn HEAD we just added a SYSCTL_ADD_CONST_STRING that should let you get rid of this cast I think. jhb: In HEAD we just added a SYSCTL_ADD_CONST_STRING that should let you get rid of this cast I… | |||||
} | |||||
int | int | ||||
cpufreq_register(device_t dev) | cpufreq_register(device_t dev) | ||||
{ | { | ||||
struct cpufreq_softc *sc; | struct cpufreq_softc *sc; | ||||
device_t cf_dev, cpu_dev; | device_t cf_dev, cpu_dev; | ||||
int error; | |||||
/* Add a sysctl to get each driver's settings separately. */ | /* Add a sysctl to get each driver's settings separately. */ | ||||
SYSCTL_ADD_PROC(device_get_sysctl_ctx(dev), | SYSCTL_ADD_PROC(device_get_sysctl_ctx(dev), | ||||
SYSCTL_CHILDREN(device_get_sysctl_tree(dev)), | SYSCTL_CHILDREN(device_get_sysctl_tree(dev)), | ||||
OID_AUTO, "freq_settings", CTLTYPE_STRING | CTLFLAG_RD, dev, 0, | OID_AUTO, "freq_settings", CTLTYPE_STRING | CTLFLAG_RD, dev, 0, | ||||
cpufreq_settings_sysctl, "A", "CPU frequency driver settings"); | cpufreq_settings_sysctl, "A", "CPU frequency driver settings"); | ||||
/* | /* | ||||
* Add only one cpufreq device to each CPU. Currently, all CPUs | * Add only one cpufreq device to each CPU. Currently, all CPUs | ||||
* must offer the same levels and be switched at the same time. | * must offer the same levels and be switched at the same time. | ||||
*/ | */ | ||||
cpu_dev = device_get_parent(dev); | cpu_dev = device_get_parent(dev); | ||||
if ((cf_dev = device_find_child(cpu_dev, "cpufreq", -1))) { | if ((cf_dev = device_find_child(cpu_dev, "cpufreq", -1))) { | ||||
sc = device_get_softc(cf_dev); | sc = device_get_softc(cf_dev); | ||||
sc->max_mhz = CPUFREQ_VAL_UNKNOWN; | sc->max_mhz = CPUFREQ_VAL_UNKNOWN; | ||||
MPASS(sc->cf_drv_dev != NULL); | |||||
Done Inline ActionsIt seems like prior to this change, theoretically multiple cpu frequency drivers could attach to a single CPU / cpufreq driver instance — hence all of the enumeration over devs. Now the attached freq_dev will just be whichever driver registered last. In practice this may not be a problem, but either we should enforce that more strictly, or somehow handle it again. cem: It seems like prior to this change, theoretically multiple cpu frequency drivers could attach… | |||||
return (0); | return (0); | ||||
} | } | ||||
/* Add the child device and possibly sysctls. */ | /* Add the child device and possibly sysctls. */ | ||||
cf_dev = BUS_ADD_CHILD(cpu_dev, 0, "cpufreq", -1); | cf_dev = BUS_ADD_CHILD(cpu_dev, 0, "cpufreq", -1); | ||||
if (cf_dev == NULL) | if (cf_dev == NULL) | ||||
return (ENOMEM); | return (ENOMEM); | ||||
device_quiet(cf_dev); | device_quiet(cf_dev); | ||||
return (device_probe_and_attach(cf_dev)); | error = device_probe_and_attach(cf_dev); | ||||
if (error) | |||||
return (error); | |||||
sc = device_get_softc(cf_dev); | |||||
sc->cf_drv_dev = dev; | |||||
cpufreq_add_freq_driver_sysctl(cf_dev); | |||||
return (error); | |||||
} | } | ||||
int | int | ||||
cpufreq_unregister(device_t dev) | cpufreq_unregister(device_t dev) | ||||
{ | { | ||||
device_t cf_dev, *devs; | device_t cf_dev; | ||||
int cfcount, devcount, error, i, type; | struct cpufreq_softc *sc; | ||||
/* | /* | ||||
* If this is the last cpufreq child device, remove the control | * If this is the last cpufreq child device, remove the control | ||||
* device as well. We identify cpufreq children by calling a method | * device as well. We identify cpufreq children by calling a method | ||||
* they support. | * they support. | ||||
*/ | */ | ||||
error = device_get_children(device_get_parent(dev), &devs, &devcount); | |||||
if (error) | |||||
return (error); | |||||
cf_dev = device_find_child(device_get_parent(dev), "cpufreq", -1); | cf_dev = device_find_child(device_get_parent(dev), "cpufreq", -1); | ||||
if (cf_dev == NULL) { | if (cf_dev == NULL) { | ||||
device_printf(dev, | device_printf(dev, | ||||
"warning: cpufreq_unregister called with no cpufreq device active\n"); | "warning: cpufreq_unregister called with no cpufreq device active\n"); | ||||
free(devs, M_TEMP); | |||||
return (0); | return (0); | ||||
} | } | ||||
cfcount = 0; | sc = device_get_softc(cf_dev); | ||||
for (i = 0; i < devcount; i++) { | MPASS(sc->cf_drv_dev == dev); | ||||
if (!device_is_attached(devs[i])) | |||||
continue; | |||||
if (CPUFREQ_DRV_TYPE(devs[i], &type) == 0) | |||||
cfcount++; | |||||
} | |||||
if (cfcount <= 1) | |||||
device_delete_child(device_get_parent(cf_dev), cf_dev); | device_delete_child(device_get_parent(cf_dev), cf_dev); | ||||
Done Inline ActionsAs above — I don't see anything enforcing this invariant. I might be missing something. cem: As above — I don't see anything enforcing this invariant. I might be missing something. | |||||
Done Inline ActionsI think my reworks address this, but please re-review. bwidawsk: I think my reworks address this, but please re-review. | |||||
Not Done Inline ActionsI don't think the rework addresses the issue. We now confirm it's not NULL when registering, but don't check that it meets the stronger condition (== cf_drv_dev) until unregister. cem: I don't think the rework addresses the issue. We now confirm it's not NULL when registering… | |||||
Not Done Inline ActionsI think you can still get multiple things attached, and that you should just add an 'if' here that bails on a mismatch. However, then you could get spurious warnings from the cf_dev == NULL check above. I think though that you could just remove the check (note that the comment is now also stale) and just make the entire thing be something like: /* If this is the active cpufreq child device, remove the control device as well. */ cf_dev = device_find_child(device_get_parent(dev), "cpufreq", -1); if (cf_dev == NULL) return (0); sc = device_get_softc(cf_dev); if (sc->cf_drv_dev == dev) device_delete_child(device_get_parent(cf_dev), cf_dev); return (0); jhb: I think you can still get multiple things attached, and that you should just add an 'if' here… | |||||
free(devs, M_TEMP); | |||||
return (0); | return (0); | ||||
} | } | ||||
int | int | ||||
cpufreq_settings_changed(device_t dev) | cpufreq_settings_changed(device_t dev) | ||||
{ | { | ||||
EVENTHANDLER_INVOKE(cpufreq_levels_changed, | EVENTHANDLER_INVOKE(cpufreq_levels_changed, | ||||
device_get_unit(device_get_parent(dev))); | device_get_unit(device_get_parent(dev))); | ||||
return (0); | return (0); | ||||
} | } |
basic -> basis, I think