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

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

[Yu-Che Hsieh]

Some SoCs (e.g. AST2700) expose more than one physical LPC controller
instance (e.g. lpc0@14c31000, lpc1@14c32000, plus pcie_lpc0@12c19000
and pcie_lpc1@12c19800 for the PCIe-facing path), each instantiating
its own independent set of KCS1-KCS4 channels using identical
IDR/ODR/STR register offsets.

kcs_bmc_device::channel currently serves two purposes at once:

  1. Selecting which HICR0/HICR2/HICRB bit-group to touch within a
     single LPC controller's register file. aspeed_kcs_of_get_channel()
     derives this purely from the KCS node's register offset, yielding
     values 1..4.

  2. Naming the misc chardev (/dev/ipmi-kcsN) exposed to userspace,
     which must be unique system-wide. kcs_bmc_cdev_ipmi.c uses
     "ipmi-kcs%u".

Both happen to be the same value only because this driver has only
ever had to support SoCs with a single LPC controller. On AST2700, KCS1
on every LPC controller computes to the same channel number, and
whichever instance probes second fails outright:

	sysfs: cannot create duplicate filename '/devices/virtual/misc/ipmi-kcs1'
	ast-kcs-bmc 14c32024.kcs: Unable to register device: -17
	ast-kcs-bmc 14c32024.kcs: Failed to add chardev for KCS channel 1: -17
	ast-kcs-bmc: probe of 14c32024.kcs failed with error -17

Split the two roles: keep a new driver-private `channel` field (1..4)
in struct aspeed_kcs_bmc for register access, leaving all the
HICR/IBFIE/LPCxE switch statements operating on it, and derive
kcs_bmc_device::channel as `bank * KCS_CHANNEL_MAX + channel`, where
`bank` identifies which LPC controller instance a KCS device belongs
to.

`bank` is obtained from aspeed_kcs_of_get_bank(), which keeps a
driver-private, mutex-protected list keyed by the LPC controller's
device_node pointer and assigns bank indices in
first-seen-during-probe order. SoCs with a single LPC controller
always end up with bank 0, so /dev/ipmi-kcsN naming is unchanged for
all existing boards, and no devicetree or binding changes are required
for any platform.

Signed-off-by: Yu-Che Hsieh <yc_hsieh@aspeedtech.com>
---
Changes in v3:
- Drop the of_alias_get_id()-based bank derivation and the dt-bindings 
  patch that documented it.
- Replace with dynamic bank discovery: the first time an LPC
  controller's device_node is seen during probe, assign it the next
  available bank index and remember it in a driver-private list. This
  needs no DT/binding changes at all.

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

Changes in v2:
  - Drop the RFC tag, per Lee Jones' feedback.
  - Keep the alias-based bank-numbering approach unchanged.

Link to v1: https://lore.kernel.org/r/20260723-upstream_kcs_multiple_lpc-v1-0-d918b5270b86@aspeedtech.com
---
 drivers/char/ipmi/kcs_bmc_aspeed.c | 71 ++++++++++++++++++++++++++++++++------
 1 file changed, 61 insertions(+), 10 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
index a13a3470c17a..751f477c09e0 100644
--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
@@ -10,8 +10,10 @@
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/irq.h>
+#include <linux/list.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
@@ -125,6 +127,8 @@ struct aspeed_kcs_bmc {
 		bool remove;
 		struct timer_list timer;
 	} obe;
+
+	u32 channel;
 };
 
 static inline struct aspeed_kcs_bmc *to_aspeed_kcs_bmc(struct kcs_bmc_device *kcs_bmc)
@@ -167,7 +171,7 @@ static void aspeed_kcs_outb(struct kcs_bmc_device *kcs_bmc, u32 reg, u8 data)
 	if (priv->upstream_irq.mode != aspeed_kcs_irq_serirq)
 		return;
 
-	switch (kcs_bmc->channel) {
+	switch (priv->channel) {
 	case 1:
 		switch (priv->upstream_irq.id) {
 		case 12:
@@ -232,7 +236,7 @@ static int aspeed_kcs_set_address(struct kcs_bmc_device *kcs_bmc, u32 addrs[2],
 	if (WARN_ON(nr_addrs < 1 || nr_addrs > 2))
 		return -EINVAL;
 
-	switch (priv->kcs_bmc.channel) {
+	switch (priv->channel) {
 	case 1:
 		regmap_update_bits(priv->map, LPC_HICR4, LPC_HICR4_LADR12AS, 0);
 		regmap_write(priv->map, LPC_LADR12H, addrs[0] >> 8);
@@ -315,7 +319,7 @@ static int aspeed_kcs_config_upstream_irq(struct aspeed_kcs_bmc *priv, u32 id, u
 	priv->upstream_irq.mode = aspeed_kcs_irq_serirq;
 	priv->upstream_irq.id = id;
 
-	switch (priv->kcs_bmc.channel) {
+	switch (priv->channel) {
 	case 1:
 		/* Needs IRQxE1 rather than (ID1IRQX, SEL1IRQX, IRQXE1) before AST2600 A3 */
 		break;
@@ -347,7 +351,7 @@ static int aspeed_kcs_config_upstream_irq(struct aspeed_kcs_bmc *priv, u32 id, u
 	default:
 		dev_warn(priv->kcs_bmc.dev,
 			 "SerIRQ configuration not supported on KCS channel %d\n",
-			 priv->kcs_bmc.channel);
+			 priv->channel);
 		return -EINVAL;
 	}
 
@@ -358,7 +362,7 @@ static void aspeed_kcs_enable_channel(struct kcs_bmc_device *kcs_bmc, bool enabl
 {
 	struct aspeed_kcs_bmc *priv = to_aspeed_kcs_bmc(kcs_bmc);
 
-	switch (kcs_bmc->channel) {
+	switch (priv->channel) {
 	case 1:
 		regmap_update_bits(priv->map, LPC_HICR0, LPC_HICR0_LPC1E, enable * LPC_HICR0_LPC1E);
 		return;
@@ -374,7 +378,7 @@ static void aspeed_kcs_enable_channel(struct kcs_bmc_device *kcs_bmc, bool enabl
 		regmap_update_bits(priv->map, LPC_HICRB, LPC_HICRB_LPC4E, enable * LPC_HICRB_LPC4E);
 		return;
 	default:
-		pr_warn("%s: Unsupported channel: %d", __func__, kcs_bmc->channel);
+		pr_warn("%s: Unsupported channel: %d", __func__, priv->channel);
 		return;
 	}
 }
@@ -435,7 +439,7 @@ static void aspeed_kcs_irq_mask_update(struct kcs_bmc_device *kcs_bmc, u8 mask,
 	if (mask & KCS_BMC_EVENT_TYPE_IBF) {
 		const bool enable = !!(state & KCS_BMC_EVENT_TYPE_IBF);
 
-		switch (kcs_bmc->channel) {
+		switch (priv->channel) {
 		case 1:
 			regmap_update_bits(priv->map, LPC_HICR2, LPC_HICR2_IBFIE1,
 					   enable * LPC_HICR2_IBFIE1);
@@ -453,7 +457,7 @@ static void aspeed_kcs_irq_mask_update(struct kcs_bmc_device *kcs_bmc, u8 mask,
 					   enable * LPC_HICRB_IBFIE4);
 			return;
 		default:
-			pr_warn("%s: Unsupported channel: %d", __func__, kcs_bmc->channel);
+			pr_warn("%s: Unsupported channel: %d", __func__, priv->channel);
 			return;
 		}
 	}
@@ -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);
+	if (!bank) {
+		mutex_unlock(&aspeed_kcs_bank_lock);
+		return -ENOMEM;
+	}
+
+	bank->lpc_np = lpc_np;
+	list_add_tail(&bank->entry, &aspeed_kcs_banks);
+
+out:
+	mutex_unlock(&aspeed_kcs_bank_lock);
+	return index;
+}
+
 static int
 aspeed_kcs_of_get_io_address(struct platform_device *pdev, u32 addrs[2])
 {
@@ -559,7 +604,7 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
 	struct device_node *np;
 	bool have_upstream_irq;
 	u32 upstream_irq[2];
-	int rc, channel;
+	int rc, channel, bank;
 	int nr_addrs;
 	u32 addrs[2];
 
@@ -575,6 +620,10 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
 	if (channel < 0)
 		return channel;
 
+	bank = aspeed_kcs_of_get_bank(np);
+	if (bank < 0)
+		return bank;
+
 	nr_addrs = aspeed_kcs_of_get_io_address(pdev, addrs);
 	if (nr_addrs < 0)
 		return nr_addrs;
@@ -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;
 

---
base-commit: f0e6f20cb52b14c2c441f04e21cef0c95d498cac
change-id: 20260722-upstream_kcs_multiple_lpc-735942211508

Best regards,
-- 
Yu-Che Hsieh <yc_hsieh@aspeedtech.com>

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

[sashiko-bot]

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