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
