Add logic to consolidate TRBs for Synopsys xHC

Sent: 20 December 2019 13:39

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.

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.

This commit adds the documentation for consolidate-trbs, and snps,consolidate-trbs property. These when set enables the quirk for XHCI driver for consolidation of TRB's when small buffer sizes are scattered over the TRB cache not making up to MPS or total transfer size with Synopsys xHC.

Signed-off-by Tejas Joglekar <[email protected]>

---
 Documentation/devicetree/bindings/usb/dwc3.txt     | 6 ++++++
 Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 +
 2 files changed, 7 insertions(+)

Documentation/devicetree/bindings/usb/dwc3.txt +0 -0

index 66780a47ad85..9851da41a442 100644

Documentation/devicetree/bindings/usb/dwc3.txt +0 -0

  - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
  - snps,incr-burst-type-adjustment: Value for INCR burst type of GSBUSCFG0

Documentation/devicetree/bindings/usb/usb-xhci.txt +1 -0

index 3f378951d624..be8d5e6a6333 100644
@@ -43,6 +43,7 @@ Optional properties:
   - quirk-broken-port-ped: set if the controller has broken port disable mechanism
   - imod-interval-ns: default interrupt moderation interval is 5000ns
   - phys : see usb-hcd.yaml in the current directory
+  - consolidate-trbs: indicate if you need to consolidate trbs
 
 additionally the properties from usb-hcd.yaml (in the current directory) are
 supported.

Signature · 1 line

2.11.0

Quoted 35 lines · Tejas Joglekar

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.

Quoted 1 line

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

Hi,
Thanks for the review comments.

Quoted 36 lines · Mathias Nyman

Quoted 35 lines · Tejas Joglekar

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.

Quoted 3 lines

Quoted 1 line

We verified this patch using our internal driver and testing framework.

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.

Quoted 39 lines · Tejas Joglekar

Hi,
Thanks for the review comments.

Quoted 36 lines · Mathias Nyman

Quoted 35 lines · Tejas Joglekar

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.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

We verified this patch using our internal driver and testing framework.

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.

In your case you know the need for a bounce buffer much earlier than in the existing TD fragment case.

Have you looked into the struct hc_driver map_urb_for_dma() and unmap_urb_for_dma() hooks? In those you could detect the need for a bounce buffer, allocate it, and bluntly copy entire scattergather buffer to the bounce buffer. It should be fairly small anyway.

-Mathias

Hi,

Quoted 52 lines · Mathias Nyman

Quoted 39 lines · Tejas Joglekar

Hi,
Thanks for the review comments.

Quoted 36 lines · Mathias Nyman

Quoted 35 lines · Tejas Joglekar

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.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

We verified this patch using our internal driver and testing framework.

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.

In your case you know the need for a bounce buffer much earlier than in the existing TD fragment case.

Quoted 5 lines

Have you looked into the struct hc_driver map_urb_for_dma() and unmap_urb_for_dma() hooks? In those you could detect the need for a bounce buffer, allocate it, and bluntly copy entire scattergather buffer to the bounce buffer. It should be fairly small anyway.

I will look into it, and get back to you. Thanks for the suggestion.

Quoted 2 lines

-Mathias

Regards,
Tejas Joglekar

Hi,

Quoted 59 lines · Tejas Joglekar

Hi,

Quoted 57 lines · Mathias Nyman

Quoted 39 lines · Tejas Joglekar

Hi,
Thanks for the review comments.

Quoted 36 lines · Mathias Nyman

Quoted 35 lines · Tejas Joglekar

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.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

We verified this patch using our internal driver and testing framework.

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.

In your case you know the need for a bounce buffer much earlier than in the existing TD fragment case.

Have you looked into the struct hc_driver map_urb_for_dma() and unmap_urb_for_dma() hooks? In those you could detect the need for a bounce buffer, allocate it, and bluntly copy entire scattergather buffer to the bounce buffer. It should be fairly small anyway.

Quoted 2 lines

I will look into it, and get back to you. Thanks for the suggestion.

I looked into it and I have a question related to approach you have suggested. I can detect need for the bounce buffer in map_urb_for_dma() function and can allocate bounce buffer and bluntly copy it, but when the SG list is having data over few MB's, I think my bounce buffer allocation might fail, over a period. Do you think it is possible?

So to avoid that, I thought of having a list of such bounce buffers held with the URB and then I can break the SG list with some fixed data length (e.g 16KB or 32 KB's) bounce buffers and copy the SG into that bounce buffer list.

Another option is to create a bounce sg, based on detection of bounce buffer requirement, where the small size SG's are combined to create a new SG list which can satisfy the TRB cache requirement for the SNPS controller.

What do you suggest? Which is the best way to go about solving the problem?

Quoted 61 lines · Tejas Joglekar

Hi,

Quoted 59 lines · Tejas Joglekar

Hi,

Quoted 57 lines · Mathias Nyman

Quoted 39 lines · Tejas Joglekar

Hi,
Thanks for the review comments.

Quoted 36 lines · Mathias Nyman

Quoted 35 lines · Tejas Joglekar

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.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

We verified this patch using our internal driver and testing framework.

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.

In your case you know the need for a bounce buffer much earlier than in the existing TD fragment case.

Have you looked into the struct hc_driver map_urb_for_dma() and unmap_urb_for_dma() hooks? In those you could detect the need for a bounce buffer, allocate it, and bluntly copy entire scattergather buffer to the bounce buffer. It should be fairly small anyway.

Quoted 6 lines

Quoted 2 lines

I will look into it, and get back to you. Thanks for the suggestion.

I looked into it and I have a question related to approach you have suggested. I can detect need for the bounce buffer in map_urb_for_dma() function and can allocate bounce buffer and bluntly copy it, but when the SG list is having data over few MB's, I think my bounce buffer allocation might fail, over a period. Do you think it is possible?

Doesn't sound very likely, the sg would need to have more than 16 entries of which the total length of consecutive 16 entries is less than 1024 bytes, and then the following entries should be large enough to fail memory allocation.
(for HS the total legth < 512 for 8 entries, and rest huge)

And no real world device has yet even triggered the first condition of having 16 (8) sg entries with total length less than max packet size.

Quoted 9 lines

So to avoid that, I thought of having a list of such bounce buffers held with the URB and then I can break the SG list with some fixed data length (e.g 16KB or 32 KB's) bounce buffers and copy the SG into that bounce buffer list.

Another option is to create a bounce sg, based on detection of bounce buffer requirement, where the small size SG's are combined to create a new SG list which can satisfy the TRB cache requirement for the SNPS controller.

Quoted 1 line

What do you suggest? Which is the best way to go about solving the problem?

As this is extremely rare (never reported in a real use case)
I'd keep this as simple as possible. Allocate one bounce buffer when needed, and copy all. If memory allocation fails (unlikely) print a warning, then we immediately know what to fix.

We are talking about sizes where 16 sg entries have in total 1024 bytes of data.

ehci-tegra.c does something related to what you want. It replaces urb->transfer_buffer with a dma aligned bounce buffer.

-Mathias

Quoted 6 lines

As this is extremely rare (never reported in a real use case)
I'd keep this as simple as possible. Allocate one bounce buffer when needed, and copy all. If memory allocation fails (unlikely) print a warning, then we immediately know what to fix.

We are talking about sizes where 16 sg entries have in total 1024 bytes of data.

Ok, I agree.

Quoted 3 lines

ehci-tegra.c does something related to what you want. It replaces urb->transfer_buffer with a dma aligned bounce buffer.

I tried this (in hcd.c) and it seems to be working with my tests. I want to override the unmap_urb_for_dma() and map_urb_for_dma() and keep all other things same as xhci driver. I thought to use xhci_driver_override for doing this instead of creating a whole new glue driver. Would that be ok ?