| Message ID | 20210322111249.32141-1-johan@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8219ab4c9a09bc746614daaf5240fec82e7fe0e7 |
| Headers | show |
| Series | USB: ehci: drop workaround for forced irq threading | expand |
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 <stern@rowland.harvard.edu>
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;
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 <sgruszka@redhat.com> Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Peter Zijlstra <peterz@infradead.org> Signed-off-by: Johan Hovold <johan@kernel.org> --- drivers/usb/host/ehci-hcd.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)