Re: [PATCH] genirq: Sanitize spurious interrupt detection of threadedirqs

[Till Straumann]

On 03/08/2013 05:12 PM, Thomas Gleixner wrote:
> On Fri, 8 Mar 2013, Till Straumann wrote:
>   
> > 1) I'm not sure adding the SPURIOUS_DEFERRED flag into
> >     threads_handled_last is OK - what happens if the atomic_t counter
> >     can hold more than 31 bits? In this case, when thread handlers
> >     increment the counter there is interference with the flag.  If
> >     this is not harmful then it is at least ugly.
> atomic_t is going to stay 32 bit otherwise we'll have more horrible
> problems than that one.
I know. But this means that when the counter overflows 31 bits (2^31 - 1)
then it spills into the SPURIOUS_DEFERRED flag, right?
> >      I'm not as familiar with the code as you are but wouldn't it be
> > simpler to always defer spurious detection thus avoiding to have to
> > keep track of the state (deferral active/inactive)? I.e., if any
> > primary handler returns IRQ_HANDLED then we simply increment the
> > counter. note_interrupt() could then always compare the previous
> > count to the current count and if they are equal conclude that the
> > interrupt was not handled:
> Yeah, we could do it that way. Would probably be simpler.
>   
> >      handle_irq_event_percpu()
> >      {
> >      ...
> >        if (!noirqdebug)
> >          note_interrupt(irq, desc, retval);
> >
> >        if ( (retval & IRQ_HANDLED) )
> >          atomic_inc(&desc->threads_handled);
> >      }
> >
> >      and in 'note_interrupt()'
> >
> >        handled = atomic_read(&desc->threads_handled);
> >        if ( desc->threads_handled_last == handled ) {
> >          action_ret = IRQ_NONE;
> >        } else {
> >          action_ret = IRQ_HANDLED;
> >          desc->threads_handled_last = handled;
> >        }
> >
> >     Either way - I'm not sure what deferral does to the part of the algorithm
> >     in note_interrupt() which deals with misrouted interrupts since the
> >     'action_ret' that goes into try_misrouted_irq() is delayed by one interrupt
> >     cycle.
> That should not matter much methinks, but I'll try what explodes on
> one of my affected machines.
>   
> > 2) note_interrupt is also called from irq/chip.c:handle_nested_irq() and I
> > believe
> >     this routine would also need to increment the 'threads_handled' counter
> > rather
> >     than calling note_interrupt.
> That's a different issue. The nested_irq handler is for interrupts
> which are demultiplexed by a primary threaded handler. That interrupt
> is never handled in hard interrupt context. It's always called from
> the context of the demultiplxing thread.
So you are saying that there 'handle_nested_irq()' can never be executed
from more than one thread for a single interrupt?

I find, however, that e.g., the gpio-sx150x.c driver calls

request_threaded_irq() with IRQF_SHARED set and it's thread_fn does call
handle_nested_irq(). It would thus be possible that multiple drivers
could share an interrupt and each driver would call handle_nested_irq()
which in-turn executes note_interrupt(). This would again raise the
issues we already discussed (note_interrupt() not serialized and thinking
that an interrupt was not handled because it was handled by a different
thread).

Probably I'm missing something regarding the use of nested interrupts
- I would really appreciate if you could help me understand why
it should be OK for handle_nested_irq() to call note_interrupt().

Cheers
- Till
> Thanks,
>
> 	tglx
>
>   

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/