Re: [PATCH v2] usbip: convert to use faux_device

[Shuah Khan <skhan@linuxfoundation.org>]

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@kylinos.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@kylinos.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?

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.

thanks,
-- Shuah