Re: [PATCH] alpha: marvel: Fix lock ordering in init_io7_irqs()

On Fri, Apr 10, 2026 at 2:58 PM Magnus Lindholm <[email protected]> wrote:
>
> On Fri, Apr 3, 2026 at 5:04 PM Matt Turner <[email protected]> wrote:
> >
> > Move irq_set_chip_and_handler() and irq_set_status_flags() calls
> > outside the io7->irq_lock raw spinlock.  These functions take
> > sparse_irq_lock, which is a mutex, and taking a sleeping lock while
> > holding a raw spinlock is invalid.  The raw spinlock only needs to
> > protect the hardware CSR accesses.
> >
> > This fixes the following lockdep splat during boot:
> >
> >   [ BUG: Invalid wait context ]
> >   swapper/0/0 is trying to lock:
> >   sparse_irq_lock{....}-{4:4}, at: irq_mark_irq
> >   other info that might help us debug this:
> >   context-{5:5}
> >   1 lock held by swapper/0/0:
> >    #0: &io7->irq_lock{....}-{2:2}, at: init_io7_irqs.constprop.0
> >
> > Assisted-by: Claude:claude-opus-4-6
> > Signed-off-by: Matt Turner <[email protected]>
> > ---
> >  arch/alpha/kernel/sys_marvel.c | 27 +++++++++++++--------------
> >  1 file changed, 13 insertions(+), 14 deletions(-)
> >
> > diff --git ./arch/alpha/kernel/sys_marvel.c ./arch/alpha/kernel/sys_marvel.c
> > index 1f99b03effc2..d0bdd5e6cfd7 100644
> > --- ./arch/alpha/kernel/sys_marvel.c
> > +++ ./arch/alpha/kernel/sys_marvel.c
> > @@ -263,6 +263,18 @@ init_io7_irqs(struct io7 *io7,
> >          */
> >         printk("  Interrupts reported to CPU at PE %u\n", boot_cpuid);
> >
> > +       /* Set up the lsi irqs.  */
> > +       for (i = 0; i < 128; ++i) {
> > +               irq_set_chip_and_handler(base + i, lsi_ops, handle_level_irq);
> > +               irq_set_status_flags(i, IRQ_LEVEL);
> > +       }
> > +
> > +       /* Set up the msi irqs.  */
> > +       for (i = 128; i < (128 + 512); ++i) {
> > +               irq_set_chip_and_handler(base + i, msi_ops, handle_level_irq);
> > +               irq_set_status_flags(i, IRQ_LEVEL);
> > +       }
> > +
> >         raw_spin_lock(&io7->irq_lock);
> >
> >         /* set up the error irqs */
> > @@ -272,26 +284,13 @@ init_io7_irqs(struct io7 *io7,
> >         io7_redirect_irq(io7, &io7->csrs->STV_CTL.csr, boot_cpuid);
> >         io7_redirect_irq(io7, &io7->csrs->HEI_CTL.csr, boot_cpuid);
> >
> > -       /* Set up the lsi irqs.  */
> > -       for (i = 0; i < 128; ++i) {
> > -               irq_set_chip_and_handler(base + i, lsi_ops, handle_level_irq);
> > -               irq_set_status_flags(i, IRQ_LEVEL);
> > -       }
> > -
> >         /* Disable the implemented irqs in hardware.  */
> > -       for (i = 0; i < 0x60; ++i)
> > +       for (i = 0; i < 0x60; ++i)
> >                 init_one_io7_lsi(io7, i, boot_cpuid);
> >
> >         init_one_io7_lsi(io7, 0x74, boot_cpuid);
> >         init_one_io7_lsi(io7, 0x75, boot_cpuid);
> >
> > -
> > -       /* Set up the msi irqs.  */
> > -       for (i = 128; i < (128 + 512); ++i) {
> > -               irq_set_chip_and_handler(base + i, msi_ops, handle_level_irq);
> > -               irq_set_status_flags(i, IRQ_LEVEL);
> > -       }
> > -
> >         for (i = 0; i < 16; ++i)
> >                 init_one_io7_msi(io7, i, boot_cpuid);
> >
> > --
>
> The lock ordering fix makes sense to me.
>
> One question though: in the moved setup loops we now have
>
>     irq_set_chip_and_handler(base + i, ...)
>
> but still
>
>     irq_set_status_flags(i, IRQ_LEVEL);
>
> Why does irq_set_status_flags() not need `base + i` as
> well? I would have expected that to match the IRQ number
> passed to irq_set_chip_and_handler(), i.e. use
>
>     irq_set_status_flags(base + i, IRQ_LEVEL);
>
>
> Magnus

Nice catch.

This is a pre-existing bug introduced in commit
08876fe8519c4f7625efba8aa3d51b1d24cd8a13. I'll include a patch in my
series to fix this.