Re: [PATCH 02/24] alpha: enter hardirq context before looking up the irq descriptor

[Magnus Lindholm]

Hi Matt,

On Sat, Sep 5, 2026 at 4:43 AM Matt Turner <mattst88@xxxxxxxxx> wrote:
>
> On Thu, Sep 3, 2026 at 6:07 PM Magnus Lindholm <linmag7@xxxxxxxxx> wrote:
> >
> > Hi Matt,
> >
> > On Tue, Sep 1, 2026 at 5:50 PM Matt Turner <mattst88@xxxxxxxxx> wrote:
> > >
> > > handle_irq() called irq_to_desc() before irq_enter(), so that lookup ran
> > > with the preempt count still saying task context and with RCU not yet
> > > watching.  Generic code called from an interrupt handler should see
> > > hardirq context, and irq_to_desc() is more than an array index once
> > > SPARSE_IRQ is in use.
> > >
> > > Move irq_enter() to the top of the function and add the matching
> > > irq_exit() to the invalid interrupt path.
> > >
> > > Assisted-by: Claude:claude-opus-5
> > > Signed-off-by: Matt Turner <mattst88@xxxxxxxxx>
> > > ---
> > >  arch/alpha/kernel/irq.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git ./arch/alpha/kernel/irq.c ./arch/alpha/kernel/irq.c
> > > index 4a6a8b1d5a8b..5867a1655045 100644
> > > --- ./arch/alpha/kernel/irq.c
> > > +++ ./arch/alpha/kernel/irq.c
> > > @@ -107,18 +107,21 @@ handle_irq(int irq)
> > >          * handled by some other CPU. (or is disabled)
> > >          */
> > >         static unsigned int illegal_count=0;
> > > -       struct irq_desc *desc = irq_to_desc(irq);
> > > -
> > > +       struct irq_desc *desc;
> > > +
> > > +       irq_enter();
> > > +
> > > +       desc = irq_to_desc(irq);
> > >         if (!desc || ((unsigned) irq > ACTUAL_NR_IRQS &&
> > >             illegal_count < MAX_ILLEGAL_IRQS)) {
> > >                 irq_err_count++;
> > >                 illegal_count++;
> > >                 printk(KERN_CRIT "device_interrupt: invalid interrupt %d\n",
> > >                        irq);
> > > +               irq_exit();
> > >                 return;
> > >         }
> > >
> > > -       irq_enter();
> > >         generic_handle_irq_desc(desc);
> > >         irq_exit();
> > >  }
> > > --
> > > 2.54.0
> > >
> >
> > Going over this code again, I noticed what may be redundant hardirq
> > accounting in the existing device-interrupt path.
> >
> > For type 3 interrupts, do_entInt() already wraps
> > alpha_mv.device_interrupt() in irq_enter()/irq_exit(). The platform
> > dispatcher then calls handle_irq(), which enters and exits hardirq
> > context again. Therefore, irq_to_desc() is already reached in hardirq
> > context before this patch. The patch moves it from nesting level one
> > to level two rather than from task context to hardirq context.
> >
> > The RTC path does not have the outer irq_enter(), so the change is
> > needed there.
> >
> > Would it be cleaner for do_entInt() to own the hardirq context for
> > both paths and remove the pair from handle_irq()? Or is there another
> > caller which requires handle_irq() to manage it itself?
> >
> > I tested the patch on a two-CPU UP2000 with lockdep, IRQ flag tracing,
> > and sparse IRQs enabled. SCSI and network interrupts operated normally,
> > I observed no lockdep or IRQ-related warnings, and debug_locks remained
> > enabled in /proc/lockdep_stats.
>
> Hi Magnus,
>
> Good question. Yes, do_entInt() should own irq_enter()/irq_exit() for
> the RTC and device-interrupt cases too, same as it already does for
> IPI, machine-check, and perf. RTC and device-interrupt are the only
> two that don't, and only because handle_irq() does it internally.
>
> There's a wrinkle. A bunch of device_interrupt() implementations call
> handle_irq() in a loop over several pending vectors: sys_takara.c,
> sys_eiger.c, sys_alcor.c, sys_dp264.c, sys_cabriolet.c,
> sys_noritake.c, sys_mikasa.c, sys_rx164.c, irq_pyxis.c, irq_i8259.c.
> Right now each of those handle_irq() calls enters and exits hardirq
> context on its own, so one hardware interrupt that demuxes to N IRQs
> does N enter/exit pairs. Moving the pair up to do_entInt() collapses
> that to one per hardware interrupt, matching how the
> IPI/machine-check/perf cases already work.
>
> I don't see anything in the per-machine device_interrupt() bodies that
> needs task context before the first handle_irq() call -- it's all
> vector decode. But I've only got one Marvel box to boot-test on, so
> I'm not comfortable folding the reshuffle into this patch without at
> least reading through the other machine types more carefully.
>
> I'll send it as a follow-up on top of the series rather than reworking
> patch 2. Easier to drop if something turns out to care. Let me know if
> you think that's wrong.
>

That sounds like a good plan, we treat that as a follow-up once this
has landed.

Regards

Magnus