[PATCH v2 0/2] soc: aspeed: ipmi: add KCS Channel 4 over PCIe

[PATCH v2 0/2] soc: aspeed: ipmi: add KCS Channel 4 over PCIe

[Grégoire Layet]

The ASPEED AST2600 has a PCIe to LPC controller. It includes a KCS
interface on channel 4. This is a fully KCS-compatible interface
that is exposed over PCIe.

Add pcie_kcs4 to existing LPC bus in aspeed-g6.dtsi.
Modify kcs_bmc_aspeed driver to detect the PCIe over LPC channel.

In the driver, this PCIe over LPC channel will be detected as Channel 5.
In the datasheet this is defined as Channel 4 over PCIe.

The Channel 4 name is still referenced in the offsets macros.

The host need a driver to activate the MSI interrupts with the PCIe BMC
Device.

Tests:
With the patch [2] adding PCIe BMC device for Virtual UART, the MSI
interrupts are correctly setups. The Host driver is build and side
loaded on the host. The ipmi_si driver on the host is loaded manually
with the KCS address given by hand. 'ipmitool mc info' is run to confirm
it's working.

Tested on : Asus IPMI Expansion Card, AST2600.
```
# insmod aspeed-host-bmc-dev.ko
# lspci -vvv -d 1a03:2402
02:01.0 Unassigned class [ff00]: ASPEED Technology, Inc. Device 2402
	Subsystem: ASPEED Technology, Inc. Device 2402
	Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin A routed to IRQ 154
	IOMMU group: 12
	Region 0: Memory at 82080000 (32-bit, non-prefetchable) [size=4K]
	Region 1: Memory at 82040000 (32-bit, non-prefetchable) [size=256K]
	Capabilities: [40] Power Management version 3
		Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0+,D1+,D2+,D3hot+,D3cold+)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [50] MSI: Enable+ Count=32/32 Maskable- 64bit+
		Address: 00000000fee00818  Data: 0000
	Kernel driver in use: aspeed-host-bmc-dev

# modprobe ipmi_si type=kcs addrs=0x82040E88 regspacings=4 regsizes=1 regshifts=0

# ipmitool mc info
Device ID                 : 32
Device Revision           : 1
Firmware Revision         : 26.04
IPMI Version              : 2.0
Manufacturer ID           : 2623
Manufacturer Name         : ASUSTek Computer Inc.
...
```

The original patch [1] review set to use the existing LPC bus and
modify the kcs_bmc_aspeed driver.

[1] https://lore.kernel.org/linux-aspeed/de49f2032f5b1f170161dcd6191c0daa48f0b0c2.1784117715.git.gregoire.layet@9elements.com/
[2] https://lore.kernel.org/linux-aspeed/51e455417bdcfe608270c6cb2806b1fd971fb998.1783524645.git.gregoire.layet@9elements.com/

Grégoire Layet (2):
  ipmi: kcs_bmc_aspeed: g6: Add KCS Channel 4 over PCIe
  ARM: dts: aspeed-g6: add pcie-kcs4

 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi |  7 +++++
 drivers/char/ipmi/kcs_bmc_aspeed.c      | 39 +++++++++++++++++++++++--
 2 files changed, 44 insertions(+), 2 deletions(-)


base-commit: 03f906d8f5541e8bb741035981304feceed5993d
--
2.54.0

[PATCH v2 1/2] ipmi: kcs_bmc_aspeed: g6: Add KCS Channel 4 over PCIe

[Grégoire Layet]

Created a 5th KCS channel to match against the KCS 4 over PCIe addresses.

The ASPEED AST2600 has a PCIe to LPC controller. It includes a KCS
interface on channel 4. This is a fully KCS-compatible interface
that is exposed over PCIe.

The 5th channel created is only valid on the AST2600.
This cannot be used for AST2400 and AST2500 chips, as they don't have LPC
over PCIE.

Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 drivers/char/ipmi/kcs_bmc_aspeed.c | 39 ++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c
index a13a3470c17a..4d93af2455b8 100644
--- a/drivers/char/ipmi/kcs_bmc_aspeed.c
+++ b/drivers/char/ipmi/kcs_bmc_aspeed.c
@@ -26,7 +26,7 @@
 
 #define DEVICE_NAME     "ast-kcs-bmc"
 
-#define KCS_CHANNEL_MAX     4
+#define KCS_CHANNEL_MAX     5
 
 /*
  * Field class descriptions
@@ -102,6 +102,12 @@
 #define     LPC_LSADR12_LSADR2_SHIFT 16
 #define     LPC_LSADR12_LSADR1_MASK  GENMASK(15, 0)
 #define     LPC_LSADR12_LSADR1_SHIFT 0
+#define PCIE_LPC_HICRB       0x900
+#define PCIE_LPC_HICRC       0x904
+#define PCIE_LPC_LADR4       0x910
+#define PCIE_LPC_IDR4        0x914
+#define PCIE_LPC_ODR4        0x918
+#define PCIE_LPC_STR4        0x91C
 
 #define OBE_POLL_PERIOD	     (HZ / 2)
 
@@ -191,6 +197,9 @@ static void aspeed_kcs_outb(struct kcs_bmc_device *kcs_bmc, u32 reg, u8 data)
 	case 4:
 		regmap_update_bits(priv->map, LPC_HICRC, LPC_HICRC_IRQXE4, LPC_HICRC_IRQXE4);
 		break;
+	case 5:
+		regmap_update_bits(priv->map, PCIE_LPC_HICRC, LPC_HICRC_IRQXE4, LPC_HICRC_IRQXE4);
+		break;
 	default:
 		break;
 	}
@@ -278,6 +287,14 @@ static int aspeed_kcs_set_address(struct kcs_bmc_device *kcs_bmc, u32 addrs[2],
 
 		break;
 
+	case 5:
+		if (nr_addrs == 1)
+			regmap_write(priv->map, PCIE_LPC_LADR4, ((addrs[0] + 1) << 16) | addrs[0]);
+		else
+			regmap_write(priv->map, PCIE_LPC_LADR4, (addrs[1] << 16) | addrs[0]);
+
+		break;
+
 	default:
 		return -EINVAL;
 	}
@@ -344,6 +361,11 @@ static int aspeed_kcs_config_upstream_irq(struct aspeed_kcs_bmc *priv, u32 id, u
 		val = (id << LPC_HICRC_ID4IRQX_SHIFT) | (hw_type << LPC_HICRC_TY4IRQX_SHIFT);
 		regmap_update_bits(priv->map, LPC_HICRC, mask, val);
 		break;
+	case 5:
+		mask = LPC_HICRC_ID4IRQX_MASK | LPC_HICRC_TY4IRQX_MASK | LPC_HICRC_OBF4_AUTO_CLR;
+		val = (id << LPC_HICRC_ID4IRQX_SHIFT) | (hw_type << LPC_HICRC_TY4IRQX_SHIFT);
+		regmap_update_bits(priv->map, PCIE_LPC_HICRC, mask, val);
+		break;
 	default:
 		dev_warn(priv->kcs_bmc.dev,
 			 "SerIRQ configuration not supported on KCS channel %d\n",
@@ -373,6 +395,9 @@ static void aspeed_kcs_enable_channel(struct kcs_bmc_device *kcs_bmc, bool enabl
 	case 4:
 		regmap_update_bits(priv->map, LPC_HICRB, LPC_HICRB_LPC4E, enable * LPC_HICRB_LPC4E);
 		return;
+	case 5:
+		regmap_update_bits(priv->map, PCIE_LPC_HICRB, LPC_HICRB_LPC4E, enable * LPC_HICRB_LPC4E);
+		return;
 	default:
 		pr_warn("%s: Unsupported channel: %d", __func__, kcs_bmc->channel);
 		return;
@@ -452,6 +477,10 @@ static void aspeed_kcs_irq_mask_update(struct kcs_bmc_device *kcs_bmc, u8 mask,
 			regmap_update_bits(priv->map, LPC_HICRB, LPC_HICRB_IBFIE4,
 					   enable * LPC_HICRB_IBFIE4);
 			return;
+		case 5:
+			regmap_update_bits(priv->map, PCIE_LPC_HICRB, LPC_HICRB_IBFIE4,
+					   enable * LPC_HICRB_IBFIE4);
+			return;
 		default:
 			pr_warn("%s: Unsupported channel: %d", __func__, kcs_bmc->channel);
 			return;
@@ -492,6 +521,7 @@ static const struct kcs_ioreg ast_kcs_bmc_ioregs[KCS_CHANNEL_MAX] = {
 	{ .idr = LPC_IDR2, .odr = LPC_ODR2, .str = LPC_STR2 },
 	{ .idr = LPC_IDR3, .odr = LPC_ODR3, .str = LPC_STR3 },
 	{ .idr = LPC_IDR4, .odr = LPC_ODR4, .str = LPC_STR4 },
+	{ .idr = PCIE_LPC_IDR4, .odr = PCIE_LPC_ODR4, .str = PCIE_LPC_STR4 },
 };
 
 static int aspeed_kcs_of_get_channel(struct platform_device *pdev)
@@ -575,11 +605,16 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
 	if (channel < 0)
 		return channel;
 
+	np = pdev->dev.of_node;
+	if (channel == 5 && !of_device_is_compatible(np, "aspeed,ast2600-kcs-bmc")) {
+		dev_err(&pdev->dev, "LPC over PCIE KCS4 only supported on ast2600\n");
+		return -ENODEV;
+	}
+
 	nr_addrs = aspeed_kcs_of_get_io_address(pdev, addrs);
 	if (nr_addrs < 0)
 		return nr_addrs;
 
-	np = pdev->dev.of_node;
 	rc = of_property_read_u32_array(np, "aspeed,lpc-interrupts", upstream_irq, 2);
 	if (rc && rc != -EINVAL)
 		return -EINVAL;
-- 
2.54.0

[PATCH v2 2/2] ARM: dts: aspeed-g6: add pcie-kcs4

[Grégoire Layet]

Add pcie_kcs4 node to the LPC bus in the ast2600 g6 common dtsi.

The ASPEED AST2600 has a PCIe to LPC controller. It includes a KCS
interface on channel 4. This is a fully KCS-compatible interface
that is exposed over PCIe.

This can be used by the host for IPMI when the PCIe BMC Device is
activated.

While the datasheet provides interrupt numbers for KCS channels 1, 2,
3 and 4 over PCI, not all 4 are described in the "PCIe to LPC Controller"
section. Since only the KCS channel 4 is described, only this channel is
added.

The pcie_kcs4 uses existing "aspeed,ast2600-kcs-bmc" compatible as it's a
standard KCS interface.

Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
index 56bb3b0444f7..a93fa4cf5d8f 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
@@ -617,6 +617,13 @@ kcs4: kcs@114 {
 					status = "disabled";
 				};
 
+				pcie_kcs4: kcs@914 {
+					compatible = "aspeed,ast2600-kcs-bmc";
+					reg = <0x914 0x1>, <0x918 0x1>, <0x91c 0x1>;
+					interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>;
+					status = "disabled";
+				};
+
 				lpc_ctrl: lpc-ctrl@80 {
 					compatible = "aspeed,ast2600-lpc-ctrl";
 					reg = <0x80 0x80>;
-- 
2.54.0

Re: [PATCH v2 1/2] ipmi: kcs_bmc_aspeed: g6: Add KCS Channel 4 over PCIe

[sashiko-bot]

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

New issues:
- [High] Missing PCIE_LPC_ODR4 in switch statement prevents upstream IRQ triggering for channel 5.

Pre-existing issues:
- [Critical] Returning an error from probe frees the kcs_bmc object while it remains linked in the global kcs_bmc_devices list.
- [High] The KCS hardware channel is enabled and the IRQ is registered before the interrupt handler's spinlock is initialized.
- [High] Hardware channel is left enabled on the probe error path if kcs_bmc_add_device fails.
--

--- Patch [1]: [PATCH v2 1/2] ipmi: kcs_bmc_aspeed: g6: Add KCS Channel 4 over PCIe ---
commit 41d8f781b8898d768896e3f02a1b1877d0c1b5ff
Author: Grégoire Layet <gregoire.layet@9elements.com>

ipmi: kcs_bmc_aspeed: g6: Add KCS Channel 4 over PCIe
    
This patch creates a 5th KCS channel to match against the KCS 4 over PCIe
addresses. It exposes a KCS-compatible interface over PCIe for the
ASPEED AST2600.

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

[ ... ]

> @@ -191,6 +197,9 @@ static void aspeed_kcs_outb(struct kcs_bmc_device *kcs_bmc, u32 reg, u8 data)
>  	case 4:
>  		regmap_update_bits(priv->map, LPC_HICRC, LPC_HICRC_IRQXE4, LPC_HICRC_IRQXE4);
>  		break;
> +	case 5:
> +		regmap_update_bits(priv->map, PCIE_LPC_HICRC, LPC_HICRC_IRQXE4, LPC_HICRC_IRQXE4);
> +		break;

[Severity: High]
Will this upstream IRQ triggering for channel 5 ever be reached?

Looking earlier in aspeed_kcs_outb(), there is a switch statement
validating the ODR registers:

aspeed_kcs_outb() {
    ...
    switch (reg) {
    case LPC_ODR1:
    case LPC_ODR2:
    case LPC_ODR3:
    case LPC_ODR4:
        break;
    default:
        return;
    }
    ...
}

Since PCIE_LPC_ODR4 is missing from this switch statement, won't any write
to the channel 5 ODR register return early and prevent this new code from
executing?

>  	default:
>  		break;
>  	}

[ ... ]

> @@ -575,11 +605,16 @@ static int aspeed_kcs_probe(struct platform_device *pdev)
>  	if (channel < 0)
>  		return channel;
>  
> +	np = pdev->dev.of_node;
> +	if (channel == 5 && !of_device_is_compatible(np, "aspeed,ast2600-kcs-bmc")) {
> +		dev_err(&pdev->dev, "LPC over PCIE KCS4 only supported on ast2600\n");
> +		return -ENODEV;
> +	}
> +

[Severity: Critical]
This isn't a bug introduced by this patch, but does returning an error from
probe leave a freed object linked in the global list?

If kcs_bmc_add_device() fails later in aspeed_kcs_probe() after linking the
device:

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

Since kcs_bmc_add_device() leaves the kcs_bmc object in the global
kcs_bmc_devices list on certain errors, and devres will free the priv
structure containing kcs_bmc when we return, could this cause a
use-after-free during subsequent list iterations?

[Severity: High]
This is also a pre-existing issue, but is it safe to enable the hardware
channel before the interrupt handler's spinlock is initialized?

In aspeed_kcs_probe(), the device interrupts are enabled and can fire
immediately:

aspeed_kcs_probe() {
    ...
    aspeed_kcs_irq_mask_update(...);
    aspeed_kcs_enable_channel(kcs_bmc, true);

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

However, kcs_bmc->lock is only initialized later inside
kcs_bmc_add_device(). If an interrupt occurs in this window, will the
handler attempt to acquire an uninitialized lock?

[Severity: High]
This is another pre-existing issue, but is the hardware channel left enabled
if the probe fails here?

In the same error path for kcs_bmc_add_device() shown above, the driver
returns without disabling the channel.

Since the IRQ is shared, could unhandled interrupts from this active device
eventually cause the core IRQ subsystem to disable the shared IRQ line?

>  	nr_addrs = aspeed_kcs_of_get_io_address(pdev, addrs);
>  	if (nr_addrs < 0)
>  		return nr_addrs;
>  
> -	np = pdev->dev.of_node;
>  	rc = of_property_read_u32_array(np, "aspeed,lpc-interrupts", upstream_irq, 2);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784732161.git.gregoire.layet@9elements.com?part=1

Re: [PATCH v2 2/2] ARM: dts: aspeed-g6: add pcie-kcs4

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Upstream IRQs will not be triggered for the new pcie_kcs4 channel because PCIE_LPC_ODR4 is omitted from the ODR check in aspeed_kcs_outb().
--

--- Patch [2]: [PATCH v2 2/2] ARM: dts: aspeed-g6: add pcie-kcs4 ---
commit ad67c996a0b89123187ac23dc1115c5dc8b37472
Author: Grégoire Layet <gregoire.layet@9elements.com>

ARM: dts: aspeed-g6: add pcie-kcs4

This commit adds the pcie_kcs4 node to the LPC bus in the AST2600 G6 common
device tree. It enables a KCS-compatible interface exposed over PCIe for
host IPMI communication.

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> index 56bb3b0444f7e..a93fa4cf5d8ff 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> +++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> @@ -617,6 +617,13 @@ kcs4: kcs@114 {
>  					status = "disabled";
>  				};
>  
> +				pcie_kcs4: kcs@914 {
> +					compatible = "aspeed,ast2600-kcs-bmc";
> +					reg = <0x914 0x1>, <0x918 0x1>, <0x91c 0x1>;
> +					interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>;
> +					status = "disabled";
> +				};

[Severity: High]
Will the upstream IRQs be triggered for this new pcie_kcs4 channel?

Looking at aspeed_kcs_outb() in drivers/char/ipmi/kcs_bmc_aspeed.c, it
verifies the write targets an ODR register using a switch statement, but
PCIE_LPC_ODR4 is omitted:

drivers/char/ipmi/kcs_bmc_aspeed.c:aspeed_kcs_outb() {
        ...
        switch (reg) {
        case LPC_ODR1:
        case LPC_ODR2:
        case LPC_ODR3:
        case LPC_ODR4:
                break;
        default:
                return;
        }
        ...
}

Since PCIE_LPC_ODR4 falls into the default case, it returns early and skips
the subsequent IRQ generation logic entirely when the BMC responds to an
IPMI request over this new channel. 

Does the driver need to be updated to handle PCIE_LPC_ODR4 before this
channel can function correctly?

> +
>  				lpc_ctrl: lpc-ctrl@80 {
>  					compatible = "aspeed,ast2600-lpc-ctrl";
>  					reg = <0x80 0x80>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784732161.git.gregoire.layet@9elements.com?part=2