Re: [PATCH 1/4] usb: xhci: add Immediate Data Transfer support

[Mathias Nyman <mathias.nyman@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 8493 bytes --]

On 9.5.2019 14.51, Nicolas Saenz Julienne wrote:
> On Thu, 2019-05-09 at 14:40 +0300, Mathias Nyman wrote:
>> On 9.5.2019 13.32, Marek Szyprowski wrote:
>>> Dear All,
>>>
>>> On 2019-04-26 15:23, Mathias Nyman wrote:
>>>> From: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>>>>
>>>> Immediate data transfers (IDT) allow the HCD to copy small chunks of
>>>> data (up to 8bytes) directly into its output transfer TRBs. This avoids
>>>> the somewhat expensive DMA mappings that are performed by default on
>>>> most URBs submissions.
>>>>
>>>> In the case an URB was suitable for IDT. The data is directly copied
>>>> into the "Data Buffer Pointer" region of the TRB and the IDT flag is
>>>> set. Instead of triggering memory accesses the HC will use the data
>>>> directly.
>>>>
>>>> The implementation could cover all kind of output endpoints. Yet
>>>> Isochronous endpoints are bypassed as I was unable to find one that
>>>> matched IDT's constraints. As we try to bypass the default DMA mappings
>>>> on URB buffers we'd need to find a Isochronous device with an
>>>> urb->transfer_buffer_length <= 8 bytes.
>>>>
>>>> The implementation takes into account that the 8 byte buffers provided
>>>> by the URB will never cross a 64KB boundary.
>>>>
>>>> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>>>> Reviewed-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>>>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>>>
>>> I've noticed that this patch causes regression on various Samsung Exynos
>>> 5420/5422/5800 boards, which have USB3.0 host ports provided by
>>> DWC3/XHCI hardware module. The regression can be observed with ASIX USB
>>> 2.0 ethernet dongle, which stops working after applying this patch (eth0
>>> interface is registered, but no packets are transmitted/received). I can
>>> provide more debugging information or do some tests, just let me know
>>> what do you need. Reverting this commit makes ASIX USB ethernet dongle
>>> operational again.
>>>
>>
>> Thanks for reporting.
>>
>> Would it be possible to check if your ASIX ethernet dongle works on some
>> desktop/laptop setup with this same IDT patch?
>>
>> Also Exynos xhci traces could help, they would show the content of the TRBs
>> using IDT.
>> Maybe byte order gets messed up?
>>
>> Take traces with:
>>
>> mount -t debugfs none /sys/kernel/debug
>> echo 81920 > /sys/kernel/debug/tracing/buffer_size_kb
>> echo 1 > /sys/kernel/debug/tracing/events/xhci-hcd/enable
>>
>> <connect ASIX eth dongle, try to use it>
>>
>> send /sys/kernel/debug/tracing/trace content to me
>>
>> If we can't get this fixed I'll revert the IDT patch
> 
> Hi Matthias, thanks for your help.
> 
> I'll also be looking into it, so please send me the logs too.
> 

Got the logs off list, thanks

The "Buffer" data in Control transfer Data stage look suspicious.

grep "flags I:" trace_fail  | grep Data
kworker/0:2-124   [000] d..1    63.092399: xhci_queue_trb: CTRL: Buffer 0000000018b65000 length 6 TD size 0 intr 0 type 'Data Stage' flags I:i:c:s:i:e:C
ifconfig-1429  [005] d..1    93.181231: xhci_queue_trb: CTRL: Buffer 0000000018b65000 length 6 TD size 0 intr 0 type 'Data Stage' flags I:i:c:s:i:e:C
ifconfig-1429  [007] dn.2    93.182050: xhci_queue_trb: CTRL: Buffer 0000000000000000 length 8 TD size 0 intr 0 type 'Data Stage' flags I:i:c:s:i:e:C
ifconfig-1429  [007] d..2    93.182499: xhci_queue_trb: CTRL: Buffer 0000000080000000 length 8 TD size 0 intr 0 type 'Data Stage' flags I:i:c:s:i:e:C
ifconfig-1429  [007] d..2    93.182736: xhci_queue_trb: CTRL: Buffer 0000000080000000 length 8 TD size 0 intr 0 type 'Data Stage' flags I:i:c:s:i:e:C
kworker/0:3-1409  [000] d..3    93.382630: xhci_queue_trb: CTRL: Buffer 0000000080000000 length 8 TD size 0 intr 0 type 'Data Stage' flags I:i:c:s:i:e:C

First guess would be that in case URB has URB_NO_TRANSFER_DMA_MAP set then data
will be mapped and urb->transfer_dma is already set.
The IDT patch uses urb->trabfer_dma as a temporary buffer, and copies the
urb->transfer_buffer there.
if transfer buffer is already dma mapped the urb->transfer_buffer can be garbage,
(shouldn't, but it can be)

Below code avoids IDT if URB_NO_TRANSFER_DMA_MAP is set, and doesn't touch
urb->transfer_dma (patch attached)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index fed3385..f080054 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3423,11 +3423,14 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
  
         if (urb->transfer_buffer_length > 0) {
                 u32 length_field, remainder;
+               u64 addr;
  
                 if (xhci_urb_suitable_for_idt(urb)) {
-                       memcpy(&urb->transfer_dma, urb->transfer_buffer,
+                       memcpy(&addr, urb->transfer_buffer,
                                urb->transfer_buffer_length);
                         field |= TRB_IDT;
+               } else {
+                       addr = (u64) urb->transfer_dma;
                 }
  
                 remainder = xhci_td_remainder(xhci, 0,
@@ -3440,8 +3443,8 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
                 if (setup->bRequestType & USB_DIR_IN)
                         field |= TRB_DIR_IN;
                 queue_trb(xhci, ep_ring, true,
-                               lower_32_bits(urb->transfer_dma),
-                               upper_32_bits(urb->transfer_dma),
+                               lower_32_bits(addr),
+                               upper_32_bits(addr),
                                 length_field,
                                 field | ep_ring->cycle_state);
         }
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index a450a99..7f8b950 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -2160,7 +2160,8 @@ static inline bool xhci_urb_suitable_for_idt(struct urb *urb)
  {
         if (!usb_endpoint_xfer_isoc(&urb->ep->desc) && usb_urb_dir_out(urb) &&
             usb_endpoint_maxp(&urb->ep->desc) >= TRB_IDT_MAX_SIZE &&
-           urb->transfer_buffer_length <= TRB_IDT_MAX_SIZE)
+           urb->transfer_buffer_length <= TRB_IDT_MAX_SIZE &&
+           !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP))
                 return true;
  
         return false;

If that doesn't help, then it's possible DATA trbs in control transfer can't
use IDT at all. IDT is supported for Normal TRBs, which have a different trb
type than DATA trbs in control transfers.

Also xhci specs 4.11.7 limit IDT usage:

"If the IDT flag is set in one TRB of a TD, then it shall be the only Transfer
  TRB of the TD"

A whole control transfer is one TD, and it already contains a SETUP transfer TRB
which is using the IDT flag.

Following disables IDT for control transfers (testpatch attached as well)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index fed3385..4c1c9ad 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3424,12 +3424,6 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
         if (urb->transfer_buffer_length > 0) {
                 u32 length_field, remainder;
  
-               if (xhci_urb_suitable_for_idt(urb)) {
-                       memcpy(&urb->transfer_dma, urb->transfer_buffer,
-                              urb->transfer_buffer_length);
-                       field |= TRB_IDT;
-               }
-
                 remainder = xhci_td_remainder(xhci, 0,
                                 urb->transfer_buffer_length,
                                 urb->transfer_buffer_length,
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index a450a99..2e16ff7 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -2158,9 +2158,11 @@ static inline struct xhci_ring *xhci_urb_to_transfer_ring(struct xhci_hcd *xhci,
   */
  static inline bool xhci_urb_suitable_for_idt(struct urb *urb)
  {
-       if (!usb_endpoint_xfer_isoc(&urb->ep->desc) && usb_urb_dir_out(urb) &&
+       if (!usb_endpoint_xfer_control(&urb->ep->desc) &&
+           !usb_endpoint_xfer_isoc(&urb->ep->desc) && usb_urb_dir_out(urb) &&
             usb_endpoint_maxp(&urb->ep->desc) >= TRB_IDT_MAX_SIZE &&
-           urb->transfer_buffer_length <= TRB_IDT_MAX_SIZE)
+           urb->transfer_buffer_length <= TRB_IDT_MAX_SIZE &&
+           !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP))
                 return true;
  
         return false;

-Mathias

[-- Attachment #2: 0001-xhci-Don-t-use-IDT-tranfers-when-not-supported.patch --]
[-- Type: text/x-patch, Size: 2095 bytes --]

From c92d0e83f24d9a8f401ef5c91ebc8263fd9d2a56 Mon Sep 17 00:00:00 2001
From: Mathias Nyman <mathias.nyman@linux.intel.com>
Date: Thu, 9 May 2019 17:47:28 +0300
Subject: [TESTPATCH2] xhci: Don't use IDT tranfers when not supported

control tranfer data stage can't support IDT as xHCI can't have two
IDT flags in the same TD.
The SETUP stage is already using IDT
see xhci 4.11.7 for details

Also don't use IDT if tranfer buffer is already dma mapped.
There is no benefit with IDT then.

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 6 ------
 drivers/usb/host/xhci.h      | 6 ++++--
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index fed3385..4c1c9ad 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3424,12 +3424,6 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 	if (urb->transfer_buffer_length > 0) {
 		u32 length_field, remainder;
 
-		if (xhci_urb_suitable_for_idt(urb)) {
-			memcpy(&urb->transfer_dma, urb->transfer_buffer,
-			       urb->transfer_buffer_length);
-			field |= TRB_IDT;
-		}
-
 		remainder = xhci_td_remainder(xhci, 0,
 				urb->transfer_buffer_length,
 				urb->transfer_buffer_length,
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index a450a99..2e16ff7 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -2158,9 +2158,11 @@ static inline struct xhci_ring *xhci_urb_to_transfer_ring(struct xhci_hcd *xhci,
  */
 static inline bool xhci_urb_suitable_for_idt(struct urb *urb)
 {
-	if (!usb_endpoint_xfer_isoc(&urb->ep->desc) && usb_urb_dir_out(urb) &&
+	if (!usb_endpoint_xfer_control(&urb->ep->desc) &&
+	    !usb_endpoint_xfer_isoc(&urb->ep->desc) && usb_urb_dir_out(urb) &&
 	    usb_endpoint_maxp(&urb->ep->desc) >= TRB_IDT_MAX_SIZE &&
-	    urb->transfer_buffer_length <= TRB_IDT_MAX_SIZE)
+	    urb->transfer_buffer_length <= TRB_IDT_MAX_SIZE &&
+	    !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP))
 		return true;
 
 	return false;
-- 
2.7.4


[-- Attachment #3: 0001-xhci-don-t-use-IDT-transfer-buffer-is-already-dma-ma.patch --]
[-- Type: text/x-patch, Size: 2157 bytes --]

From 485c5ede4d2e237c7f28f0cbb891151f88718e5b Mon Sep 17 00:00:00 2001
From: Mathias Nyman <mathias.nyman@linux.intel.com>
Date: Thu, 9 May 2019 17:47:28 +0300
Subject: [TESTPATCH1] xhci: don't use IDT transfer buffer is already dma
 mapped

Limit IDT usage if transfer buffer is already mapped.
Also don't use urb->transfer_dma as a temporary buffer

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ring.c | 9 ++++++---
 drivers/usb/host/xhci.h      | 3 ++-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index fed3385..f080054 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3423,11 +3423,14 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 
 	if (urb->transfer_buffer_length > 0) {
 		u32 length_field, remainder;
+		u64 addr;
 
 		if (xhci_urb_suitable_for_idt(urb)) {
-			memcpy(&urb->transfer_dma, urb->transfer_buffer,
+			memcpy(&addr, urb->transfer_buffer,
 			       urb->transfer_buffer_length);
 			field |= TRB_IDT;
+		} else {
+			addr = (u64) urb->transfer_dma;
 		}
 
 		remainder = xhci_td_remainder(xhci, 0,
@@ -3440,8 +3443,8 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
 		if (setup->bRequestType & USB_DIR_IN)
 			field |= TRB_DIR_IN;
 		queue_trb(xhci, ep_ring, true,
-				lower_32_bits(urb->transfer_dma),
-				upper_32_bits(urb->transfer_dma),
+				lower_32_bits(addr),
+				upper_32_bits(addr),
 				length_field,
 				field | ep_ring->cycle_state);
 	}
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index a450a99..7f8b950 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -2160,7 +2160,8 @@ static inline bool xhci_urb_suitable_for_idt(struct urb *urb)
 {
 	if (!usb_endpoint_xfer_isoc(&urb->ep->desc) && usb_urb_dir_out(urb) &&
 	    usb_endpoint_maxp(&urb->ep->desc) >= TRB_IDT_MAX_SIZE &&
-	    urb->transfer_buffer_length <= TRB_IDT_MAX_SIZE)
+	    urb->transfer_buffer_length <= TRB_IDT_MAX_SIZE &&
+	    !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP))
 		return true;
 
 	return false;
-- 
2.7.4