| Message ID | f18798-9e66-66e7-8c9d-cb8eeda364c4@pcs.com |
|---|---|
| State | New |
| Headers | show |
| Series | irq: synchronize irq_thread startup | expand |
Thomas, On Thu, Apr 28 2022 at 12:49, Thomas Pfaff wrote: please always Cc maintainers when sending patches and please also have a look at: https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-submission-notes > 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. > > If the close comes too fast, the irq_thread does not really start, > it is terminated immediately without going into irq_thread(). > But an interrupt might already been handled by > irq_default_primary_handler(), going to __irq_wake_thread() and > incrementing threads_active. > If this happens, synchronize_irq() will hang forever, because the > irq_thread is already dead, and threads_active will never be decremented. > > The fix is to make sure that the irq_thread is really started > during __setup_irq(). Right. Nice detective work. > @@ -127,6 +129,8 @@ struct irqaction { > unsigned int flags; > unsigned long thread_flags; > unsigned long thread_mask; > + atomic_t thread_is_up; > + wait_queue_head_t wait_for_thread; I don't think we need any of this extra ballast per irqaction. irqaction::thread_flags is already there and can be used as indicator. There are enough bits left. irq_desc already has a wait queue, which is used for synchronize_irq() to wait for threads to complete. That wait queue is unused at the point where an interrupt is set up, so there is no reason to introduce a new one which is only used once. This is not a problem in the rare case that the interrupt fires and wakes the thread which then in consequence might wake the waiter via wake_threads_waitq(). > - if (new->thread) > + if (new->thread) { > wake_up_process(new->thread); > - if (new->secondary) > + wait_event(new->wait_for_thread, > + atomic_read(&new->thread_is_up)); Please stick this into a helper function. Thanks, tglx
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h index f40754caaefa..82a16a12d472 100644 --- a/include/linux/interrupt.h +++ b/include/linux/interrupt.h @@ -113,6 +113,8 @@ typedef irqreturn_t (*irq_handler_t)(int, void *); * @secondary: pointer to secondary irqaction (force threading) * @thread_flags: flags related to @thread * @thread_mask: bitmask for keeping track of @thread activity + * @thread_is_up: threaded irq handler initializated + * @wait_for_thread: wait queue to wait for thread initialization * @dir: pointer to the proc/irq/NN/name entry */ struct irqaction { @@ -127,6 +129,8 @@ struct irqaction { unsigned int flags; unsigned long thread_flags; unsigned long thread_mask; + atomic_t thread_is_up; + wait_queue_head_t wait_for_thread; const char *name; struct proc_dir_entry *dir; } ____cacheline_internodealigned_in_smp; diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c index c03f71d5ec10..345e4f824c58 100644 --- a/kernel/irq/manage.c +++ b/kernel/irq/manage.c @@ -1272,6 +1272,9 @@ static int irq_thread(void *data) irq_thread_check_affinity(desc, action); + atomic_set(&action->thread_is_up, 1); + wake_up(&action->wait_for_thread); + while (!irq_wait_for_interrupt(action)) { irqreturn_t action_ret; @@ -1415,6 +1418,8 @@ setup_irq_thread(struct irqaction *new, unsigned int irq, bool secondary) { struct task_struct *t; + init_waitqueue_head(&new->wait_for_thread); + if (!secondary) { t = kthread_create(irq_thread, new, "irq/%d-%s", irq, new->name); @@ -1781,13 +1786,18 @@ __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. + * wake thread up, and wait until it is really up */ - if (new->thread) + if (new->thread) { wake_up_process(new->thread); - if (new->secondary) + wait_event(new->wait_for_thread, + atomic_read(&new->thread_is_up)); + } + if (new->secondary) { wake_up_process(new->secondary->thread); + wait_event(new->secondary->wait_for_thread, + atomic_read(&new->secondary->thread_is_up)); + } register_irq_proc(irq, desc); new->dir = NULL;