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

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

[Yiwei Lin]

From: robelin <robelin@nvidia.com>

current_power_mode was generated by UFS_ATTRIBUTE(), which calls
ufshcd_rpm_get_sync() before issuing the query.  That resumes the UFS
device WLUN, and ufshcd_wl_runtime_resume() sends START STOP UNIT to
bring the device back to Active.  As a result bCurrentPowerMode always
read back as 0x11 (Active) no matter what state the device was actually
in: the act of reading the attribute woke the device, so Sleep could
never be observed through sysfs.

Per the UFS spec, bCurrentPowerMode is the one attribute the device
must answer in any power mode, so there is no need to wake it.  Give
current_power_mode its own show function that deliberately does not
resume the WLUN.  It only resumes the host controller then reads the
attribute over whatever link state the WLUN suspend left behind:

 - link active: query directly.
 - link in Hibern8: exit Hibern8, query, then re-enter Hibern8 so the
   link is left exactly as rpm_lvl requested.
 - device powered down or link off (rpm_lvl 4/5/6): fail, the device
   cannot answer without a full resume.

Tested on hardware: with the device runtime-suspended (wb_on=0, so the
device actually reaches Sleep instead of being held Active for a WB
flush), reading current_power_mode used to return 0x11; after this
change it returns 0x22 (Sleep), matching the device's real state.

Assisted-by: LLM
Signed-off-by: Yiwei Lin <s921975628@gmail.com>

---
Change-Logs in V2:
- Synchonize the race between WLUN suspend/resume and other contexts
  that change the link power state.
- Restore UFS link if exiting Hibern8 for the query, then re-enter Hibern8 so the link   is left exactly as rpm_lvl requested.

---
 drivers/ufs/core/ufs-sysfs.c   | 74 +++++++++++++++++++++++++++++++++-
 drivers/ufs/core/ufshcd-priv.h | 11 +++++
 drivers/ufs/core/ufshcd.c      | 12 +++---
 include/ufs/ufshcd.h           |  3 ++
 4 files changed, 94 insertions(+), 6 deletions(-)

diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 63e670d1a9d9e..a500ca697e5d3 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);
+
+	ufshcd_hold(hba);
+	scoped_guard(ufshcd_wl_pm, hba) {
+		if (ufshcd_is_link_off(hba) || ufshcd_is_ufs_dev_poweroff(hba)) {
+			/*
+			 * 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 (ufshcd_is_link_hibern8(hba)) {
+			ret = ufshcd_uic_hibern8_exit(hba);
+			if (ret) {
+				dev_err(hba->dev, "%s: hibern8 exit failed %d\n",
+					__func__, ret);
+				break;
+			}
+			ufshcd_set_link_active(hba);
+			exited_h8 = true;
+		}
+
+		ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+			QUERY_ATTR_IDN_POWER_MODE, 0, 0, &value);
+
+		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);
+
 UFS_ATTRIBUTE(active_icc_level, _ACTIVE_ICC_LVL);
 UFS_ATTRIBUTE(ooo_data_enabled, _OOO_DATA_EN);
 UFS_ATTRIBUTE(bkops_status, _BKOPS_STATUS);
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index e55c2a02c1f50..df61074843f06 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -3,6 +3,7 @@
 #ifndef _UFSHCD_PRIV_H_
 #define _UFSHCD_PRIV_H_
 
+#include <linux/cleanup.h>
 #include <linux/pm_runtime.h>
 #include <ufs/ufshcd.h>
 
@@ -15,6 +16,16 @@ static inline bool ufshcd_is_user_access_allowed(struct ufs_hba *hba)
 
 void ufshcd_schedule_eh_work(struct ufs_hba *hba);
 
+DEFINE_LOCK_GUARD_1(ufshcd_wl_pm, struct ufs_hba,
+	({
+		mutex_lock(&_T->lock->wl_pm_mutex);
+		_T->lock->pm_op_in_progress = true;
+	}),
+	({
+		_T->lock->pm_op_in_progress = false;
+		mutex_unlock(&_T->lock->wl_pm_mutex);
+	}))
+
 static inline bool ufshcd_keep_autobkops_enabled_except_suspend(
 							struct ufs_hba *hba)
 {
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 2ba244cf40ac7..7431c0ae9ce74 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10294,7 +10294,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) {
 		pm_lvl = pm_op == UFS_RUNTIME_PM ?
 			 hba->rpm_lvl : hba->spm_lvl;
@@ -10467,7 +10468,6 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 		hba->clk_gating.is_suspended = false;
 		ufshcd_release(hba);
 	}
-	hba->pm_op_in_progress = false;
 	return ret;
 }
 
@@ -10475,9 +10475,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;
 
 	/*
 	 * Call vendor specific resume callback. As these callbacks may access
@@ -10568,7 +10570,6 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 		ufshcd_update_evt_hist(hba, UFS_EVT_WL_RES_ERR, (u32)ret);
 	hba->clk_gating.is_suspended = false;
 	ufshcd_release(hba);
-	hba->pm_op_in_progress = false;
 	return ret;
 }
 
@@ -11256,6 +11257,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	INIT_WORK(&hba->eeh_work, ufshcd_exception_event_handler);
 
 	sema_init(&hba->host_sem, 1);
+	mutex_init(&hba->wl_pm_mutex);
 
 	/* Initialize UIC command mutex */
 	mutex_init(&hba->uic_cmd_mutex);
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index dfd302f2dc7c1..ac7da4bd260cb 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1010,6 +1010,8 @@ enum ufshcd_mcq_opr {
  * @is_powered: flag to check if HBA is powered
  * @shutting_down: flag to check if shutdown has been invoked
  * @host_sem: semaphore used to serialize concurrent contexts
+ * @wl_pm_mutex: serializes WLUN suspend/resume against other contexts that
+ *	change the link power state while the WLUN may be suspended
  * @eh_wq: Workqueue that eh_work works on
  * @eh_work: Worker to handle UFS errors that require s/w attention
  * @eeh_work: Worker to handle exception events
@@ -1176,6 +1178,7 @@ struct ufs_hba {
 	bool is_powered;
 	bool shutting_down;
 	struct semaphore host_sem;
+	struct mutex wl_pm_mutex;
 
 	/* Work Queues */
 	struct workqueue_struct *eh_wq;
-- 
2.34.1

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

[sashiko-bot]

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

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

[Yiwei Lin]

From: robelin <robelin@nvidia.com>

current_power_mode was generated by UFS_ATTRIBUTE(), which calls
ufshcd_rpm_get_sync() before issuing the query.  That resumes the UFS
device WLUN, and ufshcd_wl_runtime_resume() sends START STOP UNIT to
bring the device back to Active.  As a result bCurrentPowerMode always
read back as 0x11 (Active) no matter what state the device was actually
in: the act of reading the attribute woke the device, so Sleep could
never be observed through sysfs.

Per the UFS spec, bCurrentPowerMode is the one attribute the device
must answer in any power mode, so there is no need to wake it.  Give
current_power_mode its own show function that deliberately does not
resume the WLUN.  It only resumes the host controller then reads the
attribute over whatever link state the WLUN suspend left behind:

 - link active: query directly.
 - link in Hibern8: exit Hibern8, query, then re-enter Hibern8 so the
   link is left exactly as rpm_lvl requested.
 - device powered down or link off (rpm_lvl 4/5/6): fail, the device
   cannot answer without a full resume.

Tested on hardware: with the device runtime-suspended (wb_on=0, so the
device actually reaches Sleep instead of being held Active for a WB
flush), reading current_power_mode used to return 0x11; after this
change it returns 0x22 (Sleep), matching the device's real state.

Assisted-by: LLM
Signed-off-by: Yiwei Lin <s921975628@gmail.com>

---
Change-Logs in V2:
- Synchonize the race between WLUN suspend/resume and other contexts
  that change the link power state.
- Restore UFS link if exiting Hibern8 for the query, then re-enter Hibern8 so the link   is left exactly as rpm_lvl requested.

---
 drivers/ufs/core/ufs-sysfs.c   | 74 +++++++++++++++++++++++++++++++++-
 drivers/ufs/core/ufshcd-priv.h | 11 +++++
 drivers/ufs/core/ufshcd.c      | 12 +++---
 include/ufs/ufshcd.h           |  3 ++
 4 files changed, 94 insertions(+), 6 deletions(-)

diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 63e670d1a9d9e..a500ca697e5d3 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);
+
+	ufshcd_hold(hba);
+	scoped_guard(ufshcd_wl_pm, hba) {
+		if (ufshcd_is_link_off(hba) || ufshcd_is_ufs_dev_poweroff(hba)) {
+			/*
+			 * 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 (ufshcd_is_link_hibern8(hba)) {
+			ret = ufshcd_uic_hibern8_exit(hba);
+			if (ret) {
+				dev_err(hba->dev, "%s: hibern8 exit failed %d\n",
+					__func__, ret);
+				break;
+			}
+			ufshcd_set_link_active(hba);
+			exited_h8 = true;
+		}
+
+		ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+			QUERY_ATTR_IDN_POWER_MODE, 0, 0, &value);
+
+		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);
+
 UFS_ATTRIBUTE(active_icc_level, _ACTIVE_ICC_LVL);
 UFS_ATTRIBUTE(ooo_data_enabled, _OOO_DATA_EN);
 UFS_ATTRIBUTE(bkops_status, _BKOPS_STATUS);
diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index e55c2a02c1f50..df61074843f06 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -3,6 +3,7 @@
 #ifndef _UFSHCD_PRIV_H_
 #define _UFSHCD_PRIV_H_
 
+#include <linux/cleanup.h>
 #include <linux/pm_runtime.h>
 #include <ufs/ufshcd.h>
 
@@ -15,6 +16,16 @@ static inline bool ufshcd_is_user_access_allowed(struct ufs_hba *hba)
 
 void ufshcd_schedule_eh_work(struct ufs_hba *hba);
 
+DEFINE_LOCK_GUARD_1(ufshcd_wl_pm, struct ufs_hba,
+	({
+		mutex_lock(&_T->lock->wl_pm_mutex);
+		_T->lock->pm_op_in_progress = true;
+	}),
+	({
+		_T->lock->pm_op_in_progress = false;
+		mutex_unlock(&_T->lock->wl_pm_mutex);
+	}))
+
 static inline bool ufshcd_keep_autobkops_enabled_except_suspend(
 							struct ufs_hba *hba)
 {
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 2ba244cf40ac7..7431c0ae9ce74 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10294,7 +10294,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) {
 		pm_lvl = pm_op == UFS_RUNTIME_PM ?
 			 hba->rpm_lvl : hba->spm_lvl;
@@ -10467,7 +10468,6 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 		hba->clk_gating.is_suspended = false;
 		ufshcd_release(hba);
 	}
-	hba->pm_op_in_progress = false;
 	return ret;
 }
 
@@ -10475,9 +10475,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;
 
 	/*
 	 * Call vendor specific resume callback. As these callbacks may access
@@ -10568,7 +10570,6 @@ static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 		ufshcd_update_evt_hist(hba, UFS_EVT_WL_RES_ERR, (u32)ret);
 	hba->clk_gating.is_suspended = false;
 	ufshcd_release(hba);
-	hba->pm_op_in_progress = false;
 	return ret;
 }
 
@@ -11256,6 +11257,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	INIT_WORK(&hba->eeh_work, ufshcd_exception_event_handler);
 
 	sema_init(&hba->host_sem, 1);
+	mutex_init(&hba->wl_pm_mutex);
 
 	/* Initialize UIC command mutex */
 	mutex_init(&hba->uic_cmd_mutex);
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index dfd302f2dc7c1..ac7da4bd260cb 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1010,6 +1010,8 @@ enum ufshcd_mcq_opr {
  * @is_powered: flag to check if HBA is powered
  * @shutting_down: flag to check if shutdown has been invoked
  * @host_sem: semaphore used to serialize concurrent contexts
+ * @wl_pm_mutex: serializes WLUN suspend/resume against other contexts that
+ *	change the link power state while the WLUN may be suspended
  * @eh_wq: Workqueue that eh_work works on
  * @eh_work: Worker to handle UFS errors that require s/w attention
  * @eeh_work: Worker to handle exception events
@@ -1176,6 +1178,7 @@ struct ufs_hba {
 	bool is_powered;
 	bool shutting_down;
 	struct semaphore host_sem;
+	struct mutex wl_pm_mutex;
 
 	/* Work Queues */
 	struct workqueue_struct *eh_wq;
-- 
2.34.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ufs: missing error recovery when hibern8_enter fails leaves link active
- [Low] cleanup: mixing goto-based error handling and scope-based cleanup
--

commit 7f21a836a7974ab0a73d76e4107550f8d10d5c90
Author: robelin <robelin@nvidia.com>

ufs: sysfs: fix current_power_mode read without SSU

This commit introduces a dedicated sysfs show function for current_power_mode
to query the power mode attribute without fully resuming the UFS device to
the Active state. It handles exiting and re-entering the Hibern8 state
directly to avoid sending START STOP UNIT commands that would wake the device.

> 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);
> +
> +	ufshcd_hold(hba);
> +	scoped_guard(ufshcd_wl_pm, hba) {

[Severity: Low]
Is it intentional to mix goto-based error handling (like the goto out_unlock
above) and scope-based cleanup helpers (scoped_guard) in the same function?

The subsystem guidelines for cleanup functions recommend either converting all
resources that need a cleanup to scope-based cleanup, or converting none of
them, to avoid confusing ownership semantics.

> +		if (ufshcd_is_link_off(hba) || ufshcd_is_ufs_dev_poweroff(hba)) {
> +			/*
> +			 * 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 (ufshcd_is_link_hibern8(hba)) {
> +			ret = ufshcd_uic_hibern8_exit(hba);
> +			if (ret) {
> +				dev_err(hba->dev, "%s: hibern8 exit failed %d\n",
> +					__func__, ret);
> +				break;
> +			}
> +			ufshcd_set_link_active(hba);
> +			exited_h8 = true;
> +		}
> +
> +		ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +			QUERY_ATTR_IDN_POWER_MODE, 0, 0, &value);
> +
> +		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);
> +			}

[Severity: High]
Does this code leave the UFS link in UIC_LINK_ACTIVE_STATE if
ufshcd_uic_hibern8_enter() fails?

If h8_ret is an error, this path returns the error without restoring the
link state or triggering error recovery. Since the WLUN remains suspended,
this can lead to the host controller runtime suspending and blindly turning off
the host clocks while the UFS link is still active. This violates hardware
constraints and guarantees a 10s command timeout on the next I/O.

> +		}
> +	}
> +	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);
> +

[Severity: Low]
Does this introduce scope-based cleanup into __ufshcd_wl_suspend(), a function
that heavily relies on goto labels for error handling?

__ufshcd_wl_suspend() has multiple error paths using goto vops_suspend,
goto enable_scaling, and goto out. Mixing goto-based error handling and
scope-based cleanup helpers in the same function can lead to confusing
ownership semantics and is generally discouraged by the subsystem guidelines.

>  	if (pm_op != UFS_SHUTDOWN_PM) {
>  		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: Low]
Similar to __ufshcd_wl_suspend(), does this mix scope-based cleanup with
existing goto-based error jumps in __ufshcd_wl_resume()?

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

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