* [PATCH] alpha: marvel: Fix lock ordering in init_io7_irqs()
@ 2026-04-03 15:03 Matt Turner
2026-04-10 18:58 ` Magnus Lindholm
0 siblings, 1 reply; 3+ messages in thread
From: Matt Turner @ 2026-04-03 15:03 UTC (permalink / raw)
To: Magnus Lindholm; +Cc: linux-alpha, linux-kernel, Matt Turner
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 <mattst88@gmail.com>
---
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);
--
2.52.0
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] alpha: marvel: Fix lock ordering in init_io7_irqs()
2026-04-03 15:03 [PATCH] alpha: marvel: Fix lock ordering in init_io7_irqs() Matt Turner
@ 2026-04-10 18:58 ` Magnus Lindholm
2026-05-26 16:06 ` Matt Turner
0 siblings, 1 reply; 3+ messages in thread
From: Magnus Lindholm @ 2026-04-10 18:58 UTC (permalink / raw)
To: Matt Turner; +Cc: linux-alpha, linux-kernel
On Fri, Apr 3, 2026 at 5:04 PM Matt Turner <mattst88@gmail.com> 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 <mattst88@gmail.com>
> ---
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] alpha: marvel: Fix lock ordering in init_io7_irqs()
2026-04-10 18:58 ` Magnus Lindholm
@ 2026-05-26 16:06 ` Matt Turner
0 siblings, 0 replies; 3+ messages in thread
From: Matt Turner @ 2026-05-26 16:06 UTC (permalink / raw)
To: Magnus Lindholm; +Cc: linux-alpha, linux-kernel
On Fri, Apr 10, 2026 at 2:58 PM Magnus Lindholm <linmag7@gmail.com> wrote:
>
> On Fri, Apr 3, 2026 at 5:04 PM Matt Turner <mattst88@gmail.com> 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 <mattst88@gmail.com>
> > ---
> > 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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-05-26 16:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-04-03 15:03 [PATCH] alpha: marvel: Fix lock ordering in init_io7_irqs() Matt Turner
2026-04-10 18:58 ` Magnus Lindholm
2026-05-26 16:06 ` Matt Turner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®