lkml.org 
[lkml]   [2013]   [Mar]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] genirq: Sanitize spurious interrupt detection of threaded irqs
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
>
>



\
 
 \ /
  Last update: 2013-03-08 19:02    [W:0.234 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site