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

[Ulf Hansson]

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

Maybe it's just safer to always do the following check:

if (dev->pm_domain && dev->pm_domain->activate|sync)
...

>> +               ret = pm_domain->activate(dev);
>> +               if (ret)
>> +                       goto probe_failed;
>> +       }
>> +
>>         if (dev->bus->probe) {
>>                 ret = dev->bus->probe(dev);
>>                 if (ret)
>> @@ -308,6 +315,9 @@ static int really_probe(struct device *d
>>                         goto probe_failed;
>>         }
>>
>> +       if (pm_domain && pm_domain->sync)
>> +               pm_domain->sync(dev);
>> +
>>         driver_bound(dev);
>>         ret = 1;
>>         pr_debug("bus: '%s': %s: bound device %s to driver %s\n",
>> @@ -319,6 +329,8 @@ probe_failed:
>>         driver_sysfs_remove(dev);
>>         dev->driver = NULL;
>>         dev_set_drvdata(dev, NULL);
>> +       if (pm_domain && pm_domain->sync)
>> +               pm_domain->sync(dev);
>>
>>         if (ret == -EPROBE_DEFER) {
>>                 /* Driver requested deferred probing */
>> @@ -522,9 +534,13 @@ static void __device_release_driver(stru
>>                         dev->bus->remove(dev);
>>                 else if (drv->remove)
>>                         drv->remove(dev);
>> +
>>                 devres_release_all(dev);
>>                 dev->driver = NULL;
>>                 dev_set_drvdata(dev, NULL);
>> +               if (dev->pm_domain && dev->pm_domain->sync)
>> +                       dev->pm_domain->sync(dev);
>> +
>>                 klist_remove(&dev->p->knode_driver);
>>                 if (dev->bus)
>>                         blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
>>

Kind regards
Uffe