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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 18 Feb 2020 16:21:47 -0800
From:   John Stultz <john.stultz@...aro.org>
To:     Jack Pham <jackp@...eaurora.org>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        Pratham Pratap <prathampratap@...eaurora.org>,
        Felipe Balbi <balbi@...nel.org>, Yang Fei <fei.yang@...el.com>,
        Thinh Nguyen <thinhn@...opsys.com>,
        Tejas Joglekar <tejas.joglekar@...opsys.com>,
        Andrzej Pietrasiewicz <andrzej.p@...labora.com>,
        Todd Kjos <tkjos@...gle.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Linux USB List <linux-usb@...r.kernel.org>,
        stable <stable@...r.kernel.org>
Subject: Re: [PATCH] usb: dwc3: gadget: Update chain bit correctly when using
 sg list

On Tue, Feb 18, 2020 at 4:07 PM Jack Pham <jackp@...eaurora.org> 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@...eaurora.org>
> >
> > 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@...inx.com>
> > who implemented a very similar fix to this issue.
> >
> > Cc: Felipe Balbi <balbi@...nel.org>
> > Cc: Yang Fei <fei.yang@...el.com>
> > Cc: Thinh Nguyen <thinhn@...opsys.com>
> > Cc: Tejas Joglekar <tejas.joglekar@...opsys.com>
> > Cc: Andrzej Pietrasiewicz <andrzej.p@...labora.com>
> > Cc: Jack Pham <jackp@...eaurora.org>
> > Cc: Todd Kjos <tkjos@...gle.com>
> > Cc: Greg KH <gregkh@...uxfoundation.org>
> > Cc: Linux USB List <linux-usb@...r.kernel.org>
> > Cc: stable <stable@...r.kernel.org>
> > Signed-off-by: Pratham Pratap <prathampratap@...eaurora.org>
> > [jstultz: Slight tweak to remove sg_is_last() usage, reworked
> >           commit message, minor comment tweak]
> > Signed-off-by: John Stultz <john.stultz@...aro.org>
> > ---
> >  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