Re: [PATCH v7 2/2] PCI: mediatek: Add support for EcoNet EN7528 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] mtk_pcie_retrain() iterates the PCI bus without locks and holds an unreferenced device pointer, leading to a Use-After-Free.
- [High] mtk_pcie_retrain() only retrains the first root port, silently ignoring any other ports on the same host.
- [High] Link retraining occurs after downstream endpoint drivers are probed, causing a race condition that can lead to system crashes.
- [High] Calling virt_to_phys() on an __iomem pointer causes undefined behavior and potential memory corruption.
- [Medium] The IS_BUILTIN() workaround silently degrades hardware capabilities when the driver is compiled as a module.
--

commit 08338fe344200ef55148194108ab3b65616af7c7
Author: Caleb James DeLisle <cjd@cjdns.fr>
Subject: PCI: mediatek: Add support for EcoNet EN7528 SoC

This commit adds support for the PCIe controller found on EcoNet EN7528 SoCs.
It introduces a hardware quirk to retrain the link after startup, as some
ports capable of Gen2 operation will initially start at Gen1 speeds.

> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
[ ... ]
> @@ -753,6 +764,135 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
[ ... ]
> +	/* Activate INTx interrupts */
> +	val = readl(port->base + PCIE_INT_MASK);
> +	val &= ~INTX_MASK;
> +	writel(val, port->base + PCIE_INT_MASK);
> +
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		mtk_pcie_enable_msi(port);

Is it safe to call mtk_pcie_enable_msi() here? Looking at
mtk_pcie_enable_msi(), it calculates the message address using:

    virt_to_phys(port->base + PCIE_MSI_VECTOR)

Since port->base is an ioremapped __iomem pointer, passing it to
virt_to_phys() produces undefined behavior and could return a garbage
physical address. If this address overlaps with valid physical RAM mapped
for DMA, could memory writes from the PCIe endpoint corrupt random system
memory?

> +
> +	/* Set AHB to PCIe translation windows */
[ ... ]
> +static int mtk_pcie_retrain(struct device *dev, struct pci_host_bridge *host)
> +{
> +	struct pci_dev *rp;
> +	int ret = -ENOENT;
> +	u16 lnksta = 0;
> +	u32 speed;
> +
> +	/* Should already have been warned about during startup_port */
> +	if (!IS_BUILTIN(CONFIG_PCIE_MEDIATEK))
> +		return 0;
> +
> +	for_each_pci_bridge(rp, host->bus) {
> +		if (pci_pcie_type(rp) == PCI_EXP_TYPE_ROOT_PORT)
> +			goto found_port;
> +	}
> +
> +	/* Should not happen */
> +	return dev_err_probe(dev, ret, "root port not found\n");
> +
> +found_port:

The commit message mentions that it is most convenient to retrain every port
after startup. Does the goto found_port here unintentionally skip all
subsequent root ports by breaking out of the loop after the first match?

Also, is it safe to traverse host->bus->devices via for_each_pci_bridge()
without holding the pci_bus_sem read lock? If a device is concurrently added
or removed, this might lead to list corruption.

Finally, pcie_retrain_link() is a sleepable function. Because the code does
not acquire a reference to the root port via pci_dev_get() before sleeping,
could a concurrent removal event free the pci_dev structure and result in a
use-after-free?

> +
> +#if IS_BUILTIN(CONFIG_PCIE_MEDIATEK)
> +	ret = pcie_retrain_link(rp, true);
> +#endif

Does this silently degrade hardware capabilities when the driver is compiled
as a loadable module? The Kconfig definition allows CONFIG_PCIE_MEDIATEK to
be a tristate module, so users compiling it as a module might find their PCIe
devices permanently stuck at Gen1 speeds.

> +
> +	if (ret)
[ ... ]
> @@ -1149,6 +1289,13 @@ static int mtk_pcie_probe(struct platform_device *pdev)
>  	if (err)
>  		goto put_resources;
>  
> +	/*
> +	 * Ignore error because pci_host_probe() was already called, and in any
> +	 * case it is possible that the port will still work as Gen1.
> +	 */
> +	if (pcie->soc->quirks & MTK_PCIE_RETRAIN)
> +		mtk_pcie_retrain(dev, host);
> +
>  	return 0;

Does retraining the link at this stage introduce a race condition with
downstream endpoints?

By the time pci_host_probe() completes successfully, the PCIe bus is fully
enumerated and downstream endpoint drivers (such as NVMe or WiFi) have been
probed. If those drivers are actively issuing MMIO or DMA requests, forcing
the physical link into the Recovery state could cause PCIe completion timeouts
and potentially lead to fatal kernel crashes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514151318.3444959-1-cjd@cjdns.fr?part=2