Re: [PATCH v8 phy-next 10/31] scsi: ufs: exynos: use dedicated API for updating PHY bus width

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] In `samsung_ufs_phy_request_bus_width()`, if `samsung_ufs_phy_power_on()` fails, the PHY clocks are left disabled but the core `phy->power_count` remains elevated, causing unbalanced clock disables on teardown.
- [High] Early return on `phy_request_bus_width()` failure skips PHY power-on, causing fatal hardware bus errors when accessing unpowered PHY registers.
--

commit 69b368f4a4a2db532d3c3f014c5fffd55e2658d0
Author: Vladimir Oltean <vladimir.oltean@nxp.com>

scsi: ufs: exynos: use dedicated API for updating PHY bus width

This commit introduces a new dedicated phy_request_bus_width() API to the 
PHY core and updates the Exynos UFS driver to use it instead of hijacking
phy_set_bus_width() and phy_init(). This update allows the UFS HCD driver
to update the lane count cleanly without altering the PHY initialization and
power states unexpectedly.

> diff --git a/drivers/phy/samsung/phy-samsung-ufs.c b/drivers/phy/samsung/phy-samsung-ufs.c
> index ee665f26c2361..b09a35ab6acde 100644
> --- a/drivers/phy/samsung/phy-samsung-ufs.c
> +++ b/drivers/phy/samsung/phy-samsung-ufs.c
[ ... ]
> @@ -204,6 +203,31 @@ static int samsung_ufs_phy_power_off(struct phy *phy)
>  	return 0;
>  }
>  
> +static int samsung_ufs_phy_request_bus_width(struct phy *phy, int bus_width)
> +{
> +	struct samsung_ufs_phy *ss_phy = get_samsung_ufs_phy(phy);
> +	u8 old_lane_cnt = ss_phy->lane_cnt;
> +	int err = 0;
> +
> +	if (bus_width != 1 && bus_width != 2)
> +		return -EINVAL;
> +
> +	ss_phy->lane_cnt = bus_width;
> +
> +	if (phy->init_count)
> +		samsung_ufs_phy_init(phy);
> +
> +	/* If the init_count is 0, the power_count should also be 0 */
> +	if (phy->power_count) {
> +		samsung_ufs_phy_power_off(phy);
> +		err = samsung_ufs_phy_power_on(phy);

If samsung_ufs_phy_power_on() fails here, the PHY clocks are left disabled but
the core phy->power_count remains elevated because this cycle bypasses the core
PHY API.

When the UFS driver later cleans up and calls phy_power_off(), the PHY core
invokes samsung_ufs_phy_power_off() a second time.

Will this second call to samsung_ufs_phy_power_off() attempt to disable already
disabled clocks and cause a clock enable count underflow warning?

> +		if (err)
> +			ss_phy->lane_cnt = old_lane_cnt;
> +	}
> +
> +	return err;
> +}
[ ... ]
> diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
> index 77a6c8e44485b..b90876b268db7 100644
> --- a/drivers/ufs/host/ufs-exynos.c
> +++ b/drivers/ufs/host/ufs-exynos.c
[ ... ]
> @@ -947,30 +1009,11 @@ static int exynos_ufs_phy_init(struct exynos_ufs *ufs)
>  			ufs->avail_ln_rx, ufs->avail_ln_tx);
>  	}
>  
> -	phy_set_bus_width(generic_phy, ufs->avail_ln_rx);
> -
> -	if (generic_phy->power_count) {
> -		phy_power_off(generic_phy);
> -		phy_exit(generic_phy);
> -	}
> -
> -	ret = phy_init(generic_phy);
> -	if (ret) {
> -		dev_err(hba->dev, "%s: phy init failed, ret = %d\n",
> -			__func__, ret);
> -		return ret;
> -	}
> -
> -	ret = phy_power_on(generic_phy);
> +	ret = phy_request_bus_width(generic_phy, ufs->avail_ln_rx);
>  	if (ret)
> -		goto out_exit_phy;
> -
> -	return 0;
> -
> -out_exit_phy:
> -	phy_exit(generic_phy);
> +		return ret;
>  
> -	return ret;
> +	return exynos_ufs_phy_power_on(ufs);
>  }

If phy_request_bus_width() fails and returns an error (for example, -EINVAL
if ufs->avail_ln_rx is 0), this code will return early and skip the call to
exynos_ufs_phy_power_on().

However, looking at exynos_ufs_pre_link() which calls this function:

exynos_ufs_pre_link() {
...
	/* m-phy */
	exynos_ufs_phy_update_bus_width(ufs);
	if (!(ufs->opts & EXYNOS_UFS_OPT_SKIP_CONFIG_PHY_ATTR)) {
		exynos_ufs_config_phy_time_attr(ufs);
		exynos_ufs_config_phy_cap_attr(ufs);
	}
...
}

The return value of exynos_ufs_phy_update_bus_width() is ignored, and
exynos_ufs_pre_link() proceeds unconditionally to access PHY attributes via
exynos_ufs_config_phy_time_attr() and exynos_ufs_config_phy_cap_attr().

Can accessing these registers while the PHY power domains and clocks are off
cause unhandled faults, bus errors, or hardware hangs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260505100523.1922388-1-vladimir.oltean@nxp.com?part=10