Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Felipe Balbi <balbi@kernel.org>
To:     John Stultz <john.stultz@linaro.org>
Cc:     Josh Gao <jmgao@google.com>, YongQin Liu <yongqin.liu@linaro.org>,
        Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>,
        Yang Fei <fei.yang@intel.com>,
        Thinh Nguyen <thinhn@synopsys.com>,
        Tejas Joglekar <tejas.joglekar@synopsys.com>,
        Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
        Jack Pham <jackp@codeaurora.org>, Todd Kjos <tkjos@google.com>,
        Greg KH <gregkh@linuxfoundation.org>,
        Linux USB List <linux-usb@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>
Subject: Re: More dwc3 gadget issues with adb
In-Reply-To: <CALAqxLUdzKRV6nrcLpWsykK+WPnqhUK4iwRe4_Xmo-TvEV5KOg@mail.gmail.com>
References: <CALAqxLW2R4d=Zm=TKbFprN-uYrerL1oCYsVC3VedEKtW0gCsyA@mail.gmail.com> <877dyfsv00.fsf@kernel.org> <CALAqxLUdzKRV6nrcLpWsykK+WPnqhUK4iwRe4_Xmo-TvEV5KOg@mail.gmail.com>
Date:   Wed, 22 Apr 2020 09:00:05 +0300
Message-ID: <87blnkcb6i.fsf@kernel.org>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
        micalg=pgp-sha256; protocol="application/pgp-signature"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable


Hi,

John Stultz <john.stultz@linaro.org> writes:
> On Thu, Apr 16, 2020 at 1:19 AM Felipe Balbi <balbi@kernel.org> wrote:
>> One thing I noticed is that we're missing a giveback on ep1out. Here's a
>> working case:
>>
>
> Hey Felipe,
>   So I found some time to dig around on this today and I started
> trying to understand this issue that you've pointed out about missing
> the giveback.
>
> It seems part of the issue is we get to a point where we have some req
> where pending_sgs is more than one.
>
> We call dwc3_prepare_one_trb_sg() on it:
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree=
/drivers/usb/dwc3/gadget.c?h=3Dv5.7-rc2#n1068
>
> And we process the sg list incrementing req->num_queued_sgs for each one.
>
> then later, dwc3_gadget_ep_cleanup_completed_request() is called on the r=
equest:
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree=
/drivers/usb/dwc3/gadget.c?h=3Dv5.7-rc2#n2522
>
> We call dwc3_gadget_ep_reclaim_trb_sg()
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree=
/drivers/usb/dwc3/gadget.c?h=3Dv5.7-rc2#n2470
>
> Where we iterate over the req->sg, ideally decrementing
> num_pending_sgs each time and return.
>
> But back in dwc3_gadget_ep_cleanup_completed_request()  and there
> we're hitting the:
>   if (!dwc3_gadget_ep_request_completed(req) ||
>       req->num_pending_sgs) {
> case which causes us to skip the call to dwc3_gadget_giveback().

This logic was modified recently. Can you check if today's linus/master
works for you?

> Looking as to why the num_pending_sgs is non zero, that's because in
> dwc3_gadget_ep_reclaim_trb_sg we're hitting the case where the trb has
> the DWC3_TRB_CTRL_HWO ctrl flag set, which breaks us out of the loop
> early before we decrement num_pending_sgs.
>
> For that trb, we're setting the HWO flag in __dwc3_prepare_one_trb()
> (called from dwc3_prepare_one_trb_sg() back at the beginning):
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree=
/drivers/usb/dwc3/gadget.c?h=3Dv5.7-rc2#n921
>
> I added logic showing every time we set or clear that flag, and it
> seems like we're always setting it but never clearing it. And often

HW clears HWO flag. We only have to manually clear in one or two cases.

> that's not an issue as we only have one sg entry. But if its set on a
> trb in a request with multiple sgs, that's where it seems to be
> causing the issue.

The issue is completing with HWO set, which should never happen. Can you
collect tracepoints with linus/master of this particular situation?

>> One interesting thing is that TRB addresses are "odd". I can't find a
>> proper lifetime for these TRBs. Do you have IOMMU enabled? Can you run
>> without it? For example, nowhere in the log can I find the place where
>> trb 0000000092deef41 was first enqueue. I'm assuming the log to be
>> ordered, which means that trb is the same as 00000000f3db4076. But why
>> are the addresses different?
>>
>> Another weird thing is that even though we CHN bit being set in
>> 0000000092deef41, we don't see where the second trb (the one its chained
>> to) was prepared. It seems like it was *never* prepared, what gives?
>
> I suspect these bits were due to the tracing happening after some
> minor amount of initial adb traffic began at bootup? So the trace
> isn't capturing all the events.

No, no. That's more likely to be IOMMU mucking up the addresses. ADB is
very sequential in its behavior and USB transfers requests in
order. Please run linus/master without IOMMU.

cheers

=2D-=20
balbi

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAl6f3WUACgkQzL64meEa
mQZdyQ//R324qRyXNq8MYvp3qnlSGkCmXCbzy6OKz3lx25FvUYXMBfnCTvfjCJAV
MuDNe3mqC8EA+ubqORcmFgshfLpLbXZUTkZD40rqbyP7BgN3xtbxwE3wD0ObNe0J
EY8eY9lwCKkjylTsUQn+3AiRRuxF3RcKPoTwuQOIbuR1MOUltGfgrFhQnGuaITrn
8CD5q5Ij45LpPUYdFZ+fCB+NQLYoKjau6ATbFUo4vqkxekBQM1CA9+NnYM9nronM
UwyM1USl7YU9nSWDwr+3pUFgO/Wv3OWe1beVMR+M+jPZ4cm8j/ABld3xnYXK7RsD
/qKgXslcPGIDefmD+TLXOfw+wzOs0f72yaWWaIaxS/mBeGLiI0bCc1WnXfc+JHGt
2fpFN9ut6s+l7AdeOVd2Xz+FhmOjSyPMCw3QcYp5EYhku7Ebf+dGzfsSecZ1TYnf
iwoJOIQCR/Ij2nU07ZCtGXHLpH0XB/RXLu5B6pGCmhkTD0BRHypwHuPtTX4TYrqx
ncjWl0zT4bRwejoN7nc6IVD5T8AFwceRbY50IigGTjHhjM7TLTt2doDo8bTROnrQ
XjHzFQXtHtHIHjeSUlBG81XZWFR9yafczP7op//YGPxoATK5bel2L2ZKmlsZ8400
NHivJbppwKkEWH7zvrGli/1Sg2OFZYueRu+F/BWy2cxpcAI9Eiw=
=oteR
-----END PGP SIGNATURE-----
--=-=-=--