Re: [PATCH v10 4/4] usb: host: enable USB offload during system sleep

[Guan-Yu Lin]

On Tue, Apr 1, 2025 at 10:28 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Apr 01, 2025 at 06:22:42AM +0000, Guan-Yu Lin wrote:
>
> >       /* 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.
>

I think it's still required for remote wakeup supported interfaces to
skip suspend as the suspend procedure. The reason is as the commit
message suggested: "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."
I plan to add the following comment to explain the situation:
/* Don't suspend interfaces with remote wakeup while the controller is active.
 * This preserves pending interrupt urbs, allowing interrupt events to be
 * handled during system suspend.
 */

> >       /* 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.
>

Thanks for identifying this, I'll check for needs_remote_wakeup as
well in the next version.