Re: [PATCH v6 3/6] serial: 8250_aspeed_vuart: add VUART over PCI
From: sashiko-bot@kernel.org
Date: 2026-08-18 10:40:44
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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. [ ... ]
quoted hunk ↗ jump to hunk
@@ -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