Messages in this thread | | | Date | Sat, 3 Dec 2011 00:21:46 +0100 (CET) | From | Thomas Gleixner <> | Subject | Re: [RFC] genirq: Flush the irq thread on synchronization |
| |
On Fri, 2 Dec 2011, Ido Yariv wrote:
> The current implementation does not always flush the threaded handler > when disabling the irq. In case the irq handler was called, but the > threaded handler hasn't started running yet, the interrupt will be > flagged as pending, and the handler will not run. This implementation > has some issues: > > First, if the interrupt is a wake source and flagged as pending, the > system will not be able to suspend. > > Second, when quickly disabling and re-enabling the irq, the threaded > handler might continue to run after the irq is re-enabled without the > irq handler being called first. This might be an unexpected behavior.
I'd wish people would stop calling disable/enable_irq() in loops and circles for no reason.
> In addition, it might be counter-intuitive that the threaded handler > will not be called even though the irq handler was called and returned > IRQ_WAKE_THREAD. > > Fix this by always waiting for the threaded handler to complete in > synchronize_irq().
I can see your problem, but this might lead to threads_active leaks under certain conditions. desc->threads_active was only meant to deal with shared interrupts.
We explicitely allow a design where the primary handler can leave the device interrupt enabled and allow further interrupts to occur while the handler is running. We only have a single bit to note that the thread should run, but your wakeup would up the threads_active count in that scenario several times w/o a counterpart which decrements it.
The solution for this is to keep the current threads_active semantics and make the wait function different. Instead of waiting for threads_active to become 0 it should wait for threads_active == 0 and the IRQTF_RUNTHREAD for all actions to be cleared. To avoid looping over the actions, we can take a similar approach as we take with the desc->threads_oneshot bitfield.
Thanks,
tglx
| |