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

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

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?

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.

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.

-Mathias