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: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