Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751606AbdIFGAw (ORCPT ); Wed, 6 Sep 2017 02:00:52 -0400 Received: from mailapp01.imgtec.com ([195.59.15.196]:14332 "EHLO mailapp01.imgtec.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750752AbdIFGAv (ORCPT ); Wed, 6 Sep 2017 02:00:51 -0400 From: Paul Burton To: Thomas Gleixner CC: LKML , jeffy , Brian Norris , Marc Zyngier , , , Subject: Re: [2/2] genirq: Warn when IRQ_NOAUTOEN is used with shared interrupts Date: Tue, 5 Sep 2017 23:00:01 -0700 Message-ID: <45610923.Yru6qcift9@np-p-burton> Organization: Imagination Technologies In-Reply-To: <20170531100212.210682135@linutronix.de> References: <20170531100212.210682135@linutronix.de> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart2240487.CrMGzSCECh"; micalg=pgp-sha256; protocol="application/pgp-signature" X-Originating-IP: [10.20.79.182] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5035 Lines: 116 --nextPart2240487.CrMGzSCECh Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 > Signed-off-by: Thomas Gleixner > --- > 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 --nextPart2240487.CrMGzSCECh Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEELIGR03D5+Fg+69wPgiDZ+mk8HGUFAlmvjuEACgkQgiDZ+mk8 HGWvgg/8DeOJxYQh5+gKOc5SsNJzEKolTZf4Mhr3KhAwgyQ9UWQ5j2xKjExF6k82 u6Ib9H2puPFwhYSwf5A3UeO+lwecms3MKPWKP9UDVkVgj+d3VzJuW+AY1ZS2E0dS 76Bo1eGfNUeJrFMxWuDEDzT0OmeKqv2aYlKcht44jrIeUEIfQZzAAorO7e3vf2JN +AB4oO1/pGKGpsLlZSajbO+JLmWmFrnK24S2s0QAGxGd2Z6gvR1a7d/iIz8MNa/7 sXTIkwIE/8HWV0l3N9p1oaBEQGzB8L5rn2EvkRY1ZYuSts0TiwiZ/XKCfJT8KRDw wiW6n2kHUwJrxt/QNyMkglVEyq1qlIKtllfCvOQT9bVcUMTciTjnvHp5Jdzdt/1o x+xrXBtjlU7eXHJnJYYYnqx4Ba/JmbyoKjSPt2drn89jvOYfMbc2mdLtK1pzLoJH FhuLp9VPWLpqVV3IJQPJIi7aw7o1E2CuI9FOhAV5Ei0E1zfuhZeS0ADdBmVD90QK /alM559c4lbZ2/blhszOQXF0BPg5g3hnS1j/VtzxurUfHW+qMSTsR5buBPOQr+SL o5EguS5xL9iV98CXU2vA/ssPQteXAoJdkn0CXl8M7IVO3uHByO8X2/einC/S1KTy Yz5PJUDg/uhlv7zT4EoR0oLOem6RdCD9co2vFCiMZbYMsL82VXY= =bHRF -----END PGP SIGNATURE----- --nextPart2240487.CrMGzSCECh--