Re: [PATCH] usb: dwc3: gadget: Update chain bit correctly when using sg list
From: John Stultz
Date: Tue Feb 18 2020 - 19:22:01 EST
On Tue, Feb 18, 2020 at 4:07 PM Jack Pham <jackp@xxxxxxxxxxxxxx> wrote:
> Hi John,
> Thanks for following-up with this! While you're doing minor tweaks
> anyway, I hope you don't mind me picking some nits below.
> On Tue, Feb 18, 2020 at 11:51:04PM +0000, John Stultz wrote:
> > From: Pratham Pratap <prathampratap@xxxxxxxxxxxxxx>
> > If scatter-gather operation is allowed, a large USB request is split
> > into multiple TRBs. For preparing TRBs for sg list, driver iterates
> > over the list and creates TRB for each sg and mark the chain bit to
> > false for the last sg. The current IOMMU driver is clubbing the list
> > of sgs which shares a page boundary into one and giving it to USB driver.
> > With this the number of sgs mapped it not equal to the the number of sgs
> > passed. Because of this USB driver is not marking the chain bit to false
> > since it couldn't iterate to the last sg. This patch addresses this issue
> > by marking the chain bit to false if it is the last mapped sg.
> > At a practical level, this patch resolves USB transfer stalls
> > seen with adb on dwc3 based db845c, pixel3 and other qcom
> > hardware after functionfs gadget added scatter-gather support
> > around v4.20.
> > Credit also to Anurag Kumar Vulisha <anurag.kumar.vulisha@xxxxxxxxxx>
> > who implemented a very similar fix to this issue.
> > Cc: Felipe Balbi <balbi@xxxxxxxxxx>
> > Cc: Yang Fei <fei.yang@xxxxxxxxx>
> > Cc: Thinh Nguyen <thinhn@xxxxxxxxxxxx>
> > Cc: Tejas Joglekar <tejas.joglekar@xxxxxxxxxxxx>
> > Cc: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxxxx>
> > Cc: Jack Pham <jackp@xxxxxxxxxxxxxx>
> > Cc: Todd Kjos <tkjos@xxxxxxxxxx>
> > Cc: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Linux USB List <linux-usb@xxxxxxxxxxxxxxx>
> > Cc: stable <stable@xxxxxxxxxxxxxxx>
> > Signed-off-by: Pratham Pratap <prathampratap@xxxxxxxxxxxxxx>
> > [jstultz: Slight tweak to remove sg_is_last() usage, reworked
> > commit message, minor comment tweak]
> > Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> > ---
> > drivers/usb/dwc3/gadget.c | 9 ++++++++-
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 1b8014ab0b25..10aa511051e8 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -1071,7 +1071,14 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
> > unsigned int rem = length % maxp;
> > unsigned chain = true;
> > - if (sg_is_last(s))
> > + /*
> > + * IOMMU driver is coalescing the list of sgs which shares a
> > + * page boundary into one and giving it to USB driver. With
> > + * this the number of sgs mapped it not equal to the the number
> ^^ s/it/is/ ^^^ /d
> Or could we more specifically say "number of sgs mapped could be less
> than number passed"?
> > + * of sgs passed. Mark the chain bit to false if it is the last
> > + * mapped sg.
> > + */
> > + if ((i == remaining - 1))
> These outer parens are superfluous.
Thanks for catching these. I'll respin here shortly.
> Also wondering if it would be more or less clear to just set the
> variable once (and awkwardly move the comment to appear above the
> local var declaration):
> unsigned chain = (i < remaining - 1);
Personally, I think doing it via the conditional makes the logic a bit
less taxing to read/skim. So I might keep that bit as is.