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

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

[Yu-Che Hsieh]

Hi,

Aspeed AST2700 SoC exposes more than one physical LPC controller
instance. Each instance provides its own KCS1-KCS4 devices, with
identical IDR/ODR/STR register offsets within that controller's register
file.

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
each LPC controller computes to the same channel number, and whichever
instance probes second fails due to the duplicate miscdevice name.

This series keeps a driver-private channel value (1..4) in struct
aspeed_kcs_bmc for register access, and computes a globally unique
kcs_bmc_device::channel as:

	bank * KCS_CHANNEL_MAX + channel

where bank identifies which LPC controller instance a KCS device belongs
to.

bank is derived via of_alias_get_id() from an optional "lpcN" alias on
the LPC controller node, mirroring the existing alias-based numbering
convention used by mmc/i2c/spi/serial. For example, with aliases
assigning lpc0 and lpc1 to banks 0 and 1, their KCS channels are exposed
as /dev/ipmi-kcs1..4 and /dev/ipmi-kcs5..8 respectively. If no lpcN
alias is present, of_alias_get_id() fails and the driver uses bank 0,
preserving the existing numbering for single-LPC-controller systems.

This puts the numbering under DT-author control instead of deriving it
from probe order or DT traversal order. It also generalizes across
however many LPC-compatible instances a future SoC exposes, regardless
of tree placement, while preserving compatibility for existing
single-LPC-controller boards.

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

---
Yu-Che Hsieh (2):
      dt-bindings: mfd: aspeed-lpc: Document lpcN alias for multi-instance SoCs
      ipmi: kcs_bmc_aspeed: Support multiple LPC controller instances

 .../devicetree/bindings/mfd/aspeed-lpc.yaml        |  7 ++++
 drivers/char/ipmi/kcs_bmc_aspeed.c                 | 37 ++++++++++++++++------
 2 files changed, 34 insertions(+), 10 deletions(-)
---
base-commit: f0e6f20cb52b14c2c441f04e21cef0c95d498cac
change-id: 20260722-upstream_kcs_multiple_lpc-735942211508

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

[PATCH v2 1/2] dt-bindings: mfd: aspeed-lpc: Document lpcN alias for multi-instance SoCs

[Yu-Che Hsieh]

Some SoCs (e.g. AST2700) expose more than one physical LPC controller
instance. Downstream devices such as KCS channels need a stable,
globally-unique index derived from which LPC controller instance they
belong to, so document the "lpcN" alias convention on the LPC
controller node that lets DT authors assign a fixed index per
instance, mirroring the existing alias-based numbering convention used
by mmc/i2c/spi/serial.

Signed-off-by: Yu-Che Hsieh <yc_hsieh@aspeedtech.com>
---
 Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml b/Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml
index cbc3a2485a2f..81c12e0fb312 100644
--- a/Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml
+++ b/Documentation/devicetree/bindings/mfd/aspeed-lpc.yaml
@@ -35,6 +35,13 @@ description:
   configuration, therefore the host portion of the controller is exposed as a
   syscon as a means to arbitrate access.
 
+  On SoCs that expose more than one LPC controller instance, it is possible to
+  assign a fixed index to a given instance (and thus to downstream devices
+  such as KCS channels that must be uniquely identified system-wide) by
+  defining an "lpcN" alias for it in the /aliases device tree node. If no such
+  alias is present, the SoC is treated as having a single LPC controller
+  instance.
+
 properties:
   compatible:
     items:

-- 
2.34.1

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

[Yu-Che Hsieh]

Some SoCs (e.g. AST2700) expose more than one physical LPC controller,
each instantiating its own independent set of KCS0-KCS3 channels using
identical IDR/ODR/STR register offsets. kcs_bmc_device::channel is
currently overloaded to serve two distinct purposes: selecting which
HICR0/HICR2/HICRB bit-group to touch within a single LPC controller's
register file, and naming the misc chardev (/dev/ipmi-kcsN) exposed to
userspace, which must be unique system-wide. The two purposes happen
to share the same value only because the driver has so far only had
to support SoCs with a single LPC controller. On a multi-controller
SoC, KCS0 on every LPC controller computes to the same channel number,
and whichever instance probes second fails to register its chardev.

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 an optional "lpcN" alias on the LPC controller
node via of_alias_get_id(), following the same convention already used
by mmc/i2c/spi/serial for instance numbering. SoCs with a single LPC
controller, and existing DTs that don't declare the alias, fall back
to bank 0, so /dev/ipmi-kcsN naming is unchanged for all existing
boards.

Signed-off-by: Yu-Che Hsieh <yc_hsieh@aspeedtech.com>
---
 drivers/char/ipmi/kcs_bmc_aspeed.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
index a13a3470c17a..591c11a44503 100644
--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
@@ -125,6 +125,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 +169,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 +234,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 +317,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 +349,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 +360,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 +376,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 +437,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 +455,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 +528,17 @@ static int aspeed_kcs_of_get_channel(struct platform_device *pdev)
 	return -EINVAL;
 }
 
+static int aspeed_kcs_of_get_bank(struct device_node *lpc_np)
+{
+	int id;
+
+	id = of_alias_get_id(lpc_np, "lpc");
+	if (id < 0)
+		return 0;
+
+	return id;
+}
+
 static int
 aspeed_kcs_of_get_io_address(struct platform_device *pdev, u32 addrs[2])
 {
@@ -559,7 +572,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 +588,8 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
 	if (channel < 0)
 		return channel;
 
+	bank = aspeed_kcs_of_get_bank(np);
+
 	nr_addrs = aspeed_kcs_of_get_io_address(pdev, addrs);
 	if (nr_addrs < 0)
 		return nr_addrs;
@@ -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;
 

-- 
2.34.1

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

[sashiko-bot]

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