Re: [PATCH v1] ARM: dts: aspeed: g6: Use KCS AST2600 compatible string

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The KCS BMC drivers register and enable their IRQ handler before initializing the device spinlock, leading to a potential crash or lockdep splat on an uninitialized spinlock.
--

commit a51ce0f5a1fd9344e679346b6911e87b35021057
Author: Haiyue Wang <haiyuewa@163.com>

ARM: dts: aspeed: g6: Use KCS AST2600 compatible string

This commit updates the device tree source for the AST2600 to use the
specific aspeed,ast2600-kcs-bmc compatible string instead of the older
ast2500 compatible string.

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> index 189bc3bbb47c9..9d09d6e0092a8 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> +++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> @@ -576,7 +576,7 @@ lpc: lpc@1e789000 {
>  				ranges = <0x0 0x1e789000 0x1000>;
>  
>  				kcs1: kcs@24 {
> -					compatible = "aspeed,ast2500-kcs-bmc-v2";
> +					compatible = "aspeed,ast2600-kcs-bmc";

[Severity: High]
This is a pre-existing issue, but modifying the compatible string draws
attention to an issue in the underlying driver's probe routine.

Does the driver initialization sequence have a race condition that could
lead to a crash on an uninitialized spinlock?

Looking at the driver for this compatible string:

drivers/char/ipmi/kcs_bmc_aspeed.c:aspeed_kcs_probe() {
    ...
    rc = aspeed_kcs_config_downstream_irq(kcs_bmc, pdev);
    if (rc)
        return rc;
    ...
    aspeed_kcs_enable_channel(kcs_bmc, true);

    rc = kcs_bmc_add_device(&priv->kcs_bmc);
    ...
}

The IRQ handler is registered in aspeed_kcs_config_downstream_irq() and
hardware interrupts are explicitly enabled via aspeed_kcs_enable_channel()
before kcs_bmc_add_device() is called. However, kcs_bmc_add_device() is
where spin_lock_init() initializes the device lock. 

If an interrupt fires (either from the hardware or on a shared IRQ line)
in the small window between enabling the channel and initializing the
spinlock, will kcs_bmc_handle_event() unconditionally attempt to acquire
the uninitialized spinlock?

As an aside, the same initialization order issue appears to be present in
the npcm7xx_kcs_probe() routine as well:

drivers/char/ipmi/kcs_bmc_npcm7xx.c:npcm7xx_kcs_probe() {
    ...
    rc = npcm7xx_kcs_config_irq(kcs_bmc, pdev);
    if (rc)
        return rc;

    npcm7xx_kcs_irq_mask_update(kcs_bmc, ...);
    npcm7xx_kcs_enable_channel(kcs_bmc, true);

    rc = kcs_bmc_add_device(kcs_bmc);
    ...
}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627052708.172918-1-haiyuewa@163.com?part=1