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

From: Paul Burton <paul.burton@imgtec.com>
To: Thomas Gleixner <tglx@linutronix.de>
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
Date: Tue, 5 Sep 2017 23:00:01 -0700	[thread overview]
Message-ID: <45610923.Yru6qcift9@np-p-burton> (raw)
In-Reply-To: <20170531100212.210682135@linutronix.de>

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

Hi Thomas,

On Wednesday, 31 May 2017 02:58:33 PDT Thomas Gleixner wrote:
> Shared interrupts do not go well with disabling auto enable:
> 
> 1) The sharing interrupt might request it while it's still disabled and
>    then wait for interrupts forever.
> 
> 2) The interrupt might have been requested by the driver sharing the line
>    before IRQ_NOAUTOEN has been set. So the driver which expects that
>    disabled state after calling request_irq() will not get what it wants.
>    Even worse, when it calls enable_irq() later, it will trigger the
>    unbalanced enable_irq() warning.
> 
> Reported-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  kernel/irq/chip.c   |    7 +++++++
>  kernel/irq/manage.c |   12 ++++++++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1328,11 +1328,19 @@ static int
>  		if (new->flags & IRQF_ONESHOT)
>  			desc->istate |= IRQS_ONESHOT;
> 
> -		if (irq_settings_can_autoenable(desc))
> +		if (irq_settings_can_autoenable(desc)) {
>  			irq_startup(desc, true);
> -		else
> +		} 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.
> +			 */
> +			WARN_ON_ONCE(new->flags & IRQF_SHARED);
>  			/* Undo nested disables: */
>  			desc->depth = 1;
> +		}
> 
>  		/* Exclude IRQ from balancing if requested */
>  		if (new->flags & IRQF_NOBALANCING) {

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?

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.

  - I could adjust your warning such that it only triggers if the shared
    interrupt is indeed already enabled. This relies on my understanding of
    the issue described above being correct though, and it doesn't match your
    comment so I'm unsure I'm imagining the same issue you were warning about.

Do you have any thoughts or suggestions?

Thanks,
    Paul

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