Re: [PATCH v2] ufs: sysfs: fix current_power_mode read without SSU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pm: unchecked pm_runtime_get_sync() return value leads to hardware access crash
- [Medium] cleanup: mixing goto-based cleanup with guard()/scoped_guard() in the same function
--

commit 5c7e45d8ae7aae3212b7ac69373b91780e44ec84
Author: robelin <robelin@nvidia.com>

ufs: sysfs: fix current_power_mode read without SSU

This commit gives current_power_mode its own show function that deliberately
does not resume the WLUN. It also introduces WLUN suspend/resume
synchronization against contexts changing the link power state.

> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
> index cf49ad0258e14..2d1ed2ca0dfa4 100644
> --- a/drivers/ufs/core/ufs-sysfs.c
> +++ b/drivers/ufs/core/ufs-sysfs.c
[ ... ]
> @@ -1786,7 +1786,79 @@ out:									\
>  static DEVICE_ATTR_RO(_name)
>  
>  UFS_ATTRIBUTE(boot_lun_enabled, _BOOT_LU_EN);
> -UFS_ATTRIBUTE(current_power_mode, _POWER_MODE);
> +
> +/*
> + * Per UFS spec, bCurrentPowerMode is the only attribute the device must
> + * respond to in any power mode.  Read it without waking the device to
> + * Active.
> + */
> +static ssize_t current_power_mode_show(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	bool exited_h8 = false;
> +	u32 value;
> +	int ret = 0;
> +
> +	down(&hba->host_sem);
> +	if (!ufshcd_is_user_access_allowed(hba)) {
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +
> +	pm_runtime_get_sync(hba->dev);

[Severity: High]
Does this need a return value check? If pm_runtime_get_sync() fails (for
example, due to a power domain or regulator failure), the device and its
clocks might not be enabled. Proceeding to access its memory-mapped
registers via ufshcd_uic_hibern8_exit() or ufshcd_query_attr() below could
trigger a synchronous external abort and crash the system.

> +
> +	ufshcd_hold(hba);
> +	scoped_guard(ufshcd_wl_pm, hba) {

[Severity: Medium]
Is it intended to mix scope-based cleanup with goto-based error handling in
this function? According to subsystem guidelines, functions should either
convert all resources to scope-based cleanup or use none. Mixing them can
create confusing ownership semantics and risk resource leak bugs.

> +		if (ufshcd_is_link_off(hba) || ufshcd_is_ufs_dev_poweroff(hba)) {
[ ... ]
> +	if (!ret)
> +		ret = sysfs_emit(buf, "0x%08X\n", value);
> +	else
> +		ret = -EINVAL;
> +
> +out_unlock:
> +	up(&hba->host_sem);
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(current_power_mode);
[ ... ]
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 234e18b5078f2..462749b850b13 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -10319,7 +10319,8 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  	enum ufs_dev_pwr_mode req_dev_pwr_mode;
>  	enum uic_link_state req_link_state;
>  
> -	hba->pm_op_in_progress = true;
> +	guard(ufshcd_wl_pm)(hba);
> +

[Severity: Medium]
Does introducing guard() here conflict with the cleanup guidelines against
mixing scope-based and goto-based cleanup? This function heavily uses
goto-based error handling, which could create confusing ownership semantics
and risks double-free or resource leak bugs.

>  	if (pm_op != UFS_SHUTDOWN_PM) {
>  		pm_lvl = pm_op == UFS_RUNTIME_PM ?
[ ... ]
> @@ -10500,9 +10500,11 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
>  {
>  	int ret;
> -	enum uic_link_state old_link_state = hba->uic_link_state;
> +	enum uic_link_state old_link_state;
> +
> +	guard(ufshcd_wl_pm)(hba);

[Severity: Medium]
Similarly, does using guard() here conflict with the goto statements and
manual cleanup present in this function?

>  
> -	hba->pm_op_in_progress = true;
> +	old_link_state = hba->uic_link_state;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915145354.8194-1-s921975628@gmail.com?part=1