Skip to contentSkip to conversation

PM / core: Fix direct_complete handling for devices with no callbacks

PATCH In review Rafael J. Wysocki 4 participants 6 messages May 22, 2018 – May 24, 2018

Subsystems (5)
Stable branch Driver core Suspend to RAM Hibernation +1 more subsystems
lore
Share & tools
lore

Rafael J. Wysocki

author
#0 lore raw

PATCH 1/1 PM / core: Fix direct_complete handling for devices with no callbacks

From: Rafael J. Wysocki <[email protected]>

Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags) inadvertently prevented the power.direct_complete flag from being set for devices without PM callbacks and with disabled runtime PM which also prevents power.direct_complete from being set for their parents. That led to problems including a resume crash on HP ZBook 14u.

Restore the previous behavior by causing power.direct_complete to be set for those devices again, but do that in a more direct way to avoid overlooking that case in the future.

Link https://bugzilla.kernel.org/show_bug.cgi?id=199693
Fixes 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags)
Reported-by Thomas Martitz <[email protected]>
Tested-by Thomas Martitz <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/base/power/main.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/base/power/main.c +3 -4
Index: linux-pm/drivers/base/power/main.c
===================================================================
@@ -1920,10 +1920,8 @@ static int device_prepare(struct device
 
 	dev->power.wakeup_path = false;
 
-	if (dev->power.no_pm_callbacks) {
-		ret = 1;	/* Let device go direct_complete */
+	if (dev->power.no_pm_callbacks)
 		goto unlock;
-	}
 
 	if (dev->pm_domain)
 		callback = dev->pm_domain->ops.prepare;

@@ -1957,7 +1955,8 @@ unlock:
 	 */
 	spin_lock_irq(&dev->power.lock);
 	dev->power.direct_complete = state.event == PM_EVENT_SUSPEND &&
-		pm_runtime_suspended(dev) && ret > 0 &&
+		((pm_runtime_suspended(dev) && ret > 0) ||
+		 dev->power.no_pm_callbacks) &&
 		!dev_pm_test_driver_flags(dev, DPM_FLAG_NEVER_SKIP);
 	spin_unlock_irq(&dev->power.lock);
 	return 0;
#1 lore raw

Hello,

thanks for for your effort and the patch.

Is this eligible for stable?

Best regards

Am 22.05.2018 um 13:02 schrieb Rafael J. Wysocki:

Quoted 49 lines

From: Rafael J. Wysocki <[email protected]>

Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags) inadvertently prevented the power.direct_complete flag from being set for devices without PM callbacks and with disabled runtime PM which also prevents power.direct_complete from being set for their parents. That led to problems including a resume crash on HP ZBook 14u.

Restore the previous behavior by causing power.direct_complete to be set for those devices again, but do that in a more direct way to avoid overlooking that case in the future.

Link https://bugzilla.kernel.org/show_bug.cgi?id=199693
Fixes 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags)
Reported-by Thomas Martitz <[email protected]>
Tested-by Thomas Martitz <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>
---
  drivers/base/power/main.c |    7 +++----
  1 file changed, 3 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/base/power/main.c
===================================================================

drivers/base/power/main.c +3 -4
@@ -1920,10 +1920,8 @@ static int device_prepare(struct device
 
  	dev->power.wakeup_path = false;
 
-	if (dev->power.no_pm_callbacks) {
-		ret = 1;	/* Let device go direct_complete */
+	if (dev->power.no_pm_callbacks)
  		goto unlock;
-	}
 
  	if (dev->pm_domain)
  		callback = dev->pm_domain->ops.prepare;

@@ -1957,7 +1955,8 @@ unlock:
  	 */
  	spin_lock_irq(&dev->power.lock);
  	dev->power.direct_complete = state.event == PM_EVENT_SUSPEND &&
-		pm_runtime_suspended(dev) && ret > 0 &&
+		((pm_runtime_suspended(dev) && ret > 0) ||
+		 dev->power.no_pm_callbacks) &&
  		!dev_pm_test_driver_flags(dev, DPM_FLAG_NEVER_SKIP);
  	spin_unlock_irq(&dev->power.lock);
  	return 0;
author
#2 lore raw
Quoted 5 lines · CEST Thomas Martitz

Hello,

thanks for for your effort and the patch.

Is this eligible for stable?

Yes, it is.

Thanks,
Rafael

Quoted 52 lines

Am 22.05.2018 um 13:02 schrieb Rafael J. Wysocki:

Quoted 49 lines

From: Rafael J. Wysocki <[email protected]>

Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags) inadvertently prevented the power.direct_complete flag from being set for devices without PM callbacks and with disabled runtime PM which also prevents power.direct_complete from being set for their parents. That led to problems including a resume crash on HP ZBook 14u.

Restore the previous behavior by causing power.direct_complete to be set for those devices again, but do that in a more direct way to avoid overlooking that case in the future.

Link https://bugzilla.kernel.org/show_bug.cgi?id=199693
Fixes 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags)
Reported-by Thomas Martitz <[email protected]>
Tested-by Thomas Martitz <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>
---
  drivers/base/power/main.c |    7 +++----
  1 file changed, 3 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/base/power/main.c
===================================================================

drivers/base/power/main.c +3 -4
@@ -1920,10 +1920,8 @@ static int device_prepare(struct device
 
  	dev->power.wakeup_path = false;
 
-	if (dev->power.no_pm_callbacks) {
-		ret = 1;	/* Let device go direct_complete */
+	if (dev->power.no_pm_callbacks)
  		goto unlock;
-	}
 
  	if (dev->pm_domain)
  		callback = dev->pm_domain->ops.prepare;

@@ -1957,7 +1955,8 @@ unlock:
  	 */
  	spin_lock_irq(&dev->power.lock);
  	dev->power.direct_complete = state.event == PM_EVENT_SUSPEND &&
-		pm_runtime_suspended(dev) && ret > 0 &&
+		((pm_runtime_suspended(dev) && ret > 0) ||
+		 dev->power.no_pm_callbacks) &&
  		!dev_pm_test_driver_flags(dev, DPM_FLAG_NEVER_SKIP);
  	spin_unlock_irq(&dev->power.lock);
  	return 0;
Reviewed-by on v1 1/1
#3 lore raw
In reply to Rafael J. Wysocki
Quoted 13 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags) inadvertently prevented the power.direct_complete flag from being set for devices without PM callbacks and with disabled runtime PM which also prevents power.direct_complete from being set for their parents. That led to problems including a resume crash on HP ZBook 14u.

Restore the previous behavior by causing power.direct_complete to be set for those devices again, but do that in a more direct way to avoid overlooking that case in the future.

Quoted 5 lines
Link https://bugzilla.kernel.org/show_bug.cgi?id=199693
Fixes 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags)
Reported-by Thomas Martitz <[email protected]>
Tested-by Thomas Martitz <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>

It seems like the resume path of HP ZBook 14u is kind of fragile, in case it *requires* dev->power.direct_complete to be set for devices like these. And that makes me wonder, that perhaps we should try to address that issue as well, no?

In either way:

Reviewed-by Ulf Hansson <[email protected]>

Kind regards
Uffe

Quoted 31 lines
---
 drivers/base/power/main.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/base/power/main.c
===================================================================

drivers/base/power/main.c +3 -4
@@ -1920,10 +1920,8 @@ static int device_prepare(struct device
 
        dev->power.wakeup_path = false;
 
-       if (dev->power.no_pm_callbacks) {
-               ret = 1;        /* Let device go direct_complete */
+       if (dev->power.no_pm_callbacks)
                goto unlock;
-       }
 
        if (dev->pm_domain)
                callback = dev->pm_domain->ops.prepare;

@@ -1957,7 +1955,8 @@ unlock:
         */
        spin_lock_irq(&dev->power.lock);
        dev->power.direct_complete = state.event == PM_EVENT_SUSPEND &&
-               pm_runtime_suspended(dev) && ret > 0 &&
+               ((pm_runtime_suspended(dev) && ret > 0) ||
+                dev->power.no_pm_callbacks) &&
                !dev_pm_test_driver_flags(dev, DPM_FLAG_NEVER_SKIP);
        spin_unlock_irq(&dev->power.lock);
        return 0;
author
#4 lore raw
Quoted 14 lines · CEST Ulf Hansson
Quoted 13 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags) inadvertently prevented the power.direct_complete flag from being set for devices without PM callbacks and with disabled runtime PM which also prevents power.direct_complete from being set for their parents. That led to problems including a resume crash on HP ZBook 14u.

Restore the previous behavior by causing power.direct_complete to be set for those devices again, but do that in a more direct way to avoid overlooking that case in the future.

Quoted 7 lines
Quoted 5 lines
Link https://bugzilla.kernel.org/show_bug.cgi?id=199693
Fixes 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags)
Reported-by Thomas Martitz <[email protected]>
Tested-by Thomas Martitz <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>

It seems like the resume path of HP ZBook 14u is kind of fragile,

Yes, it is.

Quoted 3 lines

in case it *requires* dev->power.direct_complete to be set for devices like these. And that makes me wonder, that perhaps we should try to address that issue as well, no?

Yes, in principle.

But since the direct_complete handling needs to be fixed anyway, it doesn't matter a lot in practice, because the resume issue on HP ZBook 14u will not be reproducible anyway then. And since the dependency clearly is on a device with no callbacks, I'm not worried too much about that.

maintainer Reviewed-by on v1 1/1
#5 lore raw
In reply to Rafael J. Wysocki
Quoted 9 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags) inadvertently prevented the power.direct_complete flag from being set for devices without PM callbacks and with disabled runtime PM which also prevents power.direct_complete from being set for their parents. That led to problems including a resume crash on HP ZBook 14u.

Quoted 6 lines

Restore the previous behavior by causing power.direct_complete to be set for those devices again, but do that in a more direct way to avoid overlooking that case in the future.

Link https://bugzilla.kernel.org/show_bug.cgi?id=199693
Fixes 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags)

I stumbled over this the other day as well and tracked it down to the above commit. In my case (child devices to serdev clients), this was mostly benign although it did prevent the direct-complete optimisation during suspend.

Never got around to reporting or fixing it myself, but your analysis and fix matches my initial findings.

Quoted 3 lines
Reported-by Thomas Martitz <[email protected]>
Tested-by Thomas Martitz <[email protected]>
Signed-off-by Rafael J. Wysocki <[email protected]>
Reviewed-by Johan Hovold <[email protected]>

As already suggested elsewhere in the thread, I think a stable is warranted too.

Thanks,
Johan