| Message ID | 0e26fb1421216b198a89b9164ab6c4cd9e1e127a.1692049324.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xhci: Clear EHB bit only at end of interrupt handler | expand |
On 15.8.2023 10.20, Lukas Wunner wrote: > The Event Handler Busy bit shall be cleared by software when the Event > Ring is empty. The xHC is thereby informed that it may raise another > interrupt once it has enqueued new events (sec 4.17.2). > > However since commit dc0ffbea5729 ("usb: host: xhci: update event ring > dequeue pointer on purpose"), the EHB bit is already cleared after half > a segment has been processed. > > As a result, spurious interrupts may occur: > > - xhci_irq() processes half a segment, clears EHB, continues processing > remaining events. > - xHC enqueues new events. Because EHB has been cleared, xHC sets > Interrupt Pending bit. Interrupt moderation countdown begins. > - Meanwhile xhci_irq() continues processing events. Interrupt > moderation countdown reaches zero, so an MSI interrupt is signaled. > - xhci_irq() empties the Event Ring, clears EHB again and is done. > - Because an MSI interrupt has been signaled, xhci_irq() is run again. > It discovers there's nothing to do and returns IRQ_NONE. > > Avoid by clearing the EHB bit only at the end of xhci_irq(). Thanks, Nice catch, we shouldn't clear EHB if we are in the middle of handling several events. > > Fixes: dc0ffbea5729 ("usb: host: xhci: update event ring dequeue pointer on purpose") > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Cc: stable@vger.kernel.org # v5.5+ > Cc: Peter Chen <peter.chen@nxp.com> > --- > drivers/usb/host/xhci-ring.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index 1dde53f..bc6280b 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -2996,7 +2996,8 @@ static int xhci_handle_event(struct xhci_hcd *xhci, struct xhci_interrupter *ir) > */ > static void xhci_update_erst_dequeue(struct xhci_hcd *xhci, > struct xhci_interrupter *ir, > - union xhci_trb *event_ring_deq) > + union xhci_trb *event_ring_deq, > + bool clear_ehb) > { > u64 temp_64; > dma_addr_t deq; > @@ -3022,7 +3023,8 @@ static void xhci_update_erst_dequeue(struct xhci_hcd *xhci, > } > > /* Clear the event handler busy flag (RW1C) */ > - temp_64 |= ERST_EHB; > + if (clear_ehb) > + temp_64 |= ERST_EHB; > xhci_write_64(xhci, temp_64, &ir->ir_set->erst_dequeue); > } This might still write the ERST_EHB bit even if clear_ehb is false. Earlier in xhci_update_erst_dequeue() we do: temp_64 = xhci_read_64(xhci, &ir->ir_set->erst_dequeue) ... temp_64 &= ERST_PTR_MASK; temp_64 |= ((u64) deq & (u64) ~ERST_PTR_MASK); ERST_PTR_MASK covers the ERST_EHB bit, so if it's set it will also be written back, which clears it (RW1C) #define ERST_EHB (1 << 3) #define ERST_PTR_MASK (0xf) The ERST_PTR_MASK name is really confusing as it masks everything else than the dequeue pointer. Should probably be changed -Mathias
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 1dde53f..bc6280b 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -2996,7 +2996,8 @@ static int xhci_handle_event(struct xhci_hcd *xhci, struct xhci_interrupter *ir) */ static void xhci_update_erst_dequeue(struct xhci_hcd *xhci, struct xhci_interrupter *ir, - union xhci_trb *event_ring_deq) + union xhci_trb *event_ring_deq, + bool clear_ehb) { u64 temp_64; dma_addr_t deq; @@ -3022,7 +3023,8 @@ static void xhci_update_erst_dequeue(struct xhci_hcd *xhci, } /* Clear the event handler busy flag (RW1C) */ - temp_64 |= ERST_EHB; + if (clear_ehb) + temp_64 |= ERST_EHB; xhci_write_64(xhci, temp_64, &ir->ir_set->erst_dequeue); } @@ -3103,7 +3105,7 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd) while (xhci_handle_event(xhci, ir) > 0) { if (event_loop++ < TRBS_PER_SEGMENT / 2) continue; - xhci_update_erst_dequeue(xhci, ir, event_ring_deq); + xhci_update_erst_dequeue(xhci, ir, event_ring_deq, false); event_ring_deq = ir->event_ring->dequeue; /* ring is half-full, force isoc trbs to interrupt more often */ @@ -3113,7 +3115,7 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd) event_loop = 0; } - xhci_update_erst_dequeue(xhci, ir, event_ring_deq); + xhci_update_erst_dequeue(xhci, ir, event_ring_deq, true); ret = IRQ_HANDLED; out:
The Event Handler Busy bit shall be cleared by software when the Event Ring is empty. The xHC is thereby informed that it may raise another interrupt once it has enqueued new events (sec 4.17.2). However since commit dc0ffbea5729 ("usb: host: xhci: update event ring dequeue pointer on purpose"), the EHB bit is already cleared after half a segment has been processed. As a result, spurious interrupts may occur: - xhci_irq() processes half a segment, clears EHB, continues processing remaining events. - xHC enqueues new events. Because EHB has been cleared, xHC sets Interrupt Pending bit. Interrupt moderation countdown begins. - Meanwhile xhci_irq() continues processing events. Interrupt moderation countdown reaches zero, so an MSI interrupt is signaled. - xhci_irq() empties the Event Ring, clears EHB again and is done. - Because an MSI interrupt has been signaled, xhci_irq() is run again. It discovers there's nothing to do and returns IRQ_NONE. Avoid by clearing the EHB bit only at the end of xhci_irq(). Fixes: dc0ffbea5729 ("usb: host: xhci: update event ring dequeue pointer on purpose") Signed-off-by: Lukas Wunner <lukas@wunner.de> Cc: stable@vger.kernel.org # v5.5+ Cc: Peter Chen <peter.chen@nxp.com> --- drivers/usb/host/xhci-ring.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)