[PATCH 0/2] driver core: Two more updates related to device links

[PATCH 0/2] driver core: Two more updates related to device links

[Rafael J. Wysocki]

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

[PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status()

[Rafael J. Wysocki]

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

[PATCH 2/2] driver core: Document limitation related to DL_FLAG_RPM_ACTIVE

[Rafael J. Wysocki]

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

Re: [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status()

[Ulf Hansson]

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

Re: [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status()

[Ulf Hansson]

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

Re: [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status()

[Rafael J. Wysocki]

 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.

Re: [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status()

[Rafael J. Wysocki]

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

Re: [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status()

[Ulf Hansson]

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

Re: [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status()

[Ulf Hansson]

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

Re: [PATCH 1/2] PM-runtime: Take suppliers into account in __pm_runtime_set_status()

[Ulf Hansson]

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