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

[psodagud@codeaurora.org]

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@codeaurora.org>
>> ---
>>  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