[v3] usb: ohci: Prevent missed ohci interrupts

[PATCH v3] usb: ohci: Prevent missed ohci interrupts

Posted by Guenter Roeck 1 year, 7 months ago

Testing ohci functionality with qemu's pci-ohci emulation often results
in ohci interface stalls, resulting in hung task timeouts.

The problem is caused by lost interrupts between the emulation and the
Linux kernel code. Additional interrupts raised while the ohci interrupt
handler in Linux is running and before the handler clears the interrupt
status are not handled. The fix for a similar problem in ehci suggests
that the problem is likely caused by edge-triggered MSI interrupts. See
commit 0b60557230ad ("usb: ehci: Prevent missed ehci interrupts with
edge-triggered MSI") for details.

Ensure that the ohci interrupt code handles all pending interrupts before
returning to solve the problem.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: David Laight <David.Laight@aculab.com>
Cc: stable@vger.kernel.org
Fixes: 306c54d0edb6 ("usb: hcd: Try MSI interrupts on PCI devices")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v3: Check if any interrupts are pending before reading intrenable
    Add 'Cc: stable@vger.kernel.org'
v2: Only repeat if the interface is still active

Note that I did not apply Alan's Reviewed-by: tag since I was not sure
if that was appropriate after the code change.

 drivers/usb/host/ohci-hcd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 4f9982ecfb58..5cec7640e913 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -888,6 +888,7 @@ static irqreturn_t ohci_irq (struct usb_hcd *hcd)
 	/* Check for an all 1's result which is a typical consequence
 	 * of dead, unclocked, or unplugged (CardBus...) devices
 	 */
+again:
 	if (ints == ~(u32)0) {
 		ohci->rh_state = OHCI_RH_HALTED;
 		ohci_dbg (ohci, "device removed!\n");
@@ -982,6 +983,13 @@ static irqreturn_t ohci_irq (struct usb_hcd *hcd)
 	}
 	spin_unlock(&ohci->lock);
 
+	/* repeat until all enabled interrupts are handled */
+	if (ohci->rh_state != OHCI_RH_HALTED) {
+		ints = ohci_readl(ohci, &regs->intrstatus);
+		if (ints && (ints & ohci_readl(ohci, &regs->intrenable)))
+			goto again;
+	}
+
 	return IRQ_HANDLED;
 }
 
-- 
2.39.2

Re: [PATCH v3] usb: ohci: Prevent missed ohci interrupts

Posted by Gerd Hoffmann 1 year, 7 months ago

On Mon, Apr 29, 2024 at 08:40:10AM GMT, Guenter Roeck wrote:
> Testing ohci functionality with qemu's pci-ohci emulation often results
> in ohci interface stalls, resulting in hung task timeouts.
> 
> The problem is caused by lost interrupts between the emulation and the
> Linux kernel code. Additional interrupts raised while the ohci interrupt
> handler in Linux is running and before the handler clears the interrupt
> status are not handled. The fix for a similar problem in ehci suggests
> that the problem is likely caused by edge-triggered MSI interrupts. See
> commit 0b60557230ad ("usb: ehci: Prevent missed ehci interrupts with
> edge-triggered MSI") for details.
> 
> Ensure that the ohci interrupt code handles all pending interrupts before
> returning to solve the problem.
> 
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: David Laight <David.Laight@aculab.com>
> Cc: stable@vger.kernel.org
> Fixes: 306c54d0edb6 ("usb: hcd: Try MSI interrupts on PCI devices")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v3: Check if any interrupts are pending before reading intrenable
>     Add 'Cc: stable@vger.kernel.org'
> v2: Only repeat if the interface is still active

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd

Re: [PATCH v3] usb: ohci: Prevent missed ohci interrupts

Posted by Alan Stern 1 year, 7 months ago

On Mon, Apr 29, 2024 at 08:40:10AM -0700, Guenter Roeck wrote:
> Testing ohci functionality with qemu's pci-ohci emulation often results
> in ohci interface stalls, resulting in hung task timeouts.
> 
> The problem is caused by lost interrupts between the emulation and the
> Linux kernel code. Additional interrupts raised while the ohci interrupt
> handler in Linux is running and before the handler clears the interrupt
> status are not handled. The fix for a similar problem in ehci suggests
> that the problem is likely caused by edge-triggered MSI interrupts. See
> commit 0b60557230ad ("usb: ehci: Prevent missed ehci interrupts with
> edge-triggered MSI") for details.
> 
> Ensure that the ohci interrupt code handles all pending interrupts before
> returning to solve the problem.
> 
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: David Laight <David.Laight@aculab.com>
> Cc: stable@vger.kernel.org
> Fixes: 306c54d0edb6 ("usb: hcd: Try MSI interrupts on PCI devices")
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v3: Check if any interrupts are pending before reading intrenable
>     Add 'Cc: stable@vger.kernel.org'
> v2: Only repeat if the interface is still active
> 
> Note that I did not apply Alan's Reviewed-by: tag since I was not sure
> if that was appropriate after the code change.

I'm not too confident that the guess about all interrupt bits normally 
being off is correct, but in any case the extra test won't hurt.

Reviewed-by: Alan Stern <stern@rowland.harvard.edu>

Guenter, if you want to work on a patch to cache the interrupt-enable 
value in the driver, feel free to do it.  I have too much other stuff 
going on to spend any significant time on ohci-hcd.

Alan Stern

> 
>  drivers/usb/host/ohci-hcd.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
> index 4f9982ecfb58..5cec7640e913 100644
> --- a/drivers/usb/host/ohci-hcd.c
> +++ b/drivers/usb/host/ohci-hcd.c
> @@ -888,6 +888,7 @@ static irqreturn_t ohci_irq (struct usb_hcd *hcd)
>  	/* Check for an all 1's result which is a typical consequence
>  	 * of dead, unclocked, or unplugged (CardBus...) devices
>  	 */
> +again:
>  	if (ints == ~(u32)0) {
>  		ohci->rh_state = OHCI_RH_HALTED;
>  		ohci_dbg (ohci, "device removed!\n");
> @@ -982,6 +983,13 @@ static irqreturn_t ohci_irq (struct usb_hcd *hcd)
>  	}
>  	spin_unlock(&ohci->lock);
>  
> +	/* repeat until all enabled interrupts are handled */
> +	if (ohci->rh_state != OHCI_RH_HALTED) {
> +		ints = ohci_readl(ohci, &regs->intrstatus);
> +		if (ints && (ints & ohci_readl(ohci, &regs->intrenable)))
> +			goto again;
> +	}
> +
>  	return IRQ_HANDLED;
>  }
>  
> -- 
> 2.39.2
>

Re: [PATCH v3] usb: ohci: Prevent missed ohci interrupts

Posted by Guenter Roeck 1 year, 7 months ago

On 4/29/24 09:05, Alan Stern wrote:
> On Mon, Apr 29, 2024 at 08:40:10AM -0700, Guenter Roeck wrote:
>> Testing ohci functionality with qemu's pci-ohci emulation often results
>> in ohci interface stalls, resulting in hung task timeouts.
>>
>> The problem is caused by lost interrupts between the emulation and the
>> Linux kernel code. Additional interrupts raised while the ohci interrupt
>> handler in Linux is running and before the handler clears the interrupt
>> status are not handled. The fix for a similar problem in ehci suggests
>> that the problem is likely caused by edge-triggered MSI interrupts. See
>> commit 0b60557230ad ("usb: ehci: Prevent missed ehci interrupts with
>> edge-triggered MSI") for details.
>>
>> Ensure that the ohci interrupt code handles all pending interrupts before
>> returning to solve the problem.
>>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: David Laight <David.Laight@aculab.com>
>> Cc: stable@vger.kernel.org
>> Fixes: 306c54d0edb6 ("usb: hcd: Try MSI interrupts on PCI devices")
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v3: Check if any interrupts are pending before reading intrenable
>>      Add 'Cc: stable@vger.kernel.org'
>> v2: Only repeat if the interface is still active
>>
>> Note that I did not apply Alan's Reviewed-by: tag since I was not sure
>> if that was appropriate after the code change.
> 
> I'm not too confident that the guess about all interrupt bits normally
> being off is correct, but in any case the extra test won't hurt.
> 

OHCI_INTR_RHSC and OHCI_INTR_SF are often set but disabled while the system
starts, but afterwards it is mostly 0 during normal operation, i.e., while
there are no interface state changes.

> Reviewed-by: Alan Stern <stern@rowland.harvard.edu>
> 
> Guenter, if you want to work on a patch to cache the interrupt-enable
> value in the driver, feel free to do it.  I have too much other stuff
> going on to spend any significant time on ohci-hcd.
> 

Unfortunately I am in the same situation. Also, I am not sure if doing that
would really be worth the trouble.

Guenter