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
