Re: [PATCH] usb: dwc3: gadget: Update chain bit correctly when using sg list

[John Stultz]

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.

thanks
-john