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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAOuDEK19FeZsR_0-4t02kszcAVH2JMiZ1D-z-sd9yK1beMfG_g@mail.gmail.com>
Date: Thu, 3 Apr 2025 20:09:27 +0800
From: Guan-Yu Lin <guanyulin@...gle.com>
To: linux-usb@...r.kernel.org
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH v10 4/4] usb: host: enable USB offload during system sleep

On Tue, Apr 1, 2025 at 10:28 PM Alan Stern <stern@...land.harvard.edu> 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.