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

[PATCH RFC 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
(e.g. lpc0@14c31000, lpc1@14c32000, plus pcie_lpc0@12c19000 and 
pcie_lpc1@12c19800 for the PCIe-facing path), each instantiating 
its own KCS1-KCS4 with identical IDR/ODR/STR register offsets:

	lpc0: lpc@14c31000 {
		compatible = "aspeed,ast2700-lpc", "simple-mfd", "syscon";
		lpc0_kcs1: lpc-kcs@24 {
			compatible = "aspeed,ast2600-kcs-bmc";
			...
		};
		...
	};

	lpc1: lpc@14c32000 {
		compatible = "aspeed,ast2700-lpc", "simple-mfd", "syscon";
		lpc1_kcs1: kcs@24 {
			compatible = "aspeed,ast2600-kcs-bmc";
			...
		};
		...
	};

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, 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.

This series keeps a driver-private `channel` (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.

Before settling on how `bank` is derived, I considered a few options
with different tradeoffs and no clearly "correct" answer from existing
driver conventions:

  (a) Dynamic discovery: keep a driver-global list (protected by a
      mutex), keyed by the LPC controller's device_node pointer, and
      assign bank indices in first-seen-during-probe order. No DT
      changes required, but bank numbering depends on probe order,
      which is not deterministic across boots/reprobes.

  (b) Tree walk by compatible: for_each_compatible_node() over the
      whole DT matching the LPC node's own compatible string, counting
      position. Deterministic and requires no DT changes, but AST2700's
      four LPC-compatible instances (lpc0/lpc1/pcie_lpc0/pcie_lpc1)
      don't share a common parent node and don't share a consistent
      child-node naming scheme, which also rules out parent-based or
      node-name-based tree walks I looked at.

  (c) of_alias_get_id(): require board DTs to declare
      /aliases { lpc0 = &lpc0; lpc1 = &lpc1; ... }, and read the index
      back from there. This is the pattern already used by mmc/i2c/spi/
      serial for "which instance is this" numbering, including the same
      "absent alias -> fall back to previous behaviour" compatibility
      guarantee for existing DTBs.

This series implements (c), since it puts the numbering under DT-author
control rather than driver inference, generalizes across however many
LPC-compatible instances a future SoC exposes (regardless of tree
placement), and existing single-LPC-controller boards need no DT
changes at all - of_alias_get_id() returns an error when no alias is
present and the driver falls back to bank 0, which is exactly today's
behaviour.

Patches:

  Patch 1: dt-bindings: mfd: aspeed-lpc: Document lpcN alias for
           multi-instance SoCs
  Patch 2: ipmi: kcs_bmc_aspeed: Support multiple LPC controller
           instances

Before dropping the RFC tag and posting the AST2700 follow-up patches, 
I'd particularly appreciate review feedback on the bank-numbering approach. 
In particular, does option (c) seem like an acceptable use of DT aliases 
for identifying LPC controller instances, or would the community prefer a 
different approach? 

Any guidance would be very helpful.

Signed-off-by: Yu-Che Hsieh <yc_hsieh@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 RFC 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 RFC 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 RFC 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] If `kcs_bmc_add_device()` fails, the `kcs_bmc` device remains on the global `kcs_bmc_devices` list, causing a use-after-free when devres cleans up.
- [High] The driver requests a shared IRQ before initializing the `kcs_bmc->lock` spinlock, which can cause a crash if a shared interrupt fires early.
- [High] The IPMI chardev frees its private data before all file descriptors are closed, leading to a use-after-free on driver unbind.
--

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

ipmi: kcs_bmc_aspeed: Support multiple LPC controller instances

This commit introduces support for multiple physical LPC controllers by
splitting the KCS channel representation into a driver-private channel for
register access and a globally unique channel for the chardev exposed to
userspace. It derives the global channel number using an optional "lpcN"
alias on the LPC controller node.

> 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 a failure in
kcs_bmc_add_device() leave a dangling pointer on the global list?

If any driver's add_device callback fails, kcs_bmc_add_device() returns an
error without unlinking kcs_bmc->entry from the kcs_bmc_devices list.
This error propagates back to aspeed_kcs_probe(), which returns it:

	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;
	}

The devres framework will then free the priv memory containing the kcs_bmc
object, leaving a dangling pointer in the global kcs_bmc_devices list.
Could this lead to a use-after-free during subsequent driver registrations?

[Severity: High]
This is also a pre-existing issue, but is it safe to request a shared IRQ
before the spinlock is initialized?

In aspeed_kcs_probe(), aspeed_kcs_config_downstream_irq() is called before
kcs_bmc_add_device():

	/* Host to BMC IRQ */
	rc = aspeed_kcs_config_downstream_irq(kcs_bmc, pdev);
	if (rc)
		return rc;

aspeed_kcs_config_downstream_irq() uses devm_request_irq() with IRQF_SHARED,
which activates the aspeed_kcs_irq() handler. The handler immediately tries
to acquire kcs_bmc->lock.

However, kcs_bmc->lock isn't initialized until kcs_bmc_add_device() is
called later in the probe sequence. If a shared device on the same IRQ line
fires an interrupt in this window, could the handler dereference an
uninitialized spinlock?

[Severity: High]
This is another pre-existing issue in the IPMI chardev code, but does
kcs_bmc_ipmi_remove_device() free the private data before all file
descriptors are closed?

In drivers/char/ipmi/kcs_bmc_cdev_ipmi.c, kcs_bmc_ipmi_remove_device()
unregisters the miscdev and explicitly frees the priv struct:

	misc_deregister(&priv->miscdev);
	kcs_bmc_disable_device(priv->client.dev, &priv->client);
	devm_kfree(kcs_bmc->dev, priv->kbuffer);
	// ...
	devm_kfree(kcs_bmc->dev, priv);

Since misc_deregister() doesn't close existing file descriptors or wait for
active file operations to complete, could a user process that is sleeping in
kcs_bmc_ipmi_read() access the freed priv->mutex and priv->kbuffer when it
wakes up?

	wait_event_interruptible(priv->queue,
				 priv->data_in_avail);

	mutex_lock(&priv->mutex);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-upstream_kcs_multiple_lpc-v1-0-d918b5270b86@aspeedtech.com?part=2