Re: [PATCH] phy: starfive: Fix runtime PM cleanup in JH7110 DPHY TX probe

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch fixes a runtime PM cleanup leak in the DPHY TX driver by replacing `pm_runtime_enable()` with `devm_pm_runtime_enable()`, but completely misses the exact same bug in the companion DPHY RX driver (`phy-jh7110-dphy-rx.c`).
- [Medium] Resource leaks in `stf_dphy_init()` and `stf_dphy_exit()` error paths where clocks are not correctly unwound or cleaned up.
--

--- Patch [1]: [PATCH] phy: starfive: Fix runtime PM cleanup in JH7110 DPHY TX probe ---
commit d91037f438cbeda1f5f7dbdb182d938b6883ff9c
Author: Can Peng <pengcan@kylinos.cn>

phy: starfive: Fix runtime PM cleanup in JH7110 DPHY TX probe

stf_dphy_probe() enables runtime PM before getting the clock and
reset controls, creating the PHY and registering the PHY provider. If
any of those steps fails, probe returns with runtime PM still enabled.
Use devm_pm_runtime_enable() so runtime PM is disabled automatically
on later probe failures and on driver unbind.

> diff --git a/drivers/phy/starfive/phy-jh7110-dphy-tx.c b/drivers/phy/starfive/phy-jh7110-dphy-tx.c
> index c64d1c91b1307..181491a938079 100644
> --- a/drivers/phy/starfive/phy-jh7110-dphy-tx.c
> +++ b/drivers/phy/starfive/phy-jh7110-dphy-tx.c
> @@ -392,6 +392,7 @@ static int stf_dphy_probe(struct platform_device *pdev)
>  {
>  	struct phy_provider *phy_provider;
>  	struct stf_dphy *dphy;
> +	int ret;
>  
>  	dphy = devm_kzalloc(&pdev->dev, sizeof(*dphy), GFP_KERNEL);
>  	if (!dphy)
> @@ -406,7 +407,9 @@ static int stf_dphy_probe(struct platform_device *pdev)
>  	if (IS_ERR(dphy->topsys))
>  		return PTR_ERR(dphy->topsys);
>  
> -	pm_runtime_enable(&pdev->dev);
> +	ret = devm_pm_runtime_enable(&pdev->dev);
> +	if (ret)
> +		return ret;

[Severity: High]
This isn't a bug introduced by this patch, but the companion DPHY RX driver
(drivers/phy/starfive/phy-jh7110-dphy-rx.c) has the exact same bug in
stf_dphy_probe():

	pm_runtime_enable(&pdev->dev);

	phy_set_drvdata(dphy->phy, dphy);
	phy_provider = devm_of_phy_provider_register(&pdev->dev,
						     of_phy_simple_xlate);

	return PTR_ERR_OR_ZERO(phy_provider);

If probe fails or the driver unbinds, pm_runtime_disable() is missing,
leading to unbalanced PM usage counts. Should a similar fix be applied there?

[Severity: Medium]
This is a pre-existing issue, but does stf_dphy_init() leak the prepared
clock if reset_control_deassert() fails?

stf_dphy_init() in drivers/phy/starfive/phy-jh7110-dphy-tx.c has:

	ret = clk_prepare_enable(dphy->txesc_clk);
	if (ret) {
		dev_err(dphy->dev, "Failed to prepare/enable txesc_clk\n");
		return ret;
	}

	ret = reset_control_deassert(dphy->sys_rst);
	if (ret) {
		dev_err(dphy->dev, "Failed to deassert sys_rst\n");
		return ret;
	}

If reset_control_deassert() fails, the function returns without calling
clk_disable_unprepare(dphy->txesc_clk).

Similarly, in stf_dphy_exit(), if reset_control_assert() fails, it returns
early and skips clk_disable_unprepare():

	ret = reset_control_assert(dphy->sys_rst);
	if (ret) {
		dev_err(dphy->dev, "Failed to assert sys_rst\n");
		return ret;
	}

	clk_disable_unprepare(dphy->txesc_clk);

Could these failure paths lock the clock in an enabled state?

>  
>  	dphy->txesc_clk = devm_clk_get(&pdev->dev, "txesc");
>  	if (IS_ERR(dphy->txesc_clk))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718071733.436927-1-pengcan@kylinos.cn?part=1

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