[PATCH v2] driver core / PM: Add PM domain callbacks for device setup/cleanup

[Rafael J. Wysocki]

On Friday, March 20, 2015 12:37:47 PM Ulf Hansson wrote:
> On 20 March 2015 at 08:45, Ulf Hansson <ulf.hansson at linaro.org> wrote:
> > On 19 March 2015 at 22:51, Rafael J. Wysocki <rjw at rjwysocki.net> wrote:
> >> From: Rafael J. Wysocki <rafael.j.wysocki at intel.com>
> >> Subject: driver core / PM: Add PM domain callbacks for device setup/cleanup
> >>
> >> 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, so that power is not drawn
> >> in vain.  The same should be done after removing a driver from
> >> a device, as the PM domain state may need to be changed to reflect
> >> the new situation.
> >>
> >> 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 or the driver
> >> has been removed.
> >>
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki at intel.com>
> >
> > This looks good to me!
> >
> > Acked-by: Ulf Hansson <ulf.hansson at linaro.org>
> 
> I was a bit too quick acking this a minor fix is needed. See below.
> 
> >
> >> ---
> >>> ---
> >>>
> >>> This is an update without the additional bus type callbacks.  This should
> >>> be suffucient at this point for the use cases we have.
> >>>
> >>> I've decided to also call ->sync on driver removal as that is consistent
> >>> with what happens for failing probe.
> >>>
> >>> ---
> >>
> >> One more update.
> >>
> >> I've realized that in the PM domain's ->sync callback is called after
> >> clearing the device's driver field (in the failed probe or driver removal
> >> case) rather than before it, that could be used to distinguish between the
> >> two cases (successful probe vs no driver), so I reworked the patch accordingly.
> >>
> >> ---
> >>  drivers/base/dd.c  |   16 ++++++++++++++++
> >>  include/linux/pm.h |    6 ++++++
> >>  2 files changed, 22 insertions(+)
> >>
> >> Index: linux-pm/include/linux/pm.h
> >> ===================================================================
> >> --- linux-pm.orig/include/linux/pm.h
> >> +++ linux-pm/include/linux/pm.h
> >> @@ -603,10 +603,16 @@ extern void dev_pm_put_subsys_data(struc
> >>   * Power domains provide callbacks that are executed during system suspend,
> >>   * hibernation, system resume and during runtime PM transitions along with
> >>   * subsystem-level and driver-level callbacks.
> >> + *
> >> + * @detach: Called when removing a device from the domain.
> >> + * @activate: Called before executing probe routines for bus types and drivers.
> >> + * @sync: Called after driver probe and removal.
> >>   */
> >>  struct dev_pm_domain {
> >>         struct dev_pm_ops       ops;
> >>         void (*detach)(struct device *dev, bool power_off);
> >> +       int (*activate)(struct device *dev);
> >> +       void (*sync)(struct device *dev);
> >>  };
> >>
> >>  /*
> >> Index: linux-pm/drivers/base/dd.c
> >> ===================================================================
> >> --- linux-pm.orig/drivers/base/dd.c
> >> +++ linux-pm/drivers/base/dd.c
> >> @@ -279,6 +279,7 @@ static int really_probe(struct device *d
> >>  {
> >>         int ret = 0;
> >>         int local_trigger_count = atomic_read(&deferred_trigger_count);
> >> +       struct dev_pm_domain *pm_domain = dev->pm_domain;
> >>
> >>         atomic_inc(&probe_count);
> >>         pr_debug("bus: '%s': %s: probing driver %s with device %s\n",
> >> @@ -298,6 +299,12 @@ static int really_probe(struct device *d
> >>                 goto probe_failed;
> >>         }
> >>
> 
> 
> 
> >> +       if (pm_domain && pm_domain->activate) {
> 
> You need to re-fetch the pm_domain pointer at this point, since it
> will be updated during ->probe().

Right.

> Maybe it's just safer to always do the following check:
> 
> if (dev->pm_domain && dev->pm_domain->activate|sync)

Yes, it is.

Well, overoptimization.  I seem to always forget to avoid them. :-)

Will send a v3 with that fixed and the Dmitry's comment taken into account
shortly.


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