Hi Mathias,
On 2019-05-09 17:10, Mathias Nyman wrote:
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@xxxxxxx>
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@xxxxxxx>
Reviewed-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
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
Thanks for the patches to test! Both patches applied separately (without
the other one) fixes the issue with ASIX USB dongle, but from the
discussion I assume that the first one
(0001-xhci-don-t-use-IDT-transfer-buffer-is-already-dma-ma.patch) really
fixes the issue, while the second one is just a workaround.
You can add:
Reported-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
