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

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

[Akash Sukhavasi]

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.

Use goto close_clocks instead, which disables the PLL and propagates
the original error code from hi3670_pcie_pll_ctrl().

Found while investigating a Smatch warning.

Fixes: 73075011ffff ("phy: HiSilicon: Add driver for Kirin 970 PCIe PHY")
Signed-off-by: Akash Sukhavasi <akash.sukhavasi@gmail.com>
---
Compile tested only (x86_64 and arm64 cross-compilation).
No access to Kirin 970 hardware.
---
 drivers/phy/hisilicon/phy-hi3670-pcie.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/phy/hisilicon/phy-hi3670-pcie.c b/drivers/phy/hisilicon/phy-hi3670-pcie.c
index 7396c601d874..e909d0e5be0a 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);

---
base-commit: 0e35b9b6ec0ffcc5e23cbdec09f5c622ad532b53
change-id: 20260708-b4-hi3670-f03d178f889a

Best regards,
-- 
Akash Sukhavasi <akash.sukhavasi@gmail.com>


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

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

[sashiko-bot]

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