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;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 39E0EC433D2
Date:   Fri, 24 Apr 2020 15:48:43 -0700
From:   Jack Pham <jackp@codeaurora.org>
To:     John Stultz <john.stultz@linaro.org>
Cc:     Felipe Balbi <balbi@kernel.org>, 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>,
        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
Message-ID: <20200424224843.GB20167@jackp-linux.qualcomm.com>
References: <CALAqxLW2R4d=Zm=TKbFprN-uYrerL1oCYsVC3VedEKtW0gCsyA@mail.gmail.com>
 <877dyfsv00.fsf@kernel.org>
 <CALAqxLUdzKRV6nrcLpWsykK+WPnqhUK4iwRe4_Xmo-TvEV5KOg@mail.gmail.com>
 <CALAqxLWEdHrsU+efgsp2EHsgNGA8n7SE16XNnZHcfXjdM4v-WQ@mail.gmail.com>
 <20200424171247.GA20167@jackp-linux.qualcomm.com>
 <CALAqxLUkg8gqY6kN1D=NbpgLDd_yMdvxOJCksrmXw0v8McHodw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CALAqxLUkg8gqY6kN1D=NbpgLDd_yMdvxOJCksrmXw0v8McHodw@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi John,

On Fri, Apr 24, 2020 at 11:36:24AM -0700, John Stultz wrote:
> On Fri, Apr 24, 2020 at 10:12 AM Jack Pham <jackp@codeaurora.org> wrote:
> > On Tue, Apr 21, 2020 at 10:09:27PM -0700, John Stultz wrote:
> > > Does something like this make sense? It's not causing trouble on
> > > db845c either so far in my testing.
> >
> > Ok I'll bite...
> >
> > I'm now curious why it hasn't been a problem with the Qualcomm HW. Do
> > you mind please capturing a similar trace log on the db845c?  Would be
> > good to see a side-by-side comparison and see if, first of all, whether
> > the same S/G path is getting exercised (i.e. 16KiB OUT requests from ADB
> > userspace using AIO which then get broken up into 4K chunks by f_fs),
> > and what the behaviors of the reclaim_trb and giveback are when the
> > transfer is completed.
> >
> > Preferably if you could get a trace without your patch applied that
> > would be great. And maybe also one after your patch just to see if the
> > traces are truly identical or not.
> 
> Sure. I've captured logs in the same manner with and without on db845c
> (against 5.7-rc2). See attached.

Thank you!
 
> I suspect the difference is the db845c is using an iommu (I don't
> think it will boot without it) where hikey960 isn't, but I'll let you
> take a look.

Yes I think that's exactly what's happening. Those 16KiB requests on
ep1out would normally be passed as 4x4KiB sglists from f_fs.c but after
the call to usb_gadget_map_request() the IOMMU is coalescing them back
into a single entry, so for each of those requests we end up preparing
just a single unchained TRB.

   UsbFfs-worker-532   [007] d..1    96.025897: dwc3_alloc_request: ep1out: req 0000000075c0b6d7 length 0/0 zsI ==> 0
   UsbFfs-worker-532   [007] d..2    96.025898: dwc3_ep_queue: ep1out: req 0000000075c0b6d7 length 0/16384 zsI ==> -115
   UsbFfs-worker-532   [007] d..2    96.025908: dwc3_prepare_trb: ep1out: trb 00000000c0c9cf9f (E217:D209) buf 00000000ff930000 size 16384 ctrl 00000819 (HlcS:sC:normal)
                                                                              ^^^^^^^^^^^^^^^^
-> trb c0c9cf9f enqueued at position 216 in the ring (enqueue pointer 217)
We can see the pointer to the DMA address and it's 16KiB, and the chain
bit is off.

   UsbFfs-worker-532   [007] d..2    96.025912: dwc3_readl: addr 00000000ab36a89f value 00002400
   UsbFfs-worker-532   [007] d..2    96.025915: dwc3_writel: addr 00000000057ac193 value 00000000
   UsbFfs-worker-532   [007] d..2    96.025917: dwc3_writel: addr 000000009c937859 value 00000000
   UsbFfs-worker-532   [007] d..2    96.025919: dwc3_writel: addr 00000000a91887be value 00000000
   UsbFfs-worker-532   [007] d..2    96.025922: dwc3_writel: addr 00000000679c8ad6 value 00020007
   UsbFfs-worker-532   [007] d..2    96.025924: dwc3_readl: addr 00000000679c8ad6 value 00020007
   UsbFfs-worker-532   [007] d..2    96.025925: dwc3_gadget_ep_cmd: ep1out: cmd 'Update Transfer' [20007] params 00000000 00000000 00000000 --> status: Successful

...

    irq/142-dwc3-529   [000] d..1    96.027952: dwc3_event: event (00006084): ep1out: Transfer In Progress [0] (SIm)
    irq/142-dwc3-529   [000] d..1    96.027955: dwc3_complete_trb: ep1out: trb 00000000c0c9cf9f (E224:D217) buf 00000000ff930000 size 16360 ctrl 00000818 (hlcS:sC:normal)
                                                                               ^^^^^^^^^^^^^^^^
    irq/142-dwc3-529   [000] d..1    96.027965: dwc3_gadget_giveback: ep1out: req 0000000075c0b6d7 length 24/16384 zsI ==> 0

That same trb c0c9cf9f is completed, 24 (16384 - 16360) bytes were
transferred, and dwc3 gives back the request to the function driver.
TRB dequeue pointer advanced by one to position 217.

    irq/142-dwc3-529   [000] d..1    96.027970: dwc3_readl: addr 0000000054b9cc02 value 80001000
    irq/142-dwc3-529   [000] d..1    96.027971: dwc3_writel: addr 0000000054b9cc02 value 00001000
    irq/142-dwc3-529   [000] d..1    96.027974: dwc3_writel: addr 00000000e4e556e6 value 80000000
    irq/142-dwc3-529   [000] d..1    96.027976: dwc3_writel: addr 000000001139226c value 00000001

So it's interesting that with IOMMU always enabled on the db845c we
don't hit the bug as we avoid preparing an SG/chained TRB list.

Thanks,
Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project