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 <andriy.shevchenko@xxxxxxxxxxxxxxx>
> When MSI is used by the ehci driver, it can cause interrupts to be lost which
ehci -> EHCI everywhere?
> 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?
> 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?
> Signed-off-by: David Jeffery <djeffery@xxxxxxxxxx>
> Tested-by: Laurence Oberman <loberman@xxxxxxxxxx>
> ---
>
> This is an alternative to reverting 306c54d0edb6ba94d39877524dddebaad7770cf2
> which is the patch that allowed MSI to be used with ehci.
Thanks!
>
> ehci-hcd.c | 16 +++++++++++++---
> 1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 36f5bf6a0752..2283205d4b40 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -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;
?
> 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 */
for -> For
> + current_status = ehci_readl(ehci, &ehci->regs->status);
> + if (current_status & INTR_MASK)
> + goto restart;
> +
> cmd = ehci_readl(ehci, &ehci->regs->command);
> bh = 0;
--
With Best Regards,
Andy Shevchenko
