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

[Till Straumann]

OK. Thanks for the explanation.

However - even if not strictly necessary - wouldn't it
be simpler and the code easier to understand if
spurious interrupt detection just always would use the
deferred algorithm?
 - after handling by whatever scheme (hard, threaded, nested)
   the counter is incremented
 - the next hard IRQ calls note_interrupt() which checks
   if the counter has changed since last time.

Just my 2 cents...

Thanks again
- Till

On 03/08/2013 08:41 PM, Thomas Gleixner wrote:
> On Fri, 8 Mar 2013, Till Straumann wrote:
> > 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?
> Gah, yes. /me should stop doing overoptimizations :)
>   
> > > > 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().
> The thing about nested irqs is:
>
> main irq is threaded (requested by the driver for stuff like i2c)
>
> The handler of this irq reads a pending irq register in the chip and
> then invokes handle_nested_irq() for each of the pending bits.
>
> Those interrupts cannot be shared even if the driver request them as
> shared:
>
>          irqd_set(&desc->irq_data, IRQD_IRQ_INPROGRESS);
>          raw_spin_unlock_irq(&desc->lock);
>
>          action_ret = action->thread_fn(action->irq, action->dev_id);
>          if (!noirqdebug)
>                  note_interrupt(irq, desc, action_ret);
>
>          raw_spin_lock_irq(&desc->lock);
>          irqd_clear(&desc->irq_data, IRQD_IRQ_INPROGRESS);
>
> So there is no loop over action->next. And even if that code would
> loop over action next, then it still would be serialized
>
> main irq is raised
>
>       -> wake thread
>
> thread runs
>
>         read pending reg()
>
>         for each pending bit {
>
>         	   handle_nested_irq();
> 		action = desc->action;
>
> 		while (action) {
> 		      action->thread_fn()
> 		      action = action->next)
> 		}
> 		note_interrupt();
>         }
>
> thread done
>
> Hope that helps. 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/