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>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
