Re: [RFC][PATCH] irq: support IRQ_NESTED_THREAD with non-threaded interrupt handlers

[Thomas Gleixner <tglx@linutronix.de>]

Esben,

On Sat, 5 Jun 2010, Esben Haabendal wrote:

> On Sat, Jun 5, 2010 at 5:33 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Sat, 5 Jun 2010, Marc Zyngier wrote:
> >> You may want to give request_any_context_irq() a try (available since the
> >> latest merge window). It still requires your driver to be changed, but it
> >> should then work in both threaded and non-threaded cases.
> >
> > And it nicely annotates that somebody looked at the driver in
> > question. That's the rule of least surprise and does not impose checks
> > on the fast path.
> 
> What in particular should I be looking for in a driver before changing
> from request_irq() to request_any_context_irq() ?

Whether the irq handler relies on interrupts being disabled is the
most important thing. There are other constraints like
enable_irq/disable_irq calls from the driver code, which are not
allowed to run in atomic context for interrupt hanging of a i2c irq
controller.

> As for not checking in the fast path, it should be noted that this is "only"
> in handle_nested_irq(), which is only used in few interupt controller
> drivers, all of which I assume are generally not considered very "fast".

Fair enough. Still I fundamentaly dislike the automagic handling of
this and especially the irq disabled portion of it.
 
> Unless all interrupt handlers should be rewritten to be able to in both
> thread and interrupt context, I fail to se the conflict between the patch
> proposed and the work being done on request_any_context_irq().

It's not a question of conflict. It's a question of semantics.

We had and still have enough surprises in preempt-rt where we force
thread all handlers which were caused by various assumptions in the
handler code. I really prefer that the system yells in such a case
instead of letting run people into hard to debug problems silently.

The sanity check there makes the problem entirely obvious and forces
people to look at the driver for the following reasons:

 - the code has been audited for thread safety

 - the annotation of request_any_context_irq() documents that the
   audit has been done.

 - the annotation of request_any_context_irq() makes other people who
   are changing the code aware of the fact that it _is_ used in
   threaded context on some hardware.

 - in some drivers a cleanup can be done based on the threaded code,
   e.g. for drivers which use their own worker threads or rely heavily
   on workqueues.

Thanks,

	tglx