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

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

On Friday, May 03, 2013 02:05:37 PM 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>
> > > 
> > > Split the ACPI processor driver into two parts, one that is
> > > non-modular, resides in the ACPI core and handles the enumeration
> > > and hotplug of processors and one that implements the rest of the
> > > existing processor driver functionality.
> > > 
> > > The non-modular part uses an ACPI scan handler object to enumerate
> > > processors on the basis of information provided by the ACPI namespace
> > > and to hook up with the common ACPI hotplug infrastructure.  It also
> > > populates the ACPI handle of each processor device having a
> > > corresponding object in the ACPI namespace, which allows the driver
> > > proper to bind to those devices, and makes the driver bind to them
> > > if it is readily available (i.e. loaded) when the scan handler's
> > > .attach() routine is running.
> > > 
> > > There are a few reasons to make this change.
> > > 
> > > First, switching the ACPI processor driver to using the common ACPI
> > > hotplug infrastructure reduces code duplication and size considerably,
> > > even though a new file is created along with a header comment etc.
> > > 
> > > Second, since the common hotplug code attempts to offline devices
> > > before starting the (non-reversible) removal procedure, it will abort
> > > (and possibly roll back) hot-remove operations involving processors
> > > if cpu_down() returns an error code for one of them instead of
> > > continuing them blindly (if /sys/firmware/acpi/hotplug/force_remove
> > > is unset).  That is a more desirable behavior than what the current
> > > code does.
> > > 
> > > Finally, the separation of the scan/hotplug part from the driver
> > > proper makes it possible to simplify the driver's .remove() routine,
> > > because it doesn't need to worry about the possible cleanup related
> > > to processor removal any more (the scan/hotplug part is responsible
> > > for that now) and can handle device removal and driver removal
> > > symmetricaly (i.e. as appropriate).
> > > 
> > > Some user-visible changes in sysfs are made (for example, the
> > > 'sysdev' link from the ACPI device node to the processor device's
> > > directory is gone and a 'physical_node' link is present instead,
> > > a 'firmware_node' link is present in the processor device's
> > > directory, the processor driver is now visible under
> > > /sys/bus/cpu/drivers/ and bound to the processor device), but
> > > that shouldn't affect the functionality that users care about
> > > (frequency scaling, C-states and thermal management).
> > 
> > This looks very nice.  I have one question below.
> > 
> > > Tested on my venerable Toshiba Portege R500.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  drivers/acpi/Makefile           |    1 
> > >  drivers/acpi/acpi_processor.c   |  473 +++++++++++++++++++++++
> > >  drivers/acpi/glue.c             |    6 
> > >  drivers/acpi/internal.h         |    3 
> > >  drivers/acpi/processor_driver.c |  803 +++-------------------------------------
> > >  drivers/acpi/scan.c             |    1 
> > >  drivers/base/cpu.c              |   11 
> > >  include/acpi/processor.h        |    5 
> > >  8 files changed, 574 insertions(+), 729 deletions(-)
> > 
> >  :
> > 
> > > 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().

The reason why it really is needed is because the ACPI processor driver is
modular and it may or may not be present when acpi_processor_add() is running,
but acpi_processor_start() should be called for the device once the driver has
been loaded.

Thanks,
Rafael


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