Re: [PATCH 4/4] ACPI / processor: Use common hotplug infrastructure

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Friday, May 03, 2013 12:27:54 PM Toshi Kani wrote:
> On Fri, 2013-05-03 at 14:05 +0200, Rafael J. Wysocki wrote:
> > On Thursday, May 02, 2013 05:20:12 PM Toshi Kani wrote:
> > > On Thu, 2013-05-02 at 14:31 +0200, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>  : 
> > > > Index: linux-pm/drivers/base/cpu.c
> > > > ===================================================================
> > > > --- linux-pm.orig/drivers/base/cpu.c
> > > > +++ linux-pm/drivers/base/cpu.c
> > > > @@ -13,11 +13,21 @@
> > > >  #include <linux/gfp.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/percpu.h>
> > > > +#include <linux/acpi.h>
> > > >  
> > > >  #include "base.h"
> > > >  
> > > >  static DEFINE_PER_CPU(struct device *, cpu_sys_devices);
> > > >  
> > > > +static int cpu_subsys_match(struct device *dev, struct device_driver *drv)
> > > > +{
> > > > +	/* ACPI style match is the only one that may succeed. */
> > > > +	if (acpi_driver_match_device(dev, drv))
> > > 
> > > Can you explain why this change is needed?
> > 
> > This is the mechanism by which the driver core determines which driver to use
> > with a processor device passed to device_attach().
> > 
> > Basically, it walks the list of drivers whose bus type is cpu_subsys and
> > calls cpu_subsys->match(), which points to cpu_subsys_match(), for the device
> > and each of the drivers.  The result of that tell is whether or not to use
> > the given driver with the device.
> > 
> > Now, acpi_driver_match_device() returns 'true' if (a) the device has an ACPI
> > handle and (b) at least one of the IDs of the struct acpi_device associated
> > with that handle is in the driver's .acpi_match_table table.  Since the ACPI
> > processor's .acpi_match_table contains the same set of IDs as the table
> > of device IDs of processor_handler, this guarantees that the ACPI processor
> > driver will be used for the devices prepared by acpi_processor_add().
> > 
> > What it boils down to is that acpi_processor_start() is going to be called
> > for every device whose ACPI handle is populated by acpi_processor_add().
> > 
> > > Do CPU devices still behave the same on non-ACPI systems?
> > 
> > Yes, they do.  The whole driver matching/binding is irrelevant to them, because
> > the ACPI processor driver is the only one registering itself under cpu_subsys.
> 
> Thanks for the detailed explanation!  I missed that the new processor
> driver is registered to cpu_subsys.  I now see what you did.  This is
> clever.

Well, thanks! :-)

> One minor comment.
> 
> +static __cpuinit int __acpi_processor_start(struct acpi_device *device)
> >  {
> > -	struct acpi_device *device = per_cpu(processor_device_array,
> pr->id);
> > +	struct acpi_processor *pr = acpi_driver_data(device);
> > +	acpi_status status;
> >  	int result = 0;
> >  
> > +	if (!pr)
> > +		return -ENODEV;
> > +
> > +	if (pr->flags.need_hotplug_init)
> > +		return 0;
> > +
> 
> I felt the name of "need_hotplug_init" is a bit misleading since the
> func actually skips when the need-flag is set.  It may be nice to rename
> it to defer_online_init, offline or something like that.

I just wanted to avoid making too many non-essential changes in one patch.
We can change the name of that field at any time later.

> Otherwise the changes look very good. 
> 
> Reviewed-by: Toshi Kani <toshi.kani@hp.com>

Thank you!

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.