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

[Greg KH <gregkh@linuxfoundation.org>]

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.
> 
> Failure Modes
> a.Failed standby with auto-wakeup(Log excerpt):
> [ 1449.065592][T10238] PM: suspend entry (s2idle)
> [ 1449.106146][T10238] Filesystems sync: 0.040 seconds
> [ 1449.216189][T10238] Freezing user space processes
> [ 1449.219474][T10238] Freezing user space processes completed (elapsed
> 0.002 seconds)
> [ 1449.219887][T10238] OOM killer disabled.
> [ 1449.220090][T10238] Freezing remaining freezable tasks
> [ 1469.222372][T10238] Freezing remaining freezable tasks failed after
> 20.002 seconds (0 tasks refusing to freeze, wq_busy=1):
> [ 1469.225038][T10238] Showing freezable workqueues that are still busy:
> [ 1469.226176][T10238] workqueue events_freezable_pwr_efficient: flags=0x86
> [ 1469.227453][T10238]   pwq 20: cpus=0-3 node=0 flags=0x4 nice=0 active=1
> refcnt=2
> [ 1469.227463][T10238]     in-flight: 268:disk_events_workfn
> [ 1469.233559][T10238] Restarting kernel threads ... done.
> [ 1469.235119][T10238] OOM killer enabled.
> [ 1469.235849][T10238] Restarting tasks ... done.
> [ 1469.240121][T10238] random: crng reseeded on system resumption
> [ 1469.241176][T10238] PM: suspend exit
> 
> b.Failed standby with black screen freeze:
> [ 1820.667073][T11454] PM: suspend entry (s2idle)

Are you sure this is the usbip driver causing suspend to not work?  If
you unbind the usbip devices does suspend/resume work?

I would think that we would have gotten some reports by now if this
didn't work at all :)

thanks,

greg k-h