Multi-segment Event Ring support for XHCI
Subsystems (2)
cover Multi-segment Event Ring support for XHCI
Enlarge the XHCI Event Ring to cope with high load situations by allowing more than one segment (patch [2/2]). The patch is lifted from the Raspberry Pi kernel, it has been in use there for 4 years. I've taken the liberty to slightly edit the commit message and the patch itself for upstream.
As a prerequisite for the patch, ensure that the DESI bits in the ERDP register are set correctly (patch [1/2]). Incorrect DESI bits cause an interrupt storm on Renesas uPD720201 host controllers.
Jonathan Bell (1):
xhci: Use more than one Event Ring segment
Lukas Wunner (1):
xhci: Set DESI bits in ERDP register correctly
drivers/usb/host/xhci-mem.c | 35 ++++++++++++++++++----------------- drivers/usb/host/xhci-ring.c | 2 +- drivers/usb/host/xhci.h | 6 ++++-- 3 files changed, 23 insertions(+), 20 deletions(-)
Signature · 1 line
2.39.2
PATCH 1/2 xhci: Set DESI bits in ERDP register correctly
When using more than one Event Ring segment (ERSTSZ > 1), software shall set the DESI bits in the ERDP register to the number of the segment to which the upper ERDP bits are pointing. The xHC may use the DESI bits as a shortcut to determine whether it needs to check for an Event Ring Full condition: If it's enqueueing events in a different segment, it need not compare its internal Enqueue Pointer with the Dequeue Pointer in the upper bits of the ERDP register (sec 5.5.2.3.3).
Not setting the DESI bits correctly can result in the xHC enqueueing events past the Dequeue Pointer. On Renesas uPD720201 host controllers, incorrect DESI bits cause an interrupt storm. For comparison, VIA VL805 host controllers do not exhibit such problems. Perhaps they do not take advantage of the optimization afforded by the DESI bits.
To fix the issue, assign the segment number to each struct xhci_segment in xhci_segment_alloc(). When advancing the Dequeue Pointer in xhci_update_erst_dequeue(), write the segment number to the DESI bits.
On driver probe, set the DESI bits to zero in xhci_set_hc_event_deq() as processing starts in segment 0. Likewise on driver teardown, clear the DESI bits to zero in xhci_free_interrupter() when clearing the upper bits of the ERDP register. Previously those functions (incorrectly) treated the DESI bits as if they're declared RsvdP.
--- drivers/usb/host/xhci-mem.c | 25 +++++++++++-------------- drivers/usb/host/xhci-ring.c | 2 +- drivers/usb/host/xhci.h | 1 + 3 files changed, 13 insertions(+), 15 deletions(-)
drivers/usb/host/xhci-mem.c +11 -14
@@ -29,6 +29,7 @@static struct xhci_segment *xhci_segment_alloc (struct xhci_hcd *xhci,unsigned int cycle_state,unsigned int max_packet, +unsigned int num,gfp_t flags) {struct xhci_segment *seg;
@@ -60,6 +61,7 @@ static struct xhci_segment *xhci_segment_alloc(struct xhci_hcd *xhci,for (i =0 ; i <TRBS_PER_SEGMENT ; i++) seg->trbs[i].link.control =cpu_to_le32 (TRB_CYCLE ); } + seg->num = num; seg->dma = dma; seg->next =NULL ;
@@ -325,22 +327,24 @@ static int xhci_alloc_segments_for_ring(struct xhci_hcd *xhci, {struct xhci_segment *prev;bool chain_links; +int num =0 ;/* Set chain bit for 0.95 hosts, and for isoc rings on AMD 0.96 host */ chain_links = !!(xhci_link_trb_quirk (xhci) || (type ==TYPE_ISOC && (xhci->quirks & XHCI_AMD_0x96_HOST))); - prev =xhci_segment_alloc (xhci, cycle_state, max_packet, flags); + prev =xhci_segment_alloc (xhci, cycle_state, max_packet, num, flags);if (!prev)return -ENOMEM ; - num_segs--; + num++; *first = prev; -while (num_segs >0 ) { +while (num < num_segs) {struct xhci_segment *next; - next =xhci_segment_alloc (xhci, cycle_state, max_packet, flags); + next =xhci_segment_alloc (xhci, cycle_state, max_packet, num, + flags);if (!next) { prev = *first;while (prev) {
@@ -353,7 +357,7 @@ static int xhci_alloc_segments_for_ring(struct xhci_hcd *xhci,xhci_link_segments (prev, next, type, chain_links); prev = next; - num_segs--; + num++; }xhci_link_segments (prev, *first, type, chain_links); *last = prev;
@@ -1804,7 +1808,6 @@ int xhci_alloc_erst(struct xhci_hcd *xhci, {struct device *dev =xhci_to_hcd (xhci)->self.sysdev;size_t erst_size; -u64 tmp64;u32 tmp;if (!ir)
@@ -1827,9 +1830,7 @@ int xhci_alloc_erst(struct xhci_hcd *xhci, tmp &=ERST_SIZE_MASK ;writel (tmp, &ir->ir_set->erst_size); - tmp64 =xhci_read_64 (xhci, &ir->ir_set->erst_dequeue); - tmp64 &= (u64 )ERST_PTR_MASK ; -xhci_write_64 (xhci, tmp64, &ir->ir_set->erst_dequeue); +xhci_write_64 (xhci,ERST_EHB , &ir->ir_set->erst_dequeue); }/* free interrrupter event ring */
@@ -1936,7 +1937,6 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)static void xhci_set_hc_event_deq (struct xhci_hcd *xhci,struct xhci_interrupter *ir) { -u64 temp;dma_addr_t deq; deq = xhci_trb_virt_to_dma(ir->event_ring->deq_seg,
@@ -1944,15 +1944,12 @@ static void xhci_set_hc_event_deq(struct xhci_hcd *xhci, struct xhci_interrupterif (!deq)xhci_warn (xhci,"WARN something wrong with SW event ring dequeue ptr.\n" );/* Update HC event ring dequeue pointer */ - temp =xhci_read_64 (xhci, &ir->ir_set->erst_dequeue); - temp &=ERST_PTR_MASK ;/* Don't clear the EHB bit (which is RW1C) because * there might be more events to service. */ - temp &= ~ERST_EHB ;xhci_dbg_trace (xhci, trace_xhci_dbg_init,"// Write event ring dequeue pointer, preserving EHB bit" ); -xhci_write_64 (xhci, ((u64 ) deq & (u64 ) ~ERST_PTR_MASK ) | temp, +xhci_write_64 (xhci, ((u64 ) deq & (u64 ) ~ERST_PTR_MASK ), &ir->ir_set->erst_dequeue); }
drivers/usb/host/xhci-ring.c +1 -1
@@ -3018,7 +3018,7 @@ static void xhci_update_erst_dequeue(struct xhci_hcd *xhci,return ;/* Update HC event ring dequeue pointer */ - temp_64 &=ERST_PTR_MASK ; + temp_64 = ir->event_ring->deq_seg->num &ERST_DESI_MASK ; temp_64 |= ((u64 ) deq & (u64 ) ~ERST_PTR_MASK ); }
drivers/usb/host/xhci.h +1 -0
@@ -1545,6 +1545,7 @@ struct xhci_segment {union xhci_trb *trbs;/* private to HCD */ struct xhci_segment *next; +unsigned int num;dma_addr_t dma;/* Max packet sized bounce buffer for td-fragmant alignment */ dma_addr_t bounce_dma;
Signature · 1 line
2.39.2
Re: [PATCH 1/2] xhci: Set DESI bits in ERDP register correctly
Quoted 18 lines · Lukas Wunner
When using more than one Event Ring segment (ERSTSZ > 1), software shall set the DESI bits in the ERDP register to the number of the segment to which the upper ERDP bits are pointing. The xHC may use the DESI bits as a shortcut to determine whether it needs to check for an Event Ring Full condition: If it's enqueueing events in a different segment, it need not compare its internal Enqueue Pointer with the Dequeue Pointer in the upper bits of the ERDP register (sec 5.5.2.3.3).
Not setting the DESI bits correctly can result in the xHC enqueueing events past the Dequeue Pointer. On Renesas uPD720201 host controllers, incorrect DESI bits cause an interrupt storm. For comparison, VIA VL805 host controllers do not exhibit such problems. Perhaps they do not take advantage of the optimization afforded by the DESI bits.
To fix the issue, assign the segment number to each struct
xhci_segmentinxhci_segment_alloc(). When advancing the Dequeue Pointer inxhci_update_erst_dequeue(), write the segment number to the DESI bits.
Quoted 7 lines
On driver probe, set the DESI bits to zero in
xhci_set_hc_event_deq()as processing starts in segment 0. Likewise on driver teardown, clear the DESI bits to zero inxhci_free_interrupter()when clearing the upper bits of the ERDP register. Previously those functions (incorrectly) treated the DESI bits as if they're declared RsvdP.Signed-off-by Lukas Wunner <[email protected]>
Thanks for working on this, sorting out the DESI bits
The segment numbers might become useful for transfer rings as well, but with current transfer ring expansion support those numbers need to be adjusted after expansion.
But I can make a small separate fix for that on top of this.
-Mathias
PATCH 2/2 xhci: Use more than one Event Ring segment
In reply to Lukas WunnerFrom: Jonathan Bell <[email protected]>
Users have reported log spam created by "Event Ring Full" xHC event TRBs. These are caused by interrupt latency in conjunction with a very busy set of devices on the bus. The errors are benign, but throughput will suffer as the xHC will pause processing of transfers until the Event Ring is drained by the kernel.
Commit dc0ffbea5729 ("usb: host: xhci: update event ring dequeue pointer on purpose") mitigated the issue by advancing the Event Ring Dequeue Pointer already after half a segment has been processed. Nevertheless, providing a larger Event Ring would be useful to cope with load peaks.
Expand the number of event TRB slots available by increasing the number of Event Ring segments in the ERST.
Controllers have a hardware-defined limit as to the number of ERST entries they can process, but with up to 32k it can be excessively high (sec 5.3.4). So cap the actual number at 8 (configurable through the ERST_MAX_SEGS macro), which seems like a reasonable quantity.
An alternative to increasing the number of Event Ring segments would be an increase of the segment size. But that requires allocating multiple contiguous pages, which may be impossible if memory is fragmented.
--- drivers/usb/host/xhci-mem.c | 10 +++++++--- drivers/usb/host/xhci.h | 5 +++-- 2 files changed, 10 insertions(+), 5 deletions(-)
drivers/usb/host/xhci-mem.c +7 -3
@@ -2238,14 +2238,18 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags) {struct device *dev =xhci_to_hcd (xhci)->self.sysdev;struct xhci_interrupter *ir; +unsigned int num_segs;int ret; ir =kzalloc_node (sizeof (*ir), flags,dev_to_node (dev));if (!ir)return NULL ; - ir->event_ring =xhci_ring_alloc (xhci,ERST_NUM_SEGS ,1 ,TYPE_EVENT , -0 , flags); + num_segs = min_t(unsigned int, 1 << HCS_ERST_MAX(xhci->hcs_params2), +ERST_MAX_SEGS ); + + ir->event_ring =xhci_ring_alloc (xhci, num_segs,1 ,TYPE_EVENT ,0 , + flags);if (!ir->event_ring) {xhci_warn (xhci,"Failed to allocate interrupter event ring\n" );kfree (ir);
@@ -2281,7 +2285,7 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags)/* set ERST count with the number of entries in the segment table */ erst_size =readl (&ir->ir_set->erst_size); erst_size &=ERST_SIZE_MASK ; - erst_size |=ERST_NUM_SEGS ; + erst_size |= ir->event_ring->num_segs;writel (erst_size, &ir->ir_set->erst_size); erst_base =xhci_read_64 (xhci, &ir->ir_set->erst_base);
drivers/usb/host/xhci.h +3 -2
@@ -1674,8 +1674,9 @@ struct urb_priv {* Each segment table entry is 4*32bits long. 1K seems like an ok size: * (1K bytes * 8bytes/bit) / (4*32 bits) = 64 segment entries in the table, * meaning 64 ring segments. -* Initial allocated size of the ERST, in number of entries */ -#define ERST_NUM_SEGS 1 +* +* Reasonable limit for number of Event Ring segments (spec allows 32k) */ +#define ERST_MAX_SEGS 8/* Poll every 60 seconds */ #define POLL_TIMEOUT 60/* Stop endpoint command timeout (secs) for URB cancellation watchdog timer */
Signature · 1 line
2.39.2
Re: [PATCH 2/2] xhci: Use more than one Event Ring segment
Quoted 13 lines · Lukas Wunner
From: Jonathan Bell <[email protected]>
Users have reported log spam created by "Event Ring Full" xHC event TRBs. These are caused by interrupt latency in conjunction with a very busy set of devices on the bus. The errors are benign, but throughput will suffer as the xHC will pause processing of transfers until the Event Ring is drained by the kernel.
Commit
dc0ffbea5729("usb: host: xhci: update event ring dequeue pointer on purpose") mitigated the issue by advancing the Event Ring Dequeue Pointer already after half a segment has been processed. Nevertheless, providing a larger Event Ring would be useful to cope with load peaks.
Quoted 7 lines
Expand the number of event TRB slots available by increasing the number of Event Ring segments in the ERST.
Controllers have a hardware-defined limit as to the number of ERST entries they can process, but with up to 32k it can be excessively high (sec 5.3.4). So cap the actual number at 8 (configurable through the
ERST_MAX_SEGSmacro), which seems like a reasonable quantity.
Making the new event ring default size 8 times bigger seems a bit over the top, especially when most systems worked fine with just one segment.
How about doubling the current size as a default, and then think about adding more segments dynamically if we get "Event Ring Full" events?
Thanks
Mathias