Re: [PATCH v8 phy-next 13/31] scsi: ufs: qcom: keep separate track of PHY power state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Failing to clear `phy_powered_on` upon a `phy_power_off()` error leads to calling `phy_power_off()` on an exited PHY during driver removal.
- [High] Silently ignoring `phy_power_off()` errors alters error propagation and can lead to system hangs.
--

commit 22afdb4c537193d799444f149b239ffbc8dfa455
Author: Vladimir Oltean <vladimir.oltean@nxp.com>

scsi: ufs: qcom: keep separate track of PHY power state

This commit adds a phy_powered_on boolean to the Qualcomm UFS driver to
track the PHY power state. It wraps phy_power_on() and phy_power_off()
in helper functions that check and update this state to prevent unbalanced
calls.

> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c

[ ... ]

> +static void ufs_qcom_phy_power_off(struct ufs_qcom_host *host)
> +{
> +	int ret;
> +
> +	if (!host->phy_powered_on)
> +		return;
> +
> +	ret = phy_power_off(host->generic_phy);
> +	if (ret) {
> +		dev_warn(host->hba->dev, "Failed to power off PHY: %pe\n",
> +			 ERR_PTR(ret));
> +		return;
> +	}
> +
> +	host->phy_powered_on = false;
> +}

If phy_power_off() returns an error, the early return leaves
host->phy_powered_on set to true.

During driver removal, ufshcd_hba_exit() invokes ufs_qcom_exit(), which
calls ufs_qcom_phy_power_off() followed by phy_exit(). If the power off
fails here, phy_powered_on remains true.

When ufshcd_hba_exit() subsequently calls ufshcd_setup_clocks(hba, false),
it will invoke ufs_qcom_phy_power_off() again. Because the flag is still
true, could this result in calling phy_power_off() on an already exited PHY?

[ ... ]

> @@ -1420,20 +1453,14 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
>  				ufs_qcom_dev_ref_clk_ctrl(host, false);
>  			}
>  
> -			err = phy_power_off(phy);
> -			if (err) {
> -				dev_err(hba->dev, "phy power off failed, ret=%d\n", err);
> -				return err;
> -			}
> +			ufs_qcom_phy_power_off(host);
>  		}
>  		break;

Because ufs_qcom_phy_power_off() returns void, it swallows the error from
phy_power_off(). Previously, this error was propagated back to the UFS core,
which correctly aborted the suspend or teardown flow.

By no longer returning the error, ufs_qcom_setup_clocks() silently returns 0.
The UFS core will then proceed to disable the clocks while the PHY is still
active and powered on.

Would this early disabling of clocks lead to unclocked register accesses
and asynchronous aborts causing system hangs?

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