* [PATCH 0/2] driver core: Two more updates related to device links @ 2019-02-07 18:35 Rafael J. Wysocki 2019-02-07 18:38 ` [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status() Rafael J. Wysocki 2019-02-07 18:41 ` [PATCH 2/2] driver core: Document limitation related to DL_FLAG_RPM_ACTIVE Rafael J. Wysocki 0 siblings, 2 replies; 10+ messages in thread From: Rafael J. Wysocki @ 2019-02-07 18:35 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: LKML, Linux PM, Ulf Hansson, Daniel Vetter, Lukas Wunner, Andrzej Hajda, Russell King - ARM Linux, Lucas Stach, Linus Walleij, Thierry Reding, Laurent Pinchart, Marek Szyprowski, Joerg Roedel Hi Greg at al, These two patches are on top of the recent device links updates series already in linux-next (https://lore.kernel.org/lkml/1952449.TVsm6CJCTy@aspire.rjw.lan/). The first one changes __pm_runtime_set_status() to look at device links and activate or deactivate the suppliers of the target device in accordance with its new PM-runtime status. After this change, pm_runtime_set_suspended() and pm_runtime_set_active() should handle suppliers consistently with runtime suspend and resume. The second patch is a documentation update resulting from the discussion with Ulf that followed the above patch series. Cheers, Rafael ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status() 2019-02-07 18:35 [PATCH 0/2] driver core: Two more updates related to device links Rafael J. Wysocki @ 2019-02-07 18:38 ` Rafael J. Wysocki 2019-02-11 13:27 ` Ulf Hansson 2019-02-12 16:02 ` Ulf Hansson 2019-02-07 18:41 ` [PATCH 2/2] driver core: Document limitation related to DL_FLAG_RPM_ACTIVE Rafael J. Wysocki 1 sibling, 2 replies; 10+ messages in thread From: Rafael J. Wysocki @ 2019-02-07 18:38 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: LKML, Linux PM, Ulf Hansson, Daniel Vetter, Lukas Wunner, Andrzej Hajda, Russell King - ARM Linux, Lucas Stach, Linus Walleij, Thierry Reding, Laurent Pinchart, Marek Szyprowski, Joerg Roedel From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> If the target device has any suppliers, as reflected by device links to them, __pm_runtime_set_status() does not take them into account, which is not consistent with the other parts of the PM-runtime framework and may lead to programming mistakes. Modify __pm_runtime_set_status() to take suppliers into account by activating them upfront if the new status is RPM_ACTIVE and deactivating them on exit if the new status is RPM_SUSPENDED. If the activation of one of the suppliers fails, the new status will be RPM_SUSPENDED and the (remaining) suppliers will be deactivated on exit (the child count of the device's parent will be dropped too then). Of course, adding device links locking to __pm_runtime_set_status() means that it cannot be run fron interrupt context, so make it use spin_lock_irq() and spin_unlock_irq() instead of spin_lock_irqsave() and spin_unlock_irqrestore(), respectively. Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/base/power/runtime.c | 45 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 5 deletions(-) Index: linux-pm/drivers/base/power/runtime.c =================================================================== --- linux-pm.orig/drivers/base/power/runtime.c +++ linux-pm/drivers/base/power/runtime.c @@ -1102,20 +1102,43 @@ EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_u * and the device parent's counter of unsuspended children is modified to * reflect the new status. If the new status is RPM_SUSPENDED, an idle * notification request for the parent is submitted. + * + * If @dev has any suppliers (as reflected by device links to them), and @status + * is RPM_ACTIVE, they will be activated upfront and if the activation of one + * of them fails, the status of @dev will be changed to RPM_SUSPENDED (instead + * of the @status value) and the suppliers will be deacticated on exit. The + * error returned by the failing supplier activation will be returned in that + * case. */ int __pm_runtime_set_status(struct device *dev, unsigned int status) { struct device *parent = dev->parent; - unsigned long flags; bool notify_parent = false; int error = 0; if (status != RPM_ACTIVE && status != RPM_SUSPENDED) return -EINVAL; - spin_lock_irqsave(&dev->power.lock, flags); + /* + * If the new status is RPM_ACTIVE, the suppliers can be activated + * upfront regardless of the current status, because next time + * rpm_put_suppliers() runs, the rpm_active refcounts of the links + * involved will be dropped down to one anyway. + */ + if (status == RPM_ACTIVE) { + int idx = device_links_read_lock(); + + error = rpm_get_suppliers(dev); + if (error) + status = RPM_SUSPENDED; + + device_links_read_unlock(idx); + } + + spin_lock_irq(&dev->power.lock); if (!dev->power.runtime_error && !dev->power.disable_depth) { + status = dev->power.runtime_status; error = -EAGAIN; goto out; } @@ -1147,19 +1170,31 @@ int __pm_runtime_set_status(struct devic spin_unlock(&parent->power.lock); - if (error) + if (error) { + status = RPM_SUSPENDED; goto out; + } } out_set: __update_runtime_status(dev, status); - dev->power.runtime_error = 0; + if (!error) + dev->power.runtime_error = 0; + out: - spin_unlock_irqrestore(&dev->power.lock, flags); + spin_unlock_irq(&dev->power.lock); if (notify_parent) pm_request_idle(parent); + if (status == RPM_SUSPENDED) { + int idx = device_links_read_lock(); + + rpm_put_suppliers(dev); + + device_links_read_unlock(idx); + } + return error; } EXPORT_SYMBOL_GPL(__pm_runtime_set_status); ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status() 2019-02-07 18:38 ` [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status() Rafael J. Wysocki @ 2019-02-11 13:27 ` Ulf Hansson 2019-02-11 15:50 ` Ulf Hansson 2019-02-11 22:41 ` Rafael J. Wysocki 2019-02-12 16:02 ` Ulf Hansson 1 sibling, 2 replies; 10+ messages in thread From: Ulf Hansson @ 2019-02-11 13:27 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Greg Kroah-Hartman, LKML, Linux PM, Daniel Vetter, Lukas Wunner, Andrzej Hajda, Russell King - ARM Linux, Lucas Stach, Linus Walleij, Thierry Reding, Laurent Pinchart, Marek Szyprowski, Joerg Roedel On Thu, 7 Feb 2019 at 19:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > If the target device has any suppliers, as reflected by device links > to them, __pm_runtime_set_status() does not take them into account, > which is not consistent with the other parts of the PM-runtime > framework and may lead to programming mistakes. > > Modify __pm_runtime_set_status() to take suppliers into account by > activating them upfront if the new status is RPM_ACTIVE and > deactivating them on exit if the new status is RPM_SUSPENDED. > > If the activation of one of the suppliers fails, the new status > will be RPM_SUSPENDED and the (remaining) suppliers will be > deactivated on exit (the child count of the device's parent > will be dropped too then). > > Of course, adding device links locking to __pm_runtime_set_status() > means that it cannot be run fron interrupt context, so make it use > spin_lock_irq() and spin_unlock_irq() instead of spin_lock_irqsave() > and spin_unlock_irqrestore(), respectively. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Rafael, thanks for working on this! I am running some tests at my side, but still not achieving the behavior I expect to. Will let you know when I have more details, but first some comments below. > --- > drivers/base/power/runtime.c | 45 ++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 40 insertions(+), 5 deletions(-) > > Index: linux-pm/drivers/base/power/runtime.c > =================================================================== > --- linux-pm.orig/drivers/base/power/runtime.c > +++ linux-pm/drivers/base/power/runtime.c > @@ -1102,20 +1102,43 @@ EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_u > * and the device parent's counter of unsuspended children is modified to > * reflect the new status. If the new status is RPM_SUSPENDED, an idle > * notification request for the parent is submitted. > + * > + * If @dev has any suppliers (as reflected by device links to them), and @status > + * is RPM_ACTIVE, they will be activated upfront and if the activation of one > + * of them fails, the status of @dev will be changed to RPM_SUSPENDED (instead > + * of the @status value) and the suppliers will be deacticated on exit. The > + * error returned by the failing supplier activation will be returned in that > + * case. > */ > int __pm_runtime_set_status(struct device *dev, unsigned int status) > { > struct device *parent = dev->parent; > - unsigned long flags; > bool notify_parent = false; > int error = 0; > > if (status != RPM_ACTIVE && status != RPM_SUSPENDED) > return -EINVAL; > > - spin_lock_irqsave(&dev->power.lock, flags); > + /* > + * If the new status is RPM_ACTIVE, the suppliers can be activated > + * upfront regardless of the current status, because next time > + * rpm_put_suppliers() runs, the rpm_active refcounts of the links > + * involved will be dropped down to one anyway. > + */ > + if (status == RPM_ACTIVE) { > + int idx = device_links_read_lock(); > + > + error = rpm_get_suppliers(dev); > + if (error) > + status = RPM_SUSPENDED; > + > + device_links_read_unlock(idx); > + } This doesn't look right to me, and more importantly, this isn't consistent with how we treat a parent/child. More precisely, I think you need to check "if (!dev->power.runtime_error && !dev->power.disable_depth)" and also whether "dev->power.runtime_status == status", before deciding to call rpm_get_suppliers() above. Otherwise you may end up resuming suppliers and/or increasing the link->rpm_active count, when you shouldn't. In other words, expecting __pm_runtime_set_status() to be called in "balanced" manner isn't correct. > + > + spin_lock_irq(&dev->power.lock); > > if (!dev->power.runtime_error && !dev->power.disable_depth) { > + status = dev->power.runtime_status; > error = -EAGAIN; > goto out; > } > @@ -1147,19 +1170,31 @@ int __pm_runtime_set_status(struct devic > > spin_unlock(&parent->power.lock); > > - if (error) > + if (error) { > + status = RPM_SUSPENDED; > goto out; > + } > } > > out_set: > __update_runtime_status(dev, status); > - dev->power.runtime_error = 0; > + if (!error) > + dev->power.runtime_error = 0; > + > out: > - spin_unlock_irqrestore(&dev->power.lock, flags); > + spin_unlock_irq(&dev->power.lock); > > if (notify_parent) > pm_request_idle(parent); > > + if (status == RPM_SUSPENDED) { > + int idx = device_links_read_lock(); > + > + rpm_put_suppliers(dev); > + > + device_links_read_unlock(idx); > + } > + > return error; > } > EXPORT_SYMBOL_GPL(__pm_runtime_set_status); > Kind regards Uffe ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status() 2019-02-11 13:27 ` Ulf Hansson @ 2019-02-11 15:50 ` Ulf Hansson 2019-02-11 23:05 ` Rafael J. Wysocki 2019-02-11 22:41 ` Rafael J. Wysocki 1 sibling, 1 reply; 10+ messages in thread From: Ulf Hansson @ 2019-02-11 15:50 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Greg Kroah-Hartman, LKML, Linux PM, Daniel Vetter, Lukas Wunner, Andrzej Hajda, Russell King - ARM Linux, Lucas Stach, Linus Walleij, Thierry Reding, Laurent Pinchart, Marek Szyprowski, Joerg Roedel On Mon, 11 Feb 2019 at 14:27, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 7 Feb 2019 at 19:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > If the target device has any suppliers, as reflected by device links > > to them, __pm_runtime_set_status() does not take them into account, > > which is not consistent with the other parts of the PM-runtime > > framework and may lead to programming mistakes. > > > > Modify __pm_runtime_set_status() to take suppliers into account by > > activating them upfront if the new status is RPM_ACTIVE and > > deactivating them on exit if the new status is RPM_SUSPENDED. > > > > If the activation of one of the suppliers fails, the new status > > will be RPM_SUSPENDED and the (remaining) suppliers will be > > deactivated on exit (the child count of the device's parent > > will be dropped too then). > > > > Of course, adding device links locking to __pm_runtime_set_status() > > means that it cannot be run fron interrupt context, so make it use > > spin_lock_irq() and spin_unlock_irq() instead of spin_lock_irqsave() > > and spin_unlock_irqrestore(), respectively. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Rafael, thanks for working on this! > > I am running some tests at my side, but still not achieving the > behavior I expect to. Will let you know when I have more details, but > first some comments below. Alright, this is what I found. When I call pm_runtime_set_suspended(), in the ->probe() error path of my RPM test driver (I am removing the device link afterwards), then my expectation was that this should allow the supplier to become runtime suspended (sooner or later). This isn't the case, as it turns out the runtime PM usage count of the supplier, still remains 1 after the probe failure. My observation is that with $subject patch, the link->rpm_active count is now reaching 1, before it stayed at 2 - so one step forward. :-) However, the reason to why the runtime PM usage count never reaches 0, is because of the call to pm_runtime_get_noresume(supplier) in device_link_rpm_prepare(), which is called from device_link_add(). To solve the problem, it seems like we need to call pm_runtime_put(supplier), in case the device link is deleted while the consumer is still probing. > > > --- > > drivers/base/power/runtime.c | 45 ++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 40 insertions(+), 5 deletions(-) > > > > Index: linux-pm/drivers/base/power/runtime.c > > =================================================================== > > --- linux-pm.orig/drivers/base/power/runtime.c > > +++ linux-pm/drivers/base/power/runtime.c > > @@ -1102,20 +1102,43 @@ EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_u > > * and the device parent's counter of unsuspended children is modified to > > * reflect the new status. If the new status is RPM_SUSPENDED, an idle > > * notification request for the parent is submitted. > > + * > > + * If @dev has any suppliers (as reflected by device links to them), and @status > > + * is RPM_ACTIVE, they will be activated upfront and if the activation of one > > + * of them fails, the status of @dev will be changed to RPM_SUSPENDED (instead > > + * of the @status value) and the suppliers will be deacticated on exit. The > > + * error returned by the failing supplier activation will be returned in that > > + * case. > > */ > > int __pm_runtime_set_status(struct device *dev, unsigned int status) > > { > > struct device *parent = dev->parent; > > - unsigned long flags; > > bool notify_parent = false; > > int error = 0; > > > > if (status != RPM_ACTIVE && status != RPM_SUSPENDED) > > return -EINVAL; > > > > - spin_lock_irqsave(&dev->power.lock, flags); > > + /* > > + * If the new status is RPM_ACTIVE, the suppliers can be activated > > + * upfront regardless of the current status, because next time > > + * rpm_put_suppliers() runs, the rpm_active refcounts of the links > > + * involved will be dropped down to one anyway. > > + */ > > + if (status == RPM_ACTIVE) { > > + int idx = device_links_read_lock(); > > + > > + error = rpm_get_suppliers(dev); > > + if (error) > > + status = RPM_SUSPENDED; > > + > > + device_links_read_unlock(idx); > > + } > > This doesn't look right to me, and more importantly, this isn't > consistent with how we treat a parent/child. > > More precisely, I think you need to check "if > (!dev->power.runtime_error && !dev->power.disable_depth)" and also > whether "dev->power.runtime_status == status", before deciding to call > rpm_get_suppliers() above. Otherwise you may end up resuming suppliers > and/or increasing the link->rpm_active count, when you shouldn't. > > In other words, expecting __pm_runtime_set_status() to be called in > "balanced" manner isn't correct. > > > + > > + spin_lock_irq(&dev->power.lock); > > > > if (!dev->power.runtime_error && !dev->power.disable_depth) { > > + status = dev->power.runtime_status; > > error = -EAGAIN; > > goto out; > > } > > @@ -1147,19 +1170,31 @@ int __pm_runtime_set_status(struct devic > > > > spin_unlock(&parent->power.lock); > > > > - if (error) > > + if (error) { > > + status = RPM_SUSPENDED; > > goto out; > > + } > > } > > > > out_set: > > __update_runtime_status(dev, status); > > - dev->power.runtime_error = 0; > > + if (!error) > > + dev->power.runtime_error = 0; > > + > > out: > > - spin_unlock_irqrestore(&dev->power.lock, flags); > > + spin_unlock_irq(&dev->power.lock); > > > > if (notify_parent) > > pm_request_idle(parent); > > > > + if (status == RPM_SUSPENDED) { > > + int idx = device_links_read_lock(); > > + > > + rpm_put_suppliers(dev); > > + > > + device_links_read_unlock(idx); > > + } > > + > > return error; > > } > > EXPORT_SYMBOL_GPL(__pm_runtime_set_status); > > > > Kind regards > Uffe ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status() 2019-02-11 15:50 ` Ulf Hansson @ 2019-02-11 23:05 ` Rafael J. Wysocki 2019-02-12 8:03 ` Ulf Hansson 0 siblings, 1 reply; 10+ messages in thread From: Rafael J. Wysocki @ 2019-02-11 23:05 UTC (permalink / raw) To: Ulf Hansson Cc: Rafael J. Wysocki, Greg Kroah-Hartman, LKML, Linux PM, Daniel Vetter, Lukas Wunner, Andrzej Hajda, Russell King - ARM Linux, Lucas Stach, Linus Walleij, Thierry Reding, Laurent Pinchart, Marek Szyprowski, Joerg Roedel On Mon, Feb 11, 2019 at 4:51 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Mon, 11 Feb 2019 at 14:27, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Thu, 7 Feb 2019 at 19:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > If the target device has any suppliers, as reflected by device links > > > to them, __pm_runtime_set_status() does not take them into account, > > > which is not consistent with the other parts of the PM-runtime > > > framework and may lead to programming mistakes. > > > > > > Modify __pm_runtime_set_status() to take suppliers into account by > > > activating them upfront if the new status is RPM_ACTIVE and > > > deactivating them on exit if the new status is RPM_SUSPENDED. > > > > > > If the activation of one of the suppliers fails, the new status > > > will be RPM_SUSPENDED and the (remaining) suppliers will be > > > deactivated on exit (the child count of the device's parent > > > will be dropped too then). > > > > > > Of course, adding device links locking to __pm_runtime_set_status() > > > means that it cannot be run fron interrupt context, so make it use > > > spin_lock_irq() and spin_unlock_irq() instead of spin_lock_irqsave() > > > and spin_unlock_irqrestore(), respectively. > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Rafael, thanks for working on this! > > > > I am running some tests at my side, but still not achieving the > > behavior I expect to. Will let you know when I have more details, but > > first some comments below. > > Alright, this is what I found. > > When I call pm_runtime_set_suspended(), in the ->probe() error path of > my RPM test driver (I am removing the device link afterwards), then my > expectation was that this should allow the supplier to become runtime > suspended (sooner or later). This isn't the case, as it turns out the > runtime PM usage count of the supplier, still remains 1 after the > probe failure. > > My observation is that with $subject patch, the link->rpm_active count > is now reaching 1, before it stayed at 2 - so one step forward. :-) > > However, the reason to why the runtime PM usage count never reaches 0, > is because of the call to pm_runtime_get_noresume(supplier) in > device_link_rpm_prepare(), which is called from device_link_add(). That was there previously, I've just moved it to device_link_rpm_prepare(). But good catch! > To solve the problem, it seems like we need to call > pm_runtime_put(supplier), in case the device link is deleted while the > consumer is still probing. I'd rather change the way pm_runtime_get/put_suppliers() work, so that they use the rpm_active refcount, but pm_runtime_put_suppliers() only drops it by one - unless it is one already. Then, when adding a new link with DL_FLAG_RPM_ACTIVE, device_link_add() only needs to increment its rpm_active *twice* (instead of doing that once as to does now), so it will stay above one after the subsequent pm_runtime_put_suppliers() - and if it goes away in the meantime, then it will be cleaned up by the removal. In turn, if a link is created without DL_FLAG_RPM_ACTIVE, its rpm_active is one and then pm_runtime_put_suppliers() will just skip it. A patch will follow. :-) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status() 2019-02-11 23:05 ` Rafael J. Wysocki @ 2019-02-12 8:03 ` Ulf Hansson 0 siblings, 0 replies; 10+ messages in thread From: Ulf Hansson @ 2019-02-12 8:03 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Rafael J. Wysocki, Greg Kroah-Hartman, LKML, Linux PM, Daniel Vetter, Lukas Wunner, Andrzej Hajda, Russell King - ARM Linux, Lucas Stach, Linus Walleij, Thierry Reding, Laurent Pinchart, Marek Szyprowski, Joerg Roedel On Tue, 12 Feb 2019 at 00:05, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Mon, Feb 11, 2019 at 4:51 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Mon, 11 Feb 2019 at 14:27, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > > > On Thu, 7 Feb 2019 at 19:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > If the target device has any suppliers, as reflected by device links > > > > to them, __pm_runtime_set_status() does not take them into account, > > > > which is not consistent with the other parts of the PM-runtime > > > > framework and may lead to programming mistakes. > > > > > > > > Modify __pm_runtime_set_status() to take suppliers into account by > > > > activating them upfront if the new status is RPM_ACTIVE and > > > > deactivating them on exit if the new status is RPM_SUSPENDED. > > > > > > > > If the activation of one of the suppliers fails, the new status > > > > will be RPM_SUSPENDED and the (remaining) suppliers will be > > > > deactivated on exit (the child count of the device's parent > > > > will be dropped too then). > > > > > > > > Of course, adding device links locking to __pm_runtime_set_status() > > > > means that it cannot be run fron interrupt context, so make it use > > > > spin_lock_irq() and spin_unlock_irq() instead of spin_lock_irqsave() > > > > and spin_unlock_irqrestore(), respectively. > > > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Rafael, thanks for working on this! > > > > > > I am running some tests at my side, but still not achieving the > > > behavior I expect to. Will let you know when I have more details, but > > > first some comments below. > > > > Alright, this is what I found. > > > > When I call pm_runtime_set_suspended(), in the ->probe() error path of > > my RPM test driver (I am removing the device link afterwards), then my > > expectation was that this should allow the supplier to become runtime > > suspended (sooner or later). This isn't the case, as it turns out the > > runtime PM usage count of the supplier, still remains 1 after the > > probe failure. > > > > My observation is that with $subject patch, the link->rpm_active count > > is now reaching 1, before it stayed at 2 - so one step forward. :-) > > > > However, the reason to why the runtime PM usage count never reaches 0, > > is because of the call to pm_runtime_get_noresume(supplier) in > > device_link_rpm_prepare(), which is called from device_link_add(). > > That was there previously, I've just moved it to device_link_rpm_prepare(). Correct. The problem been there before. Even without using DL_FLAG_RPM_ACTIVE. > > But good catch! > > > To solve the problem, it seems like we need to call > > pm_runtime_put(supplier), in case the device link is deleted while the > > consumer is still probing. > > I'd rather change the way pm_runtime_get/put_suppliers() work, so that > they use the rpm_active refcount, but pm_runtime_put_suppliers() only > drops it by one - unless it is one already. That seems like a very reasonable approach! The mix between calling pm_runtime_get/put*() on the supplier device directly vs using the path with the rpm_active count, is to me rather confusing. Using only the latter, would be a nice cleanup anyway, I think. > > Then, when adding a new link with DL_FLAG_RPM_ACTIVE, > device_link_add() only needs to increment its rpm_active *twice* > (instead of doing that once as to does now), so it will stay above one > after the subsequent pm_runtime_put_suppliers() - and if it goes away > in the meantime, then it will be cleaned up by the removal. Assuming you will add a check for "consumer->links.status == DL_DEV_PROBING" to understand if rpm_active should by be decreased. Yes, it seems reasonable. > > In turn, if a link is created without DL_FLAG_RPM_ACTIVE, its > rpm_active is one and then pm_runtime_put_suppliers() will just skip > it. > > A patch will follow. :-) Great, I am here to review it. :-) Kind regards Uffe ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status() 2019-02-11 13:27 ` Ulf Hansson 2019-02-11 15:50 ` Ulf Hansson @ 2019-02-11 22:41 ` Rafael J. Wysocki 2019-02-12 8:25 ` Ulf Hansson 1 sibling, 1 reply; 10+ messages in thread From: Rafael J. Wysocki @ 2019-02-11 22:41 UTC (permalink / raw) To: Ulf Hansson Cc: Rafael J. Wysocki, Greg Kroah-Hartman, LKML, Linux PM, Daniel Vetter, Lukas Wunner, Andrzej Hajda, Russell King - ARM Linux, Lucas Stach, Linus Walleij, Thierry Reding, Laurent Pinchart, Marek Szyprowski, Joerg Roedel On Mon, Feb 11, 2019 at 2:28 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Thu, 7 Feb 2019 at 19:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > If the target device has any suppliers, as reflected by device links > > to them, __pm_runtime_set_status() does not take them into account, > > which is not consistent with the other parts of the PM-runtime > > framework and may lead to programming mistakes. > > > > Modify __pm_runtime_set_status() to take suppliers into account by > > activating them upfront if the new status is RPM_ACTIVE and > > deactivating them on exit if the new status is RPM_SUSPENDED. > > > > If the activation of one of the suppliers fails, the new status > > will be RPM_SUSPENDED and the (remaining) suppliers will be > > deactivated on exit (the child count of the device's parent > > will be dropped too then). > > > > Of course, adding device links locking to __pm_runtime_set_status() > > means that it cannot be run fron interrupt context, so make it use > > spin_lock_irq() and spin_unlock_irq() instead of spin_lock_irqsave() > > and spin_unlock_irqrestore(), respectively. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Rafael, thanks for working on this! > > I am running some tests at my side, but still not achieving the > behavior I expect to. Will let you know when I have more details, but > first some comments below. > > > --- > > drivers/base/power/runtime.c | 45 ++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 40 insertions(+), 5 deletions(-) > > > > Index: linux-pm/drivers/base/power/runtime.c > > =================================================================== > > --- linux-pm.orig/drivers/base/power/runtime.c > > +++ linux-pm/drivers/base/power/runtime.c > > @@ -1102,20 +1102,43 @@ EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_u > > * and the device parent's counter of unsuspended children is modified to > > * reflect the new status. If the new status is RPM_SUSPENDED, an idle > > * notification request for the parent is submitted. > > + * > > + * If @dev has any suppliers (as reflected by device links to them), and @status > > + * is RPM_ACTIVE, they will be activated upfront and if the activation of one > > + * of them fails, the status of @dev will be changed to RPM_SUSPENDED (instead > > + * of the @status value) and the suppliers will be deacticated on exit. The > > + * error returned by the failing supplier activation will be returned in that > > + * case. > > */ > > int __pm_runtime_set_status(struct device *dev, unsigned int status) > > { > > struct device *parent = dev->parent; > > - unsigned long flags; > > bool notify_parent = false; > > int error = 0; > > > > if (status != RPM_ACTIVE && status != RPM_SUSPENDED) > > return -EINVAL; > > > > - spin_lock_irqsave(&dev->power.lock, flags); > > + /* > > + * If the new status is RPM_ACTIVE, the suppliers can be activated > > + * upfront regardless of the current status, because next time > > + * rpm_put_suppliers() runs, the rpm_active refcounts of the links > > + * involved will be dropped down to one anyway. > > + */ > > + if (status == RPM_ACTIVE) { > > + int idx = device_links_read_lock(); > > + > > + error = rpm_get_suppliers(dev); > > + if (error) > > + status = RPM_SUSPENDED; > > + > > + device_links_read_unlock(idx); > > + } > > This doesn't look right to me, and more importantly, this isn't > consistent with how we treat a parent/child. It cannot be entirely consistent with that, because you cannot walk the suppliers under the device's power.lock. The idea here is that activating suppliers upfront if the new status is RPM_ACTIVE shouldn't hurt regardless. > More precisely, I think you need to check "if > (!dev->power.runtime_error && !dev->power.disable_depth)" and also > whether "dev->power.runtime_status == status", before deciding to call > rpm_get_suppliers() above. Otherwise you may end up resuming suppliers > and/or increasing the link->rpm_active count, when you shouldn't. Resuming suppliers unnecessarily is not particularly efficient, but it is not incorrect. Incrementing their rpm_active temporarily also isn't incorrect as long as the rpm_active values are correct on exit (and note that incementing them if the consumer's status is RPM_ACTIVE doesn't even matter). > In other words, expecting __pm_runtime_set_status() to be called in > "balanced" manner isn't correct. There is no such expectation here. There is a possible race between __pm_runtime_set_status() and runtime suspend or resume of the device in case PM-runtime is enabled for it when __pm_runtime_set_status() is called, but it shouldn't occur if __pm_runtime_set_status() is used correctly (that is, when PM-runtime is disabled for the device). I think I know how to avoid that race, though, so I'm going to post an incremental fix if that works out. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status() 2019-02-11 22:41 ` Rafael J. Wysocki @ 2019-02-12 8:25 ` Ulf Hansson 0 siblings, 0 replies; 10+ messages in thread From: Ulf Hansson @ 2019-02-12 8:25 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Rafael J. Wysocki, Greg Kroah-Hartman, LKML, Linux PM, Daniel Vetter, Lukas Wunner, Andrzej Hajda, Russell King - ARM Linux, Lucas Stach, Linus Walleij, Thierry Reding, Laurent Pinchart, Marek Szyprowski, Joerg Roedel On Mon, 11 Feb 2019 at 23:41, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Mon, Feb 11, 2019 at 2:28 PM Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > > On Thu, 7 Feb 2019 at 19:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > If the target device has any suppliers, as reflected by device links > > > to them, __pm_runtime_set_status() does not take them into account, > > > which is not consistent with the other parts of the PM-runtime > > > framework and may lead to programming mistakes. > > > > > > Modify __pm_runtime_set_status() to take suppliers into account by > > > activating them upfront if the new status is RPM_ACTIVE and > > > deactivating them on exit if the new status is RPM_SUSPENDED. > > > > > > If the activation of one of the suppliers fails, the new status > > > will be RPM_SUSPENDED and the (remaining) suppliers will be > > > deactivated on exit (the child count of the device's parent > > > will be dropped too then). > > > > > > Of course, adding device links locking to __pm_runtime_set_status() > > > means that it cannot be run fron interrupt context, so make it use > > > spin_lock_irq() and spin_unlock_irq() instead of spin_lock_irqsave() > > > and spin_unlock_irqrestore(), respectively. > > > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Rafael, thanks for working on this! > > > > I am running some tests at my side, but still not achieving the > > behavior I expect to. Will let you know when I have more details, but > > first some comments below. > > > > > --- > > > drivers/base/power/runtime.c | 45 ++++++++++++++++++++++++++++++++++++++----- > > > 1 file changed, 40 insertions(+), 5 deletions(-) > > > > > > Index: linux-pm/drivers/base/power/runtime.c > > > =================================================================== > > > --- linux-pm.orig/drivers/base/power/runtime.c > > > +++ linux-pm/drivers/base/power/runtime.c > > > @@ -1102,20 +1102,43 @@ EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_u > > > * and the device parent's counter of unsuspended children is modified to > > > * reflect the new status. If the new status is RPM_SUSPENDED, an idle > > > * notification request for the parent is submitted. > > > + * > > > + * If @dev has any suppliers (as reflected by device links to them), and @status > > > + * is RPM_ACTIVE, they will be activated upfront and if the activation of one > > > + * of them fails, the status of @dev will be changed to RPM_SUSPENDED (instead > > > + * of the @status value) and the suppliers will be deacticated on exit. The > > > + * error returned by the failing supplier activation will be returned in that > > > + * case. > > > */ > > > int __pm_runtime_set_status(struct device *dev, unsigned int status) > > > { > > > struct device *parent = dev->parent; > > > - unsigned long flags; > > > bool notify_parent = false; > > > int error = 0; > > > > > > if (status != RPM_ACTIVE && status != RPM_SUSPENDED) > > > return -EINVAL; > > > > > > - spin_lock_irqsave(&dev->power.lock, flags); > > > + /* > > > + * If the new status is RPM_ACTIVE, the suppliers can be activated > > > + * upfront regardless of the current status, because next time > > > + * rpm_put_suppliers() runs, the rpm_active refcounts of the links > > > + * involved will be dropped down to one anyway. > > > + */ > > > + if (status == RPM_ACTIVE) { > > > + int idx = device_links_read_lock(); > > > + > > > + error = rpm_get_suppliers(dev); > > > + if (error) > > > + status = RPM_SUSPENDED; > > > + > > > + device_links_read_unlock(idx); > > > + } > > > > This doesn't look right to me, and more importantly, this isn't > > consistent with how we treat a parent/child. > > It cannot be entirely consistent with that, because you cannot walk > the suppliers under the device's power.lock. > > The idea here is that activating suppliers upfront if the new status > is RPM_ACTIVE shouldn't hurt regardless. I see. However, perhaps we can just read out the needed flags/states (within device's power.lock) before walking the suppliers. In principle, those flags/states shouldn't really change, in case runtime PM have been properly disabled by the caller. > > > More precisely, I think you need to check "if > > (!dev->power.runtime_error && !dev->power.disable_depth)" and also > > whether "dev->power.runtime_status == status", before deciding to call > > rpm_get_suppliers() above. Otherwise you may end up resuming suppliers > > and/or increasing the link->rpm_active count, when you shouldn't. > > Resuming suppliers unnecessarily is not particularly efficient, but it > is not incorrect. Incrementing their rpm_active temporarily also > isn't incorrect as long as the rpm_active values are correct on exit > (and note that incementing them if the consumer's status is RPM_ACTIVE > doesn't even matter). > > > In other words, expecting __pm_runtime_set_status() to be called in > > "balanced" manner isn't correct. > > There is no such expectation here. You are right! I didn't realize that rpm_put_suppliers() actually doesn't drop the usage count only by one, but instead as many times as needed to let rpm_active reach one. > > There is a possible race between __pm_runtime_set_status() and runtime > suspend or resume of the device in case PM-runtime is enabled for it > when __pm_runtime_set_status() is called, but it shouldn't occur if > __pm_runtime_set_status() is used correctly (that is, when PM-runtime > is disabled for the device). > > I think I know how to avoid that race, though, so I'm going to post an > incremental fix if that works out. Okay, let's see what comes out of this. Kind regards Uffe ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status() 2019-02-07 18:38 ` [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status() Rafael J. Wysocki 2019-02-11 13:27 ` Ulf Hansson @ 2019-02-12 16:02 ` Ulf Hansson 1 sibling, 0 replies; 10+ messages in thread From: Ulf Hansson @ 2019-02-12 16:02 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Greg Kroah-Hartman, LKML, Linux PM, Daniel Vetter, Lukas Wunner, Andrzej Hajda, Russell King - ARM Linux, Lucas Stach, Linus Walleij, Thierry Reding, Laurent Pinchart, Marek Szyprowski, Joerg Roedel On Thu, 7 Feb 2019 at 19:46, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > If the target device has any suppliers, as reflected by device links > to them, __pm_runtime_set_status() does not take them into account, > which is not consistent with the other parts of the PM-runtime > framework and may lead to programming mistakes. > > Modify __pm_runtime_set_status() to take suppliers into account by > activating them upfront if the new status is RPM_ACTIVE and > deactivating them on exit if the new status is RPM_SUSPENDED. > > If the activation of one of the suppliers fails, the new status > will be RPM_SUSPENDED and the (remaining) suppliers will be > deactivated on exit (the child count of the device's parent > will be dropped too then). > > Of course, adding device links locking to __pm_runtime_set_status() > means that it cannot be run fron interrupt context, so make it use > spin_lock_irq() and spin_unlock_irq() instead of spin_lock_irqsave() > and spin_unlock_irqrestore(), respectively. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Tested-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/base/power/runtime.c | 45 ++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 40 insertions(+), 5 deletions(-) > > Index: linux-pm/drivers/base/power/runtime.c > =================================================================== > --- linux-pm.orig/drivers/base/power/runtime.c > +++ linux-pm/drivers/base/power/runtime.c > @@ -1102,20 +1102,43 @@ EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_u > * and the device parent's counter of unsuspended children is modified to > * reflect the new status. If the new status is RPM_SUSPENDED, an idle > * notification request for the parent is submitted. > + * > + * If @dev has any suppliers (as reflected by device links to them), and @status > + * is RPM_ACTIVE, they will be activated upfront and if the activation of one > + * of them fails, the status of @dev will be changed to RPM_SUSPENDED (instead > + * of the @status value) and the suppliers will be deacticated on exit. The > + * error returned by the failing supplier activation will be returned in that > + * case. > */ > int __pm_runtime_set_status(struct device *dev, unsigned int status) > { > struct device *parent = dev->parent; > - unsigned long flags; > bool notify_parent = false; > int error = 0; > > if (status != RPM_ACTIVE && status != RPM_SUSPENDED) > return -EINVAL; > > - spin_lock_irqsave(&dev->power.lock, flags); > + /* > + * If the new status is RPM_ACTIVE, the suppliers can be activated > + * upfront regardless of the current status, because next time > + * rpm_put_suppliers() runs, the rpm_active refcounts of the links > + * involved will be dropped down to one anyway. > + */ > + if (status == RPM_ACTIVE) { > + int idx = device_links_read_lock(); > + > + error = rpm_get_suppliers(dev); > + if (error) > + status = RPM_SUSPENDED; > + > + device_links_read_unlock(idx); > + } > + > + spin_lock_irq(&dev->power.lock); > > if (!dev->power.runtime_error && !dev->power.disable_depth) { > + status = dev->power.runtime_status; > error = -EAGAIN; > goto out; > } > @@ -1147,19 +1170,31 @@ int __pm_runtime_set_status(struct devic > > spin_unlock(&parent->power.lock); > > - if (error) > + if (error) { > + status = RPM_SUSPENDED; > goto out; > + } > } > > out_set: > __update_runtime_status(dev, status); > - dev->power.runtime_error = 0; > + if (!error) > + dev->power.runtime_error = 0; > + > out: > - spin_unlock_irqrestore(&dev->power.lock, flags); > + spin_unlock_irq(&dev->power.lock); > > if (notify_parent) > pm_request_idle(parent); > > + if (status == RPM_SUSPENDED) { > + int idx = device_links_read_lock(); > + > + rpm_put_suppliers(dev); > + > + device_links_read_unlock(idx); > + } > + > return error; > } > EXPORT_SYMBOL_GPL(__pm_runtime_set_status); > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] driver core: Document limitation related to DL_FLAG_RPM_ACTIVE 2019-02-07 18:35 [PATCH 0/2] driver core: Two more updates related to device links Rafael J. Wysocki 2019-02-07 18:38 ` [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status() Rafael J. Wysocki @ 2019-02-07 18:41 ` Rafael J. Wysocki 1 sibling, 0 replies; 10+ messages in thread From: Rafael J. Wysocki @ 2019-02-07 18:41 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: LKML, Linux PM, Ulf Hansson, Daniel Vetter, Lukas Wunner, Andrzej Hajda, Russell King - ARM Linux, Lucas Stach, Linus Walleij, Thierry Reding, Laurent Pinchart, Marek Szyprowski, Joerg Roedel From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> If device_link_add() is called twice in a row to create a stateless device link for the same consumer-supplier pair without an attempt to delete the link between these calls, and the second caller passes DL_FLAG_RPM_ACTIVE to it in flags, calling either device_link_del() or device_link_remove() immediately after that will leave the link's supplier device with nonzero PM-runtime usage counter, which may prevent the supplier from being runtime-suspended going forward until the link is deleted by another invocation of device_link_del() or device_link_remove() for it. Even though this is confusing and may lead to subtle issues, trying to avoid it in the framework also may cause problems to appear, so document it as a known limitation. Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- Documentation/driver-api/device_link.rst | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) Index: linux-pm/Documentation/driver-api/device_link.rst =================================================================== --- linux-pm.orig/Documentation/driver-api/device_link.rst +++ linux-pm/Documentation/driver-api/device_link.rst @@ -86,9 +86,10 @@ integration is desired. Two other flags are specifically targeted at use cases where the device link is added from the consumer's ``->probe`` callback: ``DL_FLAG_RPM_ACTIVE`` -can be specified to runtime resume the supplier upon addition of the -device link. ``DL_FLAG_AUTOREMOVE_CONSUMER`` causes the device link to be -automatically purged when the consumer fails to probe or later unbinds. +can be specified to runtime resume the supplier and prevent it from suspending +before the consumer is runtime suspended. ``DL_FLAG_AUTOREMOVE_CONSUMER`` +causes the device link to be automatically purged when the consumer fails to +probe or later unbinds. Similarly, when the device link is added from supplier's ``->probe`` callback, ``DL_FLAG_AUTOREMOVE_SUPPLIER`` causes the device link to be automatically @@ -121,6 +122,20 @@ set) are expected to be removed by whoev to add them with the help of either :c:func:`device_link_del()` or :c:func:`device_link_remove()`. +Passing ``DL_FLAG_RPM_ACTIVE`` along with ``DL_FLAG_STATELESS`` to +:c:func:`device_link_add()` may cause the PM-runtime usage counter of the +supplier device to remain nonzero after a subsequent invocation of either +:c:func:`device_link_del()` or :c:func:`device_link_remove()` to remove the +device link returned by it. This happens if :c:func:`device_link_add()` is +called twice in a row for the same consumer-supplier pair without removing the +link between these calls, in which case allowing the PM-runtime usage counter +of the supplier to drop on an attempt to remove the link may cause it to be +suspended while the consumer is still PM-runtime-active and that has to be +avoided. [To work around this limitation it is sufficient to let the consumer +runtime suspend at least once, or call :c:func:`pm_runtime_set_suspended()` for +it with PM-runtime disabled, between the :c:func:`device_link_add()` and +:c:func:`device_link_del()` or :c:func:`device_link_remove()` calls.] + Sometimes drivers depend on optional resources. They are able to operate in a degraded mode (reduced feature set or performance) when those resources are not present. An example is an SPI controller that can use a DMA engine ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-02-12 16:03 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-07 18:35 [PATCH 0/2] driver core: Two more updates related to device links Rafael J. Wysocki 2019-02-07 18:38 ` [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status() Rafael J. Wysocki 2019-02-11 13:27 ` Ulf Hansson 2019-02-11 15:50 ` Ulf Hansson 2019-02-11 23:05 ` Rafael J. Wysocki 2019-02-12 8:03 ` Ulf Hansson 2019-02-11 22:41 ` Rafael J. Wysocki 2019-02-12 8:25 ` Ulf Hansson 2019-02-12 16:02 ` Ulf Hansson 2019-02-07 18:41 ` [PATCH 2/2] driver core: Document limitation related to DL_FLAG_RPM_ACTIVE Rafael J. Wysocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).