lkml.org 
[lkml]   [2017]   [Sep]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [2/2] genirq: Warn when IRQ_NOAUTOEN is used with shared interrupts
Date
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[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2017-09-06 08:01    [W:0.080 / U:3.992 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site