Re: [RFC PATCH 1/4] usb: xhci: Synopsys xHC consolidate TRBs

[Tejas Joglekar <Tejas.Joglekar@synopsys.com>]

Hi,
Thanks for the review comments.
On 1/3/2020 10:14 PM, Mathias Nyman wrote:
> On 20.12.2019 15.39, Tejas Joglekar wrote:
>> The Synopsys xHC has an internal TRB cache of size TRB_CACHE_SIZE for
>> each endpoint. The default value for TRB_CACHE_SIZE is 16 for SS and 8
>> for HS. The controller loads and updates the TRB cache from the transfer
>> ring in system memory whenever the driver issues a start transfer or
>> update transfer command.
>>
>> For chained TRBs, the Synopsys xHC requires that the total amount of
>> bytes for all TRBs loaded in the TRB cache be greater than or equal to 1
>> MPS. Or the chain ends within the TRB cache (with a last TRB).
>>
>> If this requirement is not met, the controller will not be able to send
>> or receive a packet and it will hang causing a driver timeout and error.
>>
>> This can be a problem if a class driver queues SG requests with many
>> small-buffer entries. The XHCI driver will create a chained TRB for each
>> entry which may trigger this issue.
>>
>> This patch adds logic to the XHCI driver to detect and prevent this from
>> happening.
>>
>> For every (TRB_CACHE_SIZE - 2) TRBs, we check the total buffer size of
>> the TRBs and if the chain continues and we don't make up at least 1 MPS,
>> we create a bounce buffer to consolidate up to the next (4 * MPS) TRBs
>> into the last TRB.
>>
>> We check at (TRB_CACHE_SIZE - 2) because it is possible that there would
>> be a link and/or event data TRB that take up to 2 of the cache entries.
>> And we consolidate the next (4 * MPS) to improve performance.
>>
>> We discovered this issue with devices on other platforms but have not
>> yet come across any device that triggers this on Linux. But it could be
>> a real problem now or in the future. All it takes is N number of small
>> chained TRBs. And other instances of the Synopsys IP may have smaller
>> values for the TRB_CACHE_SIZE which would exacerbate the problem.
>>
>> We verified this patch using our internal driver and testing framework.
> 
> If I understand the problem correctly you need to make sure the data pointed
> to by 16 (SS) or 8 (HS) chained TRBs must be equal to, or more than max packet size.
> 
> So in theory this should only be a problem for scatter gather buffers, right?
> 
Yes, this problem could be seen only with scatter gather buffers.

> This should already be handled by usb core unless no_sg_constraint flag is set,
> usb core it makes sure each sg list entry length is max packet size divisible, also
> meaning it needs to be at least max packet size. (or 0, but not an issue here)
> 
> see include/linux/usb.h: struct urb
>  
> * @sg: scatter gather buffer list, the buffer size of each element in
> *      the list (except the last) must be divisible by the endpoint's
> *      max packet size if no_sg_constraint isn't set in 'struct usb_bus'"
> 
> which is checked in drivers/usb/core/urb.c: usb_submit_urb()
> 
> for_each_sg(urb->sg, sg, urb->num_sgs - 1, i)
>     if (sg->length % max)
>         return -EINVAL;
> 
> So it seems all you need to do it make sure that the no_sg_constraint isn't set
> for this host controller vendor.
>
My understanding is if we don't set the no_sg_constraint and sg list entry is less
than max packet then transfer would not happen with the host controller. But the 
host controller supports all lengths of SG entries even less than MPS, only thing 
is the TRB_CACHE in the controller should have at least MPS size of data spread 
across it. So not setting up no_sg_constraint is not a useful solution. 
 
> This patch is too intrusive, its a very fine grained and complex solution to a
> vendor specific issue that has not caused any real life issues in Linux.
> It adds a new spinlock and list of bounce buffer to every transfer descriptor.
> 
I understand that in a first look it looks a complex solution, but you can suggest
the modifications/changes which would be required to make the patch more readable.
I have tried to keep the implementation similar to bounce buffer implementation 
only with addition of bounce buffer list. For the spinlock case, you can take a 
call if it is required or not.

> -Mathias
> 

Thanks & Regards,
Tejas Joglekar