Date: Wed, 6 Sep 2017 10:16:48 +0200 (CEST)
From: Thomas Gleixner <tglx@linutronix.de>
To: Paul Burton <paul.burton@imgtec.com>
cc: LKML <linux-kernel@vger.kernel.org>, jeffy <jeffy.chen@rock-chips.com>,
        Brian Norris <briannorris@chromium.org>,
        Marc Zyngier <marc.zyngier@arm.com>, dianders@chromium.org,
        tfiga@chromium.org, james.hogan@imgtec.com
Subject: Re: [2/2] genirq: Warn when IRQ_NOAUTOEN is used with shared
 interrupts
In-Reply-To: <45610923.Yru6qcift9@np-p-burton>
Message-ID: <alpine.DEB.2.20.1709061009180.1843@nanos>
References: <20170531100212.210682135@linutronix.de> <45610923.Yru6qcift9@np-p-burton>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2877
Lines: 68

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.

> 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?

Thanks,

	tglx