[PATCH v6 0/6] soc: aspeed: Add BMC and host driver for PCIe BMC device

[PATCH v6 0/6] soc: aspeed: Add BMC and host driver for PCIe BMC device

[Grégoire Layet]

This is the v6 for upstreaming the VUART over PCIe BMC device drivers.

The initial drivers were taken from the ASPEED kernel SDK
(master-v6.18) [1], and have been adapted greatly.

There are two drivers: a BMC-side driver and a host-side driver.
Together they enable host<->BMC VUART communication via PCIe.

The virtual UART is an 8250-compatible register set. It is called
'virtual' because it is internally a simple FIFO.
On the AST2600, two VUARTs can be exposed over PCI via the PCI BMC device.

The host cannot access the BMC's memory. Only the exposed features are
accessible. These are the KCS4 channel and 2 VUARTs. There is also some
mailbox register functionality for a communication between
the host and the BMC. More information can be found here [2].

Changes since v5 [3]:
- Fix small formatting issue in Patch 4's commit message.
- Use static const for MFD cell and uart_port in the mfd driver.

The host driver will performs multiple functions. The AST2600 also
supports LPC over PCI, with a specific KCS channel (KCS4). This host core
driver should also be used to enable the IPMI automatically via this KCS
channel.

Tested:
As with v5, VUART data flow and MSI interrupts have been verified
as working on the test hardware, in both directions and on both VUARTs.
BMC: Asus IPMI Kommando Card R1.01, AST2600 A3 running OpenBMC
Host: Linux kernel v7.0.0

This series only supports the AST2600; the AST2700 is not supported.

The findings regarding the MSI interrupt numbers are explained in the v4
cover letter [4].

As with v3 and v4, I would like to know whether I should add the
'lpc-io-reg' and 'lpc-interrupt' values to the vuart3 and vuart4 nodes
directly in the 'aspeed-g6.dtsi'. The host driver is not capable of
finding the vuart address on its own, so they are hardcoded to 0x3f8 and
0x2f8. It will not work with other addresses, so perhaps they should be
in the .dtsi to ensure the correct configuration for the two vuart over
PCI.

[1]: https://github.com/AspeedTech-BMC/linux/tree/aspeed-master-v6.18/drivers/soc/aspeed
[2]: https://lore.kernel.org/linux-aspeed/CAFi2wKYOAotiezepDqaR5PZDqDaPKKDfAEnpx5EHC0mL39hy6w@mail.gmail.com/
[3]: https://lore.kernel.org/linux-aspeed/cover.1785932211.git.gregoire.layet@9elements.com/
[4]: https://lore.kernel.org/linux-aspeed/cover.1783524645.git.gregoire.layet@9elements.com/

Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
---
Grégoire Layet (6):
      dt-bindings: serial: 8250: aspeed: add ast2600 and aspeed,vuart-over-pci
      serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string
      serial: 8250_aspeed_vuart: add VUART over PCI
      mfd: aspeed-pci-core: add AST2600 PCIe BMC device driver
      ARM: dts: aspeed: g6: Change vuart compatible string for ast2600
      ARM: dts: aspeed: g6: add aspeed,vuart-over-pci prop to vuart3 and 4

 Documentation/devicetree/bindings/serial/8250.yaml |  42 +++++++-
 MAINTAINERS                                        |   6 ++
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi            |  14 ++-
 drivers/mfd/Kconfig                                |  12 +++
 drivers/mfd/Makefile                               |   2 +
 drivers/mfd/aspeed-pci-core.c                      | 115 +++++++++++++++++++++
 drivers/tty/serial/8250/8250_aspeed_vuart.c        |  92 +++++++++++++++++
 7 files changed, 277 insertions(+), 6 deletions(-)
---
base-commit: 6fdec78001b3b94f1b6000bbb2f9ec3498f1c73e
change-id: 20260818-aspeed-driver-v6-db9953ca4141

Best regards,
--  
Grégoire Layet <gregoire.layet@9elements.com>

[PATCH v6 1/6] dt-bindings: serial: 8250: aspeed: add ast2600 and aspeed,vuart-over-pci

[Grégoire Layet]

The ASPEED AST2600 can be used as a PCI device.
The ASPEED AST2600 has 2 Virtual UARTs accessible over PCI.
2 Virtual UART can be exposed to the host via this PCI device.
These are 8250-compatible register sets and can be used to have UART
communication between the PCI BMC and the host.

This boolean can be set to specify if a VUART is used over PCI. A VUART
over PCI needs access to the SCU syscon to set the enable bits of the
PCI device.
On the aspeed's chips, the clocks phandle already points at the SCU node.
But deriving the regmap from the clock provider would misuse the clocks
binding. An explicit scu-syscon phandle states the dependency directly.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 Documentation/devicetree/bindings/serial/8250.yaml | 42 ++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
index bb7b9c87a807..8dd4b7af0c0d 100644
--- a/Documentation/devicetree/bindings/serial/8250.yaml
+++ b/Documentation/devicetree/bindings/serial/8250.yaml
@@ -18,12 +18,34 @@ allOf:
             - aspeed,lpc-io-reg
         - required:
             - aspeed,lpc-interrupts
+    then:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - aspeed,ast2500-vuart
+              - aspeed,ast2600-vuart
+  - if:
+      anyOf:
         - required:
             - aspeed,sirq-polarity-sense
     then:
       properties:
         compatible:
           const: aspeed,ast2500-vuart
+  - if:
+      required:
+        - aspeed,vuart-over-pci
+    then:
+      required:
+        - aspeed,scu-syscon
+      properties:
+        compatible:
+          contains:
+            const: aspeed,ast2600-vuart
+    else:
+      properties:
+        aspeed,scu-syscon: false
   - if:
       properties:
         compatible:
@@ -106,6 +128,9 @@ properties:
       - const: ns16850
       - const: aspeed,ast2400-vuart
       - const: aspeed,ast2500-vuart
+      - items:
+          - const: aspeed,ast2600-vuart
+          - const: aspeed,ast2500-vuart
       - const: intel,xscale-uart
       - const: mrvl,pxa-uart
       - const: nuvoton,wpcm450-uart
@@ -297,7 +322,8 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32-array
     maxItems: 1
     description: |
-      The VUART LPC address.  Only applicable to aspeed,ast2500-vuart.
+      The VUART LPC address.  Only applicable to aspeed,ast2500-vuart and
+      aspeed,ast2600-vuart.
 
   aspeed,lpc-interrupts:
     $ref: /schemas/types.yaml#/definitions/uint32-array
@@ -306,7 +332,19 @@ properties:
     description: |
       A 2-cell property describing the VUART SIRQ number and SIRQ
       polarity (IRQ_TYPE_LEVEL_LOW or IRQ_TYPE_LEVEL_HIGH).  Only
-      applicable to aspeed,ast2500-vuart.
+      applicable to aspeed,ast2500-vuart and aspeed,ast2600-vuart.
+
+  aspeed,vuart-over-pci:
+    type: boolean
+    description:
+      Enable the VUART over the BMC PCI device. Only applicable to
+      aspeed,ast2600-vuart.
+
+  aspeed,scu-syscon:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the SCU syscon, used to enable the VUART over the BMC PCI
+      device. Only applicable to aspeed,ast2600-vuart.
 
 required:
   - reg

-- 
2.54.0

Re: [PATCH v6 1/6] dt-bindings: serial: 8250: aspeed: add ast2600 and aspeed,vuart-over-pci

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The schema incorrectly enforces `aspeed,ast2500-vuart` as a mandatory fallback for PCI-routed `aspeed,ast2600-vuart` devices, breaking backward compatibility.
- [Low] The schema lacks a mutual exclusivity constraint to forbid LPC-specific properties when the VUART is explicitly configured for PCI.
--

--- Patch [1]: [PATCH v6 1/6] dt-bindings: serial: 8250: aspeed: add ast2600 and aspeed,vuart-over-pci ---
commit f87e924205ddfd55af4d5f7799223f4c9c2ffb59
Author: Grégoire Layet <gregoire.layet@9elements.com>

dt-bindings: serial: 8250: aspeed: add ast2600 and aspeed,vuart-over-pci

The ASPEED AST2600 can be used as a PCI device with 2 Virtual UARTs
accessible over PCI. This commit adds bindings for ast2600-vuart and
a boolean property for using VUART over PCI.

> diff --git a/Documentation/devicetree/bindings/serial/8250.yaml b/Documentation/devicetree/bindings/serial/8250.yaml
> index bb7b9c87a8072..8dd4b7af0c0d2 100644
> --- a/Documentation/devicetree/bindings/serial/8250.yaml
> +++ b/Documentation/devicetree/bindings/serial/8250.yaml

[ ... ]

>    - if:
>        anyOf:
>          - required:
>              - aspeed,sirq-polarity-sense
>      then:
>        properties:
>          compatible:
>            const: aspeed,ast2500-vuart
> +  - if:
> +      required:
> +        - aspeed,vuart-over-pci
> +    then:
> +      required:
> +        - aspeed,scu-syscon

[Severity: Low]
Does this allow logically contradictory device trees to pass schema
validation?

The VUART must be routed either over PCI or LPC, as UART FIFOs cannot
be safely shared between two concurrent host interfaces.

Should mutual exclusivity constraints be added here to reject
aspeed,lpc-io-reg and aspeed,lpc-interrupts when PCI mode is active?

> +      properties:
> +        compatible:
> +          contains:
> +            const: aspeed,ast2600-vuart
> +    else:
> +      properties:
> +        aspeed,scu-syscon: false

[ ... ]

> @@ -106,6 +128,9 @@ properties:
>        - const: ns16850
>        - const: aspeed,ast2400-vuart
>        - const: aspeed,ast2500-vuart
> +      - items:
> +          - const: aspeed,ast2600-vuart
> +          - const: aspeed,ast2500-vuart

[Severity: High]
Could this result in a regression if an older kernel boots and matches
the fallback?

An older kernel's aspeed_vuart_probe() driver does not recognize
aspeed,vuart-over-pci. It will fall back to its default LPC settings,
programming the VUART to route to the LPC bus at I/O port 0x3f8.

This fails to enable the intended PCI communication and actively drives
the LPC bus, potentially causing hardware conflicts or host crashes if
another device already occupies 0x3f8 on the LPC bus.

Should the fallback be omitted when aspeed,vuart-over-pci is set?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-aspeed-driver-v6-v6-0-fd7cc0b338ea@9elements.com?part=1

[PATCH v6 2/6] serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string

[Grégoire Layet]

Make the driver compatible with the ast2600-vuart.
Enable specific configuration for the AST2600.

Reviewed-by: Tan Siewert <tan.siewert@9elements.com>
Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 26fc0464f1cc..6afa2f4057e1 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -560,6 +560,7 @@ static void aspeed_vuart_remove(struct platform_device *pdev)
 static const struct of_device_id aspeed_vuart_table[] = {
 	{ .compatible = "aspeed,ast2400-vuart" },
 	{ .compatible = "aspeed,ast2500-vuart" },
+	{ .compatible = "aspeed,ast2600-vuart" },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, aspeed_vuart_table);

-- 
2.54.0

Re: [PATCH v6 2/6] serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string

[sashiko-bot]

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

Pre-existing issues:
- [High] Probe error paths after 8250 port registration fail to unregister the port, leading to Use-After-Free.
- [High] Sysfs attributes are exposed before driver data is set, leading to a NULL pointer dereference if accessed concurrently.
--

--- Patch [2]: [PATCH v6 2/6] serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string ---
commit fe8c86639134718c0c590213718a2ba091aac548
Author: Grégoire Layet <gregoire.layet@9elements.com>

serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string

Make the driver compatible with the ast2600-vuart.
Enable specific configuration for the AST2600.

> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> @@ -560,6 +560,7 @@ static void aspeed_vuart_remove(struct platform_device *pdev)
>  static const struct of_device_id aspeed_vuart_table[] = {
>  	{ .compatible = "aspeed,ast2400-vuart" },
>  	{ .compatible = "aspeed,ast2500-vuart" },
> +	{ .compatible = "aspeed,ast2600-vuart" },

[Severity: High]
This isn't a bug introduced by this patch, but since this enables the driver
for new hardware, is it possible for the probe error paths to cause a
use-after-free?

Looking at aspeed_vuart_probe() in drivers/tty/serial/8250/8250_aspeed_vuart.c,
the 8250 port is registered early:

aspeed_vuart_probe() {
    ...
    rc = serial8250_register_8250_port(&port);
    if (rc < 0)
        goto err_sysfs_remove;
    ...

If a subsequent step fails, such as the lpc-io-reg property check:

    rc = aspeed_vuart_set_lpc_address(vuart, prop);
    if (rc < 0) {
        dev_err_probe(dev, rc, "invalid value in aspeed,lpc-io-reg property\n");
        goto err_sysfs_remove;
    }

The err_sysfs_remove label returns without calling
serial8250_unregister_port(). Devres will then automatically free the vuart
struct, but the 8250 port remains registered and pointing to freed memory.

If userspace later accesses the port, such as by calling
aspeed_vuart_startup(), would this dereference the freed vuart pointer?


[Severity: High]
This is also a pre-existing issue, but does creating the sysfs group this
early expose a race condition during probe?

In aspeed_vuart_probe(), sysfs attributes are exposed before the driver
data is set:

aspeed_vuart_probe() {
    ...
    rc = sysfs_create_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
    if (rc < 0)
        return rc;
    ...
    platform_set_drvdata(pdev, vuart);
    return 0;
}

If a userspace process reads a sysfs attribute concurrently before probe
completes, sirq_show() will retrieve a NULL pointer from dev_get_drvdata():

sirq_show() {
    struct aspeed_vuart *vuart = dev_get_drvdata(dev);
    u8 reg;

    reg = aspeed_vuart_readb(vuart, ASPEED_VUART_GCRB);
    ...
}

Will this cause a NULL pointer dereference when aspeed_vuart_readb()
attempts to access vuart->port?

>  	{ },
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-aspeed-driver-v6-v6-0-fd7cc0b338ea@9elements.com?part=2

[PATCH v6 3/6] serial: 8250_aspeed_vuart: add VUART over PCI

[Grégoire Layet]

Enable the VUART over PCI for the AST2600. Activate it only if the
'aspeed,vuart-over-pci' property flag is set on an
'ast2600-vuart' compatible node.

The AST2600 has 2 VUART that are usable over PCI. These are already
defined as the VUART3 and VUART4 in the 'aspeed-g6.dtsi'.

Sets the BMC PCI device enable bits, sets the PCI class code to
unassgined/device specific, and configures MSI interrupts.

There is no disable function. Removing this driver should not disable
the BMC PCI device, as other drivers could use it.
However, if all the drivers using it are removed, the
BMC PCI device will still be activated, which is not ideal. But in reality,
this is not a use case for a BMC, the drivers will never be removed.

This is useful on PCIe BMC expansion cards that use the AST2600,
such as the ASUS Kommando IPMI Expansion Card.

Register initialisation taken from ASPEED 6.18 Kernel SDK.
Add return code checks to each register write.
Simplify the code and add macros.

The ASPEED_SCUC24 regmap update is use an undefined reserved register
ASPEED_SCUC24_RESERVED_14. I was unable to determine the purpose of this
bit. In the AST2600 A3 datasheet it is marked as 'reserved'. It is only
used on the other revision. As I only have the AST2600A3, I was unable
to try this code path. This BIT14 was set in the ASPEED SDK so I kept it.
I can remove it and the untested path if necessary.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
Signed-off-by: aspeedyh <yh_chung@aspeedtech.com>
Reviewed-by: Tan Siewert <tan.siewert@9elements.com>
Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 91 +++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 6afa2f4057e1..2360d37139bf 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -32,6 +32,28 @@
 #define ASPEED_VUART_DEFAULT_SIRQ	4
 #define ASPEED_VUART_DEFAULT_SIRQ_POLARITY	IRQ_TYPE_LEVEL_LOW
 
+#define ASPEED_SCU_SILICON_REVISION_ID			0x04
+#define AST2600A3_REVISION_ID				0x05030303
+
+#define ASPEED_SCU_PCIE_CONF_CTRL 0xC20
+#define  SCU_PCIE_CONF_BMC_DEV_EN					BIT(8)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_MMIO				BIT(9)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_MSI				BIT(11)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_IRQ				BIT(13)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_PCIE_BUS_MASTER	BIT(14)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_E2L				BIT(15)
+#define  SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE		BIT(21)
+
+#define ASPEED_SCUC24 0xC24
+#define  ASPEED_SCUC24_MSI_ROUTING_MASK				GENMASK(11, 10)
+#define  ASPEED_SCUC24_MSI_ROUTING_PCIE2LPC_PCIDEV1 \
+		FIELD_PREP(ASPEED_SCUC24_MSI_ROUTING_MASK, 2)
+#define  ASPEED_SCUC24_RESERVED_14					BIT(14)
+#define  ASPEED_SCUC24_PCIDEV1_INTX_MSI_SCU560_EN	BIT(17)
+#define  ASPEED_SCUC24_PCIDEV1_INTX_MSI_HOST2BMC_EN	BIT(18)
+
+#define ASPEED_SCU_BMC_DEV_CLASS	0xC68
+
 struct aspeed_vuart {
 	struct device		*dev;
 	int			line;
@@ -412,6 +434,66 @@ static int aspeed_vuart_map_irq_polarity(u32 dt)
 	}
 }
 
+static int aspeed_ast2600_vuart_over_pci_set_enabled(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	u32 silicon_revision_id;
+	struct regmap *scu;
+	int rc;
+
+	u32 pcie_config_ctl = SCU_PCIE_CONF_BMC_DEV_EN_IRQ |
+				SCU_PCIE_CONF_BMC_DEV_EN_MMIO |
+				SCU_PCIE_CONF_BMC_DEV_EN_MSI |
+				SCU_PCIE_CONF_BMC_DEV_EN_PCIE_BUS_MASTER |
+				SCU_PCIE_CONF_BMC_DEV_EN_E2L |
+				SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE |
+				SCU_PCIE_CONF_BMC_DEV_EN;
+
+	scu = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,scu-syscon");
+	if (IS_ERR(scu)) {
+		dev_err(dev, "failed to find SCU regmap\n");
+		return PTR_ERR(scu);
+	}
+
+	/* update class code to be an Unassigned/device specific class device */
+	if (regmap_write(scu, ASPEED_SCU_BMC_DEV_CLASS, 0xff000000)) {
+		dev_err(dev, "could not set PCIe class code\n");
+		return -EIO;
+	}
+
+	if (regmap_update_bits(scu, ASPEED_SCU_PCIE_CONF_CTRL,
+			       pcie_config_ctl, pcie_config_ctl)) {
+		dev_err(dev, "could not set PCIe configuration\n");
+		return -EIO;
+	}
+
+	if (regmap_read(scu, ASPEED_SCU_SILICON_REVISION_ID, &silicon_revision_id)) {
+		dev_err(dev, "could not read silicon revision\n");
+		return -EIO;
+	}
+
+	if (silicon_revision_id == AST2600A3_REVISION_ID)
+		rc = regmap_update_bits(scu, ASPEED_SCUC24,
+					ASPEED_SCUC24_PCIDEV1_INTX_MSI_HOST2BMC_EN |
+					ASPEED_SCUC24_MSI_ROUTING_MASK,
+					ASPEED_SCUC24_PCIDEV1_INTX_MSI_HOST2BMC_EN |
+					ASPEED_SCUC24_MSI_ROUTING_PCIE2LPC_PCIDEV1);
+	else
+		rc = regmap_update_bits(scu, ASPEED_SCUC24,
+					ASPEED_SCUC24_PCIDEV1_INTX_MSI_SCU560_EN |
+					ASPEED_SCUC24_RESERVED_14 |
+					ASPEED_SCUC24_MSI_ROUTING_MASK,
+					ASPEED_SCUC24_PCIDEV1_INTX_MSI_SCU560_EN |
+					ASPEED_SCUC24_RESERVED_14 |
+					ASPEED_SCUC24_MSI_ROUTING_PCIE2LPC_PCIDEV1);
+	if (rc) {
+		dev_err(dev, "could not set PCI device 1 MSI interrupt routing\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
 static int aspeed_vuart_probe(struct platform_device *pdev)
 {
 	struct of_phandle_args sirq_polarity_sense_args;
@@ -540,6 +622,15 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
 	aspeed_vuart_set_host_tx_discard(vuart, true);
 	platform_set_drvdata(pdev, vuart);
 
+	if (of_device_is_compatible(dev->of_node, "aspeed,ast2600-vuart") &&
+	    of_property_read_bool(dev->of_node, "aspeed,vuart-over-pci")) {
+		rc = aspeed_ast2600_vuart_over_pci_set_enabled(pdev);
+		if (rc < 0) {
+			dev_err_probe(dev, rc, "could not enable VUART over PCI\n");
+			goto err_sysfs_remove;
+		}
+	}
+
 	return 0;
 
 err_sysfs_remove:

-- 
2.54.0

Re: [PATCH v6 3/6] serial: 8250_aspeed_vuart: add VUART over PCI

[sashiko-bot]

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

New issues:
- [Low] Implicit dependency on <linux/bitfield.h> for the FIELD_PREP macro.
- [Low] Unconditional dev_err() logging in the PCI setup helper will cause log spam if the SCU syscon defers probe.
- [Low] The return values of regmap operations are ignored and incorrectly shadowed by a hardcoded -EIO.

Pre-existing issues:
- [Critical] Probe error paths after successful 8250 port registration fail to unregister the serial port, causing a severe use-after-free memory corruption and resource leak.
--

--- Patch [3]: [PATCH v6 3/6] serial: 8250_aspeed_vuart: add VUART over PCI ---
commit 873385535bf5e6c8576b6766719e73dd291a9a25
Author: Grégoire Layet <gregoire.layet@9elements.com>

serial: 8250_aspeed_vuart: add VUART over PCI

This patch enables VUART over PCI for the AST2600 by configuring the
SCU registers when the 'aspeed,vuart-over-pci' property is set. It sets
up the PCI device enable bits, sets the PCI class code, and configures MSI
interrupts.

> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> index 6afa2f4057e1b..2360d37139bfa 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> @@ -32,6 +32,28 @@
>  #define ASPEED_VUART_DEFAULT_SIRQ	4
>  #define ASPEED_VUART_DEFAULT_SIRQ_POLARITY	IRQ_TYPE_LEVEL_LOW
>  
> +#define ASPEED_SCU_SILICON_REVISION_ID			0x04
> +#define AST2600A3_REVISION_ID				0x05030303
> +
> +#define ASPEED_SCU_PCIE_CONF_CTRL 0xC20
> +#define  SCU_PCIE_CONF_BMC_DEV_EN					BIT(8)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_MMIO				BIT(9)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_MSI				BIT(11)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_IRQ				BIT(13)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_PCIE_BUS_MASTER	BIT(14)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_E2L				BIT(15)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE		BIT(21)
> +
> +#define ASPEED_SCUC24 0xC24
> +#define  ASPEED_SCUC24_MSI_ROUTING_MASK				GENMASK(11, 10)
> +#define  ASPEED_SCUC24_MSI_ROUTING_PCIE2LPC_PCIDEV1 \
> +		FIELD_PREP(ASPEED_SCUC24_MSI_ROUTING_MASK, 2)

[Severity: Low]
Does this code rely on FIELD_PREP() without including <linux/bitfield.h>?

> +#define  ASPEED_SCUC24_RESERVED_14					BIT(14)
> +#define  ASPEED_SCUC24_PCIDEV1_INTX_MSI_SCU560_EN	BIT(17)
> +#define  ASPEED_SCUC24_PCIDEV1_INTX_MSI_HOST2BMC_EN	BIT(18)
> +
> +#define ASPEED_SCU_BMC_DEV_CLASS	0xC68

[ ... ]

> @@ -412,6 +434,66 @@ static int aspeed_vuart_map_irq_polarity(u32 dt)
>  	}
>  }
>  
> +static int aspeed_ast2600_vuart_over_pci_set_enabled(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	u32 silicon_revision_id;
> +	struct regmap *scu;
> +	int rc;
> +
> +	u32 pcie_config_ctl = SCU_PCIE_CONF_BMC_DEV_EN_IRQ |
> +				SCU_PCIE_CONF_BMC_DEV_EN_MMIO |
> +				SCU_PCIE_CONF_BMC_DEV_EN_MSI |
> +				SCU_PCIE_CONF_BMC_DEV_EN_PCIE_BUS_MASTER |
> +				SCU_PCIE_CONF_BMC_DEV_EN_E2L |
> +				SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE |
> +				SCU_PCIE_CONF_BMC_DEV_EN;
> +
> +	scu = syscon_regmap_lookup_by_phandle(dev->of_node, "aspeed,scu-syscon");
> +	if (IS_ERR(scu)) {
> +		dev_err(dev, "failed to find SCU regmap\n");
> +		return PTR_ERR(scu);
> +	}

[Severity: Low]
If syscon_regmap_lookup_by_phandle() returns -EPROBE_DEFER, will this
unconditional dev_err() cause unnecessary log spam during boot? 

The caller properly wraps the return with dev_err_probe(), so removing the
dev_err() here might be cleaner.

> +
> +	/* update class code to be an Unassigned/device specific class device */
> +	if (regmap_write(scu, ASPEED_SCU_BMC_DEV_CLASS, 0xff000000)) {
> +		dev_err(dev, "could not set PCIe class code\n");
> +		return -EIO;
> +	}

[Severity: Low]
Is it better to propagate the actual regmap_write() error code instead
of shadowing it with a hardcoded -EIO? Returning the original error might
help with debugging upstream failures.

[ ... ]

> @@ -540,6 +622,15 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
>  	aspeed_vuart_set_host_tx_discard(vuart, true);
>  	platform_set_drvdata(pdev, vuart);
>  
> +	if (of_device_is_compatible(dev->of_node, "aspeed,ast2600-vuart") &&
> +	    of_property_read_bool(dev->of_node, "aspeed,vuart-over-pci")) {
> +		rc = aspeed_ast2600_vuart_over_pci_set_enabled(pdev);
> +		if (rc < 0) {
> +			dev_err_probe(dev, rc, "could not enable VUART over PCI\n");
> +			goto err_sysfs_remove;
> +		}
> +	}
> +

[Severity: Critical]
This is a pre-existing issue, but does jumping to err_sysfs_remove here leak
the registered 8250 port?

If aspeed_ast2600_vuart_over_pci_set_enabled() fails, we jump to
err_sysfs_remove which currently looks like this:

err_sysfs_remove:
	sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
	return rc;

Since serial8250_register_8250_port() was already called earlier in
aspeed_vuart_probe(), the port remains registered. The driver core will free
the devm-managed vuart structure when probe fails, potentially leaving a
dangling pointer in the TTY layer that could trigger a use-after-free upon
port open or interrupt.

Would it be safer to add a call to serial8250_unregister_port() in the error
cleanup path?

>  	return 0;
>  
>  err_sysfs_remove:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-aspeed-driver-v6-v6-0-fd7cc0b338ea@9elements.com?part=3

[PATCH v6 4/6] mfd: aspeed-pci-core: add AST2600 PCIe BMC device driver

[Grégoire Layet]

Add support for UART over PCIe between ASPEED AST2600 BMC and
host. This is the host-side driver.

Based on ASPEED 6.18 Kernel SDK and trimmed down and changed to
MFD driver.

The AST2600 has two 8250-compatible register set usable over PCIe.
These act as two UARTs. As there is only a FIFO internally between both
ends, they are called Virtual UARTs. This patch adds both VUARTs as
plat_serial8250_port. This is the core driver in charge of setting up
MSI interrupts. It doesn't bind the whole PCIe resource to allow each
sub-driver to bind the needed addresses.

This is not added as a PCI 8250 UART device, but as a MFD driver, as this
host driver can be expanded upon for IPMI over KCS. It can also be used
in the future for custom BMC<->host communication with shared memory and
doorbell.

This host module should be the entry point for setting up all features
related to an AST2600 present on the PCI bus.

Notes on the implementation:
The host can't detect the VUART addresses, so they are forced to
0x3f8 and 0x2f8, as in the initial ASPEED driver.

The MSI vector index of VUART2 has been changed from 15 to 17.
The index 15 used in the initial driver was not working.

Tested:
Data path in both direction is tested on both VUARTs.

Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com>
Signed-off-by: aspeedyh <yh_chung@aspeedtech.com>
Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 MAINTAINERS                   |   6 +++
 drivers/mfd/Kconfig           |  12 +++++
 drivers/mfd/Makefile          |   2 +
 drivers/mfd/aspeed-pci-core.c | 115 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 135 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 562754f2d166..7ea6c7d4703e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4011,6 +4011,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/crypto/aspeed,*
 F:	drivers/crypto/aspeed/
 
+ASPEED PCI BMC DEVICE DRIVER
+M:	Grégoire Layet <git@gregoirelayet.com>
+M:	Tan Siewert <tan.siewert@9elements.com>
+S:	Maintained
+F:	drivers/mfd/aspeed-pci-core.c
+
 ASPEED PECI CONTROLLER
 M:	Iwona Winiarska <iwona.winiarska@intel.com>
 L:	linux-aspeed@lists.ozlabs.org (moderated for non-subscribers)
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 763ce6a34782..dd780f72d169 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2573,5 +2573,17 @@ config MFD_MAX7360
 	  additional drivers must be enabled in order to use the functionality
 	  of the device.
 
+config MFD_ASPEED_PCI_BMC_DEVICE
+	tristate "ASPEED BMC PCIe device"
+	depends on PCI
+	select MFD_CORE
+	help
+	  Host-side driver for the ASPEED AST2600 BMC PCIe device found on
+	  BMC expansion cards. Exposes two 8250-compatible VUART
+	  ports.
+
+	  If unsure, say N. Choose M here if this machine has an
+	  AST2600-based BMC expansion card on its PCIe bus.
+
 endmenu
 endif
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index dd4bb7e77c33..a364b1b5936b 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -303,3 +303,5 @@ obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu_spi.o rsmu_core.o
 obj-$(CONFIG_MFD_UPBOARD_FPGA)	+= upboard-fpga.o
 
 obj-$(CONFIG_MFD_LOONGSON_SE)	+= loongson-se.o
+
+obj-$(CONFIG_MFD_ASPEED_PCI_BMC_DEVICE)	+= aspeed-pci-core.o
diff --git a/drivers/mfd/aspeed-pci-core.c b/drivers/mfd/aspeed-pci-core.c
new file mode 100644
index 000000000000..2f299955bc1b
--- /dev/null
+++ b/drivers/mfd/aspeed-pci-core.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (C) ASPEED Technology Inc.
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/pci.h>
+#include <linux/serial_8250.h>
+#include <linux/mfd/core.h>
+
+#define ASPEED_BMC_MULTI_MSI		32
+#define ASPEED_BMC_PCI_DEVICE_ID	0x2402
+#define ASPEED_BMC_REVISION_AST2700	0x27
+#define ASPEED_BMC_NR_VUART		2
+
+#define DRIVER_NAME "ast2600-pci-core"
+
+static const unsigned int vuart_msi_index[ASPEED_BMC_NR_VUART] = { 16, 17 };
+static const u16 vuart_port_addr[ASPEED_BMC_NR_VUART] = { 0x3f8, 0x2f8 };
+
+static struct plat_serial8250_port aspeed_uart_port[ASPEED_BMC_NR_VUART + 1] = {
+	{
+		.uartclk = 115200 * 16,
+		.iotype = UPIO_MEM32,
+		.type = PORT_16550A,
+		.flags = (UPF_IOREMAP | UPF_FIXED_PORT | UPF_FIXED_TYPE),
+		.regshift = 2
+	},
+	{
+		.uartclk = 115200 * 16,
+		.iotype = UPIO_MEM32,
+		.type = PORT_16550A,
+		.flags = (UPF_IOREMAP | UPF_FIXED_PORT | UPF_FIXED_TYPE),
+		.regshift = 2
+	},
+	{ 0 }
+};
+
+static const struct mfd_cell aspeed_bmc_cell = {
+	.name		= "serial8250",
+	.platform_data	= aspeed_uart_port,
+	.pdata_size	= sizeof(aspeed_uart_port),
+};
+
+static int aspeed_pci_bmc_device_setup_vuart(struct pci_dev *pdev)
+{
+	resource_size_t bar = pci_resource_start(pdev, 1);
+	unsigned int i;
+
+	for (i = 0; i < ASPEED_BMC_NR_VUART; i++) {
+		aspeed_uart_port[i].mapbase = bar + (vuart_port_addr[i] << 2);
+		aspeed_uart_port[i].irq = pci_irq_vector(pdev, vuart_msi_index[i]);
+	}
+
+	return 0;
+}
+
+static void aspeed_bmc_pci_free_irqs(void *pdev)
+{
+	pci_free_irq_vectors(pdev);
+}
+
+static int aspeed_pci_host_bmc_device_probe(struct pci_dev *pdev,
+					    const struct pci_device_id *ent)
+{
+	struct device *dev = &pdev->dev;
+	int rc = 0;
+
+	if (pdev->revision == ASPEED_BMC_REVISION_AST2700)
+		return dev_err_probe(dev, -ENODEV, "AST2700 detected but not supported\n");
+
+	rc = pcim_enable_device(pdev);
+	if (rc)
+		return dev_err_probe(dev, rc, "failed to enable device\n");
+
+	pci_set_master(pdev);
+
+	rc = pci_alloc_irq_vectors(pdev, ASPEED_BMC_MULTI_MSI, ASPEED_BMC_MULTI_MSI, PCI_IRQ_MSI);
+	if (rc < 0)
+		return dev_err_probe(dev, rc, "failed to allocate %d MSI vectors\n",
+				     ASPEED_BMC_MULTI_MSI);
+
+	rc = devm_add_action_or_reset(dev, aspeed_bmc_pci_free_irqs, pdev);
+	if (rc)
+		return rc;
+
+	aspeed_pci_bmc_device_setup_vuart(pdev);
+
+	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
+				    &aspeed_bmc_cell, 1, &pdev->resource[1],
+				    0, NULL);
+}
+
+static struct pci_device_id aspeed_bmc_dev_pci_ids[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_ASPEED, ASPEED_BMC_PCI_DEVICE_ID),
+		.class = PCI_CLASS_OTHERS << 16,
+		.class_mask = 0xFFFF00
+	},
+	{ 0 }
+};
+
+MODULE_DEVICE_TABLE(pci, aspeed_bmc_dev_pci_ids);
+
+static struct pci_driver aspeed_host_bmc_dev_driver = {
+	.name		= DRIVER_NAME,
+	.id_table	= aspeed_bmc_dev_pci_ids,
+	.probe		= aspeed_pci_host_bmc_device_probe,
+};
+
+module_pci_driver(aspeed_host_bmc_dev_driver);
+
+MODULE_AUTHOR("Grégoire Layet <gregoire.layet@9elements.com>");
+MODULE_DESCRIPTION("Host-side driver for the ASPEED BMC PCIe device");
+MODULE_LICENSE("GPL");

-- 
2.54.0

Re: [PATCH v6 4/6] mfd: aspeed-pci-core: add AST2600 PCIe BMC device driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The driver mutates a static global array (`aspeed_uart_port`) during device probe, causing data corruption and race conditions if multiple devices are present.
- [Medium] Using the MFD subsystem API for a single-function device violates MFD design guidelines.
- [Medium] The driver unconditionally computes memory-mapped I/O addresses from PCIe BAR 1 without verifying the BAR's existence, type, or size.
- [Low] The commit message subject violates MFD capitalization conventions.
- [Low] The PCI device ID table is missing the `const` qualifier.
--

--- Patch [4]: [PATCH v6 4/6] mfd: aspeed-pci-core: add AST2600 PCIe BMC device driver ---
commit 91187d47ec6898e7565aba419a47fd407dec5f6c
Author: Grégoire Layet <gregoire.layet@9elements.com>

mfd: aspeed-pci-core: add AST2600 PCIe BMC device driver
    
Add support for UART over PCIe between ASPEED AST2600 BMC and
host. This is the host-side driver.

> mfd: aspeed-pci-core: add AST2600 PCIe BMC device driver

[Severity: Low]
This isn't a bug, but the MFD subsystem guidelines require capitalizing the
description after the prefix. Should "add" be "Add"?

> diff --git a/drivers/mfd/aspeed-pci-core.c b/drivers/mfd/aspeed-pci-core.c
> new file mode 100644
> index 0000000000000..2f299955bc1b7
> --- /dev/null
> +++ b/drivers/mfd/aspeed-pci-core.c
> @@ -0,0 +1,115 @@

[ ... ]

> +static const unsigned int vuart_msi_index[ASPEED_BMC_NR_VUART] = { 16, 17 };
> +static const u16 vuart_port_addr[ASPEED_BMC_NR_VUART] = { 0x3f8, 0x2f8 };
> +
> +static struct plat_serial8250_port aspeed_uart_port[ASPEED_BMC_NR_VUART + 1] = {
> +	{
> +		.uartclk = 115200 * 16,
> +		.iotype = UPIO_MEM32,
> +		.type = PORT_16550A,
> +		.flags = (UPF_IOREMAP | UPF_FIXED_PORT | UPF_FIXED_TYPE),
> +		.regshift = 2
> +	},
> +	{
> +		.uartclk = 115200 * 16,
> +		.iotype = UPIO_MEM32,
> +		.type = PORT_16550A,
> +		.flags = (UPF_IOREMAP | UPF_FIXED_PORT | UPF_FIXED_TYPE),
> +		.regshift = 2
> +	},
> +	{ 0 }
> +};
> +
> +static const struct mfd_cell aspeed_bmc_cell = {
> +	.name		= "serial8250",
> +	.platform_data	= aspeed_uart_port,
> +	.pdata_size	= sizeof(aspeed_uart_port),
> +};
> +
> +static int aspeed_pci_bmc_device_setup_vuart(struct pci_dev *pdev)
> +{
> +	resource_size_t bar = pci_resource_start(pdev, 1);
> +	unsigned int i;
> +
> +	for (i = 0; i < ASPEED_BMC_NR_VUART; i++) {
> +		aspeed_uart_port[i].mapbase = bar + (vuart_port_addr[i] << 2);
> +		aspeed_uart_port[i].irq = pci_irq_vector(pdev, vuart_msi_index[i]);
> +	}

[Severity: High]
Since aspeed_uart_port is a static global array, won't concurrent device probes
overwrite the hardware parameters for other ASPEED AST2600 PCI devices?

Should the mfd_cell and aspeed_uart_port structures be dynamically allocated
locally per device during probe to avoid this data corruption?

[Severity: Medium]
Does this code compute memory-mapped I/O addresses without verifying if BAR 1
actually has IORESOURCE_MEM set, or checking if the resource length is large
enough? Could an untrusted device presenting a smaller or disabled BAR 1 lead
to an unauthorized or out-of-bounds physical memory mapping primitive?

> +
> +	return 0;
> +}

[ ... ]

> +static int aspeed_pci_host_bmc_device_probe(struct pci_dev *pdev,
> +					    const struct pci_device_id *ent)
> +{
> +	struct device *dev = &pdev->dev;
> +	int rc = 0;

[ ... ]

> +	aspeed_pci_bmc_device_setup_vuart(pdev);
> +
> +	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> +				    &aspeed_bmc_cell, 1, &pdev->resource[1],
> +				    0, NULL);
> +}

[Severity: Medium]
This isn't a bug, but MFD subsystem guidelines state that the MFD API should
only be used for devices registering multiple children. 

Since devm_mfd_add_devices() is being called with only a single cell, would it
be more appropriate to implement this as a standard PCI driver until multiple
features are actually implemented?

> +
> +static struct pci_device_id aspeed_bmc_dev_pci_ids[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_ASPEED, ASPEED_BMC_PCI_DEVICE_ID),
> +		.class = PCI_CLASS_OTHERS << 16,
> +		.class_mask = 0xFFFF00
> +	},
> +	{ 0 }
> +};

[Severity: Low]
Should this PCI device ID table be declared with the const qualifier so it
compiles into the read-only data section instead of writable memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-aspeed-driver-v6-v6-0-fd7cc0b338ea@9elements.com?part=4

[PATCH v6 5/6] ARM: dts: aspeed: g6: Change vuart compatible string for ast2600

[Grégoire Layet]

Use the ast2600 compatible string.
Make it more precise and enable specific ast2600 properties.
Still use the ast2500 compatible string as a fallback.

Reviewed-by: Tan Siewert <tan.siewert@9elements.com>
Signed-off-by: Grégoire Layet <gregoire.layet@9elements.com>
---
 arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
index edf4b74f5aba..c952e2075b4c 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
@@ -707,7 +707,7 @@ emmc: sdhci@1e750100 {
 			};
 
 			vuart1: serial@1e787000 {
-				compatible = "aspeed,ast2500-vuart";
+				compatible = "aspeed,ast2600-vuart", "aspeed,ast2500-vuart";
 				reg = <0x1e787000 0x40>;
 				reg-shift = <2>;
 				interrupts = <GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>;
@@ -717,7 +717,7 @@ vuart1: serial@1e787000 {
 			};
 
 			vuart3: serial@1e787800 {
-				compatible = "aspeed,ast2500-vuart";
+				compatible = "aspeed,ast2600-vuart", "aspeed,ast2500-vuart";
 				reg = <0x1e787800 0x40>;
 				reg-shift = <2>;
 				interrupts = <GIC_SPI 180 IRQ_TYPE_LEVEL_HIGH>;
@@ -727,7 +727,7 @@ vuart3: serial@1e787800 {
 			};
 
 			vuart2: serial@1e788000 {
-				compatible = "aspeed,ast2500-vuart";
+				compatible = "aspeed,ast2600-vuart", "aspeed,ast2500-vuart";
 				reg = <0x1e788000 0x40>;
 				reg-shift = <2>;
 				interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>;
@@ -737,7 +737,7 @@ vuart2: serial@1e788000 {
 			};
 
 			vuart4: serial@1e788800 {
-				compatible = "aspeed,ast2500-vuart";
+				compatible = "aspeed,ast2600-vuart", "aspeed,ast2500-vuart";
 				reg = <0x1e788800 0x40>;
 				reg-shift = <2>;
 				interrupts = <GIC_SPI 181 IRQ_TYPE_LEVEL_HIGH>;

-- 
2.54.0

Re: [PATCH v6 5/6] ARM: dts: aspeed: g6: Change vuart compatible string for ast2600

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch series adds a new failure path that makes it more likely. Missing serial8250_unregister_port() and timer_delete_sync() in the error handling path of aspeed_vuart_probe() leads to a use-after-free.
--

--- Patch [5]: [PATCH v6 5/6] ARM: dts: aspeed: g6: Change vuart compatible string for ast2600 ---
commit 0075a2210772d2bff98e062573ee22db87bab4f2
Author: Grégoire Layet <gregoire.layet@9elements.com>

ARM: dts: aspeed: g6: Change vuart compatible string for ast2600

This commit updates the compatible string for the vuart nodes on aspeed-g6 to
use the ast2600 compatible string. It retains the ast2500 compatible string
as a fallback, allowing the driver to utilize specific ast2600 properties.

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> index edf4b74f5aba4..c952e2075b4c8 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> +++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> @@ -707,7 +707,7 @@ emmc: sdhci@1e750100 {
>  			};
>  
>  			vuart1: serial@1e787000 {
> -				compatible = "aspeed,ast2500-vuart";
> +				compatible = "aspeed,ast2600-vuart", "aspeed,ast2500-vuart";

[Severity: High]
This isn't a bug introduced by this patch, but changing the compatible
string exposes it by enabling a new failure path. Can this regression lead
to a use-after-free during probe failure?

In drivers/tty/serial/8250/8250_aspeed_vuart.c:aspeed_vuart_probe(),
using the "aspeed,ast2600-vuart" compatible string enables new initialization
steps, such as aspeed_ast2600_vuart_over_pci_set_enabled(). If this fails,
it jumps to the err_sysfs_remove label:

err_sysfs_remove:
	sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
	return rc;

Since serial8250_register_8250_port() and timer_setup() were called
earlier in the probe function, returning an error without unregistering
the 8250 port or cleaning up the timer will leave the port registered
with a dangling pointer when the devm-allocated vuart structure is freed.
If accessed by the tty layer or userspace, it will trigger an oops or
kernel panic.

Should the error handling block in aspeed_vuart_probe() be updated to
include serial8250_unregister_port() and timer_delete_sync() before
returning?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-aspeed-driver-v6-v6-0-fd7cc0b338ea@9elements.com?part=5

[PATCH v6 6/6] ARM: dts: aspeed: g6: add aspeed,vuart-over-pci prop to vuart3 and 4

[Grégoire Layet]

The VUART 3 and 4 are VUART over PCI.
Set this flag to indicate this information.
Also set the scu-syscon phandle.

Added lines are compliant with dt-style strict mode

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

diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
index c952e2075b4c..61cdca0b3ed7 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
@@ -723,6 +723,9 @@ vuart3: serial@1e787800 {
 				interrupts = <GIC_SPI 180 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&syscon ASPEED_CLK_APB2>;
 				no-loopback-test;
+				aspeed,scu-syscon = <&syscon>;
+				aspeed,vuart-over-pci;
+
 				status = "disabled";
 			};
 
@@ -743,6 +746,9 @@ vuart4: serial@1e788800 {
 				interrupts = <GIC_SPI 181 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&syscon ASPEED_CLK_APB2>;
 				no-loopback-test;
+				aspeed,scu-syscon = <&syscon>;
+				aspeed,vuart-over-pci;
+
 				status = "disabled";
 			};
 

-- 
2.54.0

Re: [PATCH v6 6/6] ARM: dts: aspeed: g6: add aspeed,vuart-over-pci prop to vuart3 and 4

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing 8250 port unregistration on the probe error path leads to a Use-After-Free of the `vuart` structure.
--

--- Patch [6]: [PATCH v6 6/6] ARM: dts: aspeed: g6: add aspeed,vuart-over-pci prop to vuart3 and 4 ---
commit f96d2931c661be3250c723d0a37aa6edf52c56a7
Author: Grégoire Layet <gregoire.layet@9elements.com>

ARM: dts: aspeed: g6: add aspeed,vuart-over-pci prop to vuart3 and 4

The VUART 3 and 4 are VUART over PCI.
Set this flag to indicate this information.
Also set the scu-syscon phandle.

> diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> index c952e2075b4c8..61cdca0b3ed7e 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> +++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
> @@ -723,6 +723,9 @@ vuart3: serial@1e787800 {
>  				interrupts = <GIC_SPI 180 IRQ_TYPE_LEVEL_HIGH>;
>  				clocks = <&syscon ASPEED_CLK_APB2>;
>  				no-loopback-test;
> +				aspeed,scu-syscon = <&syscon>;
> +				aspeed,vuart-over-pci;

[Severity: High]
This is a pre-existing issue, but does adding these properties expose a
memory corruption risk during probe failures?

In drivers/tty/serial/8250/8250_aspeed_vuart.c:aspeed_vuart_probe(), if the
function fails after serial8250_register_8250_port() is called (for example,
if handling the newly added vuart-over-pci properties fails), it jumps
to the error path:

err_sysfs_remove:
	sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
	return rc;

This skips calling serial8250_unregister_port() and timer_delete_sync(),
leaving the port registered. Because returning an error causes devres to free
the vuart structure, could a subsequent open of the leaked port cause
aspeed_vuart_startup() to dereference the freed vuart pointer?

> +
>  				status = "disabled";
>  			};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-aspeed-driver-v6-v6-0-fd7cc0b338ea@9elements.com?part=6