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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Use-after-free on probe error path due to missing pci_remove_root_bus().
- [High] Hardware is exposed to endpoint drivers before link retraining completes, causing MMIO failures.
- [High] mtk_pcie_retrain() only retrains the first root port, ignoring others.
- [Medium] Data race and potential use-after-free due to unprotected iteration of the PCI bus list.
- [Medium] Truncation of 64-bit resource_size_t in fls() causes incorrect AXI window size calculation.
- [Medium] Silent feature degradation when the driver is compiled as a loadable module.
--

commit 940197600e9e8eb5cb8f108830edfaee6eeb6d16
Author: Caleb James DeLisle <cjd@cjdns.fr>

PCI: mediatek: Add support for EcoNet EN7528 SoC

This commit adds support for the PCIe controller on the EcoNet EN7528 and
EN751221 SoCs. It includes logic to re-train Gen2 capable ports that
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,132 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
>  	return 0;
>  }
>  
> +static int mtk_pcie_startup_port_en7528(struct mtk_pcie_port *port)
> +{
[ ... ]
> +	if (IS_ENABLED(CONFIG_PCI_MSI))
> +		mtk_pcie_enable_msi(port);
> +
> +	/* Set AHB to PCIe translation windows */
> +	val = lower_32_bits(mem->start) |
> +	      AHB2PCIE_SIZE(fls(resource_size(mem)));

Does passing resource_size(mem) to fls() risk truncating 64-bit values?
Since resource_size_t can be a 64-bit integer and fls() expects a 32-bit
unsigned int, an exact 4GB window (0x100000000) or larger could truncate
to 0, leading to an incorrect translation window size. Should this use
fls64() instead?

> +	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
[ ... ]
> +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;
> +
> +	for_each_pci_bridge(rp, host->bus) {
> +		if (pci_pcie_type(rp) == PCI_EXP_TYPE_ROOT_PORT)
> +			goto found_port;
> +	}

By jumping to found_port immediately upon finding the first root port, does
this skip retraining for any subsequent ports (like RC1 on the EN7528)? The
commit message mentions it is most convenient to re-train every port after
startup.

Also, is it safe to iterate host->bus->devices via for_each_pci_bridge()
and use the rp device pointer without holding down_read(&pci_bus_sem) or
taking a reference via pci_dev_get()? Since this function runs after
pci_host_probe() finishes, could the device list change concurrently?

> +
> +	/* Should not happen */
> +	return dev_err_probe(dev, ret, "root bridge not found\n");
> +
> +found_port:
> +	if (!IS_BUILTIN(CONFIG_PCIE_MEDIATEK)) {
> +		/* Let it go because the device will work as Gen1 */
> +		dev_warn(dev, "module must be built-in to retrain to Gen2\n");
> +		return 0;
> +	}
> +
> +#if IS_BUILTIN(CONFIG_PCIE_MEDIATEK)
> +	ret = pcie_retrain_link(rp, true);
> +#endif

If the driver is built as a loadable module, this skips link retraining and
silently leaves the hardware operating at Gen1 speeds. Is there a way to
achieve consistent hardware capabilities regardless of the build
configuration without relying on IS_BUILTIN checks?

> +
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to retrain port\n");
[ ... ]
> @@ -1149,6 +1286,11 @@ static int mtk_pcie_probe(struct platform_device *pdev)
>  	err = pci_host_probe(host);
>  	if (err)
>  		goto put_resources;
>  
> +	if (pcie->soc->quirks & MTK_PCIE_RETRAIN)
> +		err = mtk_pcie_retrain(dev, host);
> +	if (err)
> +		goto put_resources;
> +
>  	return 0;
>  
>  put_resources:

Since mtk_pcie_retrain() is called after pci_host_probe() returns, are the
devices exposed to endpoint drivers before the link retraining completes?

If endpoint drivers are actively probing and communicating with the hardware
via MMIO while the link is forced into the Recovery state, could this cause
transactions to fail or timeout?

Additionally, if mtk_pcie_retrain() fails, the code jumps to put_resources
and returns an error. This causes devres to free the host bridge memory.
However, since pci_host_probe() already fully registered the PCI root bus
and its child devices, does this error path leave dangling references in the
PCI subsystem by failing to call pci_stop_root_bus() and
pci_remove_root_bus() before freeing resources?

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