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 <[email protected]>

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/[email protected]?part=4