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

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

On 9.5.2019 18.38, Nicolas Saenz Julienne wrote:
> Hi Mathias, 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.
>>
>> 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)
> 
> 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.
> 
> 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."

True, xhci driver treats a control transfer as one TD, but TRBs aren't chained so
from hw point of view they are separate TDs

> 
> Also, for what is worth, I spent some time testing that specific case on my
> intel laptop while preparing the patch.

And a closer look at the spec shows that both Normal and Data Stage TRB support
IDT. So this is not likely the cause.

-Mathias