Re: [ISSUE + PATCH] Interrupts were enabled early by spinlock guard

[Nicolas Ferre <nicolas.ferre@microchip.com>]

On 25/08/2025 at 14:33, Nicolas Ferre wrote:
> On 14/08/2025 at 14:59, Edgar Bonet wrote:
>> Hello everybody!
>>
>> I am facing an "Interrupts were enabled early" kernel warning which, as
>> far as I can tell, is caused by a spinlock guard in an ARM/Microchip
>> IRQCHIP driver. I think I solved the issue, and I am proposing a patch
>> below, after the scissor. But I must first disclose that:
>>
>>     * I am completely new to Linux internals and its development process.
>>       This is why I chose to err on the side of providing too much
>>       information on my issue. It is not unlikely that I am doing
>>       something very wrong.
>>
>>     * I am not subscribed to either of the linux-{,arm-}kernel mailing
>>       lists
>>
>>     * I will be far from the Internet in the few days to come. I should be
>>       connected an responsive starting from 2025-08-24.
> 
> Edgar,
> 
> Thanks a lot for having investigated this.
> 
> 
>> ## Context
>>
>> I am playing with an Acmesystems Acqua[1] system on module, which is
>> based on a SAMA5D31 SoC (single core Cortex-A5). I maintain the
>> Buildroot defconfig for this board,[2] which is currently based on a
>> vanilla Linux kernel v6.12.41.
>>
>> As I wanted to check that the board runs fine on newer kernels, I built
>> a v6.16 for it using gcc 14, binutils 2.43, the in-tree sama5_defconfig
>> merged with this fragment:
>>
>>       # CONFIG_BRIDGE is not set
>>       # CONFIG_MODULES is not set
>>       # CONFIG_NET_DSA is not set
>>       # CONFIG_WIRELESS is not set
>>       # CONFIG_USB_NET_DRIVERS is not set
>>       # CONFIG_WLAN is not set
>>       # CONFIG_MEDIA_SUPPORT is not set
>>       # CONFIG_SOUND is not set
>>       # CONFIG_AUTOFS_FS is not set
>>
>> and the Buildroot-provided device tree, patched for compatibility with
>> Linux commit 510a6190cf5e ("ARM: dts: microchip: fix faulty ohci/ehci
>> node names").
>>
>> The defconfig fragment above is meant to remove module support (with
>> some unused drivers along the way), which makes testing easier for me.
>>
>>
>> ## Issue
>>
>> While booting, Linux v6.16 printed this message on the serial console:
>>
>>       ------------[ cut here ]------------
>>       WARNING: CPU: 0 PID: 0 at init/main.c:1024 start_kernel+0x4d0/0x5dc
>>       Interrupts were enabled early
>>       CPU: 0 UID: 0 PID: 0 Comm: swapper Not tainted 6.16.0 #1 NONE
>>       Hardware name: Atmel SAMA5
>>       Call trace:
>>        unwind_backtrace from show_stack+0x10/0x14
>>        show_stack from dump_stack_lvl+0x38/0x48
>>        dump_stack_lvl from __warn+0x78/0xd4
>>        __warn from warn_slowpath_fmt+0x98/0xc8
>>        warn_slowpath_fmt from start_kernel+0x4d0/0x5dc
>>        start_kernel from 0x0
>>       ---[ end trace 0000000000000000 ]---
> 
> Indeed, reproduced on sam9x75 (which has AIC5), on 6.17.0-rc3.
> 
>> The board seemed to work fine, so maybe this warning is completely
>> harmless. It looked, however, scary enough to deserve some
>> investigation.
>>
>>
>> ## Bug hunting
>>
>> I could not reproduce the issue on a qemu virtual machine. All tests
>> were then done on the real hardware.
>>
>> I looked for the Linux commit that introduced the issue. ‘git bisect’
>> told me this was 195298c3b116 ("genirq/generic-chip: Convert core code
>> to lock guards"). I then checked out a recent mainline kernel, namely
>> v6.17-rc1, and tried to revert that commit. For this, I first had to
>> revert two follow-up commits, namely 7ae844a6650c ("genirq/generic-chip:
>> Remove unused lock wrappers") and 771487050f83 ("genirq/generic-chip:
>> Fix incorrect lock guard conversions"). This was unsuccessful: I still
>> had the warning.
>>
>> I went back to a clean v6.17-rc1 and tried to find the thing that was
>> enabling interrupts early. After lots of printk() debugging, I
>> discovered it was a guard(raw_spinlock_irq) destructor. The call chain
>> is this (line numbers are for v6.17-rc1):
>>
>>       start_kernel (init/main.c:1011)
>>       -> time_init (arch/arm/kernel/time.c:96)
>>       -> timer_probe (drivers/clocksource/timer-probe.c:30)
>>       => tcb_clksrc_init (drivers/clocksource/timer-atmel-tcb.c:413)
>>       -> of_irq_get (drivers/of/irq.c:474)
>>       -> irq_create_of_mapping (kernel/irq/irqdomain.c:980)
>>       -> irq_create_fwspec_mapping (kernel/irq/irqdomain.c:848)
>>       => aic5_irq_domain_xlate (drivers/irqchip/irq-atmel-aic5.c:287)
>>       -> guard destructor (raw_spin_unlock_irq?)
>>
>> where (=>) is an indirect call through a function pointer.
>>
>>
>> ## Tentative fix
>>
>> Commit 771487050f83 gave me the inspiration. The guard in question was
>> introduced by b00bee8afaca ("irqchip: Convert generic irqchip locking to
>> guards"), which replaced calls to irq_gc_lock_irq{save,restore}() by
>> guard(raw_spinlock_irq) (with no “save” in the name). The commit log
>> states that this is “intended and correct”, but I could not make sense
>> of the explanation. My (possibly faulty) understanding is that the guard
>> constructor disables interrupts, and the destructor either
>> unconditionally enables them (raw_spinlock_irq), or restores the
>> previous interrupt state (raw_spinlock_irqsave).
>>
>> I then replaced guard(raw_spinlock_irq) with guard(raw_spinlock_irqsave)
>> and that seemed to do the job: the warning is gone. See the patch below
>> the scissors.
>>
>> Best regards, and thank-you for reading so far.
>>
>> Edgar Bonet.
>>
>> [1] https://www.acmesystems.it/acqua
>> [2] https://gitlab.com/buildroot.org/buildroot/-/blob/2025.08-rc1/configs/acmesystems_acqua_a5_512mb_defconfig
>>
>> ------------------------------------------------------------------ >8 --
>> Subject: [PATCH] irqchip/atmel-aic5: Fix incorrect lock guard conversion
>>
>> Commit b00bee8afaca ("irqchip: Convert generic irqchip locking to guards")
>> replaced calls to irq_gc_lock_irq{save,restore}() with
>> guard(raw_spinlock_irq). However, in irq-atmel-aic5.c, one such guard is
>> created early in the boot process, before interrupts are initially enabled.
>> As its destructor enables interrupts, this results in the following warning
>> on a SAMA5D31-based system:
>>
>>       ------------[ cut here ]------------
>>       WARNING: CPU: 0 PID: 0 at init/main.c:1024 start_kernel+0x4d0/0x5dc
>>       Interrupts were enabled early
>>       CPU: 0 UID: 0 PID: 0 Comm: swapper Not tainted 6.16.0 #1 NONE
>>       Hardware name: Atmel SAMA5
>>       Call trace:
>>        unwind_backtrace from show_stack+0x10/0x14
>>        show_stack from dump_stack_lvl+0x38/0x48
>>        dump_stack_lvl from __warn+0x78/0xd4
>>        __warn from warn_slowpath_fmt+0x98/0xc8
>>        warn_slowpath_fmt from start_kernel+0x4d0/0x5dc
>>        start_kernel from 0x0
>>       ---[ end trace 0000000000000000 ]---
>>
>> Fix this by using guard(raw_spinlock_irqsave) instead.
>>
>> Fixes: b00bee8afaca ("irqchip: Convert generic irqchip locking to guards")
>> Signed-off-by: Edgar Bonet <bonet@grenoble.cnrs.fr>
> 
> Indeed. Thanks for the detailed explanation and the patch:
> 
> Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com> # on sam9x75

For you Thomas, also this test:
Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com> # on sam9x35 (for 
similar changes to irq-atmel-aic.c)

Thanks, best regards,
   Nicolas


> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> (if needed)
> 
> Best regards,
>     Nicolas
> 
>> ---
>>    drivers/irqchip/irq-atmel-aic5.c | 2 +-
>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c
>> index 60b00d2c3d7a..1f14b401f71d 100644
>> --- a/drivers/irqchip/irq-atmel-aic5.c
>> +++ b/drivers/irqchip/irq-atmel-aic5.c
>> @@ -279,7 +279,7 @@ static int aic5_irq_domain_xlate(struct irq_domain *d,
>>           if (ret)
>>                   return ret;
>>
>> -       guard(raw_spinlock_irq)(&bgc->lock);
>> +       guard(raw_spinlock_irqsave)(&bgc->lock);
>>           irq_reg_writel(bgc, *out_hwirq, AT91_AIC5_SSR);
>>           smr = irq_reg_readl(bgc, AT91_AIC5_SMR);
>>           aic_common_set_priority(intspec[2], &smr);
>> --
>> 2.43.0
>