The header is already included at line 43 above. Is the second time needed ?

If you export the value with SYSCTL_UINT, then the type of the var better be uint. Or use SYSCTL_U32.

This doesn't match the extern uint32_t declarations of the variable.

Style(9) nit: spaces around = operator.

Given the core kernel refers to this variable (ACPI), this driver cannot be compiled out (even as a module). I mean, I've said before and will repeat that I think this mechanism should die anyway, but — as-is, it creates some additional limitations. (E.g., the "device cpufreq" line in the new manual page is entirely moot since an amd64 kernel without the cpufreq device won't link.)

I'd add a comment referring to est_identify, as well as explaining why we do this funky thing for now. And IMO, the preferred commented-out code style is #if 0 / foo / #endif, but I'm not sure if that's just personal preference or something we actually follow consistently in the tree.

DEVMETHOD_END is canonical. I don't feel strongly about it, though.

This comment is now mildly stale.

The canonical way to do something like this would be with a DEVICE_PROBE(9) implementation. There is a manual page and also some documentation in kern/device_if.m.

All drivers that identify themselves as candidates for a given devclass are PROBEd to determine which driver should be attached. It is possible for multiple drivers to successfully PROBE a given device; the priority is used to select the best driver for a given device.

For Intel CPUs, you would have est probe a lower priority and hwpstate_intel probe a higher priority (if available), on the same device or pseudo-device.

I think this call can sleep and lock some VM locks as well. It is not good to lock while the thread is bound. How hard is to restructure the code to read all MSRs first, then unbind, then do printfs ?

Please add a symbol for the bit name.

Style: put all declarations at the start of the function. Sorry.

In here, after finish and before delete, you would add:

if (ret == 0)
    ret = SYSCTL_OUT(req, sbuf_data(sb), sbuf_len(sb));

This is good but you missed the sbuf_new change that goes along with it above.

Too many blank lines.

inline keyword is probably gratuitous (below as well).

The function is small enough (certainly for non-DEBUG builds) to be inlined automatically, and even if it is not, these routines are only invoked from an (administrative) sysctl anyway — there isn't going to be any significant performance impact either way.

It is fine to leave it if you prefer, though.

I'd avoid double underscore names here too

Can we get symbolic names for the bits ?

The compiler will probably complain about the val < 0 check since val is unsigned. if (val > 100) ... alone is sufficient.

Don't you leak the binding after the end of the loop ?

Usually I'd reject invalid values rather than clamping them to the valid range, unless it makes sense that the valid range may grow or shrink in the future; here it doesn't (IMO).

I'd still encourage just rejecting invalid percentages at the sysctl boundary. For this use, negative and >100 percentages do not make sense.

You need a blank line after '{' if there is no locals.

what does this number represent?

style(9) nit: space before =. Ditto below

I'm not sure this check is robust; I don't understand why we would bail if an INFO_ONLY driver is also attached under the same parent? But not if a !INFO_ONLY driver is attached? The condition in est is the opposite — *allow* an INFO_ONLY sibling, but bail if there are existing !INFO_ONLY siblings.

it doesn't make sense to me to size buf to 128 but hardcode 18 here.

usually i'd suggest snprintf(foo, sizeof(foo), ...) and then hardcode foo[N], if anything.

either way, there may be no need for this buf? you could just use device_get_nameunit. it would be "debug.intel_hwpstateN" instead of "debug.intel_hwp_debugN", but arguably that is better anyway.

I'm not sure that placing debug sysctls under "debug." instead of just using CTLFLAG_SKIP and placing them in the normal device_get_sysctl_tree location is canonical; it may be.

Is this a leftover debugging print? It doesn't seem like something that should go to console on every invocation of the GET method.

Isn't this backwards? If we already attached intel hwpstate, we're using intel_speed_shift and it should remain true.

This is probably not necessary, or should be conditional on bootverbose.

Because the device isn't marked "quiet," the description set in intel_hwpstate_probe is automatically printed at boot time when the device is attached. E.g.,

hwpstate0: <Cool`n'Quiet 2.0> on cpu0

Where hwpstate is the driver, and "Cool`n'Quiet 2.0" is the string from device_set_desc() below.

BUS_PROBE_NOWILDCARD might be more appropriate for now. I don't think the current value will break anything, though, so feel free to leave as-is.

(If/when est and intel_hwpstate are separated and probe for the same newbus pseudo-device, BUS_PROBE_DEFAULT for this one and BUS_PROBE_LOW_PRIORITY for est might make sense.)