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

[Dmitry Torokhov]

On Thu, Mar 19, 2015 at 5:43 PM, Rafael J. Wysocki <rjw at rjwysocki.net> wrote:
> On Thursday, March 19, 2015 03:42:03 PM Dmitry Torokhov wrote:
>> On Thu, Mar 19, 2015 at 2:51 PM, 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 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.
>>
>> That is very non-obvious. dev->drv belongs to driver core and maybe
>> bus, nothing else should really be looking at it. Why can't we add an
>> explicit argument to the callback indicating device state?
>
> Well, if that's a concern, I'd rather introduce a separate callback to
> be run on driver removal/failing probe, like in the patch below, as it is
> then clear what the conditions for each callback's execution are (and one
> can always define them as wrappers around the same routine if need be).

Works for me.

> I still think that this ordering of code is the right one, though.

I was not arguing with that, just with the proposal of checking
dev->drv in callback.

>
> Rafael
>
>
> ---
>  drivers/base/dd.c  |   16 ++++++++++++++++
>  include/linux/pm.h |    8 ++++++++
>  2 files changed, 24 insertions(+)
>
> Index: linux-pm/include/linux/pm.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm.h
> +++ linux-pm/include/linux/pm.h
> @@ -603,10 +603,18 @@ 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 successful driver probe.
> + * @dismiss: Called after unsuccessful driver probe and after driver 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);
> +       void (*dismiss)(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) {
> +               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->dismiss)
> +               pm_domain->dismiss(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->dismiss)
> +                       dev->pm_domain->dismiss(dev);
> +
>                 klist_remove(&dev->p->knode_driver);
>                 if (dev->bus)
>                         blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
>



-- 
Dmitry