Message-ID: <4fc877f0-b55b-4fa3-8df4-2de4ba1ac51b@163.com>
Date: Thu, 3 Jul 2025 14:04:52 +0800
From: Zongmin Zhou <min_halo@....com>
To: Shuah Khan <skhan@...uxfoundation.org>,
 Greg KH <gregkh@...uxfoundation.org>
Cc: shuah@...nel.org, valentina.manea.m@...il.com, i@...ithal.me,
 linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
 zhouzongmin@...inos.cn
Subject: Re: [PATCH v2] usbip: convert to use faux_device


On 2025/7/3 07:54, Shuah Khan wrote:
> On 7/1/25 20:12, Zongmin Zhou wrote:
>>
>> On 2025/7/2 06:56, Shuah Khan wrote:
>>> On 6/23/25 21:21, Zongmin Zhou wrote:
>>>>
>>>> On 2025/6/21 01:26, Shuah Khan wrote:
>>>>> On 6/20/25 03:27, Greg KH wrote:
>>>>>> On Fri, Jun 20, 2025 at 05:19:33PM +0800, Zongmin Zhou wrote:
>>>>>>>
>>>>>>> On 2025/6/20 12:29, Greg KH wrote:
>>>>>>>> On Fri, Jun 20, 2025 at 10:16:16AM +0800, Zongmin Zhou wrote:
>>>>>>>>> On 2025/6/19 19:01, Greg KH wrote:
>>>>>>>>>> On Wed, Jun 04, 2025 at 02:54:10PM +0800, Zongmin Zhou wrote:
>>>>>>>>>>> From: Zongmin Zhou <zhouzongmin@...inos.cn>
>>>>>>>>>>>
>>>>>>>>>>> The vhci driver does not need to create a platform device,
>>>>>>>>>>> it only did so because it was simple to do that in order to
>>>>>>>>>>> get a place in sysfs to hang some device-specific attributes.
>>>>>>>>>>> Now the faux device interface is more appropriate,change it
>>>>>>>>>>> over to use the faux bus instead.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Zongmin Zhou <zhouzongmin@...inos.cn>
>>>>>>>>>>> ---
>>>>>>>>>>> Changes in v2:
>>>>>>>>>>> - don't change faux create api,just call probe on 
>>>>>>>>>>> vhci_hcd_init.
>>>>>>>>>>>
>>>>>>>>>>>     drivers/usb/usbip/vhci.h             |  4 +-
>>>>>>>>>>>     drivers/usb/usbip/vhci_hcd.c         | 86 
>>>>>>>>>>> +++++++++++-----------------
>>>>>>>>>>>     drivers/usb/usbip/vhci_sysfs.c       | 68 
>>>>>>>>>>> +++++++++++-----------
>>>>>>>>>>>     tools/usb/usbip/libsrc/vhci_driver.h |  2 +-
>>>>>>>>>>>     4 files changed, 72 insertions(+), 88 deletions(-)
>>>>>>>>>> I get the following build errors from this patch:
>>>>>>>>>>
>>>>>>>>>> drivers/usb/usbip/vhci_hcd.c:1462:12: error: 
>>>>>>>>>> ‘vhci_hcd_resume’ defined but not used [-Werror=unused-function]
>>>>>>>>>>     1462 | static int vhci_hcd_resume(struct faux_device *fdev)
>>>>>>>>>>          |            ^~~~~~~~~~~~~~~
>>>>>>>>>> drivers/usb/usbip/vhci_hcd.c:1418:12: error: 
>>>>>>>>>> ‘vhci_hcd_suspend’ defined but not used 
>>>>>>>>>> [-Werror=unused-function]
>>>>>>>>>>     1418 | static int vhci_hcd_suspend(struct faux_device 
>>>>>>>>>> *fdev, pm_message_t state)
>>>>>>>>>>          |            ^~~~~~~~~~~~~~~~
>>>>>>>>>> cc1: all warnings being treated as errors
>>>>>>>>>>
>>>>>>>>>> Are you sure you tested this?
>>>>>>>>> I apologize for not enabling -Werror, which resulted in 
>>>>>>>>> missing this error
>>>>>>>>> warning.
>>>>>>>>> I have tested usbip feature use the new patch,but not test system
>>>>>>>>> suspend/resume.
>>>>>>>>> The faux bus type don't add pm function,and vhci-hcd driver 
>>>>>>>>> can't register
>>>>>>>>> it.
>>>>>>>>> Maybe have to add suspend/resume for it.like below:
>>>>>>>>> static const struct bus_type faux_bus_type = {
>>>>>>>>>       .name        = "faux",
>>>>>>>>>       .match        = faux_match,
>>>>>>>>>       .probe        = faux_probe,
>>>>>>>>>       .remove        = faux_remove,
>>>>>>>>>       .resume     = faux_resume,
>>>>>>>>>       .suspend    = faux_suspend,
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> Is that right?
>>>>>>>>> Your expertise would be greatly valued.
>>>>>>>> As this is not real hardware, why do you need the suspend/resume
>>>>>>>> callbacks at all?  What is happening here that requires them?
>>>>>>> @greg,
>>>>>>> The vhci_hcd_suspend/vhci_hcd_resume interfaces are not designed 
>>>>>>> for this
>>>>>>> faux device, but rather to
>>>>>>> manipulate the HCD_FLAG_HW_ACCESSIBLE bit in the hcd flags 
>>>>>>> associated with
>>>>>>> the faux device.
>>>>>>> For example:
>>>>>>> During system standby: clear_bit(HCD_FLAG_HW_ACCESSIBLE, 
>>>>>>> &hcd->flags)
>>>>>>> During system wakeup: set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags)
>>>>>>>
>>>>>>> Previously, these two functions were registered through 
>>>>>>> platform_driver,
>>>>>>> but faux bus does not have the relevant interface, so they were 
>>>>>>> not called,
>>>>>>> resulting in this compilation warning error.
>>>>>>>
>>>>>>> This raises the question: Should the faux bus implement PM-related
>>>>>>> interface?
>>>>>>> I'm uncertain whether these functions are essential for the 
>>>>>>> vhci-hcd driver
>>>>>>> or if they can be safely removed.
>>>>>>>
>>>>>>> However, during system standby/wakeup tests with remote USB 
>>>>>>> devices bound to
>>>>>>> the vhci-hcd driver,
>>>>>>> I observed consistent failure scenarios across both the original 
>>>>>>> platform
>>>>>>> bus and faux bus patch implementations.
>>>>>
>>>>> suspend and resume hooks have been in the code from beginning
>>>>> in the CONFIG_PM path. The original authors are skeptical about
>>>>> what should happen during suspend"
>>>>>
>>>>> /* what should happen for USB/IP under suspend/resume? */
>>>>> suspend hook checks for active connections and sends EBBUSY
>>>>> back to pm core.
>>>>>
>>>>> Active connection means any of the ports are in 
>>>>> USB_PORT_STAT_CONNECTION
>>>>> state. So as long as there are active connections between vhci client
>>>>> and the host, suspend won't work. So we really don't know what 
>>>>> happens
>>>>> to the active connections if there are no suspend/resume hooks.
>>>>>
>>>>> If there are no active connections, then it will clear the 
>>>>> HCD_FLAG_HW_ACCESSIBLE
>>>>> bit and returns to pm core to continue with suspend.
>>>>>
>>>>> resume sets the HCD_FLAG_HW_ACCESSIBLE flag to indicate hardware 
>>>>> is accessible
>>>>> and initiates port status poll.
>>>>>
>>>>> - suspend hook prevents suspend
>>>>>
>>>>> With faux device since there is no suspend and resume hook, I 
>>>>> would expect
>>>>> these hooks are not a factor in suspend and resume.
>>>>>
>>>>> vhci_hcd is a special case virtual driver as it is a proxy for 
>>>>> controlling
>>>>> hardware on the host.
>>>>>
>>>>> Zongmin,
>>>>>
>>>>> Do suspend/resume work when vhci_hcd is not loaded?
>>>>> What happens when when system suspends and resumes with faux device?
>>>>> Can you send me dmesg logs and pm logs?
>>>>>
>>>> Hi Greg,shuah
>>>>
>>>> Yes, system with vhci_hcd unload or just load vhci_hcd driver
>>>> without usbip devices bound to vhci_hcd,system suspend/resume 
>>>> worked well.
>>>> Some logs for you.
>>>
>>> Sorry for the delay. I was at a conference last week.
>>> Thank you for sending these logs.
>>>
>>>> 1.system with vhci_hcd driver load,but don't bound any usbip 
>>>> devices to vhci_hcd,suspend/resume worked well.
>>>> see dmesg-faux bus-load.log
>>>
>>>
>>>>
>>>> 2.usbip device bound to vhci_hcd,and do system suspend 
>>>> action,suspend/resume worked failed.
>>>> I observed two distinct failure scenario:
>>>> Scenario 1: System failed to enter suspend state,and will auto resume;
>>>
>>>
>>>> the log for use platform bus:
>>>> dmesg-platform bus-device bound-auto resume.log
>>>
>>> This is clear from the suspend hook in the vhci_hcd.
>>> When it returns EBUSY, suspend will fail and move to
>>> auto resume.
>>>
>>>> the log for use faux bus:
>>>> dmesg-faux bus-device bound-auto resume.log
>>>
>>> It is good that the behavior is same with faux bus even though
>>> suspend hook isn't called. I will take a look at the log.
>>>
>>>>
>>>> Scenario 2: System resume failed with black screen freeze,a forced 
>>>> restart of the machine is require.
>>>> the log for use platform bus:
>>>> dmesg-platform bus-device bound-black screen.log
>>>> the log for use faux bus:
>>>> dmesg-faux bus-device bound-black screen.log
>>>
>>> That is bad. When does this happen? Is there a difference in
>>> configuration?
>>>
>> No, there's no difference. The same code is used for two different 
>> failure scenarios.
>> I just tested many times. These two different situations occur 
>> probabilistically.
>> But they both happened only when the USBIP device is bound to the 
>> vhci_hcd driver.
>>> The behavior same for platform bus and faux bus. Sounds like
>>> an existing problem that needs to be debugged separately.
>>>
>>> I will take a look at all these logs and get back to you in
>>> a day or two.
>>>
>
> I looked at the logs and here is what I found:
>
> Scenario 1:
>   dmesg-faux bus-device bound-auto resume.log
>   dmesg-platform bus-device bound-auto resume.log
>
> In this case suspend bailed out way before driver suspend
> when vhci_hcd is using platform and faux bus.
>
> Freezing remaining freezable tasks failed after 20.006 seconds (0 
> tasks refusing to freeze, wq_busy=1)
> Restarting kernel threads ... done
> OOM killer enabled.
> Restarting tasks ... done.
> random: crng reseeded on system resumption
> PM: suspend exit
>
> Auto-resume of the user-space worked. Scenario 1 isn't really
> interesting.
>
> Scenario 2:
>   dmesg-faux bus-device bound-black screen.log
>   dmesg-platform bus-device bound-black screen.log
>
> Even though the result is the same in seeing blank screen, how
> we get there is different.
>
> =================
> faux-bus device:
> =================
> - suspend worked - looks like
>   usb 4-1: PM: calling usb_dev_suspend @ 6054, parent: usb4
>   usb 4-1: PM: usb_dev_suspend returned 0 after 13675 usecs
>   usb usb4: PM: calling usb_dev_suspend @ 6055, parent: vhci_hcd.0
>   usb usb4: PM: usb_dev_suspend returned 0 after 9 usecs
>
> vhci_hcd suspend isn't in play here. usb_dev_suspend() returns.
> See below
>
> usb 4-1: PM: usb_dev_suspend returned message.
>
> -------------------------------------------------------------
>
> - resume started (assuming it has been initiated by user)
>
> [  650.027491][ T6056] pci 0000:00:01.0: PM: pci_pm_suspend_noirq 
> returned 0 after 304 usecs
>
> See see timestamp difference between the last suspend message and the
> first resume message.
> [  674.000257][   T39] pci 0000:00:00.0: PM: calling 
> pci_pm_resume_noirq @ 39, parent: pci0000:00
>
> usb 4-1 usb_dev_resume never returns.
>
> [  674.071125][ T6117] usb usb4: PM: usb_dev_resume returned 0 after 
> 21110 usecs
> [  674.113991][ T6126] usb 4-1: PM: calling usb_dev_resume @ 6126, 
> parent: usb4
>
> -------------------------------------------------------------
>
> =====================
> platform bus device
> =====================
>
> - suspend was aborted because vhci_hcd suspend routine returned error
>
> [  297.854621][ T9402] usb 4-1: PM: calling usb_dev_suspend @ 9402, 
> parent: usb4
> [  297.868524][ T9402] usb 4-1: PM: usb_dev_suspend returned 0 after 
> 13214 usecs
> [  297.869994][ T9403] usb usb4: PM: calling usb_dev_suspend @ 9403, 
> parent: vhci_hcd.0
> [  297.871959][ T9403] usb usb4: PM: usb_dev_suspend returned 0 after 
> 30 usecs
> [  297.873644][ T9394] vhci_hcd vhci_hcd.0: PM: calling 
> platform_pm_suspend @ 9394, parent: platform
> [  297.874738][ T9394] vhci_hcd vhci_hcd.0: We have 1 active 
> connection. Do not suspend.
> [  297.875369][ T9394] vhci_hcd vhci_hcd.0: PM: dpm_run_callback(): 
> platform_pm_suspend returns -16
> [  297.876078][ T9394] vhci_hcd vhci_hcd.0: PM: platform_pm_suspend 
> returned -16 after 1341 usecs
> [  297.876774][ T9394] vhci_hcd vhci_hcd.0: PM: failed to suspend: 
> error -16
>
> - the following triggers resume
> [  297.877321][ T9394] PM: Some devices failed to suspend, or early 
> wake event detected
>
> [  297.881065][ T9403] usb usb3: PM: usb_dev_resume returned 0 after 
> 19 usecs
> [  297.904551][ T9408] usb usb4: PM: usb_dev_resume returned 0 after 
> 22911 usecs
> [  297.905148][ T9418] usb 4-1: PM: calling usb_dev_resume @ 9418, 
> parent: usb4
>
> usb 4-1 usb_dev_resume never returns.
>
> Note - In both cases, usb_dev_resume doesn't return. When it is called 
> is the
> difference.
>
> I don't think suspend/resume works when devices are bound. Suspend 
> exits and
> starts resume which seems to fail because it doesn't handle the 
> virtual usb
> device resume. There is a missing piece here.
>
> When vhci_hcd imports a device and acts as a proxy between the virtual 
> mass
> storage device (e.g in this case) - it appears suspend and resume are
> handled as if it is a usb device. Maybe this is incorrect?
>
> usb_dev_suspend() works and usb_dev_resume() on these virtual usb 
> devices?
> Do we need to handle this in usb_dev_resume()?
>
> Talking out loud - I have to do some looking into.
>
Re:
Yes, your analysis is completely correct.

In fact, I've experimented with adding PM hooks to the faux bus,
and found that faux bus devices then behave identically to platform bus 
devices during suspend/resume.
See the attachment.

This is likely a historical legacy issue.
Regarding this matter, is there anything else you'd like me to assist with?

> thanks,
> -- Shuah
>
>
>
View attachment "dmesg-faux bus-device
 bound-with pm hook-black screen.log" of type "text/x-log" (178589 bytes)

View attachment "0001-driver-core-add-pm-hook-on-faux-bus.patch" of type "text/x-patch" (2843 bytes)

Powered by blists - more mailing lists