Re: [PATCH v2 2/2] ipmi: kcs_bmc_aspeed: Support multiple LPC controller instances

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Uninitialized spinlock crash when IRQ is triggered before `kcs_bmc_add_device()` initializes it.
- [High] Missing hardware cleanup on probe error leads to a shared IRQ storm.
- [High] Use-After-Free due to dangling device pointer on `kcs_bmc_add_device()` failure.
--

--- Patch [2]: [PATCH v2 2/2] ipmi: kcs_bmc_aspeed: Support multiple LPC controller instances ---
commit 17a4462de4be19f6ce20e258d80cf2c95a1f7589
Author: Yu-Che Hsieh <yc_hsieh@aspeedtech.com>

ipmi: kcs_bmc_aspeed: Support multiple LPC controller instances

Some SoCs (e.g. AST2700) expose more than one physical LPC controller,
each instantiating its own independent set of KCS0-KCS3 channels.
The driver is updated to derive kcs_bmc_device::channel based on an
optional LPC controller alias, allowing multiple instances to register
their chardevs properly.

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

[ ... ]

> @@ -590,9 +605,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 isn't a bug introduced by this patch, but does this probe function have a
race condition where an uninitialized spinlock can be acquired?

The downstream IRQ is requested in aspeed_kcs_config_downstream_irq() and
the hardware channel is enabled before kcs_bmc_add_device() is called:

drivers/char/ipmi/kcs_bmc_aspeed.c:aspeed_kcs_probe() {
    ...
	/* Host to BMC IRQ */
	rc = aspeed_kcs_config_downstream_irq(kcs_bmc, pdev);
    ...
	aspeed_kcs_irq_mask_update(kcs_bmc, (KCS_BMC_EVENT_TYPE_IBF | KCS_BMC_EVENT_TYPE_OBE), 0);
	aspeed_kcs_enable_channel(kcs_bmc, true);

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

The spinlock kcs_bmc->lock is only initialized inside kcs_bmc_add_device().
If a shared interrupt fires, or a pending interrupt from the hardware is
triggered right after enabling the channel, could kcs_bmc_handle_event()
attempt to lock kcs_bmc->lock before it has been initialized?

[Severity: High]
This is a pre-existing issue, but is there a missing hardware cleanup on the
probe error path?

If kcs_bmc_add_device() fails, the function returns immediately:

drivers/char/ipmi/kcs_bmc_aspeed.c:aspeed_kcs_probe() {
    ...
	rc = kcs_bmc_add_device(&priv->kcs_bmc);
	if (rc) {
		dev_warn(&pdev->dev, "Failed to register channel %d: %d\n", kcs_bmc->channel, rc);
		return rc;
	}
    ...
}

Because devm cleans up the IRQ handler but the hardware remains enabled and
unmasked, will this unhandled shared IRQ fire continuously and cause an IRQ
storm?

[Severity: High]
This is a pre-existing issue, but does a failure in kcs_bmc_add_device() lead
to a use-after-free?

In kcs_bmc_add_device(), kcs_bmc->entry is added to the global kcs_bmc_devices
list. If any registered driver fails to add the device, the function returns an
error without removing the device from the list:

drivers/char/ipmi/kcs_bmc.c:kcs_bmc_add_device() {
    ...
	list_add(&kcs_bmc->entry, &kcs_bmc_devices);
    ...
	return error;
}

Since list_del() is not called on the error path, and the probe error causes
devres to free the memory for priv (which embeds kcs_bmc), will any subsequent
driver registration or removal iterate over the global list and access freed
memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813-upstream_kcs_multiple_lpc-v2-0-775b1db3fe95@aspeedtech.com?part=2