Force-threaded interrupt handlers used to run with interrupts enabled,
something which could lead to deadlocks in case a threaded handler
shared a lock with code running in hard interrupt context (e.g. timer
callbacks) and did not explicitly disable interrupts.
Since commit 81e2073c175b ("genirq: Disable interrupts for force
threaded handlers") interrupt handlers always run with interrupts
disabled on non-RT so that drivers no longer need to do handle forced
threading ("threadirqs").
Drop the now obsolete workaround added by commit a1227f3c1030 ("usb:
ehci: fix deadlock when threadirqs option is used").
Cc: Stanislaw Gruszka <[email protected]>
Cc: Alan Stern <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Sebastian Andrzej Siewior <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/usb/host/ehci-hcd.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 1926b328b6aa..403bd3d6991f 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -705,15 +705,8 @@ 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;
int bh;
- unsigned long flags;
- /*
- * For threadirqs option we use spin_lock_irqsave() variant to prevent
- * deadlock with ehci hrtimer callback, because hrtimer callbacks run
- * in interrupt context even when threadirqs is specified. We can go
- * back to spin_lock() variant when hrtimer callbacks become threaded.
- */
- spin_lock_irqsave(&ehci->lock, flags);
+ spin_lock(&ehci->lock);
status = ehci_readl(ehci, &ehci->regs->status);
@@ -731,7 +724,7 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
/* Shared IRQ? */
if (!masked_status || unlikely(ehci->rh_state == EHCI_RH_HALTED)) {
- spin_unlock_irqrestore(&ehci->lock, flags);
+ spin_unlock(&ehci->lock);
return IRQ_NONE;
}
@@ -842,7 +835,7 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
if (bh)
ehci_work (ehci);
- spin_unlock_irqrestore(&ehci->lock, flags);
+ spin_unlock(&ehci->lock);
if (pcd_status)
usb_hcd_poll_rh_status(hcd);
return IRQ_HANDLED;
--
2.26.3
On Mon, Mar 22, 2021 at 12:12:49PM +0100, Johan Hovold wrote:
> Force-threaded interrupt handlers used to run with interrupts enabled,
> something which could lead to deadlocks in case a threaded handler
> shared a lock with code running in hard interrupt context (e.g. timer
> callbacks) and did not explicitly disable interrupts.
>
> Since commit 81e2073c175b ("genirq: Disable interrupts for force
> threaded handlers") interrupt handlers always run with interrupts
> disabled on non-RT so that drivers no longer need to do handle forced
> threading ("threadirqs").
What happens on RT systems? Are they smart enough to avoid the whole
problem by enabling interrupts during _all_ callbacks?
Alan Stern
On 2021-03-22 12:42:00 [-0400], Alan Stern wrote:
> What happens on RT systems? Are they smart enough to avoid the whole
> problem by enabling interrupts during _all_ callbacks?
tl;dr: Yes.
The referenced commit (id 81e2073c175b) disables interrupts only on !RT
configs so for RT everything remains unchanged (the backports are
already adjusted for the old stable trees to use the proper CONFIG_* for
enabled RT).
All hrtimer callbacks run as HRTIMER_MODE_SOFT by default. The
HRTIMER_MODE_HARD ones (which expire in HARDIRQ context) were audited /
explicitly enabled.
The same goes irq_work.
The printk code is different compared to mainline. A printk() on RT in
HARDIRQ context is printed once the HARDIRQ context is left. So the
serial/console/… driver never gets a chance to acquire its lock in
hardirq context.
An interrupt handler which is not forced-threaded must be marked as such
and must not use any spinlock_t based locking. lockdep/might_sleep
complain here already.
> Alan Stern
Sebastian
On Mon, Mar 22, 2021 at 05:59:17PM +0100, Sebastian Andrzej Siewior wrote:
> On 2021-03-22 12:42:00 [-0400], Alan Stern wrote:
> > What happens on RT systems? Are they smart enough to avoid the whole
> > problem by enabling interrupts during _all_ callbacks?
>
> tl;dr: Yes.
>
> The referenced commit (id 81e2073c175b) disables interrupts only on !RT
> configs so for RT everything remains unchanged (the backports are
> already adjusted for the old stable trees to use the proper CONFIG_* for
> enabled RT).
>
> All hrtimer callbacks run as HRTIMER_MODE_SOFT by default. The
> HRTIMER_MODE_HARD ones (which expire in HARDIRQ context) were audited /
> explicitly enabled.
> The same goes irq_work.
> The printk code is different compared to mainline. A printk() on RT in
> HARDIRQ context is printed once the HARDIRQ context is left. So the
> serial/console/… driver never gets a chance to acquire its lock in
> hardirq context.
>
> An interrupt handler which is not forced-threaded must be marked as such
> and must not use any spinlock_t based locking. lockdep/might_sleep
> complain here already.
Okay, in that case:
Acked-by: Alan Stern <[email protected]>