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

[Shuah Khan]

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@xxxxxxxxxx>
> > > > > >
> > > > > > 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@xxxxxxxxxx>
> > > > > > ---
> > > > > > 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?

thanks,
-- Shuah