I'd avoid double underscore names here too

The logical percent range can change to something other than 0-100%? I'm confused.

The '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'.

'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.

To 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)'

In HEAD we just added a SYSCTL_ADD_CONST_STRING that should let you get rid of this cast I think.

I 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);

This is fine as-is. I would perhaps be inclined to invert this flag's sense to assume an actual "real" get method by default and falling back to using the previously set level on "dumb" drivers, something like 'CPUFREQ_FLAG_SETONLY' maybe. I don't think it's worth changing right now though.

I mostly can't think of a non-crappy header to put it in.

I have wondered if we don't want to effectively merge the two drivers in some way.

You don't actually have to rename this driver FWIW. It's fine to have multiple drivers with the same name.

I would probably just leave the name as-is as well FWIW.

I can't think of an immediate downside to this idea (and like it better than the current approach). Ben, is it objectionable to you?

I don't object to naming different things differently. It can reduce confusion in reports / debugging.

The main downside is that suppose after this is in I do the cpufreq rework I sketched out in my overall comment last week. In that world, it's quite easy to have the 'speedshift' driver use a higher probe priority than est and have them be separate drivers. Given that they work quite differently I think this is cleaner long term. I had typed this comment above before my general comment (so it was "earlier" in my thinking).

The extern definition and the check for this variable needs to be #if defined(__amd64__), otherwise there's a linking error on aarch64

The whole declaration should be removed. See discussion on the definition @ https://reviews.freebsd.org/D18028#inline-117423 . (Just curious, why are you trying to build the intel_hwpstate in-review driver on aarch64?)

(I just have a single git fork of src where I apply every patch I need to master. Not building this *driver*, but acpi_perf was being built, and it was affected by this patch.)

I 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.

Does this seem like a reasonable way to intertwine the two identify routines?

Sorry missed this one in reading through the old comments. I think it's fixed properly now.

Probably put this in a header (it can be a totally trivial intel_cpufreq_internal.h, for example) rather than declaring it in both drivers.

It's kinda funky, but tolerable IMO. If John has strong objections, he overrules me :-). Newbus doesn't really make this easy/clean on us.

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.

This comment is now mildly stale.

Looks good to me, thanks!

Alternatively you could just read the 1-4 64-bit MSRs in the CPU-bound section, storing them on the stack, and use sbuf printing only after unbinding the CPU. That'd avoid malloc/free and also avoid possibly truncating output. I'm not requesting any change, it's just another valid approach.

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

FWIW, you can just check the flag in the cached cpu_power_eax instead of cpuid(6) + regs[0] after rS353712. cpu_power_foo represents the cpuid 6 leaf. (The name isn't especially creative; sorry.)

Fine to leave as-is, especially if you want to MFC this.

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.)

Ditto other comment about cpuid

This is fine for now.

we shouldn't be dependent on the ordering now, so i've taken away the order and the comment.

That seems like a reasonable idea but I left it as-is for now.

I'd prefer to do that as a follow on change for ease of mfc.

works for me.

works for me.