Hi Matthias, thanks for spending the time debugging this :)
On Thu, 2019-05-09 at 18:10 +0300, Mathias Nyman wrote:
> 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;
>
This could be it, I broadly checked and assumed everyone was playing nice with
the transfer_dma pointer, but I guess I might have missed something.
> 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)
This one I'm not so sure as the standard defines a control transfer as a 2 or 3
TD operation (see 4.11.2.2):
"The Control Transfer Ring may contain Setup Stage and Status Stage TDs, and
optionally Data Stage TDs."
Also, for what is worth, I spent some time testing that specific case on my
intel laptop while preparing the patch.
Regards,
Nicolas
Attachment:
signature.asc
Description: This is a digitally signed message part
