Re: [PATCH v2 17/17] irq: remove handle_domain_{irq,nmi}()

Re: [PATCH v2 17/17] irq: remove handle_domain_{irq,nmi}()

[Lukas Wunner]

On Tue, Oct 26, 2021 at 10:25:04AM +0100, Mark Rutland wrote:
> Now that entry code handles IRQ entry (including setting the IRQ regs)
> before calling irqchip code, irqchip code can safely call
> generic_handle_domain_irq(), and there's no functional reason for it to
> call handle_domain_irq().
> 
> Let's cement this split of responsibility and remove handle_domain_irq()
> entirely, updating irqchip drivers to call generic_handle_domain_irq().
> 
> For consistency, handle_domain_nmi() is similarly removed and replaced
> with a generic_handle_domain_nmi() function which also does not perform
> any entry logic.
> 
> Previously handle_domain_{irq,nmi}() had a WARN_ON() which would fire
> when they were called in an inappropriate context. So that we can
> identify similar issues going forward, similar WARN_ON_ONCE() logic is
> added to the generic_handle_*() functions, and comments are updated for
> clarity and consistency.
[...]
>  int generic_handle_domain_irq(struct irq_domain *domain, unsigned int hwirq)
>  {
> +	WARN_ON_ONCE(!in_irq());
>  	return handle_irq_desc(irq_resolve_mapping(domain, hwirq));
>  }
>  EXPORT_SYMBOL_GPL(generic_handle_domain_irq);

Why isn't the WARN_ON_ONCE() conditional on handle_enforce_irqctx()?
(See handle_irq_desc() and c16816acd086.)

I believe the above change causes a regression in drivers/gpio/gpio-dln2.c
such that a gratuitous WARN splat is now emitted.

Thanks,

Lukas

Re: [PATCH v2 17/17] irq: remove handle_domain_{irq,nmi}()

[Mark Rutland]

On Fri, May 06, 2022 at 10:32:42PM +0200, Lukas Wunner wrote:
> On Tue, Oct 26, 2021 at 10:25:04AM +0100, Mark Rutland wrote:
> > Now that entry code handles IRQ entry (including setting the IRQ regs)
> > before calling irqchip code, irqchip code can safely call
> > generic_handle_domain_irq(), and there's no functional reason for it to
> > call handle_domain_irq().
> > 
> > Let's cement this split of responsibility and remove handle_domain_irq()
> > entirely, updating irqchip drivers to call generic_handle_domain_irq().
> > 
> > For consistency, handle_domain_nmi() is similarly removed and replaced
> > with a generic_handle_domain_nmi() function which also does not perform
> > any entry logic.
> > 
> > Previously handle_domain_{irq,nmi}() had a WARN_ON() which would fire
> > when they were called in an inappropriate context. So that we can
> > identify similar issues going forward, similar WARN_ON_ONCE() logic is
> > added to the generic_handle_*() functions, and comments are updated for
> > clarity and consistency.
> [...]
> >  int generic_handle_domain_irq(struct irq_domain *domain, unsigned int hwirq)
> >  {
> > +	WARN_ON_ONCE(!in_irq());
> >  	return handle_irq_desc(irq_resolve_mapping(domain, hwirq));
> >  }
> >  EXPORT_SYMBOL_GPL(generic_handle_domain_irq);
> 
> Why isn't the WARN_ON_ONCE() conditional on handle_enforce_irqctx()?
> (See handle_irq_desc() and c16816acd086.)

I did this for consistency with the in_nmi() check in
generic_handle_domain_nmi(); I was unaware of commit c16816acd086 and
IRQD_HANDLE_ENFORCE_IRQCTX.

I'll have ot leave it to Marc and Thomas as to what we should do there.

> I believe the above change causes a regression in drivers/gpio/gpio-dln2.c
> such that a gratuitous WARN splat is now emitted.

Do you mean you beleive that from inspection, or are you actually seeing this
go wrong in practice?

If it's the latter, are you able to give a copy of the dmesg splat?

Thanks,
Mark.

Re: [PATCH v2 17/17] irq: remove handle_domain_{irq,nmi}()

[Marc Zyngier]

On Mon, 09 May 2022 09:54:21 +0100,
Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Fri, May 06, 2022 at 10:32:42PM +0200, Lukas Wunner wrote:
> > On Tue, Oct 26, 2021 at 10:25:04AM +0100, Mark Rutland wrote:
> > > Now that entry code handles IRQ entry (including setting the IRQ regs)
> > > before calling irqchip code, irqchip code can safely call
> > > generic_handle_domain_irq(), and there's no functional reason for it to
> > > call handle_domain_irq().
> > > 
> > > Let's cement this split of responsibility and remove handle_domain_irq()
> > > entirely, updating irqchip drivers to call generic_handle_domain_irq().
> > > 
> > > For consistency, handle_domain_nmi() is similarly removed and replaced
> > > with a generic_handle_domain_nmi() function which also does not perform
> > > any entry logic.
> > > 
> > > Previously handle_domain_{irq,nmi}() had a WARN_ON() which would fire
> > > when they were called in an inappropriate context. So that we can
> > > identify similar issues going forward, similar WARN_ON_ONCE() logic is
> > > added to the generic_handle_*() functions, and comments are updated for
> > > clarity and consistency.
> > [...]
> > >  int generic_handle_domain_irq(struct irq_domain *domain, unsigned int hwirq)
> > >  {
> > > +	WARN_ON_ONCE(!in_irq());
> > >  	return handle_irq_desc(irq_resolve_mapping(domain, hwirq));
> > >  }
> > >  EXPORT_SYMBOL_GPL(generic_handle_domain_irq);
> > 
> > Why isn't the WARN_ON_ONCE() conditional on handle_enforce_irqctx()?
> > (See handle_irq_desc() and c16816acd086.)
> 
> I did this for consistency with the in_nmi() check in
> generic_handle_domain_nmi(); I was unaware of commit c16816acd086 and
> IRQD_HANDLE_ENFORCE_IRQCTX.
> 
> I'll have ot leave it to Marc and Thomas as to what we should do there.

My preference would be to not introduce things that result in
different behaviours for drivers, specially for things that are
evidently cross-architecture such as USB drivers (which seems to be
the case here).

I'd rather do something that allows these to be handled in the right
context such as a self-IPI. This would certainly work for the GIC. No
idea whether this is valid for x86, which is the other user.

Thanks,

	M.


-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH v2 17/17] irq: remove handle_domain_{irq,nmi}()

[Thomas Gleixner]

On Mon, May 09 2022 at 10:09, Marc Zyngier wrote:
> On Mon, 09 May 2022 09:54:21 +0100,
> Mark Rutland <mark.rutland@arm.com> wrote:
>> 
>> On Fri, May 06, 2022 at 10:32:42PM +0200, Lukas Wunner wrote:
>> > On Tue, Oct 26, 2021 at 10:25:04AM +0100, Mark Rutland wrote:
>> > >  int generic_handle_domain_irq(struct irq_domain *domain, unsigned int hwirq)
>> > >  {
>> > > +	WARN_ON_ONCE(!in_irq());
>> > >  	return handle_irq_desc(irq_resolve_mapping(domain, hwirq));
>> > >  }
>> > >  EXPORT_SYMBOL_GPL(generic_handle_domain_irq);
>> > 
>> > Why isn't the WARN_ON_ONCE() conditional on handle_enforce_irqctx()?
>> > (See handle_irq_desc() and c16816acd086.)
>> 
>> I did this for consistency with the in_nmi() check in
>> generic_handle_domain_nmi(); I was unaware of commit c16816acd086 and
>> IRQD_HANDLE_ENFORCE_IRQCTX.
>> 
>> I'll have ot leave it to Marc and Thomas as to what we should do there.
>
> My preference would be to not introduce things that result in
> different behaviours for drivers, specially for things that are
> evidently cross-architecture such as USB drivers (which seems to be
> the case here).
>
> I'd rather do something that allows these to be handled in the right
> context such as a self-IPI. This would certainly work for the GIC. No
> idea whether this is valid for x86, which is the other user.

We have to differentiate between interrupts which require hard interrupt
context due to constraints in the hardware, e.g. the horrors of affinity
changes on x86, and interrupts like the dln one which are synthetic. The
latter never go through the regular interrupt entry ASM muck.

Thanks,

        tglx

Re: [PATCH v2 17/17] irq: remove handle_domain_{irq,nmi}()

[Lukas Wunner]

On Mon, May 09, 2022 at 09:54:21AM +0100, Mark Rutland wrote:
> On Fri, May 06, 2022 at 10:32:42PM +0200, Lukas Wunner wrote:
> > On Tue, Oct 26, 2021 at 10:25:04AM +0100, Mark Rutland wrote:
> > >  int generic_handle_domain_irq(struct irq_domain *domain, unsigned int hwirq)
> > >  {
> > > +	WARN_ON_ONCE(!in_irq());
> > >  	return handle_irq_desc(irq_resolve_mapping(domain, hwirq));
> > >  }
> > >  EXPORT_SYMBOL_GPL(generic_handle_domain_irq);
> > 
> > Why isn't the WARN_ON_ONCE() conditional on handle_enforce_irqctx()?
> > (See handle_irq_desc() and c16816acd086.)
> 
> I did this for consistency with the in_nmi() check in
> generic_handle_domain_nmi(); I was unaware of commit c16816acd086 and
> IRQD_HANDLE_ENFORCE_IRQCTX.

Actually, since you're mentioning the in_nmi() check, I suspect
there's another problem here:

generic_handle_domain_nmi() warns if !in_nmi(), then calls down 
to handle_irq_desc() which warns if !in_hardirq().  Doesn't this
cause a false-positive !in_hardirq() warning for a NMI on GIC/GICv3?

The only driver calling request_nmi() or request_percpu_nmi() is
drivers/perf/arm_pmu.c.  So that's the only one affected.
You may want to test if that driver indeed exhibits such a
false-positive warning since c16816acd086.


> > I believe the above change causes a regression in drivers/gpio/gpio-dln2.c
> > such that a gratuitous WARN splat is now emitted.
> 
> Do you mean you beleive that from inspection, or are you actually seeing this
> go wrong in practice?
> 
> If it's the latter, are you able to give a copy of the dmesg splat?

For gpio-dln2.c, I believe it from inspection.

For smsc95xx.c, I'm actually seeing it go wrong in practice,
unedited dmesg splat is included below FWIW.

That's with the following series applied on top of current net-next:
https://lore.kernel.org/netdev/cover.1651574194.git.lukas@wunner.de/

You need to remove the __irq_enter_raw() in patch [5/7] of that series
to actually see the WARN splat.  I used it to work around your
WARN_ON_ONCE() and that's what Marc took exception to.

With gpio-dln2.c, the call stack looks like this:

  dln2_rx()                         # drivers/mfd/dln2.c
    dln2_run_event_callbacks()
      dln2_gpio_event()             # drivers/gpio/gpio-dln2.c
        generic_handle_domain_irq()

That's basically the same as the callstack for smsc95xx.c below.
Interrupts are disabled in dln2_rx() via spin_lock_irqsave(),
but there isn't an __irq_enter_raw() anywhere to be seen
and that would be necessary to avoid the false-positive
WARN splat in generic_handle_domain_irq().

Thanks,

Lukas

-- >8 --

[ 1227.718928] WARNING: CPU: 3 PID: 75 at kernel/irq/irqdesc.c:702 generic_handle_domain_irq+0x88/0x94
[ 1227.718951] Modules linked in: sha256_generic cfg80211 rfkill 8021q garp stp llc ax88796b asix raspberrypi_hwmon bcm2835_codec(C) bcm2835_v4l2(C) bcm2835_isp(C) v4l2_mem2mem bcm2835_mmal_vchiq(C) snd_bcm2835(C) vc_sm_cma(C) videobuf2_dma_contig videobuf2_vmalloc snd_pcm videobuf2_memops videobuf2_v4l2 videobuf2_common snd_timer snd videodev mc micrel ks8851_spi ks8851_common uio_pdrv_genirq uio eeprom_93cx6 piControl(O) ad5446 ti_dac082s085 mcp320x iio_mux mux_gpio mux_core fixed gpio_74x164 spi_bcm2835aux spi_bcm2835 gpio_max3191x crc8 industrialio i2c_dev ip_tables x_tables ipv6
[ 1227.719076] CPU: 3 PID: 75 Comm: irq/89-dwc_otg_ Tainted: G         C O      5.17.0-rt15-v7+ #2
[ 1227.719084] Hardware name: BCM2835
[ 1227.719087] Backtrace: 
[ 1227.719091]  dump_backtrace from show_stack+0x20/0x24
[ 1227.719106]  r7:00000009 r6:00000080 r5:80d25844 r4:60000093
[ 1227.719109]  show_stack from dump_stack_lvl+0x74/0x9c
[ 1227.719120]  dump_stack_lvl from dump_stack+0x18/0x1c
[ 1227.719134]  r7:00000009 r6:801957f4 r5:000002be r4:80d1e8ac
[ 1227.719137]  dump_stack from __warn+0xdc/0x190
[ 1227.719148]  __warn from warn_slowpath_fmt+0x70/0xcc
[ 1227.719161]  r8:00000009 r7:801957f4 r6:000002be r5:80d1e8ac r4:00000000
[ 1227.719163]  warn_slowpath_fmt from generic_handle_domain_irq+0x88/0x94
[ 1227.719177]  r8:81e87600 r7:00000000 r6:81d28000 r5:00000000 r4:81d4be00
[ 1227.719179]  generic_handle_domain_irq from smsc95xx_status+0x54/0xb0
[ 1227.719195]  r7:00000000 r6:00008000 r5:81f28640 r4:60000013
[ 1227.719197]  smsc95xx_status from intr_complete+0x80/0x84
[ 1227.719213]  r9:81d40a00 r8:00000001 r7:00000000 r6:00000000 r5:81f28640 r4:81d4bd80
[ 1227.719216]  intr_complete from __usb_hcd_giveback_urb+0xa4/0x12c
[ 1227.719229]  r5:815b3000 r4:81d4bd80
[ 1227.719232]  __usb_hcd_giveback_urb from usb_hcd_giveback_urb+0x118/0x11c
[ 1227.719245]  r7:811498e0 r6:81d4bd80 r5:81682000 r4:815b3000
[ 1227.719248]  usb_hcd_giveback_urb from completion_tasklet_func+0x7c/0xc8
[ 1227.719262]  r7:811498e0 r6:81d4bd80 r5:81682000 r4:8639f800
[ 1227.719264]  completion_tasklet_func from tasklet_callback+0x20/0x24
[ 1227.719277]  r6:80f8d140 r5:b6b5c330 r4:00000000
[ 1227.719279]  tasklet_callback from tasklet_action_common.constprop.0+0x148/0x220
[ 1227.719288]  tasklet_action_common.constprop.0 from tasklet_hi_action+0x28/0x30
[ 1227.719301]  r10:81d28000 r9:00000001 r8:00000000 r7:811498e0 r6:00000000 r5:00000001
[ 1227.719304]  r4:81003080
[ 1227.719306]  tasklet_hi_action from __do_softirq+0x154/0x3e8
[ 1227.719316]  __do_softirq from __local_bh_enable_ip+0x12c/0x1a8
[ 1227.719328]  r10:801970b0 r9:80f85320 r8:00000001 r7:00000000 r6:00000001 r5:00000100
[ 1227.719332]  r4:40000013
[ 1227.719334]  __local_bh_enable_ip from irq_forced_thread_fn+0x7c/0xac
[ 1227.719347]  r9:81d40ac0 r8:814f3c00 r7:00000001 r6:00000001 r5:814f3c00 r4:81d40ac0
[ 1227.719350]  irq_forced_thread_fn from irq_thread+0x16c/0x228
[ 1227.719363]  r7:00000001 r6:81d40ae4 r5:81d28000 r4:00000000
[ 1227.719365]  irq_thread from kthread+0x100/0x140
[ 1227.719380]  r10:00000000 r9:8154fbfc r8:81d43000 r7:81d40ac0 r6:80196dcc r5:81d40b00
[ 1227.719383]  r4:81d28000
[ 1227.719386]  kthread from ret_from_fork+0x14/0x34
[ 1227.719394] Exception stack(0x81777fb0 to 0x81777ff8)
[ 1227.719401] 7fa0:                                     00000000 00000000 00000000 00000000
[ 1227.719408] 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 1227.719414] 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 1227.719422]  r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:8014a6c8 r4:81d40b00
[ 1227.719425] ---[ end trace 0000000000000000 ]---

Re: [PATCH v2 17/17] irq: remove handle_domain_{irq,nmi}()

[Mark Rutland]

On Tue, May 10, 2022 at 02:13:20PM +0200, Lukas Wunner wrote:
> On Mon, May 09, 2022 at 09:54:21AM +0100, Mark Rutland wrote:
> > On Fri, May 06, 2022 at 10:32:42PM +0200, Lukas Wunner wrote:
> > > On Tue, Oct 26, 2021 at 10:25:04AM +0100, Mark Rutland wrote:
> > > >  int generic_handle_domain_irq(struct irq_domain *domain, unsigned int hwirq)
> > > >  {
> > > > +	WARN_ON_ONCE(!in_irq());
> > > >  	return handle_irq_desc(irq_resolve_mapping(domain, hwirq));
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(generic_handle_domain_irq);
> > > 
> > > Why isn't the WARN_ON_ONCE() conditional on handle_enforce_irqctx()?
> > > (See handle_irq_desc() and c16816acd086.)
> > 
> > I did this for consistency with the in_nmi() check in
> > generic_handle_domain_nmi(); I was unaware of commit c16816acd086 and
> > IRQD_HANDLE_ENFORCE_IRQCTX.
> 
> Actually, since you're mentioning the in_nmi() check, I suspect
> there's another problem here:
> 
> generic_handle_domain_nmi() warns if !in_nmi(), then calls down 
> to handle_irq_desc() which warns if !in_hardirq().  Doesn't this
> cause a false-positive !in_hardirq() warning for a NMI on GIC/GICv3?

I agree that doesn't look right.

> The only driver calling request_nmi() or request_percpu_nmi() is
> drivers/perf/arm_pmu.c.  So that's the only one affected.
> You may want to test if that driver indeed exhibits such a
> false-positive warning since c16816acd086.

In testing with v5.18-rc5, I can't see that going wrong.

I also hacked the following in:

-------->8--------
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 939d21cd55c38..3c85608a8779f 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -718,6 +718,7 @@ EXPORT_SYMBOL_GPL(generic_handle_domain_irq);
 int generic_handle_domain_nmi(struct irq_domain *domain, unsigned int hwirq)
 {
        WARN_ON_ONCE(!in_nmi());
+       WARN_ON_ONCE(!in_hardirq());
        return handle_irq_desc(irq_resolve_mapping(domain, hwirq));
 }
 #endif
-------->8--------

... and that never fired, which doesn't seem right.

I also see some other problems which may or may not be related (lockdep
tracking gone wrong, which could indicate a more general failure to track irq
state correctly). I'll go dig into that...

> > > I believe the above change causes a regression in drivers/gpio/gpio-dln2.c
> > > such that a gratuitous WARN splat is now emitted.
> > 
> > Do you mean you beleive that from inspection, or are you actually seeing this
> > go wrong in practice?
> > 
> > If it's the latter, are you able to give a copy of the dmesg splat?
> 
> For gpio-dln2.c, I believe it from inspection.
> 
> For smsc95xx.c, I'm actually seeing it go wrong in practice,
> unedited dmesg splat is included below FWIW.

Thanks; having the trace makes this much easier to analyse.

Mark.

> That's with the following series applied on top of current net-next:
> https://lore.kernel.org/netdev/cover.1651574194.git.lukas@wunner.de/
> 
> You need to remove the __irq_enter_raw() in patch [5/7] of that series
> to actually see the WARN splat.  I used it to work around your
> WARN_ON_ONCE() and that's what Marc took exception to.
> 
> With gpio-dln2.c, the call stack looks like this:
> 
>   dln2_rx()                         # drivers/mfd/dln2.c
>     dln2_run_event_callbacks()
>       dln2_gpio_event()             # drivers/gpio/gpio-dln2.c
>         generic_handle_domain_irq()
> 
> That's basically the same as the callstack for smsc95xx.c below.
> Interrupts are disabled in dln2_rx() via spin_lock_irqsave(),
> but there isn't an __irq_enter_raw() anywhere to be seen
> and that would be necessary to avoid the false-positive
> WARN splat in generic_handle_domain_irq().
> 
> Thanks,
> 
> Lukas
> 
> -- >8 --
> 
> [ 1227.718928] WARNING: CPU: 3 PID: 75 at kernel/irq/irqdesc.c:702 generic_handle_domain_irq+0x88/0x94
> [ 1227.718951] Modules linked in: sha256_generic cfg80211 rfkill 8021q garp stp llc ax88796b asix raspberrypi_hwmon bcm2835_codec(C) bcm2835_v4l2(C) bcm2835_isp(C) v4l2_mem2mem bcm2835_mmal_vchiq(C) snd_bcm2835(C) vc_sm_cma(C) videobuf2_dma_contig videobuf2_vmalloc snd_pcm videobuf2_memops videobuf2_v4l2 videobuf2_common snd_timer snd videodev mc micrel ks8851_spi ks8851_common uio_pdrv_genirq uio eeprom_93cx6 piControl(O) ad5446 ti_dac082s085 mcp320x iio_mux mux_gpio mux_core fixed gpio_74x164 spi_bcm2835aux spi_bcm2835 gpio_max3191x crc8 industrialio i2c_dev ip_tables x_tables ipv6
> [ 1227.719076] CPU: 3 PID: 75 Comm: irq/89-dwc_otg_ Tainted: G         C O      5.17.0-rt15-v7+ #2
> [ 1227.719084] Hardware name: BCM2835
> [ 1227.719087] Backtrace: 
> [ 1227.719091]  dump_backtrace from show_stack+0x20/0x24
> [ 1227.719106]  r7:00000009 r6:00000080 r5:80d25844 r4:60000093
> [ 1227.719109]  show_stack from dump_stack_lvl+0x74/0x9c
> [ 1227.719120]  dump_stack_lvl from dump_stack+0x18/0x1c
> [ 1227.719134]  r7:00000009 r6:801957f4 r5:000002be r4:80d1e8ac
> [ 1227.719137]  dump_stack from __warn+0xdc/0x190
> [ 1227.719148]  __warn from warn_slowpath_fmt+0x70/0xcc
> [ 1227.719161]  r8:00000009 r7:801957f4 r6:000002be r5:80d1e8ac r4:00000000
> [ 1227.719163]  warn_slowpath_fmt from generic_handle_domain_irq+0x88/0x94
> [ 1227.719177]  r8:81e87600 r7:00000000 r6:81d28000 r5:00000000 r4:81d4be00
> [ 1227.719179]  generic_handle_domain_irq from smsc95xx_status+0x54/0xb0
> [ 1227.719195]  r7:00000000 r6:00008000 r5:81f28640 r4:60000013
> [ 1227.719197]  smsc95xx_status from intr_complete+0x80/0x84
> [ 1227.719213]  r9:81d40a00 r8:00000001 r7:00000000 r6:00000000 r5:81f28640 r4:81d4bd80
> [ 1227.719216]  intr_complete from __usb_hcd_giveback_urb+0xa4/0x12c
> [ 1227.719229]  r5:815b3000 r4:81d4bd80
> [ 1227.719232]  __usb_hcd_giveback_urb from usb_hcd_giveback_urb+0x118/0x11c
> [ 1227.719245]  r7:811498e0 r6:81d4bd80 r5:81682000 r4:815b3000
> [ 1227.719248]  usb_hcd_giveback_urb from completion_tasklet_func+0x7c/0xc8
> [ 1227.719262]  r7:811498e0 r6:81d4bd80 r5:81682000 r4:8639f800
> [ 1227.719264]  completion_tasklet_func from tasklet_callback+0x20/0x24
> [ 1227.719277]  r6:80f8d140 r5:b6b5c330 r4:00000000
> [ 1227.719279]  tasklet_callback from tasklet_action_common.constprop.0+0x148/0x220
> [ 1227.719288]  tasklet_action_common.constprop.0 from tasklet_hi_action+0x28/0x30
> [ 1227.719301]  r10:81d28000 r9:00000001 r8:00000000 r7:811498e0 r6:00000000 r5:00000001
> [ 1227.719304]  r4:81003080
> [ 1227.719306]  tasklet_hi_action from __do_softirq+0x154/0x3e8
> [ 1227.719316]  __do_softirq from __local_bh_enable_ip+0x12c/0x1a8
> [ 1227.719328]  r10:801970b0 r9:80f85320 r8:00000001 r7:00000000 r6:00000001 r5:00000100
> [ 1227.719332]  r4:40000013
> [ 1227.719334]  __local_bh_enable_ip from irq_forced_thread_fn+0x7c/0xac
> [ 1227.719347]  r9:81d40ac0 r8:814f3c00 r7:00000001 r6:00000001 r5:814f3c00 r4:81d40ac0
> [ 1227.719350]  irq_forced_thread_fn from irq_thread+0x16c/0x228
> [ 1227.719363]  r7:00000001 r6:81d40ae4 r5:81d28000 r4:00000000
> [ 1227.719365]  irq_thread from kthread+0x100/0x140
> [ 1227.719380]  r10:00000000 r9:8154fbfc r8:81d43000 r7:81d40ac0 r6:80196dcc r5:81d40b00
> [ 1227.719383]  r4:81d28000
> [ 1227.719386]  kthread from ret_from_fork+0x14/0x34
> [ 1227.719394] Exception stack(0x81777fb0 to 0x81777ff8)
> [ 1227.719401] 7fa0:                                     00000000 00000000 00000000 00000000
> [ 1227.719408] 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [ 1227.719414] 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [ 1227.719422]  r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:8014a6c8 r4:81d40b00
> [ 1227.719425] ---[ end trace 0000000000000000 ]---

Re: [PATCH v2 17/17] irq: remove handle_domain_{irq,nmi}()

[Thomas Gleixner]

On Tue, May 10 2022 at 15:15, Mark Rutland wrote:
> On Tue, May 10, 2022 at 02:13:20PM +0200, Lukas Wunner wrote:
>> For gpio-dln2.c, I believe it from inspection.
>> 
>> For smsc95xx.c, I'm actually seeing it go wrong in practice,
>> unedited dmesg splat is included below FWIW.
>
> Thanks; having the trace makes this much easier to analyse.

which confirmes what I talked about before:

>> WARNING: CPU: 3 PID: 75 at kernel/irq/irqdesc.c:702 generic_handle_domain_irq+0x88/0x94
>>  generic_handle_domain_irq from smsc95xx_status+0x54/0xb0
>>  smsc95xx_status from intr_complete+0x80/0x84
>>  intr_complete from __usb_hcd_giveback_urb+0xa4/0x12c
>>  __usb_hcd_giveback_urb from usb_hcd_giveback_urb+0x118/0x11c
>>  usb_hcd_giveback_urb from completion_tasklet_func+0x7c/0xc8
>>  completion_tasklet_func from tasklet_callback+0x20/0x24
>>  tasklet_callback from tasklet_action_common.constprop.0+0x148/0x220
>>  tasklet_action_common.constprop.0 from tasklet_hi_action+0x28/0x30
>>  tasklet_hi_action from __do_softirq+0x154/0x3e8
>>  __do_softirq from __local_bh_enable_ip+0x12c/0x1a8
>>  __local_bh_enable_ip from irq_forced_thread_fn+0x7c/0xac
>>  irq_forced_thread_fn from irq_thread+0x16c/0x228
>>  irq_thread from kthread+0x100/0x140

So what happens here:

 interrupt
    -> wakeup threaded handler

 threaded handler runs
    local_bh_disable();
    ....
    schedules tasklet
    ...
    local_bh_enable()
      do_softirq()
        run_tasklet()
          urb_completion()
            smsc95xx_status()
              generic_handle_domain_irq()

That interrupt in question is an interrupt, which is not handled by the
primary CPU interrupt chips. It's a synthetic interrupt which is
generated from the received USB packet.

+	/* USB interrupts are received in softirq (tasklet) context.
+	 * Switch to hardirq context to make genirq code happy.
+	 */
+	local_irq_save(flags);
+	__irq_enter_raw();
+
 	if (intdata & INT_ENP_PHY_INT_)
-		;
+		generic_handle_domain_irq(pdata->irqdomain, PHY_HWIRQ);

This __irq_enter_raw() is really wrong. This is _not_ running in hard
interrupt context. Pretending so creates more problems than it
solves. It breaks context tracking, confuses lockdep ...

We also have demultiplexed interrupts which are nested in a threaded
interrupt handler and share the thread context. No, we are not going to
pretend that they run in hard interrupt context either.

So we need a clear distinction between interrupts which really happen in
hard interrupt context and those which are synthetic and can be invoked
from pretty much any context.

Anything else is just a recipe for disaster and endless supply of half
baken hacks.

Thanks,

        tglx

Re: [PATCH v2 17/17] irq: remove handle_domain_{irq,nmi}()

[Thomas Gleixner]

On Mon, May 09 2022 at 10:09, Marc Zyngier wrote:
> On Mon, 09 May 2022 09:54:21 +0100,
> Mark Rutland <mark.rutland@arm.com> wrote:
>> > Why isn't the WARN_ON_ONCE() conditional on handle_enforce_irqctx()?
>> > (See handle_irq_desc() and c16816acd086.)
>> 
>> I did this for consistency with the in_nmi() check in
>> generic_handle_domain_nmi(); I was unaware of commit c16816acd086 and
>> IRQD_HANDLE_ENFORCE_IRQCTX.
>> 
>> I'll have ot leave it to Marc and Thomas as to what we should do there.
>
> My preference would be to not introduce things that result in
> different behaviours for drivers, specially for things that are
> evidently cross-architecture such as USB drivers (which seems to be
> the case here).

No. USB drivers which synthesize their interrupts from a received packet
work perfectly fine on all architectures because the interrupt domain
and interrupt chip they are using are a software construct designed for
the purpose and have no hard interrupt context requirements.

The reason why this is done is to leverage the interrupt driven PHY
status changes instead of enforcing timer based polling. The charm is
that the phy code does not have to grow another wart and just uses the
offered synthetic interrupt. There are other places which do similar
things for the same reason. This also provides the beloved statistics in
/proc/interrupt, tracepoints etc. out of the box without having to add
extra muck into yet another subsystem.

> I'd rather do something that allows these to be handled in the right
> context such as a self-IPI. This would certainly work for the GIC. No
> idea whether this is valid for x86, which is the other user.

This interrupt is neither directly nor indirectly connected to GIC
or APIC. It's synthesized. So what would the self-IPI help?

And no, I don't want to create infrastructure to allocate a pseudo
device vector on x86 just to be able to self-IPI this USB synthesized
interrupt. That'd be yet another horrorshow and worse a horrorshow for
no reason and zero value.

IRQD_HANDLE_ENFORCE_IRQCTX was introduced to be able to differentiate
between interrupt chips which require a particular context and chips
which can handle it perfectly fine to have e.g. their software retrigger
handled by directly invoking the handler from an arbitrary context.

I wish this would be the case on x86. That would eliminate a boatload of
horrible code.

Thanks,

        tglx

Re: [PATCH v2 17/17] irq: remove handle_domain_{irq,nmi}()

[Thomas Gleixner]

On Tue, May 10 2022 at 15:15, Mark Rutland wrote:
> On Tue, May 10, 2022 at 02:13:20PM +0200, Lukas Wunner wrote:
>> Actually, since you're mentioning the in_nmi() check, I suspect
>> there's another problem here:
>> 
>> generic_handle_domain_nmi() warns if !in_nmi(), then calls down 
>> to handle_irq_desc() which warns if !in_hardirq().  Doesn't this
>> cause a false-positive !in_hardirq() warning for a NMI on GIC/GICv3?
>
> I agree that doesn't look right.
>
>> The only driver calling request_nmi() or request_percpu_nmi() is
>> drivers/perf/arm_pmu.c.  So that's the only one affected.
>> You may want to test if that driver indeed exhibits such a
>> false-positive warning since c16816acd086.
>
> In testing with v5.18-rc5, I can't see that going wrong.
>
> I also hacked the following in:
>
> -------->8--------
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index 939d21cd55c38..3c85608a8779f 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -718,6 +718,7 @@ EXPORT_SYMBOL_GPL(generic_handle_domain_irq);
>  int generic_handle_domain_nmi(struct irq_domain *domain, unsigned int hwirq)
>  {
>         WARN_ON_ONCE(!in_nmi());
> +       WARN_ON_ONCE(!in_hardirq());
>         return handle_irq_desc(irq_resolve_mapping(domain, hwirq));

which is pointless because NMI entry code has to invoke [__]nmi_enter()
before invoking this function. [__]nmi_enter() does:

    __preempt_count_add(NMI_OFFSET + HARDIRQ_OFFSET);

So it's more than bloody obvious why there is no warning triggered for a
regular hardware induced NMI invocation.

For a software invocation from the wrong context it does not matter how
many redundant WARN_ONs you add. The existing ones are covering it
nicely already.

Thanks,

        tglx

Re: [PATCH v2 17/17] irq: remove handle_domain_{irq,nmi}()

[Mark Rutland]

On Wed, May 11, 2022 at 02:11:52AM +0200, Thomas Gleixner wrote:
> On Tue, May 10 2022 at 15:15, Mark Rutland wrote:
> > On Tue, May 10, 2022 at 02:13:20PM +0200, Lukas Wunner wrote:
> >> Actually, since you're mentioning the in_nmi() check, I suspect
> >> there's another problem here:
> >> 
> >> generic_handle_domain_nmi() warns if !in_nmi(), then calls down 
> >> to handle_irq_desc() which warns if !in_hardirq().  Doesn't this
> >> cause a false-positive !in_hardirq() warning for a NMI on GIC/GICv3?
> >
> > I agree that doesn't look right.
> >
> >> The only driver calling request_nmi() or request_percpu_nmi() is
> >> drivers/perf/arm_pmu.c.  So that's the only one affected.
> >> You may want to test if that driver indeed exhibits such a
> >> false-positive warning since c16816acd086.
> >
> > In testing with v5.18-rc5, I can't see that going wrong.
> >
> > I also hacked the following in:
> >
> > -------->8--------
> > diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> > index 939d21cd55c38..3c85608a8779f 100644
> > --- a/kernel/irq/irqdesc.c
> > +++ b/kernel/irq/irqdesc.c
> > @@ -718,6 +718,7 @@ EXPORT_SYMBOL_GPL(generic_handle_domain_irq);
> >  int generic_handle_domain_nmi(struct irq_domain *domain, unsigned int hwirq)
> >  {
> >         WARN_ON_ONCE(!in_nmi());
> > +       WARN_ON_ONCE(!in_hardirq());
> >         return handle_irq_desc(irq_resolve_mapping(domain, hwirq));
> 
> which is pointless because NMI entry code has to invoke [__]nmi_enter()
> before invoking this function. [__]nmi_enter() does:
> 
>     __preempt_count_add(NMI_OFFSET + HARDIRQ_OFFSET);
> 
> So it's more than bloody obvious why there is no warning triggered for a
> regular hardware induced NMI invocation.

Ugh, yes; clearly I need new eyes and/or more sleep. I entirely missed that we
treat an NMI as *also* being a hardirq rather than something completely
independent, and that means that this is *not* a problem for NMI.

Thanks for pointing that out!

> For a software invocation from the wrong context it does not matter how
> many redundant WARN_ONs you add. The existing ones are covering it
> nicely already.

Yup; as above I was clearly not thinknig straight here.

Mark.

Re: [PATCH v2 17/17] irq: remove handle_domain_{irq,nmi}()

[Mark Rutland]

On Wed, May 11, 2022 at 12:52:29AM +0200, Thomas Gleixner wrote:
> On Tue, May 10 2022 at 15:15, Mark Rutland wrote:
> > On Tue, May 10, 2022 at 02:13:20PM +0200, Lukas Wunner wrote:
> >> For gpio-dln2.c, I believe it from inspection.
> >> 
> >> For smsc95xx.c, I'm actually seeing it go wrong in practice,
> >> unedited dmesg splat is included below FWIW.
> >
> > Thanks; having the trace makes this much easier to analyse.
> 
> which confirmes what I talked about before:
> 
> >> WARNING: CPU: 3 PID: 75 at kernel/irq/irqdesc.c:702 generic_handle_domain_irq+0x88/0x94
> >>  generic_handle_domain_irq from smsc95xx_status+0x54/0xb0
> >>  smsc95xx_status from intr_complete+0x80/0x84
> >>  intr_complete from __usb_hcd_giveback_urb+0xa4/0x12c
> >>  __usb_hcd_giveback_urb from usb_hcd_giveback_urb+0x118/0x11c
> >>  usb_hcd_giveback_urb from completion_tasklet_func+0x7c/0xc8
> >>  completion_tasklet_func from tasklet_callback+0x20/0x24
> >>  tasklet_callback from tasklet_action_common.constprop.0+0x148/0x220
> >>  tasklet_action_common.constprop.0 from tasklet_hi_action+0x28/0x30
> >>  tasklet_hi_action from __do_softirq+0x154/0x3e8
> >>  __do_softirq from __local_bh_enable_ip+0x12c/0x1a8
> >>  __local_bh_enable_ip from irq_forced_thread_fn+0x7c/0xac
> >>  irq_forced_thread_fn from irq_thread+0x16c/0x228
> >>  irq_thread from kthread+0x100/0x140
> 
> So what happens here:
> 
>  interrupt
>     -> wakeup threaded handler
> 
>  threaded handler runs
>     local_bh_disable();
>     ....
>     schedules tasklet
>     ...
>     local_bh_enable()
>       do_softirq()
>         run_tasklet()
>           urb_completion()
>             smsc95xx_status()
>               generic_handle_domain_irq()
> 
> That interrupt in question is an interrupt, which is not handled by the
> primary CPU interrupt chips. It's a synthetic interrupt which is
> generated from the received USB packet.
> 
> +	/* USB interrupts are received in softirq (tasklet) context.
> +	 * Switch to hardirq context to make genirq code happy.
> +	 */
> +	local_irq_save(flags);
> +	__irq_enter_raw();
> +
>  	if (intdata & INT_ENP_PHY_INT_)
> -		;
> +		generic_handle_domain_irq(pdata->irqdomain, PHY_HWIRQ);
> 
> This __irq_enter_raw() is really wrong. This is _not_ running in hard
> interrupt context. Pretending so creates more problems than it
> solves. It breaks context tracking, confuses lockdep ...
> 
> We also have demultiplexed interrupts which are nested in a threaded
> interrupt handler and share the thread context. No, we are not going to
> pretend that they run in hard interrupt context either.
> 
> So we need a clear distinction between interrupts which really happen in
> hard interrupt context and those which are synthetic and can be invoked
> from pretty much any context.
> 
> Anything else is just a recipe for disaster and endless supply of half
> baken hacks.

Agreed. IIUC everyone agrees the __irq_enter_raw() usage is a hack, but what's
not clear is what we *should* do -- sorry if I'm being thick here.

I suspect that given we have generic_handle_irq_safe() for situations like this
we should add a generic_handle_domain_irq_safe(), and use that in this driver?
That way we can keep the `WARN_ON_ONCE(!in_hardirq())` in
generic_handle_domain_irq().

... or do you think we should do something else entirely?

Thanks,
Mark.

Re: [PATCH v2 17/17] irq: remove handle_domain_{irq,nmi}()

[Lukas Wunner]

On Wed, May 11, 2022 at 09:23:56AM +0100, Mark Rutland wrote:
> On Wed, May 11, 2022 at 12:52:29AM +0200, Thomas Gleixner wrote:
> > +	/* USB interrupts are received in softirq (tasklet) context.
> > +	 * Switch to hardirq context to make genirq code happy.
> > +	 */
> > +	local_irq_save(flags);
> > +	__irq_enter_raw();
> > +
> >  	if (intdata & INT_ENP_PHY_INT_)
> > -		;
> > +		generic_handle_domain_irq(pdata->irqdomain, PHY_HWIRQ);
> 
> Agreed. IIUC everyone agrees the __irq_enter_raw() usage is a hack,
> but what's not clear is what we *should* do
> 
> I suspect that given we have generic_handle_irq_safe() for situations
> like this we should add a generic_handle_domain_irq_safe(), and use
> that in this driver?
> That way we can keep the `WARN_ON_ONCE(!in_hardirq())` in
> generic_handle_domain_irq().

Thomas applied 792ea6a074ae ("genirq: Remove WARN_ON_ONCE() in
generic_handle_domain_irq()") tonight:

http://git.kernel.org/tip/tip/c/792ea6a074ae

That allows me to drop the controversial __irq_enter_raw()
and thus unblock my smsc95xx series.

generic_handle_domain_irq_safe() would merely be a wrapper for
generic_handle_domain_irq() which disables local interrupts.
Then I wouldn't have to do that in smsc95xx.c.  IMHO that's a
cosmetic improvement, though I'll be happy to provide a patch
if desired?

Thanks,

Lukas

Re: [PATCH v2 17/17] irq: remove handle_domain_{irq,nmi}()

[Mark Rutland]

On Wed, May 11, 2022 at 10:57:41AM +0200, Lukas Wunner wrote:
> On Wed, May 11, 2022 at 09:23:56AM +0100, Mark Rutland wrote:
> > On Wed, May 11, 2022 at 12:52:29AM +0200, Thomas Gleixner wrote:
> > > +	/* USB interrupts are received in softirq (tasklet) context.
> > > +	 * Switch to hardirq context to make genirq code happy.
> > > +	 */
> > > +	local_irq_save(flags);
> > > +	__irq_enter_raw();
> > > +
> > >  	if (intdata & INT_ENP_PHY_INT_)
> > > -		;
> > > +		generic_handle_domain_irq(pdata->irqdomain, PHY_HWIRQ);
> > 
> > Agreed. IIUC everyone agrees the __irq_enter_raw() usage is a hack,
> > but what's not clear is what we *should* do
> > 
> > I suspect that given we have generic_handle_irq_safe() for situations
> > like this we should add a generic_handle_domain_irq_safe(), and use
> > that in this driver?
> > That way we can keep the `WARN_ON_ONCE(!in_hardirq())` in
> > generic_handle_domain_irq().
> 
> Thomas applied 792ea6a074ae ("genirq: Remove WARN_ON_ONCE() in
> generic_handle_domain_irq()") tonight:
> 
> http://git.kernel.org/tip/tip/c/792ea6a074ae

Ah; I missed that. Sorry for the noise!

> That allows me to drop the controversial __irq_enter_raw()
> and thus unblock my smsc95xx series.
> 
> generic_handle_domain_irq_safe() would merely be a wrapper for
> generic_handle_domain_irq() which disables local interrupts.
> Then I wouldn't have to do that in smsc95xx.c.  IMHO that's a
> cosmetic improvement, though I'll be happy to provide a patch
> if desired?

I think it's a nice-to-have, but I don't have a strong feelings about it, so I
think we can forget about it for now unless Marc or Thomas want it.

Thanks,
Mark.