Re: [PATCH] PCI: tegra: Unwind partial PHY power-on failures

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The new error unwinding path for `tegra_pcie_phy_power_on` uses `tegra_pcie_port_phy_power_off` to power off fully initialized previous ports. However, `tegra_pcie_port_phy_power_off` iterates over the port's lanes in forward order and returns early on the first `phy_power_off` failure. This violates the commit message's claim to unwind in reverse order at both levels, and introduces a resource leak if a `phy_power_off` call fails, because the remaining lanes for that port will incorrectly remain powered on.
--

commit 98ccb4105337a9e0fdf96c19a4beba14a45d67f0
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

PCI: tegra: Unwind partial PHY power-on failures

This commit adds error handling to unwind successfully powered PHYs in the
Tegra PCIe PHY setup. It attempts to power off PHYs in reverse order for
both nested acquisition loops (lanes within a port, and ports within the
controller) when an error occurs.

> diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
> index 59b56446c507e..443fa33b7f0ec 100644
> --- a/drivers/pci/controller/pci-tegra.c
> +++ b/drivers/pci/controller/pci-tegra.c

[ ... ]

> @@ -1051,6 +1053,8 @@ static int tegra_pcie_phy_power_on(struct tegra_pcie *pcie)
>  			dev_err(dev,
>  				"failed to power on PCIe port %u PHY: %d\n",
>  				port->index, err);
> +			list_for_each_entry_continue_reverse(port, &pcie->ports, list)
> +				tegra_pcie_port_phy_power_off(port);

[Severity: Medium]
The commit message states the code will unwind successfully powered PHYs in
reverse order at both levels. However, does delegating to
tegra_pcie_port_phy_power_off() here contradict this design?

Looking at tegra_pcie_port_phy_power_off(), it iterates over the lanes in
forward order rather than reverse:

static int tegra_pcie_port_phy_power_off(struct tegra_pcie_port *port)
{
	...
	for (i = 0; i < port->lanes; i++) {
		err = phy_power_off(port->phys[i]);
	...
}

Additionally, if an error occurs during unwinding, does this cause a resource
leak of the remaining PHY power states?

If phy_power_off() fails for any lane, tegra_pcie_port_phy_power_off()
returns early:

		err = phy_power_off(port->phys[i]);
		if (err < 0) {
			dev_err(dev, "failed to power off PHY#%u: %d\n", i,
				err);
			return err;
		}

This early return abandons the power-off sequence for the remaining lanes
in the port, leaving those PHYs incorrectly powered on.

>  			return err;
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828091747.7976-1-pengpeng@iscas.ac.cn?part=1