Re: [PATCH] phy: exynos5-usbdrd: fix HS phy init count imbalance on Exynos2200

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] phy: exynos5-usbdrd: init_count imbalance in
  exynos2200_usbdrd_phy_exit()
- [Medium] phy: exynos5-usbdrd: unchecked phy_init() in
  exynos2200_usbdrd_utmi_init()
--

commit 7267261ad8128548bbc38d39a400e9086f676fce
Author: Soheyb <soheyb_boulaa@proton.me>

phy: exynos5-usbdrd: fix HS phy init count imbalance on Exynos2200

This commit calls phy_exit() on phy_drd->hs_phy when exiting the UTMI PHY
in exynos2200_usbdrd_phy_exit() so that the high-speed PHY init count returns
to zero and the PHY is re-initialized on resume.

> diff --git a/drivers/phy/samsung/phy-exynos5-usbdrd.c b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> index 087d552eb5119..0bee92844245d 100644
> --- a/drivers/phy/samsung/phy-exynos5-usbdrd.c
> +++ b/drivers/phy/samsung/phy-exynos5-usbdrd.c
> @@ -1486,6 +1486,13 @@ static int exynos2200_usbdrd_phy_exit(struct phy *phy)
>  	u32 reg;
>  	int ret;
>  
> +	/*
> +	 * The UTMI init initializes the high-speed phy; exit it in turn, so
> +	 * that the next init sets it up again after it lost power.
> +	 */
> +	if (inst->phy_cfg->id == EXYNOS5_DRDPHY_UTMI)
> +		phy_exit(phy_drd->hs_phy);

[Severity: Medium]
Can this underflow phy_drd->hs_phy->init_count to -1 if the earlier
phy_init(phy_drd->hs_phy) failed?

exynos2200_usbdrd_utmi_init() ignores the return value of phy_init():

drivers/phy/samsung/phy-exynos5-usbdrd.c:exynos2200_usbdrd_utmi_init() {
	/* Configure non-Samsung IP PHY, responsible for UTMI */
	phy_init(phy_drd->hs_phy);

	return 0;
}

When phy_init(phy_drd->hs_phy) fails, hs_phy->init_count stays at 0 while
the outer UTMI PHY init succeeds and increments its init_count to 1.
Calling phy_exit() on the outer UTMI PHY then calls
phy_exit(phy_drd->hs_phy) with hs_phy->init_count == 0, which skips
ops->exit() and decrements hs_phy->init_count to -1, preventing future
phy_init(phy_drd->hs_phy) calls from ever running ops->init().

exynos2200_usbdrd_utmi_init() should return the result of
phy_init(phy_drd->hs_phy).

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

[Severity: Medium]
Does calling phy_exit(phy_drd->hs_phy) before clk_bulk_prepare_enable()
and ignoring its return value leave the outer UTMI PHY and phy_drd->hs_phy
init_count values out of sync on error?

If clk_bulk_prepare_enable() fails, phy_drd->hs_phy has already been
exited (hs_phy->init_count == 0), while phy_exit() on the outer UTMI PHY
sees the error from ops->exit() and leaves the outer PHY's init_count at
1.  A later phy_exit() on the outer PHY then decrements hs_phy->init_count
to -1, and a later phy_init() skips exynos2200_usbdrd_phy_init() while
leaving phy_drd->hs_phy uninitialized.  Conversely, if
phy_exit(phy_drd->hs_phy) itself fails, the outer PHY's init_count drops
to 0 while hs_phy->init_count remains 1.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109221522.2796426-1-soheyb_boulaa@proton.me?part=1

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