Re: [PATCH v3] ipmi: kcs_bmc_aspeed: Support multiple LPC controller instances

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Dynamically allocated `struct aspeed_kcs_bank` entries are never freed on module unload, causing a memory leak.
- [Medium] The driver stores the parent `device_node` pointer in a global list without acquiring a reference, risking a dangling pointer and ABA aliasing.

Pre-existing issues:
- [High] The IRQ handler is registered before `kcs_bmc->lock` is initialized, leading to uninitialized spinlock access if a shared interrupt fires.
- [High] The KCS hardware channel is left enabled if `kcs_bmc_add_device` fails, potentially causing an unhandled IRQ storm.
--

--- Patch [1]: [PATCH v3] ipmi: kcs_bmc_aspeed: Support multiple LPC controller instances ---
commit 6a9db3a3464fe3254462564cc2c5b13db5dfb6b3
Author: Yu-Che Hsieh <yc_hsieh@aspeedtech.com>

ipmi: kcs_bmc_aspeed: Support multiple LPC controller instances

This patch adds support for multiple physical LPC controller instances on
SoCs like AST2700. It separates the hardware channel index from the global
KCS device channel index. A dynamic bank index is assigned to each LPC
controller node during probe to ensure unique chardev names.

Link: https://lore.kernel.org/r/20260813-upstream_kcs_multiple_lpc-v2-0-775b1db3fe95@aspeedtech.com

> diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
> index a13a3470c17a4..751f477c09e0d 100644
> --- a/drivers/char/ipmi/kcs_bmc_aspeed.c
> +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c

[ ... ]

> @@ -526,6 +530,47 @@ static int aspeed_kcs_of_get_channel(struct platform_device *pdev)
>  	return -EINVAL;
>  }
>  
> +struct aspeed_kcs_bank {
> +	struct device_node *lpc_np;
> +	struct list_head entry;
> +};
> +
> +static DEFINE_MUTEX(aspeed_kcs_bank_lock);
> +static LIST_HEAD(aspeed_kcs_banks);
> +
> +/*
> + * Assign each distinct LPC controller device_node a stable bank index the
> + * first time it's seen, so that KCS devices instantiated from different LPC
> + * controllers on the same SoC (e.g. AST2700) don't collide on the same
> + * global kcs_bmc_device::channel value.
> + */
> +static int aspeed_kcs_of_get_bank(struct device_node *lpc_np)
> +{
> +	struct aspeed_kcs_bank *bank;
> +	int index = 0;
> +
> +	mutex_lock(&aspeed_kcs_bank_lock);
> +
> +	list_for_each_entry(bank, &aspeed_kcs_banks, entry) {
> +		if (bank->lpc_np == lpc_np)
> +			goto out;
> +		index++;
> +	}
> +
> +	bank = kzalloc(sizeof(*bank), GFP_KERNEL);

[Severity: Medium]
Does this leak memory on module unload?

Since the bank entries are dynamically allocated and added to the global
aspeed_kcs_banks list, and the driver uses the default module exit routine,
it looks like unloading the module might permanently leak these allocations.

> +	if (!bank) {
> +		mutex_unlock(&aspeed_kcs_bank_lock);
> +		return -ENOMEM;
> +	}
> +
> +	bank->lpc_np = lpc_np;
> +	list_add_tail(&bank->entry, &aspeed_kcs_banks);

[Severity: Medium]
Is it safe to store the raw device_node pointer without pinning its reference?

If the LPC node is removed via Device Tree overlays, this pointer might
become dangling. If a new node is later allocated at the same memory address,
could it cause an ABA problem where an unrelated device incorrectly matches
this old bank index?

Should this use of_node_get() to keep the reference alive?

> +
> +out:
> +	mutex_unlock(&aspeed_kcs_bank_lock);
> +	return index;
> +}

[ ... ]

> @@ -590,9 +639,11 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	priv->channel = channel;
> +
>  	kcs_bmc = &priv->kcs_bmc;
>  	kcs_bmc->dev = &pdev->dev;
> -	kcs_bmc->channel = channel;
> +	kcs_bmc->channel = bank * KCS_CHANNEL_MAX + channel;
>  	kcs_bmc->ioreg = ast_kcs_bmc_ioregs[channel - 1];
>  	kcs_bmc->ops = &aspeed_kcs_ops;
>  

[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition in the
probe function just below this hunk.

The driver requests a shared irq before the lock is initialized:

aspeed_kcs_probe() {
    ...
    rc = aspeed_kcs_config_downstream_irq(kcs_bmc, pdev); // Requests IRQ
    ...
    rc = kcs_bmc_add_device(&priv->kcs_bmc); // Initializes kcs_bmc->lock
}

If a shared interrupt fires immediately after the IRQ is requested, could the
handler try to acquire the uninitialized spinlock in kcs_bmc_handle_event()
and cause a kernel panic?

[Severity: High]
This is also a pre-existing issue, but does the error path for device
registration leave the hardware channel enabled?

aspeed_kcs_probe() {
    ...
    aspeed_kcs_enable_channel(kcs_bmc, true);
    
    rc = kcs_bmc_add_device(&priv->kcs_bmc);
    if (rc) {
        dev_warn(&pdev->dev, ...);
        return rc;
    }
}

If kcs_bmc_add_device() fails, the function returns without calling
aspeed_kcs_enable_channel(kcs_bmc, false). Since the probe failure will
clean up the IRQ handler, could this leave the hardware asserting an
unhandled interrupt and eventually disable the shared IRQ line?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-upstream_kcs_multiple_lpc-v3-1-b15bb0f77462@aspeedtech.com?part=1