Re: [PATCH v6 03/10] PCI: dw-rockchip: Move devm_phy_get out of phy_init

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but iterating through `bus->devices` without holding the `pci_bus_sem` read lock risks a Use-After-Free.
- [High] This problem wasn't introduced by this patch, but repeated devres allocations in rockchip_pcie_rc_reset_root_port() via rockchip_pcie_clk_init() cause a resource leak.
--

commit 2321e2492c7e546beab0727fbd3c7122dbf07abc
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

PCI: dw-rockchip: Move devm_phy_get out of phy_init

This patch moves the devm_phy_get() call from rockchip_pcie_phy_init() to
the probe routine. This allows the PCIe PHY to be re-initialized without
re-requesting the managed resource, which is necessary after operations
like system suspend/resume.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index f67ad3c31c35b..7472ec78b4738 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
[ ... ]
> @@ -858,6 +852,13 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  					     "failed to enable vpcie3v3 regulator\n");
>  	}
>  
> +	rockchip->phy = devm_phy_get(dev, "pcie-phy");
> +	if (IS_ERR(rockchip->phy)) {
> +		ret = PTR_ERR(rockchip->phy);
> +		dev_err_probe(dev, ret, "missing PHY\n");
> +		goto disable_regulator;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but while moving devm_phy_get() 
to the probe function correctly prevents resource leaks during 
re-initialization, there is a similar leak involving devm_clk_bulk_get_all().

In rockchip_pcie_clk_init(), devm_clk_bulk_get_all() is called
unconditionally:

rockchip_pcie_clk_init() {
        ...
        ret = devm_clk_bulk_get_all(dev, &rockchip->clks);
        ...
}

This causes a leak because rockchip_pcie_rc_reset_root_port() calls
rockchip_pcie_clk_init() during a recurring reset flow:

rockchip_pcie_rc_reset_root_port() {
        ...
        ret = rockchip_pcie_clk_init(rockchip);
        ...
}

Since rockchip_pcie_rc_reset_root_port() is assigned to
pp->bridge->reset_root_port, it can be triggered dynamically via sysfs or
error recovery. Because devres resources are only freed when the device
unbinds, does every root port reset allocate a new set of clock handles
and leak the previous devres memory? Could the clock acquisition be moved
to the probe function as well, similar to what this patch does for the PHY?

[Severity: High]
This is also a pre-existing issue, but iterating through bus->devices
without holding the pci_bus_sem read lock risks a Use-After-Free.

In rockchip_pcie_rc_sys_irq_thread(), the thread processes hot reset or
link-down events and iterates over the bus:

rockchip_pcie_rc_sys_irq_thread() {
        ...
        for_each_pci_bridge(port, pp->bridge->bus) {
                if (pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
                        pci_host_handle_link_down(port);
        }
        ...
}

Because bus->devices can be dynamically modified concurrently via PCIe
hotplug or user-initiated unbinds via sysfs, if a device is hot-removed
while the thread is iterating, could it dereference an invalid list
pointer? Should this iteration be protected by down_read(&pci_bus_sem)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-rockchip-pcie-system-suspend-v6-0-fbabcca9921b@collabora.com?part=3