[PATCH] driver core / PM: Add callbacks for PM domain initialization/cleanup

Thu Mar 19 08:24:06 PDT 2015

On Thursday, March 19, 2015 03:12:23 PM Greg Kroah-Hartman wrote:
> On Thu, Mar 19, 2015 at 03:21:14PM +0100, Rafael J. Wysocki wrote:
> > On Thursday, March 19, 2015 02:29:07 PM Greg Kroah-Hartman wrote:
> > > On Wed, Mar 18, 2015 at 04:02:11PM +0100, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki at intel.com>
> > > > 
> > > > If PM domains are in use, it may be necessary to prepare the code
> > > > handling a PM domain for driver probing.  For example, in some
> > > > cases device drivers rely on the ability to power on the devices
> > > > with the help of the IO runtime PM framework and the PM domain
> > > > code needs to be ready for that.  Also, if that code has not been
> > > > fully initialized yet, the driver probing should be deferred.
> > > > 
> > > > Moreover, after the probing is complete, it may be necessary to
> > > > put the PM domain in question into the state reflecting the current
> > > > needs of the devices in it, for example, to prevent power from being
> > > > drawn in vain.
> > > > 
> > > > For these reasons, introduce new PM domain callbacks, ->activate
> > > > and ->sync, called, respectively, before probing for a device
> > > > driver and after the probing has been completed.
> > > > 
> > > > That is not sufficient, however, because the device's PM domain
> > > > pointer has to be populated for the ->activate callback to be
> > > > executed, so setting it in bus type ->probe callback routines
> > > > would be too late.  Also, there are bus types where PM domains
> > > > are not used at all and the core should not attempt to set the
> > > > pm_domain pointer for the devices on those buses.
> > > > 
> > > > To overcome that difficulty, introduce two new bus type
> > > > callbacks, ->init and ->release, called by bus_add_device() and
> > > > bus_remove_device(), respectively.  That will allow ->init to
> > > > be used to populate the pm_domain pointer for the bus types
> > > > that want to do that and ->release will be useful for any
> > > > cleanup that may be necessary after removing a device that
> > > > was part of a PM domain.
> > > > 
> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki at intel.com>
> > > > ---
> > > > 
> > > > It occured to me that we might want to ->sync regardless of whether or
> > > > not the probing had been succenssful, so I changed the code in
> > > > really_probe() along these lines.  Please let me know if that's
> > > > not OK.
> > > > 
> > > > ---
> > > >  drivers/base/bus.c     |   12 +++++++++++-
> > > >  drivers/base/dd.c      |   20 ++++++++++++++------
> > > >  include/linux/device.h |    5 +++++
> > > >  include/linux/pm.h     |    6 ++++++
> > > >  4 files changed, 36 insertions(+), 7 deletions(-)
> > > > 
> > > > Index: linux-pm/drivers/base/bus.c
> > > > ===================================================================
> > > > --- linux-pm.orig/drivers/base/bus.c
> > > > +++ linux-pm/drivers/base/bus.c
> > > > @@ -509,10 +509,15 @@ int bus_add_device(struct device *dev)
> > > >  	int error = 0;
> > > >  
> > > >  	if (bus) {
> > > > +		if (bus->init) {
> > > > +			error = bus->init(dev);
> > > > +			if (error)
> > > > +				goto out_put;
> > > > +		}
> > > 
> > > This doesn't make sense to me.  A bus just called bus_add_device, it can
> > > do whatever it wanted to right before calling this function, no need for
> > > another callback.
> > 
> > The only caller of bus_add_device() is device_add().
> > 
> > What do you mean by "bus just called bus_add_device"?  Do you think that
> > the pm_domain pointer should be populated before calling device_add()?
> 
> If it's needed, sure.  The bus itself (i.e. PCI, USB, etc.) just called
> device_add() which calls bus_add_device(), so it could set up the
> pm_domain pointer as it knows what is up for this device.

It doesn't work exactly like that.  PCI, for one, doesn't know whether or not
it is going to use ACPI with this particular device when it is calling
device_add().  That only turns out during device_add(), when platform_notify()
is called from device_add().  Fortunately, that doesn't matter for the ACPI
PM domain.

For bus types that know upfront whether or not they are going to use ACPI
with the device in question, we could hook the device up to the ACPI PM domain
before device_add() is called.  The removal part could just be covered with the
PM domain's ->detach callback I suppose.  I'm not sure if all bus types using
the ACPI PM domain fall into this category, so I'll need to double check

However, there still is the generic PM domains framework that has grown DT
support and I'm not sure if we can hook that up to devices before device_add()
is called for them.

Ulf, what do you think?

> > That wouldn't work for the ACPI PM domain at least, because ACPI companions
> > are generally added during device_add() and we arguably cannot point a
> > device to the ACPI PM domain before its ACPI companion is set.
> 
> I don't understand, what is not set up at device_add() time that is
> somehow set up at bus_add_device() time?  Our number of callbacks seems
> to be getting deep and messy :)
>
> What happens in device_pm_add(dev); in device_add()?

Nothing particularly interesting.  It just adds the device to the list used by
system suspend/resume.

> > > >  		pr_debug("bus: '%s': add device %s\n", bus->name, dev_name(dev));
> > > >  		error = device_add_attrs(bus, dev);
> > > >  		if (error)
> > > > -			goto out_put;
> > > > +			goto out_release;
> > > >  		error = device_add_groups(dev, bus->dev_groups);
> > > >  		if (error)
> > > >  			goto out_groups;
> > > > @@ -534,6 +539,9 @@ out_groups:
> > > >  	device_remove_groups(dev, bus->dev_groups);
> > > >  out_id:
> > > >  	device_remove_attrs(bus, dev);
> > > > +out_release:
> > > > +	if (bus->release)
> > > > +		bus->release(dev);
> > > 
> > > >  out_put:
> > > >  	bus_put(dev->bus);
> > > >  	return error;
> > > > @@ -597,6 +605,8 @@ void bus_remove_device(struct device *de
> > > >  	device_remove_groups(dev, dev->bus->dev_groups);
> > > >  	if (klist_node_attached(&dev->p->knode_bus))
> > > >  		klist_del(&dev->p->knode_bus);
> > > > +	if (bus->release)
> > > > +		bus->release(dev);
> > > 
> > > Same with release(), this happens when a bus wants to remove a device,
> > > it controls this, why have a callback right away?  These both shouldn't
> > > be needed.
> > 
> > This is for symmetry with bus_add_device() and please see the argument there.
> > 
> > > sorry if I missed this before, I hadn't noticed these callbacks in
> > > previous patches but I wasn't paying much attention.
> > 
> > No, they were not present before.
> > 
> > There are two alternatives to them.  One is to do PM domain attach/detach in
> > the bus type's ->probe and ->remove, but that's suboptimal, because it is
> > then carried out every time a driver is probed/removed for a device.  The
> > other one would be to have each interested bus type register a bus type
> > notifier for itself, but that would be rather ugly, wouldn't it?
> 
> This seems really messy, and you are adding more complexity, isn't there
> some other easier way to do it with all of the different callbacks and
> notifications we have today?

Well, we've explored multiple alternatives already and none of them has
turned out to be particularly attractive.  I guess we need to explore some
more of them, then. :-)

For one, I wouldn't consider adding more callbacks if I saw a clean way
forward without them.

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