Re: [PATCH v4 5/7] soc: aspeed: add host-side PCIe BMC device driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The physical address of the PCI BAR is stored in an `unsigned long`, which silently truncates addresses on 32-bit architectures with 64-bit physical addressing.
- [Medium] The driver requests legacy INTx interrupt fallback but enforces a minimum vector count of 32 and hardcodes MSI vector indices, making INTx support impossible.
- [Low] Missing newline character `\n` at the end of a `pr_err` message string.
- [Low] The probe error message references a non-existent function `aspeed_pci_setup_irq_resource()` instead of `pci_alloc_irq_vectors()`.
- [Low] The driver computes a physical memory address for the UART mapped registers by adding a fixed offset to the base of PCI BAR 1, but fails to verify if BAR 1 exists or is large enough to contain this offset.
--

--- Patch [5]: [PATCH v4 5/7] soc: aspeed: add host-side PCIe BMC device driver ---
commit 466ab6d1aa3de4b6bd606a9368f52aa1b3fbe52e
Author: Grégoire Layet <gregoire.layet@9elements.com>
Subject: soc: aspeed: add host-side PCIe BMC device driver

This patch adds a host-side driver for the ASPEED AST2600 BMC PCIe device.
It exposes two 8250-compatible VUART ports over PCIe between the BMC and 
the host.

> diff --git a/drivers/soc/aspeed/aspeed-host-bmc-dev.c b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
> new file mode 100644
> index 0000000000000..e586d05055775
> --- /dev/null
> +++ b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
[ ... ]
> +struct aspeed_pci_bmc_dev {
> +	unsigned long message_bar_base;

[Severity: High]
Can this cause address truncation on 32-bit architectures with 64-bit physical
addressing?

The physical address returned by pci_resource_start() is stored in an
unsigned long, which is 32 bits on 32-bit platforms.

Should message_bar_base be typed as resource_size_t or phys_addr_t instead?

[ ... ]
> +static int aspeed_pci_bmc_device_setup_vuart(struct pci_dev *pdev, int idx)
> +{
[ ... ]
> +	uart->port.iotype = UPIO_MEM32;
> +	uart->port.iobase = 0;
> +	uart->port.mapbase = pci_bmc_dev->message_bar_base + vuart_ioport;

[Severity: Low]
Is it safe to add the VUART offset to the base of BAR 1 without validation?

If BAR 1 is uninitialized or not large enough to contain vuart_ioport, this
could result in an invalid physical memory address.

Should this code check pci_resource_len() before adding the offset?

[ ... ]
> +static int aspeed_pci_host_setup(struct pci_dev *pdev)
> +{
> +	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> +	int rc = 0;
> +
> +	pci_bmc_dev->message_bar_base = pci_resource_start(pdev, 1);
> +
> +	if (pdev->revision == 0x27) {
> +		pr_err("AST2700 detected but not supported");

[Severity: Low]
This isn't a bug, but there's a missing newline character at the end of this
error message. Without the \n, subsequent printk messages might be accidentally
concatenated onto the same line.

[ ... ]
> +static int aspeed_pci_host_bmc_device_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
[ ... ]
> +	pci_set_master(pdev);
> +	pci_set_drvdata(pdev, pci_bmc_dev);
> +
> +	rc = pci_alloc_irq_vectors(pdev, BMC_MULTI_MSI, BMC_MULTI_MSI, PCI_IRQ_INTX | PCI_IRQ_MSI);

[Severity: Medium]
Does this PCI_IRQ_INTX fallback function as intended?

The allocation requests a minimum of 32 vectors (BMC_MULTI_MSI), but INTx only
supports a single vector, so the fallback will always fail.

Also, aspeed_pci_bmc_device_setup_vuart() hardcodes the vector indices:

	uart->port.irq = pci_irq_vector(pdev, vuart_msi_index[idx]);

Since indices 16 and 17 are invalid for INTx, will this driver completely fail
to initialize on systems where MSI is disabled or unavailable?

> +	if (rc < 0) {
> +		dev_err(&pdev->dev, "aspeed_pci_setup_irq_resource() returned error %d\n", rc);

[Severity: Low]
Is the function name in this log message incorrect? It references
aspeed_pci_setup_irq_resource() instead of pci_alloc_irq_vectors(), which
might make debugging confusing.

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