Re: [PATCH] PM: sleep: core: Avoid setting power.must_resume to false

From: psodagud
Date: Fri Aug 06 2021 - 11:07:37 EST

On 2021-08-03 10:16, Greg KH wrote:
On Mon, Jul 26, 2021 at 08:24:34AM -0700, Prasad Sodagudi wrote:
There are variables(power.may_skip_resume and dev->power.must_resume)
and DPM_FLAG_MAY_SKIP_RESUME flags to control the resume of devices after
a system wide suspend transition.

Setting the DPM_FLAG_MAY_SKIP_RESUME flag means that the driver allows
its "noirq" and "early" resume callbacks to be skipped if the device
can be left in suspend after a system-wide transition into the working
state. PM core determines that the driver's "noirq" and "early" resume
callbacks should be skipped or not with dev_pm_skip_resume() function by
checking power.may_skip_resume variable.

power.must_resume variable is getting set to false in __device_suspend()
function without checking device's DPM_FLAG_MAY_SKIP_RESUME and
dev->power.usage_count variables. This is leading to failure to call
resume handler for some of the devices suspended in early suspend phase.
So check device's DPM_FLAG_MAY_SKIP_RESUME flag before
setting power.must_resume variable.

Signed-off-by: Prasad Sodagudi <psodagud@xxxxxxxxxxxxxx>
---
drivers/base/power/main.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index d568772..8eebc4d 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1642,7 +1642,11 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
}

dev->power.may_skip_resume = true;
- dev->power.must_resume = false;
+ if ((atomic_read(&dev->power.usage_count) <= 1) &&
+ (dev_pm_test_driver_flags(dev, DPM_FLAG_MAY_SKIP_RESUME)))

What is preventing that atomic value from changing _right_ after you
just read this?

and very odd indentation, checkpatch didn't complain about this?
Sure. I will fix indentation problem once Rafael reviewed this patch.

What commit does this fix? Does it need to be backported to older
kernels?

No. LTS - 5.4 do not have this problem.

Wait, how is your "noirq" device even getting called here? Shouldn't
__device_suspend_noirq() be called instead? Why isn't that the path for
your device?

Hi Gregh and Rafael,

This is regarding suspend/resume(s2idle) scenario of devices and differences between the LTS kernel 5.4 and 5.10 with respect to devices suspend and resume. Observing that devices suspended in suspend_late stage are not getting resumed in resume_early stage.
1) LTS kernel 5.4 kernel do not have this problem but 5.10 kernel shows this problem.
2) Commit - 6e176bf8d46194353163c2cb660808bc633b45d9 (PM: sleep: core: Do not skip callbacks in the resume phase) is skipping the driver early_resume callbacks.
@@ -804,15 +793,25 @@ static int device_resume_early(struct device *dev, pm_message_t state, bool asyn
} else if (dev->bus && dev->bus->pm) {
info = "early bus ";
callback = pm_late_early_op(dev->bus->pm, state);
- } else if (dev->driver && dev->driver->pm) {
+ }
+ if (callback)
+ goto Run;
+
+ if (dev_pm_may_skip_resume(dev))
+ goto Skip;
In device_resume_early function dev->power.must_resume is used to skip the resume call back. It looks this function is expecting that, __device_suspend_noirq() would set dev->power.must_resume = true for the devices which does not have DPM_FLAG_MAY_SKIP_RESUME flag set.

static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool async)
{
…
…
/*
* Skipping the resume of devices that were in use right before the
* system suspend (as indicated by their PM-runtime usage counters)
* would be suboptimal. Also resume them if doing that is not allowed
* to be skipped.
*/
if (atomic_read(&dev->power.usage_count) > 1 ||
!(dev_pm_test_driver_flags(dev, DPM_FLAG_MAY_SKIP_RESUME) &&
dev->power.may_skip_resume))
dev->power.must_resume = true;


3) Problematic scenario is as follows - During the device suspend/resume scenario all the devices in the suspend_late stage are successful and some device can fail to suspend in suspend_noirq(device_suspend_noirq-> __device_suspend_noirq) phase.
As a device failed in dpm_noirq_suspend_devices phase, dpm_resume_noirq is getting called to resume devices in dpm_late_early_list in the noirq phase.
4) During the Devices_early_resume stage dpm_resume_early()-->device_resume_early() functions skipping the devices early resume callbacks.
799 if (dev_pm_skip_resume(dev))
800 goto Skip;

5) Devices suspended in suspend_late stage are not getting resumed in Devices_early_resume stage because of Commit - 6e176bf8d46194353163c2cb660808bc633b45d9 (PM: sleep: core: Do not skip callbacks in the resume phase) is skipping the driver early_resume callbacks when dev->power.must_resume is false.

6) Below portion of the code in __device_suspend_noirq is not getting executed for some drivers successfully suspended in suspend_late stage and there is no chance to set must_resume to true. So these devices are always having dev->power.must_resume=false.
For example -
i) Devices A, B, C have suspend_late and resume_early handlers.
ii) Devices X, Y, Z have suspend_noirq and resume_noirq handlers.
Devices are getting suspended in this order – A, B, X , C , Y and Z and device X return failure for suspend_noirq callback. In this scenario, device C would never execute below portion of the code to set dev->power.must_resume = true and device – C would not get resumed in resume_early stage.

1192 static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool async)
1193 {
….
….
1245 /*
1246 * Skipping the resume of devices that were in use right before the
1247 * system suspend (as indicated by their PM-runtime usage counters)
1248 * would be suboptimal. Also resume them if doing that is not allowed
1249 * to be skipped.
1250 */
1251 if (atomic_read(&dev->power.usage_count) > 1 ||
1252 !(dev_pm_test_driver_flags(dev, DPM_FLAG_MAY_SKIP_RESUME) &&
1253 dev->power.may_skip_resume))
1254 dev->power.must_resume = true;
1255
1256 if (dev->power.must_resume)
1257 dpm_superior_set_must_resume(dev);
1258

thanks,

greg k-h