| Message ID | 552fe7b4-9224-b183-bb87-a8f36d335690@pcs.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] irq/core: synchronize irq_thread startup | expand |
On Mon, 2 May 2022, Thomas Gleixner wrote: > On Mon, May 02 2022 at 13:28, Thomas Pfaff wrote: > > While running > > "while /bin/true; do setserial /dev/ttyS0 uart none; > > setserial /dev/ttyS0 uart 16550A; done" > > on a kernel with threaded irqs, setserial is hung after some calls. > > > > setserial opens the device, this will install an irq handler if the uart is > > not none, followed by TIOCGSERIAL and TIOCSSERIAL ioctls. > > Then the device is closed. On close, synchronize_irq() is called by > > serial_core. > > This comment made me look deeper because I expected that free_irq() > would hang. > > But free_irq() stopped issuing synchronize_irq() with commit > 519cc8652b3a ("genirq: Synchronize only with single thread on > free_irq()"). And that turns out to be the root cause of the problem. > I should have caught that back then, but in hindsight .... > Sorry for coming back to this again late, but this makes me believe that the real problem for the freeze in setserial is that uart_port_shutdown() is calling synchronize_irq() after free_irq(), which is illegal in my opinion. It can be done only before the interrupt thread is stopped, and free_irq() itself is already taking care about synchronizing, no matter if its done by __synchronize_hardirq() or by synchronize_irq(), like it was before commit 519cc8652b3a. If it is called after free_irq(), the context is already lost. I am not sure about all the other drivers, but at least serial_core should be fixed if you agree. Thanks, Thomas
On Tue, May 10 2022 at 10:43, Thomas Pfaff wrote: > On Mon, 2 May 2022, Thomas Gleixner wrote: >> On Mon, May 02 2022 at 13:28, Thomas Pfaff wrote: >> This comment made me look deeper because I expected that free_irq() >> would hang. >> >> But free_irq() stopped issuing synchronize_irq() with commit >> 519cc8652b3a ("genirq: Synchronize only with single thread on >> free_irq()"). And that turns out to be the root cause of the problem. >> I should have caught that back then, but in hindsight .... >> > > Sorry for coming back to this again late, but this makes me believe that > the real problem for the freeze in setserial is that uart_port_shutdown() > is calling synchronize_irq() after free_irq(), which is illegal in my > opinion. Well, I'd say pointless. But it's not the real problem, it's the messenger which unearthed the underlying issue. Even if you remove that call, the underlying problem persists because the interrupt descriptor is in inconsistent state. > It can be done only before the interrupt thread is stopped, and free_irq() > itself is already taking care about synchronizing, no matter if its done by > __synchronize_hardirq() or by synchronize_irq(), like it was before commit > 519cc8652b3a. No, it does not really take care about it. It can return with irq_desc::threads_active > 0 due to the interrupt thread being stopped before reaching the thread function. Think about shared interrupts. > If it is called after free_irq(), the context is already lost. That's correct. > I am not sure about all the other drivers, but at least serial_core should > be fixed if you agree. Yes, that call is pointless. Thanks, tglx
On Tue, May 10 2022 at 13:34, Thomas Gleixner wrote: > On Tue, May 10 2022 at 10:43, Thomas Pfaff wrote: >> It can be done only before the interrupt thread is stopped, and free_irq() >> itself is already taking care about synchronizing, no matter if its done by >> __synchronize_hardirq() or by synchronize_irq(), like it was before commit >> 519cc8652b3a. > > No, it does not really take care about it. It can return with > irq_desc::threads_active > 0 due to the interrupt thread being stopped > before reaching the thread function. Think about shared interrupts. Duh. Hit send too fast. It does matter whether the synchronization is done via __synchronize_hardirq() or via synchronize_irq(). The latter ensured that the thread reached the thread function and handled the pending wakeup _before_ kthread_stop() become true. So the fix is required to undo the damage created by 519cc8652b3a. The synchronize_irq() after free_irq() is a completely different problem. Thanks, tglx
On Tue, 10 May 2022, Thomas Gleixner wrote: > It does matter whether the synchronization is done via > __synchronize_hardirq() or via synchronize_irq(). The latter ensured > that the thread reached the thread function and handled the pending > wakeup _before_ kthread_stop() become true. > > So the fix is required to undo the damage created by 519cc8652b3a. > > The synchronize_irq() after free_irq() is a completely different > problem. Thank you for the clarification. I was unsure if I missed something. Thomas
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h index 99cbdf55a8bd..f09c60393e55 100644 --- a/kernel/irq/internals.h +++ b/kernel/irq/internals.h @@ -29,12 +29,14 @@ extern struct irqaction chained_action; * IRQTF_WARNED - warning "IRQ_WAKE_THREAD w/o thread_fn" has been printed * IRQTF_AFFINITY - irq thread is requested to adjust affinity * IRQTF_FORCED_THREAD - irq action is force threaded + * IRQTF_READY - signals that irq thread is ready */ enum { IRQTF_RUNTHREAD, IRQTF_WARNED, IRQTF_AFFINITY, IRQTF_FORCED_THREAD, + IRQTF_READY, }; /* diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c index 939d21cd55c3..02f3b5bf5145 100644 --- a/kernel/irq/irqdesc.c +++ b/kernel/irq/irqdesc.c @@ -407,6 +407,7 @@ static struct irq_desc *alloc_desc(int irq, int node, unsigned int flags, lockdep_set_class(&desc->lock, &irq_desc_lock_class); mutex_init(&desc->request_mutex); init_rcu_head(&desc->rcu); + init_waitqueue_head(&desc->wait_for_threads); desc_set_defaults(irq, desc, node, affinity, owner); irqd_set(&desc->irq_data, flags); @@ -575,6 +576,7 @@ int __init early_irq_init(void) raw_spin_lock_init(&desc[i].lock); lockdep_set_class(&desc[i].lock, &irq_desc_lock_class); mutex_init(&desc[i].request_mutex); + init_waitqueue_head(&desc->wait_for_threads); desc_set_defaults(i, &desc[i], node, NULL, NULL); } return arch_early_irq_init(); diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index f1d5a94c6c9f..4dca48506d38 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -1263,6 +1263,31 @@ static void irq_wake_secondary(struct irq_desc *desc, struct irqaction *action) raw_spin_unlock_irq(&desc->lock); } +/* + * Internal function to notify that irq_thread is ready + */ +static void irq_thread_set_ready(struct irq_desc *desc, + struct irqaction *action) +{ + set_bit(IRQTF_READY, &action->thread_flags); + wake_up(&desc->wait_for_threads); +} + +/* + * Internal function to wake up irq_thread + * and wait until it is ready + */ +static void wake_up_and_wait_for_irq_thread_ready(struct irq_desc *desc, + struct irqaction *action) +{ + if (!action || !action->thread) + return; + + wake_up_process(action->thread); + wait_event(desc->wait_for_threads, + test_bit(IRQTF_READY, &action->thread_flags)); +} + /* * Interrupt handler thread */ @@ -1287,6 +1312,8 @@ static int irq_thread(void *data) irq_thread_check_affinity(desc, action); + irq_thread_set_ready(desc, action); + while (!irq_wait_for_interrupt(action)) { irqreturn_t action_ret; @@ -1698,8 +1725,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) } if (!shared) { - init_waitqueue_head(&desc->wait_for_threads); - /* Setup the type (level, edge polarity) if configured: */ if (new->flags & IRQF_TRIGGER_MASK) { ret = __irq_set_trigger(desc, @@ -1795,14 +1820,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) irq_setup_timings(desc, new); - /* - * Strictly no need to wake it up, but hung_task complains - * when no hard interrupt wakes the thread up. - */ - if (new->thread) - wake_up_process(new->thread); - if (new->secondary) - wake_up_process(new->secondary->thread); + wake_up_and_wait_for_irq_thread_ready(desc, new); + wake_up_and_wait_for_irq_thread_ready(desc, new->secondary); register_irq_proc(irq, desc); new->dir = NULL;