Re: [PATCH] ufs: rockchip: propagate controller reset errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Propagated reset errors during HCE enable are ignored by the UFS core, defeating the patch's stated goal.

Pre-existing issues:
- [High] Clock enable count leaks when the UFS core resume functions fail in runtime or system resume.
- [High] Premature clock disable in ufs_rockchip_runtime_suspend() leads to unsafe hardware access and a state mismatch on failure.
--

commit 5aa3f0967e7ed951d7f28ee06f36a7216652dda8
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

ufs: rockchip: propagate controller reset errors

This commit modifies the Rockchip UFS driver to propagate reset controller
errors instead of discarding them. It updates initialization, HCE enable,
and runtime resume paths to return these errors, and it unwinds the
reference clock on failure in the resume path.

> diff --git a/drivers/ufs/host/ufs-rockchip.c b/drivers/ufs/host/ufs-rockchip.c
> index bac68f238e1c4..3b9661befe6ae 100644
> --- a/drivers/ufs/host/ufs-rockchip.c
> +++ b/drivers/ufs/host/ufs-rockchip.c
[ ... ]
> @@ -46,9 +51,7 @@ static int ufs_rockchip_hce_enable_notify(struct ufs_hba *hba,
>  	}
>  
>  	/* PRE_CHANGE */
> -	ufs_rockchip_controller_reset(host);
> -
> -	return 0;
> +	return ufs_rockchip_controller_reset(host);
>  }
>  

[Severity: Medium]
Will this propagated error actually be handled by the core?

Looking at the caller in the UFS core, ufshcd_hba_execute_hce() appears to
unconditionally ignore the return value of the PRE_CHANGE notification and
continues the HCE enable sequence regardless:

drivers/ufs/core/ufshcd.c:ufshcd_hba_execute_hce() {
	...
	ufshcd_vops_hce_enable_notify(hba, PRE_CHANGE);
	
	/* start controller initialization sequence */
	ufshcd_hba_start(hba);
	...
}

Could this lead to the driver continuing initialization despite the reset
failure, defeating the purpose of propagating this error?

[ ... ]
> @@ -291,7 +296,11 @@ static int ufs_rockchip_runtime_resume(struct device *dev)
>  		return err;
>  	}
>  
> -	ufs_rockchip_controller_reset(host);
> +	err = ufs_rockchip_controller_reset(host);
> +	if (err) {
> +		clk_disable_unprepare(host->ref_out_clk);
> +		return err;
> +	}
>  
>  	return ufshcd_runtime_resume(dev);
>  }

[Severity: High]
This is a pre-existing issue, but does this function leak the clock enable
count if the core resume fails?

When ufshcd_runtime_resume(dev) is called, its return value is passed
verbatim to the caller. If it returns an error, the function exits without
unwinding the host->ref_out_clk that was enabled earlier in this function.
This could result in an unbounded clock reference count leak on repeated
runtime resume failures, preventing power domains from turning off.

[Severity: High]
This is also a pre-existing issue, but looking at the counterpart function
ufs_rockchip_runtime_suspend() in this same file, is the reference clock
disabled prematurely?

drivers/ufs/host/ufs-rockchip.c:ufs_rockchip_runtime_suspend() {
	...
	clk_disable_unprepare(host->ref_out_clk);

	/* Do not power down the genpd if rpm_lvl is less than level 5 */
	dev_pm_genpd_rpm_always_on(dev, hba->rpm_lvl < UFS_PM_LVL_5);

	return ufshcd_runtime_suspend(dev);
}

Disabling the reference clock before calling ufshcd_runtime_suspend(dev)
might cause bus timeouts and abort the suspend process if the UFS core
attempts to access the hardware. If ufshcd_runtime_suspend() fails, the PM
core considers the suspend aborted, but the physical clock remains disabled,
which could leave the hardware in an unrecoverable state for subsequent
accesses.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830132723.5855-1-pengpeng@iscas.ac.cn?part=1