Re: [PATCH] pm: sleep: do not set is_prepared when no_pm_callbacks is set

["David Wang" <00107082@163.com>]

At 2024-09-03 20:31:04, "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>On Mon, Sep 2, 2024 at 2:59 PM David Wang <00107082@163.com> wrote:
>>
>> When resume, a parent device with no pm callbacks
>> would have "is_prepared" and "direct_complete" bit
>> set, and skip the "fib" chance to unset "is_prepared"
>> in device_resume because of the direct_complete bit.
>
>Sure, but is_prepared will be cleared in device_complete() AFAICS.

Yes, you're right. I made a wrong reasoning...

>
>> This will trigger a kernel warning when resume its child
>> For example, when suspend system with an USB webcam
>> opened, following warning would show up during resume:
>>
>>  >usb 3-1.1: reset high-speed USB device number 4 using xhci_hcd
>>  >..
>>  >ep_81: PM: parent 3-1.1:1.1 should not be sleeping
>
>This is printed in device_pm_add(), so apparently something new has
>appeared under the parent while it's between "resume" and "prepare".

Yes, after some debug, it turns out "uvcvideo 3-1.1:1.1" created a new
"ep_81" when "ep_81" resumed

>
>The parent is actually still regarded as "suspended" because any
>resume callbacks have not been called for it, but new children can be
>added under it at this point because doing so does not break the
>dpm_list ordering and all of its ancestors have been already resumed.
>
>> The device parenting relationships are:
>> [usb 3-1.1] << [uvcvideo 3-1.1:1.1] << [ep_81].
>> When resume, since the virtual [uvcvideo 3-1.1:1.1] device
>> has no pm callbacks, it would not clear "is_prepared"
>> once set.  Then, when resume [ep_81], pm module would
>> yield a warn seeing [ep_81]'s parent [uvcvideo 3-1.1:1.1]
>> having "is_prepared".
>>
>> Do not set "is_prepared" for virtual devices having
>> no pm callbacks can clear those kernel warnings.
>>
>> Signed-off-by: David Wang <00107082@163.com>
>> ---
>>  drivers/base/power/main.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> index 934e5bb61f13..e2149ccf2c3e 100644
>> --- a/drivers/base/power/main.c
>> +++ b/drivers/base/power/main.c
>> @@ -1880,7 +1880,8 @@ int dpm_prepare(pm_message_t state)
>>                 mutex_lock(&dpm_list_mtx);
>>
>>                 if (!error) {
>> -                       dev->power.is_prepared = true;
>> +                       if (!dev->power.no_pm_callbacks)
>> +                               dev->power.is_prepared = true;
>
>This is not the way to address the issue IMV.
>
>power.is_prepared set means that the device is in dpm_prepared_list
>and I wouldn't depart from that even for devices without PM callbacks.
>
>>                         if (!list_empty(&dev->power.entry))
>>                                 list_move_tail(&dev->power.entry, &dpm_prepared_list);
>>                 } else if (error == -EAGAIN) {
>> --
>
>It would be better to add a power.no_pm_callbacks check for the parent
>to device_pm_add(), but this would still suppress the warning is some
>cases in which it should be printed (for example, the new device's
>parent is a "virtual" device without PM callbacks, but its grandparent
>is a regular device that has PM callbacks and is suspended).
>
>Something like the attached patch (untested) might work, though.

I tried the patch on 6.11.0-rc6, the warn is gone when I made following test:

1. open webcam (via obs, e.g.)
2. systemctl suspend
3. resume the system, and check kernel log

(Would it safe to walk the parent link without any device lock? and I still suggest moving
those code our of &dpm_list_mtx lock)

Thanks
David.