diff mbox series

pm: sleep: do not set is_prepared when no_pm_callbacks is set

Message ID 20240902125933.5742-1-00107082@163.com
State New
Headers show
Series pm: sleep: do not set is_prepared when no_pm_callbacks is set | expand

Commit Message

David Wang Sept. 2, 2024, 12:59 p.m. UTC 
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.
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

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(-)

Comments

Greg KH Sept. 3, 2024, 10:23 a.m. UTC | #1 
On Mon, Sep 02, 2024 at 08:59:33PM +0800, David Wang 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.
> 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
> 
> 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(-)

What commit id does this fix?

thanks,

greg k-h
David Wang Sept. 3, 2024, 11:26 a.m. UTC | #2 
HI, 

At 2024-09-03 18:23:55, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>On Mon, Sep 02, 2024 at 08:59:33PM +0800, David Wang 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.
>> 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
>> 
>> 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(-)
>
>What commit id does this fix?

Well, the state management of PM devices is quite complicated to me, lots of commits make small changes 
and  I cannot identify a single commit that solely introduced the kernel warning when suspend an opened USB webcam.

Most obvious commit seems to be 
aa8e54b559479d0cb7eb632ba443b8cacd20cd4b " "PM / sleep: Go direct_complete if driver has no callbacks"
c62ec4610c40bcc44f2d3d5ed1c312737279e2f3 "PM / core: Fix direct_complete handling for devices with no callbacks"

and I will try revert those logic and update later.
  
 
>
>thanks,
>
>greg k-h


David
Rafael J. Wysocki Sept. 3, 2024, 12:31 p.m. UTC | #3 
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.

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

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.
Rafael J. Wysocki Sept. 3, 2024, 12:32 p.m. UTC | #4 
On Tue, Sep 3, 2024 at 12:42 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Sep 02, 2024 at 08:59:33PM +0800, David Wang 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.
> > 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
> >
> > 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(-)
>
> What commit id does this fix?

It doesn't fix anything, it is introducing a potential issue.
David Wang Sept. 3, 2024, 5:09 p.m. UTC | #5 
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.
David Wang Sept. 3, 2024, 5:30 p.m. UTC | #6 
At 2024-09-03 20:32:14, "Rafael J. Wysocki" <rafael@kernel.org> wrote:
>On Tue, Sep 3, 2024 at 12:42 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>> On Mon, Sep 02, 2024 at 08:59:33PM +0800, David Wang 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.
>> > 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
>> >
>> > 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(-)
>>
>> What commit id does this fix?
>
>It doesn't fix anything, it is introducing a potential issue.

I admit I only have my system to test and dose not have a whole picture in mind, 
and also I really do not quite understand the module, for now.

It turned out my reasoning in the commit message is also quite wrong, after some debug, I realized
that the is_prepared would be cleared eventually, but after ep_81 was added. And 
the new device "ep_81" was added during usb_resume, while "ep_81"'s parent "uvcvideo 3-1.1:1.1"
is also in device_resume but waiting on "if (!dpm_wait_for_superior(dev, async))"
the whole call stack when adding the new "ep" device is as follow:

	  device_pm_add+0xde/0x130
	  device_add+0x3d0/0x870
	  usb_create_ep_devs+0x96/0x100 [usbcore]
	  create_intf_ep_devs.isra.0+0x52/0x80 [usbcore]
	  usb_set_interface+0x2b8/0x3c0 [usbcore]
	  uvc_video_start_transfer+0x1c6/0x600 [uvcvideo]
	  __uvc_resume+0x60/0x150 [uvcvideo]
	  usb_resume_interface.isra.0+0x41/0xe0 [usbcore]
	  usb_resume_both+0x103/0x180 [usbcore]
	  ? __pfx_usb_dev_resume+0x10/0x10 [usbcore]
	  usb_resume+0x15/0x60 [usbcore]
	  dpm_run_callback+0x8b/0x1e0
	  device_resume+0x9c/0x220
	  async_resume+0x19/0x30
	  async_run_entry_fn+0x30/0x130
	  process_one_work+0x17c/0x390
	  worker_thread+0x245/0x350
	  ? __pfx_worker_thread+0x10/0x10
	  kthread+0xdd/0x110
	  ? __pfx_kthread+0x10/0x10
	  ret_from_fork+0x30/0x50
	  ? __pfx_kthread+0x10/0x10
	  ret_from_fork_asm+0x1a/0x30
	  </TASK>
	  ep_81: PM: parent [3-1.1:1.1] of [No Bus:ep_81] should not be sleeping

Base on this call stack, my reading is:
When usb device start to resume, it call uvc_resume directly, 
and then uvc_resume would create a new "ep" device directly,  ignoring pm's device_resume for uvc_video device totally and trigger the warn,

More like a corporation issue between uvc-video device and PM module.



Thanks your time reviewing the issue/code, and sorry about those nonsense wide guesses in commit message.
David
diff mbox series

Patch

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;
 			if (!list_empty(&dev->power.entry))
 				list_move_tail(&dev->power.entry, &dpm_prepared_list);
 		} else if (error == -EAGAIN) {