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

[Rafael J. Wysocki]

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()?

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.

> >  		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?

Rafael