From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6FDF7C3404D for ; Wed, 19 Feb 2020 00:22:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 278742465A for ; Wed, 19 Feb 2020 00:22:01 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="uXpGi2Jt" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727940AbgBSAV7 (ORCPT ); Tue, 18 Feb 2020 19:21:59 -0500 Received: from mail-oi1-f193.google.com ([209.85.167.193]:44891 "EHLO mail-oi1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726858AbgBSAV7 (ORCPT ); Tue, 18 Feb 2020 19:21:59 -0500 Received: by mail-oi1-f193.google.com with SMTP id d62so22031570oia.11 for ; Tue, 18 Feb 2020 16:21:58 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=iwevfYaMGMnWqDp7BRBH9ndZK1ueB9FpG7BWDRha664=; b=uXpGi2JtCaKA3afhjgPnx3+k6bu9jRJFFsxC1tdxkFA2xmvGo813n+ZBpK+kHNH/39 aZ7yNwhZeAq6N113hFT+Z7i3q+igLWsyXdAmQCx0FBwKnVkgREb0kVWKfguOHKueZveq ZvOaEfMgOMt2ySw4a1QyCrQJC2t/FNxIRmHkuSurQ9D6XOB2qmC2ne59d1ee/3/6jwaM JtifkDeTXpnBdsiIlxZf5GoQkjGQc5fpmIu0DLDptiQoBobekKlKZo6hn+pha4aLWmPr B+bYAjtTgusI/aORPogBkG+bpo4h/Bu6olv6xoFBj2IjpY4R4Lva9xceCo8cbzAhNQPZ v9Nw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=iwevfYaMGMnWqDp7BRBH9ndZK1ueB9FpG7BWDRha664=; b=CYjF3wXh+CRXbHDK8x62r/n8GD56AV77GindIEr6XcVt0NbjQ3fFkJl6qSJGJ2cHRw EwArBukcL6UyRFLbfp2Ww6aebWyLuuDg+VeG3ZwSyjKPdoDJQeTdyyLiAh2P/sW079eq F8iETwXBACcns1vGd8tuBKOSVFHA6piVa3pmPvDpOPq98dynZx+xGPD/zEImaA2fpIcy NTvcwiyW3vAhKtxEnhUM5MnqOeKMoRXjzl59v+ZbOTqFJP9ZvXGgf807eOvRrYYosYzm AurgNFsa0z8j3agQNX2u18DyKc8ZI9M12JYgYqqsfQf4qp0E1u18JYOrE1zmBh3p6cCm hSWg== X-Gm-Message-State: APjAAAW3uRmxHLBxs1QbrswZSgyXj0xh8Btslhy2Fgg9KlE6w7m3B+n0 5MkA6ypQOKqqS2NDfpokuHGp8Or3Cgyybf8gH6mSAQ== X-Google-Smtp-Source: APXvYqyYiXJ4jQH3RxiDV6J7/uaLgk6BiS53+aqHNeUal6TNgdsW5hDDsiGSQ/HeW/6snAkmK42XqN3LCG9NUaItzAg= X-Received: by 2002:a54:4396:: with SMTP id u22mr3027403oiv.128.1582071718379; Tue, 18 Feb 2020 16:21:58 -0800 (PST) MIME-Version: 1.0 References: <20200218235104.112323-1-john.stultz@linaro.org> <20200219000736.GA5511@jackp-linux.qualcomm.com> In-Reply-To: <20200219000736.GA5511@jackp-linux.qualcomm.com> From: John Stultz Date: Tue, 18 Feb 2020 16:21:47 -0800 Message-ID: Subject: Re: [PATCH] usb: dwc3: gadget: Update chain bit correctly when using sg list To: Jack Pham Cc: lkml , Pratham Pratap , Felipe Balbi , Yang Fei , Thinh Nguyen , Tejas Joglekar , Andrzej Pietrasiewicz , Todd Kjos , Greg KH , Linux USB List , stable Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Feb 18, 2020 at 4:07 PM Jack Pham 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 > > > > 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 > > who implemented a very similar fix to this issue. > > > > Cc: Felipe Balbi > > Cc: Yang Fei > > Cc: Thinh Nguyen > > Cc: Tejas Joglekar > > Cc: Andrzej Pietrasiewicz > > Cc: Jack Pham > > Cc: Todd Kjos > > Cc: Greg KH > > Cc: Linux USB List > > Cc: stable > > Signed-off-by: Pratham Pratap > > [jstultz: Slight tweak to remove sg_is_last() usage, reworked > > commit message, minor comment tweak] > > Signed-off-by: John Stultz > > --- > > 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