diff mbox series

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

Message ID 20200220060616.54389-1-john.stultz@linaro.org (mailing list archive)
State Superseded
Headers show
Series [v2] usb: dwc3: gadget: Update chain bit correctly when using sg list | expand

Commit Message

John Stultz Feb. 20, 2020, 6:06 a.m. UTC 
From: Pratham Pratap <prathampratap@codeaurora.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@xilinx.com>
who implemented a very similar fix to this issue.

Cc: Felipe Balbi <balbi@kernel.org>
Cc: Yang Fei <fei.yang@intel.com>
Cc: Thinh Nguyen <thinhn@synopsys.com>
Cc: Tejas Joglekar <tejas.joglekar@synopsys.com>
Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc: Jack Pham <jackp@codeaurora.org>
Cc: Todd Kjos <tkjos@google.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Linux USB List <linux-usb@vger.kernel.org>
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Pratham Pratap <prathampratap@codeaurora.org>
[jstultz: Slight tweak to remove sg_is_last() usage, reworked
          commit message, minor comment tweak]
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v2:
* Fix typeos and unnecssary parens as suggested by Jack
---
 drivers/usb/dwc3/gadget.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

John Stultz Feb. 25, 2020, 12:14 a.m. UTC | #1 
On Thu, Feb 20, 2020 at 4:49 AM Sasha Levin <sashal@kernel.org> wrote:
>
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all
>
> The bot has tested the following trees: v5.5.4, v5.4.20, v4.19.104, v4.14.171, v4.9.214, v4.4.214.
>
> v5.5.4: Build OK!
> v5.4.20: Build OK!
> v4.19.104: Build OK!
> v4.14.171: Build failed! Errors:
>     drivers/usb/dwc3/gadget.c:1098:12: error: `remaining` undeclared (first use in this function)
>
> v4.9.214: Failed to apply! Possible dependencies:
>     Unable to calculate
>
> v4.4.214: Failed to apply! Possible dependencies:
>     36b68aae8e39 ("usb: dwc3: gadget: use link TRB for all endpoint types")
>     4faf75504a7d ("usb: dwc3: gadget: move % operation to increment helpers")
>     53fd88189e08 ("usb: dwc3: gadget: rename busy/free_slot to trb_enqueue/dequeue")
>     5ee85d890f8d ("usb: dwc3: gadget: split __dwc3_gadget_kick_transfer()")
>     70fdb273db37 ("usb: dwc3: get rid of DWC3_TRB_MASK")
>     8495036e986b ("usb: dwc3: increase maximum number of TRBs per endpoint")
>     c4233573f6ee ("usb: dwc3: gadget: prepare TRBs on update transfers too")
>     e901aa159dac ("usb: dwc3: gadget: fix endpoint renaming")
>     ef966b9d3353 ("usb: dwc3: gadget: add trb enqueue/dequeue helpers")
>
>
> NOTE: The patch will not be queued to stable trees until it is upstream.
>
> How should we proceed with this patch?

Sorry, I only see this change as critical for 4.20+ kernels (where it
started biting folks), but I'd defer to Felipe if he'd like to see it
go any further back.

thanks
-john
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 1b8014ab0b25..721d897fef94 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 is not equal to the number of
+		 * sgs passed. So mark the chain bit to false if it isthe last
+		 * mapped sg.
+		 */
+		if (i == remaining - 1)
 			chain = false;
 
 		if (rem && usb_endpoint_dir_out(dep->endpoint.desc) && !chain) {