From: Rafael J. Wysocki <[email protected]>
It has been reported that system-wide suspend may be aborted in the
absence of any wakeup events due to unforseen interactions of it with
the runtume PM framework.
One failing scenario is when there are multiple devices sharing an
ACPI power resource and runtime-resume needs to be carried out for
one of them during system-wide suspend (for example, because it needs
to be reconfigured before the whole system goes to sleep). In that
case, the runtime-resume of that device involves turning the ACPI
power resource "on" which in turn causes runtime-resume requests
to be queued up for all of the other devices sharing it. Those
requests go to the runtime PM workqueue which is frozen during
system-wide suspend, so they are not actually taken care of until
the resume of the whole system, but the pm_runtime_barrier()
call in __device_suspend() sees them and triggers system wakeup
events for them which then cause the system-wide suspend to be
aborted if wakeup source objects are in active use.
Of course, the logic that leads to triggering those wakeup events is
questionable in the first place, because clearly there are cases in
which a pending runtime resume request for a device is not connected
to any real wakeup events in any way (like the one above). Moreover,
it is racy, because the device may be resuming already by the time
the pm_runtime_barrier() runs and so if the driver doesn't take care
of signaling the wakeup event as appropriate, it will be lost.
However, if the driver does take care of that, the extra
pm_wakeup_event() call in the core is redundant.
Accordingly, drop the conditional pm_wakeup_event() call fron
__device_suspend() and make the latter call pm_runtime_barrier()
alone. Also modify the comment next to that call to reflect the new
code and extend it to mention the need to avoid unwanted interactions
between runtime PM and system-wide device suspend callbacks.
Fixes: 1e2ef05bb8cf8 ("PM: Limit race conditions between runtime PM and system sleep (v2)")
Reported-by: Mika Westerberg <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
-> v2:
* Do not call pm_runtime_resume() if pm_runtime_barrier() returns 1,
because the device have been resumed by it already.
* Extend the comment next to the pm_runtime_barrier() call.
* Update the changelog in accordance with the above.
---
drivers/base/power/main.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -1606,13 +1606,17 @@ static int __device_suspend(struct devic
}
/*
- * If a device configured to wake up the system from sleep states
- * has been suspended at run time and there's a resume request pending
- * for it, this is equivalent to the device signaling wakeup, so the
- * system suspend operation should be aborted.
+ * Wait for possible runtime PM transitions of the device in progress
+ * to complete and if there's a runtime resume request pending for it,
+ * resume it before proceeding with invoking the system-wide suspend
+ * callbacks for it.
+ *
+ * If the system-wide suspend callbacks below change the configuration
+ * of the device, they must disable runtime PM for it or otherwise
+ * ensure that its runtime-resume callbacks will not be confused by that
+ * change in case they are invoked going forward.
*/
- if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
- pm_wakeup_event(dev, 0);
+ pm_runtime_barrier(dev);
if (pm_wakeup_pending()) {
dev->power.direct_complete = false;
On Mon, Aug 24, 2020 at 07:35:31PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> It has been reported that system-wide suspend may be aborted in the
> absence of any wakeup events due to unforseen interactions of it with
> the runtume PM framework.
>
> One failing scenario is when there are multiple devices sharing an
> ACPI power resource and runtime-resume needs to be carried out for
> one of them during system-wide suspend (for example, because it needs
> to be reconfigured before the whole system goes to sleep). In that
> case, the runtime-resume of that device involves turning the ACPI
> power resource "on" which in turn causes runtime-resume requests
> to be queued up for all of the other devices sharing it. Those
> requests go to the runtime PM workqueue which is frozen during
> system-wide suspend, so they are not actually taken care of until
> the resume of the whole system, but the pm_runtime_barrier()
> call in __device_suspend() sees them and triggers system wakeup
> events for them which then cause the system-wide suspend to be
> aborted if wakeup source objects are in active use.
>
> Of course, the logic that leads to triggering those wakeup events is
> questionable in the first place, because clearly there are cases in
> which a pending runtime resume request for a device is not connected
> to any real wakeup events in any way (like the one above). Moreover,
> it is racy, because the device may be resuming already by the time
> the pm_runtime_barrier() runs and so if the driver doesn't take care
> of signaling the wakeup event as appropriate, it will be lost.
> However, if the driver does take care of that, the extra
> pm_wakeup_event() call in the core is redundant.
>
> Accordingly, drop the conditional pm_wakeup_event() call fron
> __device_suspend() and make the latter call pm_runtime_barrier()
> alone. Also modify the comment next to that call to reflect the new
> code and extend it to mention the need to avoid unwanted interactions
> between runtime PM and system-wide device suspend callbacks.
>
> Fixes: 1e2ef05bb8cf8 ("PM: Limit race conditions between runtime PM and system sleep (v2)")
> Reported-by: Mika Westerberg <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
>
> -> v2:
> * Do not call pm_runtime_resume() if pm_runtime_barrier() returns 1,
> because the device have been resumed by it already.
> * Extend the comment next to the pm_runtime_barrier() call.
> * Update the changelog in accordance with the above.
>
> ---
> drivers/base/power/main.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -1606,13 +1606,17 @@ static int __device_suspend(struct devic
> }
>
> /*
> - * If a device configured to wake up the system from sleep states
> - * has been suspended at run time and there's a resume request pending
> - * for it, this is equivalent to the device signaling wakeup, so the
> - * system suspend operation should be aborted.
> + * Wait for possible runtime PM transitions of the device in progress
> + * to complete and if there's a runtime resume request pending for it,
> + * resume it before proceeding with invoking the system-wide suspend
> + * callbacks for it.
> + *
> + * If the system-wide suspend callbacks below change the configuration
> + * of the device, they must disable runtime PM for it or otherwise
> + * ensure that its runtime-resume callbacks will not be confused by that
> + * change in case they are invoked going forward.
> */
> - if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
> - pm_wakeup_event(dev, 0);
> + pm_runtime_barrier(dev);
>
> if (pm_wakeup_pending()) {
> dev->power.direct_complete = false;
Acked-by: Alan Stern <[email protected]>
Hi Rafael,
On Mon, Aug 24, 2020 at 07:35:31PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> It has been reported that system-wide suspend may be aborted in the
> absence of any wakeup events due to unforseen interactions of it with
> the runtume PM framework.
>
> One failing scenario is when there are multiple devices sharing an
> ACPI power resource and runtime-resume needs to be carried out for
> one of them during system-wide suspend (for example, because it needs
> to be reconfigured before the whole system goes to sleep). In that
> case, the runtime-resume of that device involves turning the ACPI
> power resource "on" which in turn causes runtime-resume requests
> to be queued up for all of the other devices sharing it. Those
> requests go to the runtime PM workqueue which is frozen during
> system-wide suspend, so they are not actually taken care of until
> the resume of the whole system, but the pm_runtime_barrier()
> call in __device_suspend() sees them and triggers system wakeup
> events for them which then cause the system-wide suspend to be
> aborted if wakeup source objects are in active use.
>
> Of course, the logic that leads to triggering those wakeup events is
> questionable in the first place, because clearly there are cases in
> which a pending runtime resume request for a device is not connected
> to any real wakeup events in any way (like the one above). Moreover,
> it is racy, because the device may be resuming already by the time
> the pm_runtime_barrier() runs and so if the driver doesn't take care
> of signaling the wakeup event as appropriate, it will be lost.
> However, if the driver does take care of that, the extra
> pm_wakeup_event() call in the core is redundant.
>
> Accordingly, drop the conditional pm_wakeup_event() call fron
> __device_suspend() and make the latter call pm_runtime_barrier()
> alone. Also modify the comment next to that call to reflect the new
> code and extend it to mention the need to avoid unwanted interactions
> between runtime PM and system-wide device suspend callbacks.
>
> Fixes: 1e2ef05bb8cf8 ("PM: Limit race conditions between runtime PM and system sleep (v2)")
> Reported-by: Mika Westerberg <[email protected]>
I guess the more correct here is
Reported-by: Utkarsh H Patel <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
I also got confirmation that this fixes the reported issue and did not
seem to cause regressions either :)
Please add the following tags:
Tested-by: Utkarsh H Patel <[email protected]>
Tested-by: Pengfei Xu <[email protected]>
Thanks for fixing this!