Re: [PATCH 1/9] usb: vhci-hcd: Prevent suspending virtually attached devices

[Shuah Khan <skhan@linuxfoundation.org>]

On 7/17/25 12:26, Alan Stern wrote:
> On Thu, Jul 17, 2025 at 06:54:50PM +0300, Cristian Ciocaltea wrote:
>> The VHCI platform driver aims to forbid entering system suspend when at
>> least one of the virtual USB ports are bound to an active USB/IP
>> connection.
>>
>> However, in some cases, the detection logic doesn't work reliably, i.e.
>> when all devices attached to the virtual root hub have been already
>> suspended, leading to a broken suspend state, with unrecoverable resume.
>>
>> Ensure the attached devices do not enter suspend by setting the syscore
>> PM flag.
>>
>> Fixes: 04679b3489e0 ("Staging: USB/IP: add client driver")
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>   drivers/usb/usbip/vhci_hcd.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
>> index e70fba9f55d6a0edf3c5fde56a614dd3799406a1..762b60e10a9415e58147cde2f615045da5804a0e 100644
>> --- a/drivers/usb/usbip/vhci_hcd.c
>> +++ b/drivers/usb/usbip/vhci_hcd.c
>> @@ -765,6 +765,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>>   				 ctrlreq->wValue, vdev->rhport);
>>   
>>   			vdev->udev = usb_get_dev(urb->dev);
>> +			dev_pm_syscore_device(&vdev->udev->dev, true);
>>   			usb_put_dev(old);
>>   
>>   			spin_lock(&vdev->ud.lock);
>> @@ -785,6 +786,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>>   					"Not yet?:Get_Descriptor to device 0 (get max pipe size)\n");
>>   
>>   			vdev->udev = usb_get_dev(urb->dev);
>> +			dev_pm_syscore_device(&vdev->udev->dev, true);
>>   			usb_put_dev(old);
>>   			goto out;
> 
> This looks very strange indeed.
> 
> First, why is vhci_urb_enqueue() the right place to do this?  I should
> think you would want to do this just once per device, at the time it is
> attached.  Not every time a new URB is enqueued.

Correct. This isn't the right place to do this even if we want to go with
the option to prevent suspend. The possible place to do this would be
from rh_port_connect() in which case you will have access to usb_hcd device.

This has to be undone from rh_port_disconnect(). Also how does this impact
the usbip_host - we still need to handle usbip_host suspend.

> 
> Second, how do these devices ever go back to being regular non-syscore
> things?
> 
> Third, if this change isn't merely a temporary placeholder, it certainly
> needs to have a comment in the code to explain what it does and why.
> 
> Fourth, does calling dev_pm_syscore_device() really prevent the device
> from going into suspend?  What about runtime suspend?  And what good
> does it to do prevent the device from being suspended if the entire
> server gets suspended?
> 
> Fifth, the patch description says the purpose is to prevent the server
> from going into system suspend.  How does marking some devices with
> dev_pm_syscore_device() accomplish this?
> 

We have been discussing suspend/resume and reboot behavior in another thread
that proposed converting vhci_hcd to use faux bus.

In addition to what Alan is asking, To handle suspend/resume cleanly, the
following has to happen at a higher level:

- Let the usbip hots host know client is suspending the connection.
   The physical device isn't suspended on the host.
- suspend the virtual devices and vhci_hcd

Do the reverse to resume.

I would say:

- We don't want vhci_hcd and usbip_host preventing suspend
- It might be cleaner and safer to detach the devices during
   suspend on both ends. This is similar to what happens now when
   usbip host and vhci_hcd are removed.
- Note that usbip_host and vhci_hcd don't fully support suspend and
   resume at the moment.

thanks,
-- Shuah