Re: [PATCH] phy: HiSilicon: Fix error handling in hi3670_pcie_allclk_ctrl()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The error handling for is_pipe_clk_stable() incorrectly returns 0 (success) instead of a negative error code, masking the PHY initialization failure.
--

--- Patch [1]: [PATCH] phy: HiSilicon: Fix error handling in hi3670_pcie_allclk_ctrl() ---
commit ab89588a8f12e5a9bc6c5af4bf87aaa4dd4ec0ae
Author: Akash Sukhavasi <akash.sukhavasi@gmail.com>

phy: HiSilicon: Fix error handling in hi3670_pcie_allclk_ctrl()

When hi3670_pcie_pll_ctrl() fails, hi3670_pcie_allclk_ctrl() returns
-EINVAL directly, skipping the close_clocks cleanup path. This leaves
the PLL with its power-down bit cleared.

> diff --git a/drivers/phy/hisilicon/phy-hi3670-pcie.c b/drivers/phy/hisilicon/phy-hi3670-pcie.c
> index 7396c601d8747..e909d0e5be0ac 100644
> --- a/drivers/phy/hisilicon/phy-hi3670-pcie.c
> +++ b/drivers/phy/hisilicon/phy-hi3670-pcie.c

[ ... ]

> @@ -480,7 +480,7 @@ static int hi3670_pcie_allclk_ctrl(struct hi3670_pcie_phy *phy, bool clk_on)
>  	ret = hi3670_pcie_pll_ctrl(phy, true);
>  	if (ret) {
>  		dev_err(dev, "Failed to enable pll\n");
> -		return -EINVAL;
> +		goto close_clocks;
>  	}
>  	hi3670_pcie_hp_debounce_gt(phy, true);
>  	hi3670_pcie_phyref_gt(phy, true);

[Severity: High]
This isn't a bug introduced by this patch, but while looking at the error
handling in this driver, I noticed an issue in hi3670_pcie_phy_power_on().

When checking if the pipe clock is stable, the driver calls
is_pipe_clk_stable() which returns a boolean:

drivers/phy/hisilicon/phy-hi3670-pcie.c:hi3670_pcie_phy_power_on() {
    ...
	ret = is_pipe_clk_stable(phy);
	if (!ret)
		goto disable_clks;
    ...
disable_clks:
	kirin_pcie_clk_ctrl(phy, false);
	return ret;
}

Since is_pipe_clk_stable() returns false (0) on failure, this assigns 0 to
ret and branches to disable_clks, ultimately returning 0 (success) instead
of a negative error code.

Could this mislead the caller (the PCIe controller driver) into believing the
PHY successfully powered on? If the controller then proceeds to access the
PCIe config space with the PHY off and clocks disabled, will this cause an
abort or kernel crash?

Additionally, a FIXME comment in hi3670_pcie_phy_power_off() notes that
calling kirin_pcie_clk_ctrl(phy, false) causes an Asynchronous SError panic
on this platform. Does invoking it here in the disable_clks error path
trigger that same panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-b4-hi3670-v1-1-f9236f3e585c@gmail.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy