On 2021-08-03 10:16, Greg KH wrote:

quoted

On Mon, Jul 26, 2021 at 08:24:34AM -0700, Prasad Sodagudi wrote:

quoted

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 <redacted>
---
 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.

quoted

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

No. LTS - 5.4 do not have this problem.

quoted

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