| Message ID | 2317791.ICLpdqLgyu@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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
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
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
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
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
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
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.
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
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.
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
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?
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.
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",