usb: ehci: Prevent missed ehci interrupts with edge-triggered MSI
Subsystems (2)
PATCH 1/1 usb: ehci: Prevent missed ehci interrupts with edge-triggered MSI
When MSI is used by the ehci driver, it can cause interrupts to be lost which results in ehci only continuing to work due to its polling fallback. But the reliance of polling drasticly reduces performance of any I/O through ehci.
Interrupts are lost as ehci's interrupt handler does not safely handle edge-triggered interrupts. It fails to ensure all interrupt status bits are cleared, which works with level-triggered interrupts but not the edge-triggered interrupts typical from using MSI.
To fix this problem, check if the driver may have raced with the hardware setting additional interrupt status bits and clear status until it is in a stable state.
---
This is an alternative to reverting 306c54d0edb6ba94d39877524dddebaad7770cf2 which is the patch that allowed MSI to be used with ehci.
ehci-hcd.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
drivers/usb/host/ehci-hcd.c +13 -3
@@ -704,14 +704,18 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd) {struct ehci_hcd *ehci =hcd_to_ehci (hcd);u32 status, masked_status, pcd_status =0 , cmd; +u32 current_status;int bh;spin_lock (&ehci->lock); - status =ehci_readl (ehci, &ehci->regs->status); + status =0 ; + current_status =ehci_readl (ehci, &ehci->regs->status); +restart: + status |= current_status;/* e.g. cardbus physical eject */ -if (status == ~(u32 )0 ) { +if (current_status == ~(u32 )0 ) {ehci_dbg (ehci,"device removed\n" ); goto dead; }
@@ -720,7 +724,7 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)* We don't use STS_FLR, but some controllers don't like it to * remain on, so mask it out along with the other status bits. */ - masked_status = status & (INTR_MASK |STS_FLR ); + masked_status = current_status & (INTR_MASK |STS_FLR );/* Shared IRQ? */ if (!masked_status ||unlikely (ehci->rh_state ==EHCI_RH_HALTED )) {
@@ -730,6 +734,12 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)/* clear (just) interrupts */ ehci_writel (ehci, masked_status, &ehci->regs->status); + +/* for edge interrupts, don't race with an interrupt bit being raised */ + current_status =ehci_readl (ehci, &ehci->regs->status); +if (current_status &INTR_MASK ) + goto restart; + cmd =ehci_readl (ehci, &ehci->regs->command); bh =0 ;
On Wed, Jul 14, 2021 at 10:03:09AM -0400, David Jeffery wrote:
Thanks, I have few minor comments, after addressing them feel free to add
Quoted 1 line
When MSI is used by the ehci driver, it can cause interrupts to be lost which
ehci -> EHCI everywhere?
Quoted 6 lines
results in ehci only continuing to work due to its polling fallback. But the reliance of polling drasticly reduces performance of any I/O through ehci.
Interrupts are lost as ehci's interrupt handler does not safely handle edge-triggered interrupts. It fails to ensure all interrupt status bits are cleared, which works with level-triggered interrupts but not the
Drop trailing white space here and double spaces in the couple of places above?
Quoted 5 lines
edge-triggered interrupts typical from using MSI.
To fix this problem, check if the driver may have raced with the hardware setting additional interrupt status bits and clear status until it is in a stable state.
Fixes tag?
Quoted 6 lines
Signed-off-by David Jeffery <[email protected]>Tested-by Laurence Oberman <[email protected]>---
This is an alternative to reverting
306c54d0edb6ba94d39877524dddebaad7770cf2which is the patch that allowed MSI to be used with ehci.
Thanks!
drivers/usb/host/ehci-hcd.c static irqreturn_t ehci_irq (struct usb_hcd *hcd) · line 707
Quoted 13 lines
ehci-hcd.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)drivers/usb/host/ehci-hcd.c +1
-0@@ -704,14 +704,18 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd) {struct ehci_hcd *ehci =hcd_to_ehci (hcd);u32 status, masked_status, pcd_status =0 , cmd; +u32 current_status;
Perhaps
u32 status, current_status, masked_status, pcd_status = 0; u32 cmd;
?
Quoted 26 lines
drivers/usb/host/ehci-hcd.c +6
-3int bh;spin_lock (&ehci->lock); - status =ehci_readl (ehci, &ehci->regs->status); + status =0 ; + current_status =ehci_readl (ehci, &ehci->regs->status); +restart: + status |= current_status;/* e.g. cardbus physical eject */ -if (status == ~(u32 )0 ) { +if (current_status == ~(u32 )0 ) {ehci_dbg (ehci,"device removed\n" ); goto dead; }@@ -720,7 +724,7 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)* We don't use STS_FLR, but some controllers don't like it to * remain on, so mask it out along with the other status bits. */ - masked_status = status & (INTR_MASK |STS_FLR ); + masked_status = current_status & (INTR_MASK |STS_FLR );/* Shared IRQ? */ if (!masked_status ||unlikely (ehci->rh_state ==EHCI_RH_HALTED )) {
drivers/usb/host/ehci-hcd.c static irqreturn_t ehci_irq (struct usb_hcd *hcd) · line 738
Quoted 5 lines
drivers/usb/host/ehci-hcd.c +2
-0@@ -730,6 +734,12 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)/* clear (just) interrupts */ ehci_writel (ehci, masked_status, &ehci->regs->status); + +/* for edge interrupts, don't race with an interrupt bit being raised */
for -> For
Quoted 6 lines
drivers/usb/host/ehci-hcd.c +4
-0+ current_status =ehci_readl (ehci, &ehci->regs->status); +if (current_status &INTR_MASK ) + goto restart; + cmd =ehci_readl (ehci, &ehci->regs->command); bh =0 ;
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 10 lines · Andy Shevchenko
On Wed, Jul 14, 2021 at 10:03:09AM -0400, David Jeffery wrote:
Thanks, I have few minor comments, after addressing them feel free to add
Reviewed-by Andy Shevchenko <[email protected]>Quoted 1 line
When MSI is used by the ehci driver, it can cause interrupts to be lost which
ehci -> EHCI everywhere?
Are you asking for a capitalization change in the text or asking what all is affected by the bug?
Quoted 12 lines
drivers/usb/host/ehci-hcd.c static irqreturn_t ehci_irq (struct usb_hcd *hcd) · line 707
Quoted 9 lines
drivers/usb/host/ehci-hcd.c +1
-0@@ -704,14 +704,18 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd) {struct ehci_hcd *ehci =hcd_to_ehci (hcd);u32 status, masked_status, pcd_status =0 , cmd; +u32 current_status;Perhaps
Quoted 5 lines
u32 status, current_status, masked_status, pcd_status = 0; u32 cmd;?
Is this a style preference? I can change it and just did it in a way to minimize line changes.
Quoted 28 lines
Quoted 28 lines
drivers/usb/host/ehci-hcd.c +6
-3int bh;spin_lock (&ehci->lock); - status =ehci_readl (ehci, &ehci->regs->status); + status =0 ; + current_status =ehci_readl (ehci, &ehci->regs->status); +restart: + status |= current_status;/* e.g. cardbus physical eject */ -if (status == ~(u32 )0 ) { +if (current_status == ~(u32 )0 ) {ehci_dbg (ehci,"device removed\n" ); goto dead; }@@ -720,7 +724,7 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)* We don't use STS_FLR, but some controllers don't like it to * remain on, so mask it out along with the other status bits. */ - masked_status = status & (INTR_MASK |STS_FLR ); + masked_status = current_status & (INTR_MASK |STS_FLR );/* Shared IRQ? */ if (!masked_status ||unlikely (ehci->rh_state ==EHCI_RH_HALTED )) {@@ -730,6 +734,12 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
Quoted 7 lines
drivers/usb/host/ehci-hcd.c static irqreturn_t ehci_irq (struct usb_hcd *hcd) · line 738
Quoted 4 lines
drivers/usb/host/ehci-hcd.c +2
-0/* clear (just) interrupts */ ehci_writel (ehci, masked_status, &ehci->regs->status); + +/* for edge interrupts, don't race with an interrupt bit being raised */ for -> For
I'll update it.
Quoted 7 lines · David Jeffery
Quoted 5 lines · Andy Shevchenko
On Wed, Jul 14, 2021 at 10:03:09AM -0400, David Jeffery wrote:
Thanks, I have few minor comments, after addressing them feel free to add
Reviewed-by Andy Shevchenko <[email protected]>
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
When MSI is used by the ehci driver, it can cause interrupts to be lost which
ehci -> EHCI everywhere?
Are you asking for a capitalization change in the text or asking what all is affected by the bug?
I'm suggesting to capitalize the abbreviation (maybe not in all cases, but where you refer to the hardware, for the driver it's called ehci-hcd).
...
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
u32 status, masked_status, pcd_status = 0, cmd;+ u32
current_status;Perhaps
Quoted 7 lines
Quoted 4 lines
u32 status, current_status, masked_status, pcd_status = 0; u32 cmd;?
Is this a style preference? I can change it and just did it in a way to minimize line changes.
Yes, I believe it's a preference of having something unified semantically on one line.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 38 lines · David Jeffery
When MSI is used by the ehci driver, it can cause interrupts to be lost which results in ehci only continuing to work due to its polling fallback. But the reliance of polling drasticly reduces performance of any I/O through ehci.
Interrupts are lost as ehci's interrupt handler does not safely handle edge-triggered interrupts. It fails to ensure all interrupt status bits are cleared, which works with level-triggered interrupts but not the edge-triggered interrupts typical from using MSI.
To fix this problem, check if the driver may have raced with the hardware setting additional interrupt status bits and clear status until it is in a stable state.
Signed-off-by David Jeffery <[email protected]>Tested-by Laurence Oberman <[email protected]>---
This is an alternative to reverting
306c54d0edb6ba94d39877524dddebaad7770cf2which is the patch that allowed MSI to be used with ehci.ehci-hcd.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)drivers/usb/host/ehci-hcd.c +2
-1@@ -704,14 +704,18 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd) {struct ehci_hcd *ehci =hcd_to_ehci (hcd);u32 status, masked_status, pcd_status =0 , cmd; +u32 current_status;int bh;spin_lock (&ehci->lock); - status =ehci_readl (ehci, &ehci->regs->status); + status =0 ;
drivers/usb/host/ehci-hcd.c static irqreturn_t ehci_irq (struct usb_hcd *hcd) · line 721
Quoted 10 lines
drivers/usb/host/ehci-hcd.c +4
-1+ current_status =ehci_readl (ehci, &ehci->regs->status); +restart: + status |= current_status;/* e.g. cardbus physical eject */ -if (status == ~(u32 )0 ) { +if (current_status == ~(u32 )0 ) {ehci_dbg (ehci,"device removed\n" ); goto dead; }
Mild stylistic quibble: I generally prefer to have a blank line before a /* ... */ comment. And it doesn't seem reasonable to have a blank line between "status = 0" and the current_status assignment, since those are similar once-only things before the beginning of the "restart" loop. Also, I would move the "status |= current_status" line after the test for device removed, since that test doesn't use status.
But obviously none of these things affect the patch's correntness.
Quoted 11 lines
drivers/usb/host/ehci-hcd.c +1
-1@@ -720,7 +724,7 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)* We don't use STS_FLR, but some controllers don't like it to * remain on, so mask it out along with the other status bits. */ - masked_status = status & (INTR_MASK |STS_FLR ); + masked_status = current_status & (INTR_MASK |STS_FLR );/* Shared IRQ? */ if (!masked_status ||unlikely (ehci->rh_state ==EHCI_RH_HALTED )) {@@ -730,6 +734,12 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
drivers/usb/host/ehci-hcd.c static irqreturn_t ehci_irq (struct usb_hcd *hcd) · line 744
Quoted 10 lines
drivers/usb/host/ehci-hcd.c +6
-0/* clear (just) interrupts */ ehci_writel (ehci, masked_status, &ehci->regs->status); + +/* for edge interrupts, don't race with an interrupt bit being raised */ + current_status =ehci_readl (ehci, &ehci->regs->status); +if (current_status &INTR_MASK ) + goto restart; + cmd =ehci_readl (ehci, &ehci->regs->command); bh =0 ;
You can choose to submit a new version of the patch with those stylistic changes, or if you prefer, just stick with this version. Either way,
Does this issue affect any other PCI-based host controller drivers?
Alan Stern
Quoted 37 lines · Alan Stern
Quoted 16 lines · David Jeffery
drivers/usb/host/ehci-hcd.c +2
-1@@ -704,14 +704,18 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd) {struct ehci_hcd *ehci =hcd_to_ehci (hcd);u32 status, masked_status, pcd_status =0 , cmd; +u32 current_status;int bh;spin_lock (&ehci->lock); - status =ehci_readl (ehci, &ehci->regs->status); + status =0 ;drivers/usb/host/ehci-hcd.c static irqreturn_t ehci_irq (struct usb_hcd *hcd) · line 721
Quoted 10 lines
drivers/usb/host/ehci-hcd.c +4
-1+ current_status =ehci_readl (ehci, &ehci->regs->status); +restart: + status |= current_status;/* e.g. cardbus physical eject */ -if (status == ~(u32 )0 ) { +if (current_status == ~(u32 )0 ) {ehci_dbg (ehci,"device removed\n" ); goto dead; }Mild stylistic quibble: I generally prefer to have a blank line before a /* ... */ comment. And it doesn't seem reasonable to have a blank line between "status = 0" and the
current_statusassignment, since those are similar once-only things before the beginning of the "restart" loop. Also, I would move the "status |=current_status" line after the test for device removed, since that test doesn't use status.
Quoted 1 line
But obviously none of these things affect the patch's correntness.
I'll update it with requested changes and resend it.
Quoted 10 lines
You can choose to submit a new version of the patch with those stylistic changes, or if you prefer, just stick with this version. Either way,
Acked-by Alan Stern <[email protected]>Does this issue affect any other PCI-based host controller drivers?
Alan Stern
Possibly. The uhci driver should have the same issue if hardware exists with it and has MSI for it. I suspect the ohci driver has a similar issue, but I'm not familiar enough with its interfaces and specification to say for sure.
David Jeffery
Alan Stern
maintainerQuoted 11 lines · David Jeffery
Quoted 5 lines · Alan Stern
Does this issue affect any other PCI-based host controller drivers?
Alan Stern
Possibly. The uhci driver should have the same issue if hardware exists with it and has MSI for it. I suspect the ohci driver has a similar issue, but I'm not familiar enough with its interfaces and specification to say for sure.
Hmmm. I've never heard of an MSI implementation of either of those, although it may be possible for OHCI. UHCI was used only by Intel and one or two others; as far as I know none of them are producing those controllers any more. Nowadays when people want to support USB-1.1 devices but not USB-3, they use an EHCI controller together with a "rate-matching" hub, which avoids the need for any UHCI or OHCI controller.
I was wondering more about dwc2 or other non-EHCI USB-2 controller drivers.
Alan Stern