[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <cc96ceae-304e-4113-86f8-06b6037ffd4b@rowland.harvard.edu>
Date: Tue, 1 Apr 2025 10:27:57 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: Guan-Yu Lin <guanyulin@...gle.com>
Cc: gregkh@...uxfoundation.org, mathias.nyman@...el.com,
sumit.garg@...nel.org, kekrby@...il.com,
jeff.johnson@....qualcomm.com, elder@...nel.org,
quic_zijuhu@...cinc.com, ben@...adent.org.uk,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v10 4/4] usb: host: enable USB offload during system sleep
On Tue, Apr 01, 2025 at 06:22:42AM +0000, Guan-Yu Lin wrote:
> Sharing a USB controller with another entity via xhci-sideband driver
> creates power management complexities. To prevent the USB controller
> from being inadvertently deactivated while in use by the other entity, a
> usage-count based mechanism is implemented. This allows the system to
> manage power effectively, ensuring the controller remains available
> whenever needed.
> In order to maintain full functionality of an offloaded USB devices,
> several changes are made within the suspend flow of such devices:
> - skip usb_suspend_device() so that the port/hub are still active for
> USB transfers via offloaded path.
> - not suspending the endpoints which are used by USB interfaces marked
> with needs_remote_wakeup. Namely, skip usb_suspend_interface() and
> usb_hcd_flush_endpoint() on associated USB interfaces. This reserves a
> pending interrupt urb during system suspend for handling the interrupt
> transfer, which is necessary since remote wakeup doesn't apply in the
> offloaded USB devices when controller is still active.
> - not flushing the endpoints of actively offloaded USB devices. Given
> that the USB devices is used by another entity, unilaterally flush the
> endpoint might lead to unexpected behavior on another entity.
> - not suspending the xhci controller. This is done by skipping the
> suspend/resume callbacks in the xhci platform driver.
>
> Signed-off-by: Guan-Yu Lin <guanyulin@...gle.com>
> ---
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -1420,11 +1420,20 @@ static int usb_suspend_both(struct usb_device *udev, pm_message_t msg)
> udev->state == USB_STATE_SUSPENDED)
> goto done;
>
> + if (msg.event == PM_EVENT_SUSPEND && usb_offload_check(udev)) {
> + dev_dbg(&udev->dev, "device offload active, skip suspend.\n");
> + udev->offload_at_suspend = 1;
> + }
> +
> /* Suspend all the interfaces and then udev itself */
> if (udev->actconfig) {
> n = udev->actconfig->desc.bNumInterfaces;
> for (i = n - 1; i >= 0; --i) {
> intf = udev->actconfig->interface[i];
> + if (udev->offload_at_suspend && intf->needs_remote_wakeup) {
Why is intf->needs_remote_wakeup part of this test? There should be a
comment explaining this.
> + dev_dbg(&intf->dev, "active interface on offloaded devices\n");
> + continue;
> + }
> status = usb_suspend_interface(udev, intf, msg);
>
> /* Ignore errors during system sleep transitions */
> @@ -1524,17 +1536,26 @@ static int usb_resume_both(struct usb_device *udev, pm_message_t msg)
> udev->can_submit = 1;
>
> /* Resume the device */
> - if (udev->state == USB_STATE_SUSPENDED || udev->reset_resume)
> - status = usb_resume_device(udev, msg);
> + if (udev->state == USB_STATE_SUSPENDED || udev->reset_resume) {
> + if (!udev->offload_at_suspend)
> + status = usb_resume_device(udev, msg);
> + else
> + dev_dbg(&udev->dev, "device offload active, skip resume.\n");
> + }
>
> /* Resume the interfaces */
> if (status == 0 && udev->actconfig) {
> for (i = 0; i < udev->actconfig->desc.bNumInterfaces; i++) {
> intf = udev->actconfig->interface[i];
> + if (udev->offload_at_suspend) {
> + dev_dbg(&intf->dev, "active interface on offloaded devices\n");
> + continue;
If intf->needs_remote_wakeup wasn't set above then the interface was
suspended. But here you're not going to resume it. That can't be
right.
> + }
> usb_resume_interface(udev, intf, msg,
> udev->reset_resume);
> }
> }
> + udev->offload_at_suspend = 0;
> usb_mark_last_busy(udev);
>
> done:
Alan Stern
Powered by blists - more mailing lists