Re: [2/2] genirq: Warn when IRQ_NOAUTOEN is used with shared interrupts

[Paul Burton <paul.burton@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 3257 bytes --]

Hi Thomas,

On Wednesday, 6 September 2017 01:16:48 PDT Thomas Gleixner wrote:
> On Tue, 5 Sep 2017, Paul Burton wrote:
> > I'm currently attempting to clean up a hack that we have in the MIPS GIC
> > irqchip driver - we have some interrupts which are really per-CPU, but are
> > currently used with the regular non-per-CPU IRQ APIs. Please search for
> > usage of gic_all_vpes_local_irq_controller (or for the string "HACK") in
> > drivers/ irqchip/irq-mips-gic.c if you wish to find what I'm talking
> > about. The important details are that the interrupts in question are both
> > per-CPU and on many systems are shared (between the CPU timer,
> > performance counters & fast debug channel).
> > 
> > I have been attempting to move towards using the per-CPU APIs instead in
> > order to remove this hack - ie. using setup_percpu_irq() &
> > enable_percpu_irq() in> 
> > place of plain old setup_irq(). Unfortunately what I've run into is this:
> >   - Per-CPU interrupts get the IRQ_NOAUTOEN flag set by default, in
> >   
> >     irq_set_percpu_devid_flags(). I can see why this makes sense in the
> >     general case, since the alternative is setup_percpu_irq() enabling the
> >     interrupt on the CPU that calls it & leaving it disabled on others,
> >     which
> >     feels a little unclean.
> >   
> >   - Your warning above triggers when a shared interrupt has the
> >   IRQ_NOAUTOEN
> >   
> >     flag set. I can see why your warning makes sense if another driver has
> >     already enabled the shared interrupt, which would make IRQ_NOAUTOEN
> >     ineffective. I'm not sure I follow your comment above the warning
> >     though -
> >     it sounds like you're trying to describe something else?
> > > 
> > > +			/*
> > > +			 * Shared interrupts do not go well with disabling
> > > +			 * auto enable. The sharing interrupt might request
> > > +			 * it while it's still disabled and then wait for
> > > +			 * interrupts forever.
> > > +			 */
> 
> Assume the following:
> 
>        request_irq(X, handler1, NOAUTOEN|SHARED, dev1);
> 
> now the second device does:
> 
>        request_irq(X, handler2, SHARED, dev2):
> 
> which will see the first handler installed, so it wont run into the code
> path which starts up the interrupt. That means as long as dev1 does not
> explicitely enable the interrupt dev2 will wait for it forever.

Ok, makes sense.

> > For my interrupts which are both per-CPU & shared the combination of these
> > 2> 
> > facts mean I end up triggering your warning. My current ideas include:
> >   - I could clear the IRQ_NOAUTOEN flag before calling setup_percpu_irq().
> >   In
> >   
> >     my cases that should be fine - we call enable_percpu_irq() anyway, and
> >     would just enable the IRQ slightly earlier on the CPU which calls
> >     setup_percpu_irq() which wouldn't be a problem. It feels a bit yucky
> >     though.
> 
> What's the problem with IRQ_NOAUTOEN and do
> 
>        setup_percpu_irq();
>        enable_percpu_irq();
> 
> on the boot CPU and then later call it when the secondary CPUs come up in
> cpu bringup code or a hotplug state callback?

There's no problem with that at all, apart from that it triggers your warning 
when the boot CPU calls setup_percpu_irq().

Thanks,
    Paul

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]