diff mbox

driver core / PM: Add callbacks for PM domain initialization/cleanup

Message ID 2317791.ICLpdqLgyu@vostro.rjw.lan (mailing list archive)
State New, archived
Headers show

Commit Message

Rafael J. Wysocki March 18, 2015, 3:02 p.m. UTC
From: Rafael J. Wysocki <rafael.j.wysocki@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@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(-)

Comments

Ulf Hansson March 19, 2015, 8:49 a.m. UTC | #1
On 18 March 2015 at 16:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@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@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.

Make perfect sense!

>
> ---
>  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;
> +               }
>                 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);

I think we should move this new code block below the call to
device_release_driver(), since else the bus'/driver's ->remove()
callbacks may be invoked after the ->pm_domain pointer for the device
has been removed.

Moving the code below the call to device_release_driver() also means
device's devm* managed resources will be freed prior invoking the bus'
->release() callback. Genpd don't have any issues to cope with that I
believe that's okay for ACPI as well, but not sure.

Moreover, considering the case where device won't be removed but only
its corresponding driver. In that case, the PM domain won't be
notified other than through runtime PM transitions. I don't think
that's enough, we will likely need to add yet another callback in the
struct dev_pm_domain, to be invoked from __device_release_driver().

>
>         pr_debug("bus: '%s': remove device %s\n",
>                  dev->bus->name, dev_name(dev));
> Index: linux-pm/include/linux/device.h
> ===================================================================
> --- linux-pm.orig/include/linux/device.h
> +++ linux-pm/include/linux/device.h
> @@ -69,6 +69,8 @@ extern void bus_remove_file(struct bus_t
>   * @bus_groups:        Default attributes of the bus.
>   * @dev_groups:        Default attributes of the devices on the bus.
>   * @drv_groups: Default attributes of the device drivers on the bus.
> + * @init:      Called when registering a device on this bus.
> + * @release:   Called when unregistering a device on this bus.
>   * @match:     Called, perhaps multiple times, whenever a new device or driver
>   *             is added for this bus. It should return a nonzero value if the
>   *             given device can be handled by the given driver.
> @@ -111,6 +113,9 @@ struct bus_type {
>         const struct attribute_group **dev_groups;
>         const struct attribute_group **drv_groups;
>
> +       int (*init)(struct device *dev);
> +       void (*release)(struct device *dev);
> +
>         int (*match)(struct device *dev, struct device_driver *drv);
>         int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
>         int (*probe)(struct device *dev);
> 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 successful execiton of the probe routines.
>   */
>  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,16 +299,23 @@ static int really_probe(struct device *d
>                 goto probe_failed;
>         }
>
> -       if (dev->bus->probe) {
> -               ret = dev->bus->probe(dev);
> -               if (ret)
> -                       goto probe_failed;
> -       } else if (drv->probe) {
> -               ret = drv->probe(dev);
> +       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);
> +       else if (drv->probe)
> +               ret = drv->probe(dev);
> +
> +       if (pm_domain && pm_domain->sync)
> +               pm_domain->sync(dev);
> +
> +       if (ret)
> +               goto probe_failed;
> +
>         driver_bound(dev);
>         ret = 1;
>         pr_debug("bus: '%s': %s: bound device %s to driver %s\n",
>

Kind regards
Uffe
Rafael J. Wysocki March 19, 2015, 11:45 a.m. UTC | #2
On Thursday, March 19, 2015 09:49:19 AM Ulf Hansson wrote:
> On 18 March 2015 at 16:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@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@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.
> 
> Make perfect sense!
> 
> >
> > ---
> >  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;
> > +               }
> >                 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);
> 
> I think we should move this new code block below the call to
> device_release_driver(), since else the bus'/driver's ->remove()
> callbacks may be invoked after the ->pm_domain pointer for the device
> has been removed.

Good point, I'll fix that.

> Moving the code below the call to device_release_driver() also means
> device's devm* managed resources will be freed prior invoking the bus'
> ->release() callback. Genpd don't have any issues to cope with that I
> believe that's okay for ACPI as well, but not sure.

It is.

> Moreover, considering the case where device won't be removed but only
> its corresponding driver. In that case, the PM domain won't be
> notified other than through runtime PM transitions. I don't think
> that's enough, we will likely need to add yet another callback in the
> struct dev_pm_domain, to be invoked from __device_release_driver().

Right, so do we want to ->sync then or do we want a separate callback?

The only use case I have which is not genpd doesn't need that, so genpd
will be the only user of it for the time being.
Ulf Hansson March 19, 2015, 1:16 p.m. UTC | #3
On 19 March 2015 at 12:45, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, March 19, 2015 09:49:19 AM Ulf Hansson wrote:
>> On 18 March 2015 at 16:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > From: Rafael J. Wysocki <rafael.j.wysocki@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@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.
>>
>> Make perfect sense!
>>
>> >
>> > ---
>> >  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;
>> > +               }
>> >                 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);
>>
>> I think we should move this new code block below the call to
>> device_release_driver(), since else the bus'/driver's ->remove()
>> callbacks may be invoked after the ->pm_domain pointer for the device
>> has been removed.
>
> Good point, I'll fix that.
>
>> Moving the code below the call to device_release_driver() also means
>> device's devm* managed resources will be freed prior invoking the bus'
>> ->release() callback. Genpd don't have any issues to cope with that I
>> believe that's okay for ACPI as well, but not sure.
>
> It is.
>
>> Moreover, considering the case where device won't be removed but only
>> its corresponding driver. In that case, the PM domain won't be
>> notified other than through runtime PM transitions. I don't think
>> that's enough, we will likely need to add yet another callback in the
>> struct dev_pm_domain, to be invoked from __device_release_driver().
>
> Right, so do we want to ->sync then or do we want a separate callback?

For genpd, I guess using ->sync() should work.

Typically I envision the ->sync() callback for genpd to check whether
runtime PM is enabled or disabled for the device
(pm_runtime_enable()), at take appropriate actions.

The prerequisite to make this work, is that all drivers/buses make
sure to disable runtime PM from their ->remove() callbacks. I guess we
should expect them to behave like this, else they must be fixed.

>
> The only use case I have which is not genpd doesn't need that, so genpd
> will be the only user of it for the time being.
>

Currently acpi_dev_pm_attach() do more things than just assigning the
device's pm_domain pointer.

My idea was to split that into two parts.

1) The first part takes care of assigning the device's pm_domain
pointer to "&acpi_general_pm_domain" which is done when bus_type's
->init() callback get invoked.

2) The second part, which basically will be the remaining operations
from acpi_dev_pm_attach(), should be done from the PM domain's
->activate() callback.

To be able to reverse these actions (acpi_dev_pm_dettach()), for both
device-remove and driver-remove, don't you think ACPI also will need
to get some notification from a callback, during
__device_release_driver()?

Kind regards
Uffe
Greg KH March 19, 2015, 1:29 p.m. UTC | #4
On Wed, Mar 18, 2015 at 04:02:11PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@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@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.


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

sorry if I missed this before, I hadn't noticed these callbacks in
previous patches but I wasn't paying much attention.

thanks,

greg k-h
Greg KH March 19, 2015, 2:12 p.m. UTC | #5
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@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@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.

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

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

thanks,

greg k-h
Alan Stern March 19, 2015, 2:20 p.m. UTC | #6
On Thu, 19 Mar 2015, Rafael J. Wysocki wrote:

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


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

Driver probing and removal is not a hot path.  Successful probes 
usually occur only once for each device.  I don't know how many 
unsuccessful probes there are on average, but probably not many.

Doing the PM domain attach/detach in ->probe and ->remove makes sense 
to me.

Alan Stern
Rafael J. Wysocki March 19, 2015, 2:21 p.m. UTC | #7
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@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@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
Ulf Hansson March 19, 2015, 2:45 p.m. UTC | #8
On 19 March 2015 at 15:20, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 19 Mar 2015, Rafael J. Wysocki wrote:
>
>> > 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.
>
>
>> 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?
>
> Driver probing and removal is not a hot path.  Successful probes
> usually occur only once for each device.  I don't know how many
> unsuccessful probes there are on average, but probably not many.
>
> Doing the PM domain attach/detach in ->probe and ->remove makes sense
> to me.

Let me elaborate a bit on the current approach, with PM domain
attach/detach in ->probe() and ->remove(). This comes with the
following limitations.

1) If the device gets probed earlier than the PM domain has been
initialized, we can't return -EPROBE_DEFER since we just don't know
anything about the PM domain yet.

2) The PM domain won't be able to operate on a device without having a
driver bound to it, since the device's PM domain pointer is removed at
->remove().

Perhaps Rafael want to add something to these? I don't know whether we
think it's okay to keep these limitations or not.

Kind regards
Uffe
Geert Uytterhoeven March 19, 2015, 2:46 p.m. UTC | #9
On Thu, Mar 19, 2015 at 3:20 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> 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?
>
> Driver probing and removal is not a hot path.  Successful probes
> usually occur only once for each device.  I don't know how many
> unsuccessful probes there are on average, but probably not many.

Do not underestimate the power of deferred probing:

sh_mobile_sdhi ee100000.sd: GPIO lookup for consumer cd
sh_mobile_sdhi ee100000.sd: using device tree for GPIO lookup
of_get_named_gpiod_flags: parsed 'cd-gpios' property of node
'/sd@ee100000[0]' - status (0)
sh_mobile_sdhi ee100000.sd: Got CD GPIO
sh_mobile_sdhi ee100000.sd: GPIO lookup for consumer wp
sh_mobile_sdhi ee100000.sd: using device tree for GPIO lookup
of_get_named_gpiod_flags: parsed 'wp-gpios' property of node
'/sd@ee100000[0]' - status (0)
sh_mobile_sdhi ee100000.sd: Got WP GPIO
platform ee100000.sd: Driver sh_mobile_sdhi requests probe deferral
...
sh_mobile_sdhi ee100000.sd: GPIO lookup for consumer cd
sh_mobile_sdhi ee100000.sd: using device tree for GPIO lookup
of_get_named_gpiod_flags: parsed 'cd-gpios' property of node
'/sd@ee100000[0]' - status (0)
sh_mobile_sdhi ee100000.sd: Got CD GPIO
sh_mobile_sdhi ee100000.sd: GPIO lookup for consumer wp
sh_mobile_sdhi ee100000.sd: using device tree for GPIO lookup
of_get_named_gpiod_flags: parsed 'wp-gpios' property of node
'/sd@ee100000[0]' - status (0)
sh_mobile_sdhi ee100000.sd: Got WP GPIO
gpio_rcar e6055400.gpio: sense irq = 6, type = 3
sh_mobile_sdhi ee100000.sd: mmc0 base at 0xee100000 clock rate 97 MHz

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Rafael J. Wysocki March 19, 2015, 3:24 p.m. UTC | #10
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@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@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.
Ulf Hansson March 19, 2015, 3:37 p.m. UTC | #11
On 19 March 2015 at 16:44, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, March 19, 2015 03:45:07 PM Ulf Hansson wrote:
>> On 19 March 2015 at 15:20, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Thu, 19 Mar 2015, Rafael J. Wysocki wrote:
>> >
>> >> > 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.
>> >
>> >
>> >> 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?
>> >
>> > Driver probing and removal is not a hot path.  Successful probes
>> > usually occur only once for each device.  I don't know how many
>> > unsuccessful probes there are on average, but probably not many.
>> >
>> > Doing the PM domain attach/detach in ->probe and ->remove makes sense
>> > to me.
>>
>> Let me elaborate a bit on the current approach, with PM domain
>> attach/detach in ->probe() and ->remove(). This comes with the
>> following limitations.
>>
>> 1) If the device gets probed earlier than the PM domain has been
>> initialized, we can't return -EPROBE_DEFER since we just don't know
>> anything about the PM domain yet.
>>
>> 2) The PM domain won't be able to operate on a device without having a
>> driver bound to it, since the device's PM domain pointer is removed at
>> ->remove().
>>
>> Perhaps Rafael want to add something to these? I don't know whether we
>> think it's okay to keep these limitations or not.
>
> There are arguments both ways as you can see.
>
> At this point, the most straightforward approach would be to apply the Russell's
> patch adding the ->sync callback alone.
>
> I will then need the ->activate callback for another use case (a PM domain that
> may not be ready for the device to be probed), but in that case I actually know
> that the pm_domain pointer is populated at the really_probe() time.

Then, how about adding only the ->sync() and ->activate() callback in
one patch - and invoke them from really_probe()? That's seems to fit
both our short and long term approach.

Genpd may then implement the ->sync() callback, which is similar to
Russell's patch, except that it will now cover all buses and not just
the platform bus as it where before.

>
> Going forward we may want to move the attach/detach operations for genpd and
> the ACPI PM domain out of bus type ->probe, but I think that can wait.

Agree!

Kind regards
Uffe
Rafael J. Wysocki March 19, 2015, 3:44 p.m. UTC | #12
On Thursday, March 19, 2015 03:45:07 PM Ulf Hansson wrote:
> On 19 March 2015 at 15:20, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Thu, 19 Mar 2015, Rafael J. Wysocki wrote:
> >
> >> > 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.
> >
> >
> >> 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?
> >
> > Driver probing and removal is not a hot path.  Successful probes
> > usually occur only once for each device.  I don't know how many
> > unsuccessful probes there are on average, but probably not many.
> >
> > Doing the PM domain attach/detach in ->probe and ->remove makes sense
> > to me.
> 
> Let me elaborate a bit on the current approach, with PM domain
> attach/detach in ->probe() and ->remove(). This comes with the
> following limitations.
> 
> 1) If the device gets probed earlier than the PM domain has been
> initialized, we can't return -EPROBE_DEFER since we just don't know
> anything about the PM domain yet.
> 
> 2) The PM domain won't be able to operate on a device without having a
> driver bound to it, since the device's PM domain pointer is removed at
> ->remove().
> 
> Perhaps Rafael want to add something to these? I don't know whether we
> think it's okay to keep these limitations or not.

There are arguments both ways as you can see.

At this point, the most straightforward approach would be to apply the Russell's
patch adding the ->sync callback alone.

I will then need the ->activate callback for another use case (a PM domain that
may not be ready for the device to be probed), but in that case I actually know
that the pm_domain pointer is populated at the really_probe() time.

Going forward we may want to move the attach/detach operations for genpd and
the ACPI PM domain out of bus type ->probe, but I think that can wait.
Ulf Hansson March 19, 2015, 3:48 p.m. UTC | #13
On 19 March 2015 at 17:04, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, March 19, 2015 04:37:26 PM Ulf Hansson wrote:
>> On 19 March 2015 at 16:44, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Thursday, March 19, 2015 03:45:07 PM Ulf Hansson wrote:
>> >> On 19 March 2015 at 15:20, Alan Stern <stern@rowland.harvard.edu> wrote:
>> >> > On Thu, 19 Mar 2015, Rafael J. Wysocki wrote:
>> >> >
>> >> >> > 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.
>> >> >
>> >> >
>> >> >> 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?
>> >> >
>> >> > Driver probing and removal is not a hot path.  Successful probes
>> >> > usually occur only once for each device.  I don't know how many
>> >> > unsuccessful probes there are on average, but probably not many.
>> >> >
>> >> > Doing the PM domain attach/detach in ->probe and ->remove makes sense
>> >> > to me.
>> >>
>> >> Let me elaborate a bit on the current approach, with PM domain
>> >> attach/detach in ->probe() and ->remove(). This comes with the
>> >> following limitations.
>> >>
>> >> 1) If the device gets probed earlier than the PM domain has been
>> >> initialized, we can't return -EPROBE_DEFER since we just don't know
>> >> anything about the PM domain yet.
>> >>
>> >> 2) The PM domain won't be able to operate on a device without having a
>> >> driver bound to it, since the device's PM domain pointer is removed at
>> >> ->remove().
>> >>
>> >> Perhaps Rafael want to add something to these? I don't know whether we
>> >> think it's okay to keep these limitations or not.
>> >
>> > There are arguments both ways as you can see.
>> >
>> > At this point, the most straightforward approach would be to apply the Russell's
>> > patch adding the ->sync callback alone.
>> >
>> > I will then need the ->activate callback for another use case (a PM domain that
>> > may not be ready for the device to be probed), but in that case I actually know
>> > that the pm_domain pointer is populated at the really_probe() time.
>>
>> Then, how about adding only the ->sync() and ->activate() callback in
>> one patch - and invoke them from really_probe()? That's seems to fit
>> both our short and long term approach.
>>
>> Genpd may then implement the ->sync() callback, which is similar to
>> Russell's patch, except that it will now cover all buses and not just
>> the platform bus as it where before.
>
> OK
>
> So do we need to do ->sync at the driver removal time too?  I guess we do?

For genpd, it will currently not matter since the device will be
detached from its PM domain before that ->sync() would be called.

Though, to prepare for the "long term" solution we could decide to add it...

Kind regards
Uffe
Rafael J. Wysocki March 19, 2015, 4:04 p.m. UTC | #14
On Thursday, March 19, 2015 04:37:26 PM Ulf Hansson wrote:
> On 19 March 2015 at 16:44, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Thursday, March 19, 2015 03:45:07 PM Ulf Hansson wrote:
> >> On 19 March 2015 at 15:20, Alan Stern <stern@rowland.harvard.edu> wrote:
> >> > On Thu, 19 Mar 2015, Rafael J. Wysocki wrote:
> >> >
> >> >> > 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.
> >> >
> >> >
> >> >> 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?
> >> >
> >> > Driver probing and removal is not a hot path.  Successful probes
> >> > usually occur only once for each device.  I don't know how many
> >> > unsuccessful probes there are on average, but probably not many.
> >> >
> >> > Doing the PM domain attach/detach in ->probe and ->remove makes sense
> >> > to me.
> >>
> >> Let me elaborate a bit on the current approach, with PM domain
> >> attach/detach in ->probe() and ->remove(). This comes with the
> >> following limitations.
> >>
> >> 1) If the device gets probed earlier than the PM domain has been
> >> initialized, we can't return -EPROBE_DEFER since we just don't know
> >> anything about the PM domain yet.
> >>
> >> 2) The PM domain won't be able to operate on a device without having a
> >> driver bound to it, since the device's PM domain pointer is removed at
> >> ->remove().
> >>
> >> Perhaps Rafael want to add something to these? I don't know whether we
> >> think it's okay to keep these limitations or not.
> >
> > There are arguments both ways as you can see.
> >
> > At this point, the most straightforward approach would be to apply the Russell's
> > patch adding the ->sync callback alone.
> >
> > I will then need the ->activate callback for another use case (a PM domain that
> > may not be ready for the device to be probed), but in that case I actually know
> > that the pm_domain pointer is populated at the really_probe() time.
> 
> Then, how about adding only the ->sync() and ->activate() callback in
> one patch - and invoke them from really_probe()? That's seems to fit
> both our short and long term approach.
> 
> Genpd may then implement the ->sync() callback, which is similar to
> Russell's patch, except that it will now cover all buses and not just
> the platform bus as it where before.

OK

So do we need to do ->sync at the driver removal time too?  I guess we do?
Rafael J. Wysocki March 19, 2015, 4:18 p.m. UTC | #15
On Thursday, March 19, 2015 04:48:22 PM Ulf Hansson wrote:
> On 19 March 2015 at 17:04, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Thursday, March 19, 2015 04:37:26 PM Ulf Hansson wrote:
> >> On 19 March 2015 at 16:44, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > On Thursday, March 19, 2015 03:45:07 PM Ulf Hansson wrote:
> >> >> On 19 March 2015 at 15:20, Alan Stern <stern@rowland.harvard.edu> wrote:
> >> >> > On Thu, 19 Mar 2015, Rafael J. Wysocki wrote:
> >> >> >
> >> >> >> > 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.
> >> >> >
> >> >> >
> >> >> >> 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?
> >> >> >
> >> >> > Driver probing and removal is not a hot path.  Successful probes
> >> >> > usually occur only once for each device.  I don't know how many
> >> >> > unsuccessful probes there are on average, but probably not many.
> >> >> >
> >> >> > Doing the PM domain attach/detach in ->probe and ->remove makes sense
> >> >> > to me.
> >> >>
> >> >> Let me elaborate a bit on the current approach, with PM domain
> >> >> attach/detach in ->probe() and ->remove(). This comes with the
> >> >> following limitations.
> >> >>
> >> >> 1) If the device gets probed earlier than the PM domain has been
> >> >> initialized, we can't return -EPROBE_DEFER since we just don't know
> >> >> anything about the PM domain yet.
> >> >>
> >> >> 2) The PM domain won't be able to operate on a device without having a
> >> >> driver bound to it, since the device's PM domain pointer is removed at
> >> >> ->remove().
> >> >>
> >> >> Perhaps Rafael want to add something to these? I don't know whether we
> >> >> think it's okay to keep these limitations or not.
> >> >
> >> > There are arguments both ways as you can see.
> >> >
> >> > At this point, the most straightforward approach would be to apply the Russell's
> >> > patch adding the ->sync callback alone.
> >> >
> >> > I will then need the ->activate callback for another use case (a PM domain that
> >> > may not be ready for the device to be probed), but in that case I actually know
> >> > that the pm_domain pointer is populated at the really_probe() time.
> >>
> >> Then, how about adding only the ->sync() and ->activate() callback in
> >> one patch - and invoke them from really_probe()? That's seems to fit
> >> both our short and long term approach.
> >>
> >> Genpd may then implement the ->sync() callback, which is similar to
> >> Russell's patch, except that it will now cover all buses and not just
> >> the platform bus as it where before.
> >
> > OK
> >
> > So do we need to do ->sync at the driver removal time too?  I guess we do?
> 
> For genpd, it will currently not matter since the device will be
> detached from its PM domain before that ->sync() would be called.
> 
> Though, to prepare for the "long term" solution we could decide to add it...

OK, at that time.

Fine, I'll send an update, then.
diff mbox

Patch

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;
+		}
 		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);
 
 	pr_debug("bus: '%s': remove device %s\n",
 		 dev->bus->name, dev_name(dev));
Index: linux-pm/include/linux/device.h
===================================================================
--- linux-pm.orig/include/linux/device.h
+++ linux-pm/include/linux/device.h
@@ -69,6 +69,8 @@  extern void bus_remove_file(struct bus_t
  * @bus_groups:	Default attributes of the bus.
  * @dev_groups:	Default attributes of the devices on the bus.
  * @drv_groups: Default attributes of the device drivers on the bus.
+ * @init:	Called when registering a device on this bus.
+ * @release:	Called when unregistering a device on this bus.
  * @match:	Called, perhaps multiple times, whenever a new device or driver
  *		is added for this bus. It should return a nonzero value if the
  *		given device can be handled by the given driver.
@@ -111,6 +113,9 @@  struct bus_type {
 	const struct attribute_group **dev_groups;
 	const struct attribute_group **drv_groups;
 
+	int (*init)(struct device *dev);
+	void (*release)(struct device *dev);
+
 	int (*match)(struct device *dev, struct device_driver *drv);
 	int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
 	int (*probe)(struct device *dev);
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 successful execiton of the probe routines.
  */
 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,16 +299,23 @@  static int really_probe(struct device *d
 		goto probe_failed;
 	}
 
-	if (dev->bus->probe) {
-		ret = dev->bus->probe(dev);
-		if (ret)
-			goto probe_failed;
-	} else if (drv->probe) {
-		ret = drv->probe(dev);
+	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);
+	else if (drv->probe)
+		ret = drv->probe(dev);
+
+	if (pm_domain && pm_domain->sync)
+		pm_domain->sync(dev);
+
+	if (ret)
+		goto probe_failed;
+
 	driver_bound(dev);
 	ret = 1;
 	pr_debug("bus: '%s': %s: bound device %s to driver %s\n",