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

From: Paul Burton
Date: Wed Sep 06 2017 - 10:02:14 EST


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: signature.asc
Description: This is a digitally signed message part.