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 --- 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 + * of sgs passed. Mark the chain bit to false if it is the last + * mapped sg. + */ + if ((i == remaining - 1)) chain = false;
if (rem && usb_endpoint_dir_out(dep->endpoint.desc) && !chain) {
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@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
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.
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);
chain = false;if (rem && usb_endpoint_dir_out(dep->endpoint.desc) && !chain) {
Jack
On Tue, Feb 18, 2020 at 4:07 PM Jack Pham jackp@codeaurora.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@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
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/ ^^^ /dOr 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
Hi,
John Stultz john.stultz@linaro.org writes:
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.
I agree, it's easier to follow the code. Compiler is, mostly, likely optimizing that anyway.
linux-stable-mirror@lists.linaro.org
-
Felipe Balbi -
Jack Pham -
John Stultz