Re: [PATCH v2 1/2] ufs: always notify POST_CHANGE even on gear switch failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Passing `NULL` for `dev_req_params` on failure causes spurious error logs in unmodified vendor drivers (`ufs-qcom`, `ufs-hisi`) and destroys the context needed for proper resource rollback.
--

commit e9e864935f979762e71567d19e93d8a30870f23b
Author: Peter Wang <peter.wang@mediatek.com>

ufs: always notify POST_CHANGE even on gear switch failure

This commit modifies ufshcd_change_power_mode() to always notify POST_CHANGE,
even if the gear switch fails. On failure, it passes NULL for dev_req_params
and updates ufs-exynos and ufshcd-pci to handle the NULL argument.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 234e18b5078f2..2e7a592dbf2f1 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5003,8 +5003,12 @@ int ufshcd_change_power_mode(struct ufs_hba *hba,
>  
>  	ret = ufshcd_dme_change_power_mode(hba, pwr_mode, pmc_policy);
>  
> -	if (!ret)
> -		ufshcd_vops_pwr_change_notify(hba, POST_CHANGE, pwr_mode);
> +	/*
> +	 * Always notify POST_CHANGE to pair with PRE_CHANGE, even on failure.
> +	 * On failure, pass dev_req_params = NULL to indicate nothing to apply.
> +	 * POST_CHANGE return value is advisory and does not mask @ret.
> +	 */
> +	ufshcd_vops_pwr_change_notify(hba, POST_CHANGE, ret ? NULL : pwr_mode);

[Severity: Medium]
Will passing NULL for dev_req_params on failure cause issues for unmodified
vendor drivers?

Looking at ufs_qcom_pwr_change_notify() in drivers/ufs/host/ufs-qcom.c, it
has an early check that will trap this NULL argument and log a spurious error
before any POST_CHANGE cleanup can execute:

drivers/ufs/host/ufs-qcom.c:ufs_qcom_pwr_change_notify() {
	if (!dev_req_params) {
		pr_err("%s: incoming dev_req_params is NULL\n", __func__);
		return -EINVAL;
	}
	...
}

Similarly, ufs_hisi_pwr_change_notify() in drivers/ufs/host/ufs-hisi.c will
also log a spurious error on every power mode change failure:

drivers/ufs/host/ufs-hisi.c:ufs_hisi_pwr_change_notify() {
	if (!dev_req_params) {
		dev_err(hba->dev,
			    "%s: incoming dev_req_params is NULL\n", __func__);
		ret = -EINVAL;
		goto out;
	}
	...
}

Additionally, does passing NULL here destroy the context needed for proper
resource rollback?

For instance, in ufs_qcom_pwr_change_notify(), resources are acquired in
PRE_CHANGE based on the dev_req_params:

drivers/ufs/host/ufs-qcom.c:ufs_qcom_pwr_change_notify() {
	...
		/* enable the device ref clock before changing to HS mode */
		if (!ufshcd_is_hs_mode(&hba->pwr_info) &&
			ufshcd_is_hs_mode(dev_req_params))
			ufs_qcom_dev_ref_clk_ctrl(host, true);
	...
}

By omitting the original parameters on failure, the target drivers are prevented
from using the requested mode context in POST_CHANGE to safely revert their
state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914085148.136429-1-peter.wang@mediatek.com?part=1