Re: [PATCH v1] PM: runtime: Update device status before letting suppliers suspend
From: Rafael J. Wysocki
Date: Thu Feb 25 2021 - 07:07:40 EST
On Thu, Feb 25, 2021 at 2:32 AM elaine.zhang <zhangqing@xxxxxxxxxxxxxx> wrote:
>
>
Hi, Rafael:
>
>
在 2021年2月25日 上午1:53, Rafael J. Wysocki 写道:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
>
>
> Because the PM-runtime status of the device is not updated in
>
> __rpm_callback(), attempts to suspend the suppliers of the given
>
> device triggered by rpm_put_suppliers() called by it may fail.
>
>
>
> Fix this by making __rpm_callback() update the device's status to
>
> RPM_SUSPENDED before calling rpm_put_suppliers() if the current
>
> status of the device is RPM_SUSPENDING and the callback just invoked
>
> by it has returned 0 (success).
>
>
>
> While at it, modify the code in __rpm_callback() to always check
>
> the device's PM-runtime status under its PM lock.
>
>
>
> Link: https://lore.kernel.org/linux-pm/CAPDyKFqm06KDw_p8WXsM4dijDbho4bb6T4k50UqqvR1_COsp8g@xxxxxxxxxxxxxx/
>
> Fixes: 21d5c57b3726 ("PM / runtime: Use device links")
>
> Reported-by: elaine.zhang <zhangqing@xxxxxxxxxxxxxx>
>
> Diagnosed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> ---
>
>
>
> This is different from the previously posted tentative patch, please retest.
>
>
>
> ---
>
> drivers/base/power/runtime.c | 61 +++++++++++++++++++++++++------------------
>
> 1 file changed, 36 insertions(+), 25 deletions(-)
>
>
>
> Index: linux-pm/drivers/base/power/runtime.c
>
> ===================================================================
>
> --- linux-pm.orig/drivers/base/power/runtime.c
>
> +++ linux-pm/drivers/base/power/runtime.c
>
> @@ -325,22 +325,21 @@ static void rpm_put_suppliers(struct dev
>
> static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
>
> __releases(&dev->power.lock) __acquires(&dev->power.lock)
>
> {
>
> - int retval, idx;
>
> bool use_links = dev->power.links_count > 0;
>
> + int retval, idx;
>
> + bool get, put;
>
>
>
> if (dev->power.irq_safe) {
>
> spin_unlock(&dev->power.lock);
>
> + } else if (!use_links) {
>
> + spin_unlock_irq(&dev->power.lock);
>
> } else {
>
> + get = dev->power.runtime_status == RPM_RESUMING;
>
> +
>
> spin_unlock_irq(&dev->power.lock);
>
>
>
> - /*
>
> - * Resume suppliers if necessary.
>
> - *
>
> - * The device's runtime PM status cannot change until this
>
> - * routine returns, so it is safe to read the status outside of
>
> - * the lock.
>
> - */
>
> - if (use_links && dev->power.runtime_status == RPM_RESUMING) {
>
> + /* Resume suppliers if necessary. */
>
> + if (get) {
>
> idx = device_links_read_lock();
>
>
>
> retval = rpm_get_suppliers(dev);
>
> @@ -355,24 +354,36 @@ static int __rpm_callback(int (*cb)(stru
>
>
>
> if (dev->power.irq_safe) {
>
> spin_lock(&dev->power.lock);
>
> - } else {
>
> - /*
>
> - * If the device is suspending and the callback has returned
>
> - * success, drop the usage counters of the suppliers that have
>
> - * been reference counted on its resume.
>
> - *
>
> - * Do that if resume fails too.
>
> - */
>
> - if (use_links
>
> - && ((dev->power.runtime_status == RPM_SUSPENDING && !retval)
>
> - || (dev->power.runtime_status == RPM_RESUMING && retval))) {
>
> - idx = device_links_read_lock();
>
> + return retval;
>
> + }
>
>
>
> - fail:
>
> - rpm_put_suppliers(dev);
>
> + spin_lock_irq(&dev->power.lock);
>
>
>
> - device_links_read_unlock(idx);
>
> - }
>
> + if (!use_links)
>
> + return retval;
>
> +
>
> + /*
>
> + * If the device is suspending and the callback has returned success,
>
> + * drop the usage counters of the suppliers that have been reference
>
> + * counted on its resume.
>
> + *
>
> + * Do that if the resume fails too.
>
> + */
>
> + put = dev->power.runtime_status == RPM_SUSPENDING && !retval;
>
> + if (put)
>
> + __update_runtime_status(dev, RPM_SUSPENDED);
>
> + else
>
> + put = get && retval;
>
> +
>
> + if (put) {
>
> + spin_unlock_irq(&dev->power.lock);
>
> +
>
> + idx = device_links_read_lock();
>
> +
>
> +fail:
>
> + rpm_put_suppliers(dev);
>
> +
>
> + device_links_read_unlock(idx);
>
>
>
> spin_lock_irq(&dev->power.lock);
>
> }
>
drivers/base/power/runtime.c: In function '__rpm_callback':
>
drivers/base/power/runtime.c:355:13: warning: 'get' may be used
>
uninitialized in this function [-Wmaybe-uninitialized]
>
error, forbidden warning:runtime.c:355
This really is a false-positive, because in fact the "get" variable
cannot be uninitialized at this point.
>
put = get && retval;
>
>
There is a compilation error. I change it as:
>
>
put = dev->power.runtime_status == RPM_SUSPENDING && retval;
I prefer to initialize "get" to false (even though that is not
strictly necessary).
>
And test works well.Please check it.
OK, thanks for testing!
I will send an updated patch later today.