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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] ufs: sysfs: missing check for pm_runtime_get_sync() failure
- [High] ufs: core: sysfs read breaks SCSI error recovery by setting pm_op_in_progress
- [High] ufs: core: ufshcd_hold() during WLUN suspend clobbers clk_gating.is_suspended
- [Medium] ufs: cleanup: mixing goto and scoped guards violates subsystem rules
--

commit d0c0842fc07bdb2ba7fa8889f82851b6387be9b5
Author: Yiwei Lin <s921975628@gmail.com>
Subject: ufs: sysfs: fix current_power_mode read without SSU

This patch modifies the current_power_mode sysfs attribute to read the
value without fully resuming the WLUN. It uses Hibern8 exit/enter instead
of waking the device to Active mode.

> 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 code need to check the return value of pm_runtime_get_sync()?

If it fails, the controller power domain remains off, but the function ignores
the error, proceeds to call ufshcd_hold(), and directly accesses hardware
registers via ufshcd_uic_hibern8_exit() and ufshcd_query_attr(), which could
cause a system crash (synchronous external abort) due to accessing unpowered
hardware registers.

> +
> +	ufshcd_hold(hba);

[Severity: High]
Can calling ufshcd_hold() without waking the WLUN clobber the clock gating
state?

If clocks are off, this queues ufshcd_ungate_work(). In ufshcd_ungate_work(),
the code unconditionally overwrites hba->clk_gating.is_suspended = false at
the end of its Hibern8 exit block.

Because the WLUN is actually still suspended, this state corruption causes the
subsequent ufshcd_release(hba) to schedule ufshcd_gate_work(), which will then
attempt to issue Hibern8 commands to a link that may be in DeepSleep (powered
off), resulting in unrecoverable failures.

> +	scoped_guard(ufshcd_wl_pm, hba) {
> +		if (ufshcd_is_link_off(hba) || ufshcd_is_ufs_dev_poweroff(hba)) {

[Severity: Medium]
Does mixing scoped_guard() with the manual goto out_unlock cleanup below
violate the cleanup subsystem architectural rules? 

The rules state that code should not mix goto-based error handling with
guard()/scoped_guard() declarations in the same function to avoid confusing
ownership semantics.

[Severity: High]
Can holding this scoped_guard() break SCSI error recovery by setting
pm_op_in_progress?

The DEFINE_LOCK_GUARD_1(ufshcd_wl_pm) macro sets
hba->pm_op_in_progress = true. If a concurrent background I/O times out while
this sysfs read holds the guard, ufshcd_eh_timed_out() observes the flag,
bypasses standard aborts/resets to avoid deadlocking with PM, and directly
completes the command. This leaves the hardware permanently hung since no
actual PM recovery operation is taking place.

> +			/*
> +			 * Device is powered down (rpm_lvl 4/5) or link is off
> +			 * (rpm_lvl 5/6): nothing to query until a full resume.
> +			 */
> +			ret = -EINVAL;
> +			break;
> +		}
[ ... ]
> +		if (exited_h8) {
> +			int h8_ret = ufshcd_uic_hibern8_enter(hba);
> +
> +			if (h8_ret) {
> +				dev_err(hba->dev, "%s: hibern8 enter failed %d\n",
> +					__func__, h8_ret);
> +				ret = h8_ret;
> +			} else {
> +				ufshcd_set_link_hibern8(hba);
> +			}
> +		}
> +	}
> +	ufshcd_release(hba);
> +	pm_runtime_put_sync(hba->dev);
> +
> +	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);
> +
>  	if (pm_op != UFS_SHUTDOWN_PM) {

[Severity: Medium]
Does the use of guard() here mix with the extensive goto state transitions
present later in this function (e.g., goto vops_suspend)? 

The cleanup.md guidelines explicitly prohibit mixing goto-based error handling
with guard() declarations in the same function.

>  		pm_lvl = pm_op == UFS_RUNTIME_PM ?
>  			 hba->rpm_lvl : hba->spm_lvl;
[ ... ]
> @@ -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);
>  
> -	hba->pm_op_in_progress = true;
> +	old_link_state = hba->uic_link_state;

[Severity: Medium]
Does this mix guard() with goto out cleanup jumps in this function?

The rules state that a function should convert all resources to scope-based
cleanup or none of them.

>  
>  	/*
>  	 * Call vendor specific resume callback. As these callbacks may access

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