linux-kernel - Re: [RFC][PATCH 1/2] usb: dwc3: gadget: Check for IOC/LST bit in both event->status and TRB->ctrl fields

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALAqxLUeBf2Jx2tLW1yzJk6JHM0RP9cJbTt7m19Qdz-rWMw2mQ@mail.gmail.com>
Date:   Thu, 23 Jan 2020 16:47:51 -0800
From:   John Stultz <john.stultz@...aro.org>
To:     Felipe Balbi <balbi@...nel.org>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        Anurag Kumar Vulisha <anurag.kumar.vulisha@...inx.com>,
        Yang Fei <fei.yang@...el.com>,
        Thinh Nguyen <thinhn@...opsys.com>,
        Tejas Joglekar <tejas.joglekar@...opsys.com>,
        Andrzej Pietrasiewicz <andrzej.p@...labora.com>,
        Jack Pham <jackp@...eaurora.org>, Todd Kjos <tkjos@...gle.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Linux USB List <linux-usb@...r.kernel.org>,
        stable <stable@...r.kernel.org>
Subject: Re: [RFC][PATCH 1/2] usb: dwc3: gadget: Check for IOC/LST bit in both
 event->status and TRB->ctrl fields

On Wed, Jan 22, 2020 at 11:23 PM Felipe Balbi <balbi@...nel.org> wrote:
> > From: Anurag Kumar Vulisha <anurag.kumar.vulisha@...inx.com>
> >
> > The present code in dwc3_gadget_ep_reclaim_completed_trb() will check
> > for IOC/LST bit in the event->status and returns if IOC/LST bit is
> > set. This logic doesn't work if multiple TRBs are queued per
> > request and the IOC/LST bit is set on the last TRB of that request.
> > Consider an example where a queued request has multiple queued TRBs
> > and IOC/LST bit is set only for the last TRB. In this case, the Core
> > generates XferComplete/XferInProgress events only for the last TRB
> > (since IOC/LST are set only for the last TRB). As per the logic in
> > dwc3_gadget_ep_reclaim_completed_trb() event->status is checked for
> > IOC/LST bit and returns on the first TRB. This makes the remaining
> > TRBs left unhandled.
> > To aviod this, changed the code to check for IOC/LST bits in both
>      avoid
>
> > event->status & TRB->ctrl. This patch does the same.
>
> We don't need to check both. It's very likely that checking the TRB is
> enough.

Sorry, just to clarify, are you suggesting instead of:
-       if (event->status & DEPEVT_STATUS_IOC)
+       if ((event->status & DEPEVT_STATUS_IOC) &&
+           (trb->ctrl & DWC3_TRB_CTRL_IOC))


We do something like:
-       if (event->status & DEPEVT_STATUS_IOC)
+       if (trb->ctrl & DWC3_TRB_CTRL_IOC)
+               return 1;
+
+       if (trb->ctrl & DWC3_TRB_CTRL_LST)
                return 1;

?

> > At a practical level, this patch resolves USB transfer stalls seen
> > with adb on dwc3 based HiKey960 after functionfs gadget added
> > scatter-gather support around v4.20.
>
> Right, I remember asking for tracepoint data showing this problem
> happening. It's the best way to figure out what's really going on.
>
> Before we accept these two patches, could you collect dwc3 tracepoint
> data and share here?

Sure. Attached is trace logs and regdumps for hikey960.

The one gotcha with the logs is that in the working case (with this
patch applied), I booted with the usb-c cable disconnected (as
suggested in the dwc3.rst doc), enabled tracing and plugged in the
device, then ran adb logcat a few times to validate no stalls.

In the failure case (without this patch), I booted with the usb-c
cable disconnected, enabled tracing and then when I plugged in the
device, it never was detected by adb (it seems perhaps the problem had
already struck?).

So I generated the failure2 log by booting with USB-C plugged in,
enabling tracing, and running adb logcat on the host to observe the
stall.

Anyway, all three sets of logs are included. Let me know if you need
me to try anything else.

thanks
-john

Download attachment "hikey960.tar.xz" of type "application/octet-stream" (322412 bytes)