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

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 [off-list ref]

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"?

[ ... ]

+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