Re: [PATCH] phy: samsung: usbdrd: propagate GS101 PIPE3 lock failures

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Resource leak (regulators and PHY isolation state) in error path of exynos2200_usbdrd_phy_init.
--

commit 20eac936223675450b3e9e458566653299d0b09b
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
Subject: phy: samsung: usbdrd: propagate GS101 PIPE3 lock failures

This commit updates the SoC-specific phy_init hooks to return an integer
error code instead of void. This allows helpers with real failure results,
such as the GS101 PIPE3 PLL or CDR locks failing, to propagate those errors
through the existing PHY initialization paths.

> diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> index 5a181cb4597e9..513a8f9c25e9a 100644
> --- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
> +++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c

[ ... ]

> @@ -1458,11 +1471,11 @@ static int exynos2200_usbdrd_phy_init(struct phy *phy)
>  	exynos2200_usbdrd_link_attach_detach_pipe3_phy(inst);
>  
>  	/* UTMI or PIPE3 specific init */
> -	inst->phy_cfg->phy_init(phy_drd);
> +	ret = inst->phy_cfg->phy_init(phy_drd);
>  
>  	clk_bulk_disable_unprepare(phy_drd->drv_data->n_clks, phy_drd->clks);
>  
> -	return 0;
> +	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but while reviewing the error path updates in
exynos2200_usbdrd_phy_init(), I noticed a potential resource leak earlier
in the same function.

If clk_bulk_prepare_enable() fails, the function returns immediately:

	if (inst->phy_cfg->id == EXYNOS5_DRDPHY_UTMI) {
		ret = regulator_bulk_enable(phy_drd->drv_data->n_regulators,
					    phy_drd->regulators);
		if (ret) {
			...
			return ret;
		}
	}

	exynos5_usbdrd_phy_isol(inst, false);

	ret = clk_bulk_prepare_enable(phy_drd->drv_data->n_clks, phy_drd->clks);
	if (ret)
		return ret;

Does this skip tearing down the regulators (enabled for EXYNOS5_DRDPHY_UTMI)
and leave the PHY un-isolated, considering the generic PHY framework does
not call .exit() when .init() fails?

Would it be appropriate to add an error label here to disable the regulators
and re-assert isolation via exynos5_usbdrd_phy_isol(inst, true), similar to
the cleanup path recently added for exynos5_usbdrd_gs101_phy_init()?

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

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