Message-ID: <a29703bd-08b7-489b-8fb0-15838a1245ab@163.com>
Date: Wed, 2 Jul 2025 10:12:12 +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/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.
>
> thanks,
> -- Shuah

Powered by blists - more mailing lists