Re: [PATCH v8 phy-next 12/31] scsi: ufs: qcom: make use of QMP PHY dynamic gear switching ability

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing error check on qmp_ufs_power_on() leads to a system crash on unclocked register access.
- [Medium] Bypassing the PHY framework for power cycling desynchronizes the framework's power state, causing resource double-disables on failure.
--

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

scsi: ufs: qcom: make use of QMP PHY dynamic gear switching ability

This commit absorbs the PHY power cycling and calibration sequence into
qmp_ufs_set_mode() to simplify the consumer driver. It updates both the UFS
HCD consumer and the QMP PHY provider simultaneously to avoid breaking the
power sequence.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> index 771bc7c2ab505..e4e7966eb39a6 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
> @@ -2012,6 +2012,12 @@ static int qmp_ufs_set_mode(struct phy *phy, enum phy_mode mode, int submode)
>  	qmp->mode = mode;
>  	qmp->submode = submode;
>  
> +	if (phy->power_count) {
> +		qmp_ufs_power_off(phy);
> +		qmp_ufs_power_on(phy);

Does the return value of qmp_ufs_power_on() need to be checked here?

If qmp_ufs_power_on() fails, the PHY clocks and regulators will remain
disabled.

Because the error is ignored, execution unconditionally proceeds to
qmp_ufs_phy_calibrate(), which performs read/modify/write operations on the
PHY memory-mapped registers. Accessing peripheral registers while their
clocks are disabled can cause a synchronous external abort and a kernel
panic.

Additionally, does calling the internal power routines directly cause issues
with the PHY framework state tracking?

By calling qmp_ufs_power_off() and qmp_ufs_power_on() instead of the public
phy_power_off() and phy_power_on() APIs, phy->power_count remains unchanged.

If qmp_ufs_power_on() fails leaving the hardware unpowered, the framework
will still believe the PHY is powered on. During error handling or teardown,
the core might call phy_power_off(). Would this result in a second call
to qmp_ufs_power_off() and cause reference count underflow warnings for the
clocks and regulators?

> +		return qmp_ufs_phy_calibrate(phy);
> +	}
> +
>  	return 0;
>  }

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