Re: [PATCH 01/10] Add driver auto probing for x86 features

[Jean Delvare]

Hi Andi,

On Thu, 8 Dec 2011 15:45:16 +0100, Andi Kleen wrote:
> On Thu, Dec 08, 2011 at 10:35:40AM +0100, Jean Delvare wrote:
> > > +/**
> > > + * x86_match_cpu - match current CPU again an array of x86_cpu_ids
> > 
> > The code is actually matching the boot cpu, which isn't necessarily the
> > current CPU. I think it would be better to let the caller pass a
> > specific cpu as a parameter. At least the hwmon drivers would benefit
> > from that. Then you can add an helper function x86_match_boot_cpu()
> > calling x86_match_cpu() on &boot_cpu_data if you want.
> 
> I don't really see a point of this currently -- the udev/modprobe loading is 
> global anyways.

Irrelevant. You load a driver when any, not all, of the devices in the
system are supported by said driver. This obviously holds for PCI
devices and drivers (you don't refrain from loading the network card's
driver because that driver doesn't support your graphics card), the CPU
device case is no different (in theory at least.)

As a matter of fact, the sensors-detect script (which is in charge of
finding the right hwmon drivers to load on a given system when these
drivers do not support auto-loading via module aliases) does exactly
this for CPU thermal sensor detection. It loops over the complete list
of online CPUs, and if any is supported by coretemp or via-cputemp,
then it suggests loading the driver in question. The search is not
limited to CPU#0.

> If we really want to support asymmetric configs a lot 
> more work all over the kernel is needed and this could be still
> changed.

Can you guarantee that there is no asymmetric system already working
today? If you can't, then your approach could cause a regression. This
is why I am asking for an API change to let drivers pass a specific CPU
to x86_match_cpu(). There are at least two drivers who would take
benefit of this. Your patch set is supposed to only add driver
auto-loading, and while cleaning up the code in the process is nice, I
think you should avoid driver behavior changes, these are out of scope
for such a patch set.

Seriously, this API change is very small and unintrusive, so I just
can't think of a valid reason to not do it.

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/