Skip to contentSkip to conversation

Enable power management for ufs wlun

PATCH Superseded Asutosh Das 2 participants 12 messages Apr 16, 2021 – Apr 23, 2021

Subsystems (5)
SCSI Arm Qualcomm mailing list Arm MediaTek Arm Samsung Exynos +1 more subsystems

v20 of 21 Superseded Latest is v21 →

lore
Share & tools
lore

Asutosh Das

author
#0 lore raw

v20 cover Enable power management for ufs wlun

This patch attempts to fix a deadlock in ufs while sending SSU. Recently, blk_queue_enter() added a check to not process requests if the queue is suspended. That leads to a resume of the associated device which is suspended. In ufs, that device is ufs device wlun and it's parent is ufs_hba. This resume tries to resume ufs device wlun which in turn tries to resume ufs_hba, which is already in the process of suspending, thus causing a deadlock.

This patch takes care of:
* Suspending the ufs device lun only after all other luns are suspended
* Sending SSU during ufs device wlun suspend
* Clearing uac for rpmb and ufs device wlun
* Not sending commands to the device during host suspend

v19 -> v20:
- Addressed Adrian's comments
- Addressed Bart's comments

v18 -> v19:
- Addressed Adrian's comments
- Fixed smatch warnings

v17 -> v18:
- Addressed Adrian's comments

v16 -> v17:
- Addressed Adrian's & Daejun's comments

v15 -> v16:
- Brought back the missing changes
* Added scsi_autopm_[get/put] to ufs_debugfs[get/put]_user_access()
* Fix ufshcd_wl_poweroff()

v14 -> v15:
- Rebased on 5.13/scsi-staging

v13 -> v14:
- Addressed Adrian's comments
* Rebased it on top of scsi-next
* Added scsi_autopm_[get/put] to ufs_debugfs[get/put]_user_access()
* Resume the device in ufshcd_remove()
* Unregister ufs_rpmb_wlun before ufs_dev_wlun
* hba->shutting_down moved to ufshcd_wl_shutdown()

v12 -> v13:
- Addressed Adrian's comments
* Paired pm_runtime_get_noresume() with pm_runtime_put()
* no rpm_autosuspend for ufs device wlun
* Moved runtime-pm init functionality to ufshcd_wl_probe()
- Addressed Bart's comments
* Expanded abbrevs in commit message

v11 -> v12:
- Addressed Adrian's comments
* Fixed ahit for Mediatek driver
* Fixed error handling in ufshcd_core_init()
* Tested this patch and the issue is still seen.

v10 -> v11:
- Fixed supplier suspending before consumer race
- Addressed Adrian's comments
* Added proper resume/suspend cb to ufshcd_auto_hibern8_update()
* Cosmetic changes to ufshcd-pci.c
* Cleaned up ufshcd_system_suspend()
* Added ufshcd_debugfs_eh_exit to ufshcd_core_init()

v9 -> v10:
- Addressed Adrian's comments
* Moved suspend/resume vops to __ufshcd_wl_[suspend/resume]()
* Added correct resume in ufs_bsg

v8 -> v9:
- Addressed Adrian's comments
* Moved link transition to __ufshcd_wl_[suspend/resume]()
* Fixed the other minor comments

v7 -> v8:
- Addressed Adrian's comments
* Removed separate autosuspend delay for ufs-device lun
* Fixed the ee handler getting scheduled during pm
* Always runtime resume in suspend_prepare()
* Added CONFIG_PM_SLEEP where needed

v6 -> v7:
* Resume the ufs device before shutting it down

v5 -> v6:
- Addressed Adrian's comments
* Added complete() cb
* Added suspend_prepare() and complete() to all drivers
* Moved suspend_prepare() and complete() to ufshcd
* .poweroff() uses ufhcd_wl_poweroff()
* Removed several forward declarations
* Moved scsi_register_driver() to ufshcd_core_init()

v4 -> v5:
- Addressed Adrian's comments
* Used the rpmb driver contributed by Adrian
* Runtime-resume the ufs device during suspend to honor spm-lvl
* Unregister the scsi_driver in ufshcd_remove()
* Currently shutdown() puts the ufs device to power-down mode so, just removed ufshcd_pci_poweroff()
* Quiesce the scsi device during shutdown instead of remove

v3 RFC -> v4:
- Addressed Bart's comments
* Except that I didn't get any checkpatch failures
- Addressed Avri's comments
- Addressed Adrian's comments
* Added a check for deepsleep power mode
* Removed a couple of forward declarations
* Didn't separate the scsi drivers because in rpmb case it just sends uac in resume and it seemed pretty neat to me.
- Added sysfs changes to resume the devices before accessing

Asutosh Das (2):
scsi: ufs: Enable power management for wlun
ufs: sysfs: Resume the proper scsi device

 drivers/scsi/ufs/cdns-pltfrm.c     |   2 +
 drivers/scsi/ufs/tc-dwc-g210-pci.c |   2 +
 drivers/scsi/ufs/ufs-debugfs.c     |   6 +-
 drivers/scsi/ufs/ufs-debugfs.h     |   2 +-
 drivers/scsi/ufs/ufs-exynos.c      |   2 +
 drivers/scsi/ufs/ufs-hisi.c        |   2 +
 drivers/scsi/ufs/ufs-mediatek.c    |  12 +-
 drivers/scsi/ufs/ufs-qcom.c        |   2 +
 drivers/scsi/ufs/ufs-sysfs.c       |  24 +-
 drivers/scsi/ufs/ufs_bsg.c         |   6 +-
 drivers/scsi/ufs/ufshcd-pci.c      |  36 +-
 drivers/scsi/ufs/ufshcd.c          | 719 ++++++++++++++++++++++++++-----------
 drivers/scsi/ufs/ufshcd.h          |  21 ++
 include/trace/events/ufs.h         |  20 ++
 14 files changed, 592 insertions(+), 264 deletions(-)
Signature · 1 line
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
author
#1 lore raw

PATCH v20 1/2 scsi: ufs: Enable power management for wlun

During runtime-suspend of ufs host, the scsi devices are already suspended and so are the queues associated with them. But the ufs host sends SSU (START_STOP_UNIT) to wlun
during its runtime-suspend.
During the process blk_queue_enter checks if the queue is not in suspended state. If so, it waits for the queue to resume, and never comes out of it.
The commit
(d55d15a33: scsi: block: Do not accept any requests while suspended) adds the check if the queue is in suspended state in blk_queue_enter().

Call trace:
 __switch_to+0x174/0x2c4
 __schedule+0x478/0x764
 schedule+0x9c/0xe0
 blk_queue_enter+0x158/0x228
 blk_mq_alloc_request+0x40/0xa4
 blk_get_request+0x2c/0x70
 __scsi_execute+0x60/0x1c4
 ufshcd_set_dev_pwr_mode+0x124/0x1e4
 ufshcd_suspend+0x208/0x83c
 ufshcd_runtime_suspend+0x40/0x154
 ufshcd_pltfrm_runtime_suspend+0x14/0x20
 pm_generic_runtime_suspend+0x28/0x3c
 __rpm_callback+0x80/0x2a4
 rpm_suspend+0x308/0x614
 rpm_idle+0x158/0x228
 pm_runtime_work+0x84/0xac
 process_one_work+0x1f0/0x470
 worker_thread+0x26c/0x4c8
 kthread+0x13c/0x320
 ret_from_fork+0x10/0x18

Fix this by registering ufs device wlun as a scsi driver and registering it for block runtime-pm. Also make this as a supplier for all other luns. That way, this device wlun suspends after all the consumers and resumes after
hba resumes. This also registers a new scsi driver for rpmb wlun. This new driver is mostly used to clear rpmb uac.

Fixed smatch warnings:

Reported-by kernel test robot <[email protected]>
Reported-by Dan Carpenter <[email protected]>
Co-developed-by Can Guo <[email protected]>
Signed-off-by Can Guo <[email protected]>
Signed-off-by Asutosh Das <[email protected]>
---
 drivers/scsi/ufs/cdns-pltfrm.c     |   2 +
 drivers/scsi/ufs/tc-dwc-g210-pci.c |   2 +
 drivers/scsi/ufs/ufs-debugfs.c     |   6 +-
 drivers/scsi/ufs/ufs-debugfs.h     |   2 +-
 drivers/scsi/ufs/ufs-exynos.c      |   2 +
 drivers/scsi/ufs/ufs-hisi.c        |   2 +
 drivers/scsi/ufs/ufs-mediatek.c    |  12 +-
 drivers/scsi/ufs/ufs-qcom.c        |   2 +
 drivers/scsi/ufs/ufs_bsg.c         |   6 +-
 drivers/scsi/ufs/ufshcd-pci.c      |  36 +-
 drivers/scsi/ufs/ufshcd.c          | 719 ++++++++++++++++++++++++++-----------
 drivers/scsi/ufs/ufshcd.h          |  21 ++
 include/trace/events/ufs.h         |  20 ++
 13 files changed, 580 insertions(+), 252 deletions(-)
drivers/scsi/ufs/cdns-pltfrm.c +2 -0
index 13d9204..908ff39 100644
@@ -323,6 +323,8 @@ static const struct dev_pm_ops cdns_ufs_dev_pm_ops = {
 	.runtime_suspend = ufshcd_pltfrm_runtime_suspend,
 	.runtime_resume  = ufshcd_pltfrm_runtime_resume,
 	.runtime_idle    = ufshcd_pltfrm_runtime_idle,
+	.prepare	 = ufshcd_suspend_prepare,
+	.complete	 = ufshcd_resume_complete,
 };
 
 static struct platform_driver cdns_ufs_pltfrm_driver = {
drivers/scsi/ufs/tc-dwc-g210-pci.c +2 -0
index 67a6a61..ec4589a 100644
@@ -148,6 +148,8 @@ static const struct dev_pm_ops tc_dwc_g210_pci_pm_ops = {
 	.runtime_suspend = tc_dwc_g210_pci_runtime_suspend,
 	.runtime_resume  = tc_dwc_g210_pci_runtime_resume,
 	.runtime_idle    = tc_dwc_g210_pci_runtime_idle,
+	.prepare	 = ufshcd_suspend_prepare,
+	.complete	 = ufshcd_resume_complete,
 };
 
 static const struct pci_device_id tc_dwc_g210_pci_tbl[] = {
drivers/scsi/ufs/ufs-debugfs.c +3 -3
index ced9ef4..4e1ff20 100644
@@ -13,7 +13,7 @@ void __init ufs_debugfs_init(void)
 	ufs_debugfs_root = debugfs_create_dir("ufshcd", NULL);
 }
 
-void __exit ufs_debugfs_exit(void)
+void ufs_debugfs_exit(void)
 {
 	debugfs_remove_recursive(ufs_debugfs_root);
 }

@@ -60,14 +60,14 @@ __acquires(&hba->host_sem)
 		up(&hba->host_sem);
 		return -EBUSY;
 	}
-	pm_runtime_get_sync(hba->dev);
+	ufshcd_rpm_get_sync(hba);
 	return 0;
 }
 
 static void ufs_debugfs_put_user_access(struct ufs_hba *hba)
 __releases(&hba->host_sem)
 {
-	pm_runtime_put_sync(hba->dev);
+	ufshcd_rpm_put_sync(hba);
 	up(&hba->host_sem);
 }
drivers/scsi/ufs/ufs-debugfs.h +1 -1
index 3ca29d3..97548a3 100644
@@ -9,7 +9,7 @@ struct ufs_hba;
 
 #ifdef CONFIG_DEBUG_FS
 void __init ufs_debugfs_init(void);
-void __exit ufs_debugfs_exit(void);
+void ufs_debugfs_exit(void);
 void ufs_debugfs_hba_init(struct ufs_hba *hba);
 void ufs_debugfs_hba_exit(struct ufs_hba *hba);
 void ufs_debugfs_exception_event(struct ufs_hba *hba, u16 status);
drivers/scsi/ufs/ufs-exynos.c +2 -0
index 70647ea..8b31efb 100644
@@ -1267,6 +1267,8 @@ static const struct dev_pm_ops exynos_ufs_pm_ops = {
 	.runtime_suspend = ufshcd_pltfrm_runtime_suspend,
 	.runtime_resume  = ufshcd_pltfrm_runtime_resume,
 	.runtime_idle    = ufshcd_pltfrm_runtime_idle,
+	.prepare	 = ufshcd_suspend_prepare,
+	.complete	 = ufshcd_resume_complete,
 };
 
 static struct platform_driver exynos_ufs_pltform = {
drivers/scsi/ufs/ufs-hisi.c +2 -0
index 0aa5813..a473349 100644
@@ -574,6 +574,8 @@ static const struct dev_pm_ops ufs_hisi_pm_ops = {
 	.runtime_suspend = ufshcd_pltfrm_runtime_suspend,
 	.runtime_resume  = ufshcd_pltfrm_runtime_resume,
 	.runtime_idle    = ufshcd_pltfrm_runtime_idle,
+	.prepare	 = ufshcd_suspend_prepare,
+	.complete	 = ufshcd_resume_complete,
 };
 
 static struct platform_driver ufs_hisi_pltform = {
drivers/scsi/ufs/ufs-mediatek.c +6 -6
index a981f26..4862a7b 100644
@@ -810,12 +810,10 @@ static int ufs_mtk_post_link(struct ufs_hba *hba)
 	/* enable unipro clock gating feature */
 	ufs_mtk_cfg_unipro_cg(hba, true);
 
-	/* configure auto-hibern8 timer to 10ms */
-	if (ufshcd_is_auto_hibern8_supported(hba)) {
-		ufshcd_auto_hibern8_update(hba,
-			FIELD_PREP(UFSHCI_AHIBERN8_TIMER_MASK, 10) |
-			FIELD_PREP(UFSHCI_AHIBERN8_SCALE_MASK, 3));
-	}
+	/* will be configured during probe hba */
+	if (ufshcd_is_auto_hibern8_supported(hba))
+		hba->ahit = FIELD_PREP(UFSHCI_AHIBERN8_TIMER_MASK, 10) |
+			FIELD_PREP(UFSHCI_AHIBERN8_SCALE_MASK, 3);
 
 	ufs_mtk_setup_clk_gating(hba);
 

@@ -1097,6 +1095,8 @@ static const struct dev_pm_ops ufs_mtk_pm_ops = {
 	.runtime_suspend = ufshcd_pltfrm_runtime_suspend,
 	.runtime_resume  = ufshcd_pltfrm_runtime_resume,
 	.runtime_idle    = ufshcd_pltfrm_runtime_idle,
+	.prepare	 = ufshcd_suspend_prepare,
+	.complete	= ufshcd_resume_complete,
 };
 
 static struct platform_driver ufs_mtk_pltform = {
drivers/scsi/ufs/ufs-qcom.c +2 -0
index 9b711d6..6651b87 100644
@@ -1556,6 +1556,8 @@ static const struct dev_pm_ops ufs_qcom_pm_ops = {
 	.runtime_suspend = ufshcd_pltfrm_runtime_suspend,
 	.runtime_resume  = ufshcd_pltfrm_runtime_resume,
 	.runtime_idle    = ufshcd_pltfrm_runtime_idle,
+	.prepare	 = ufshcd_suspend_prepare,
+	.complete	= ufshcd_resume_complete,
 };
 
 static struct platform_driver ufs_qcom_pltform = {
drivers/scsi/ufs/ufs_bsg.c +3 -3
index 5b2bc1a..39bf204 100644
@@ -97,7 +97,7 @@ static int ufs_bsg_request(struct bsg_job *job)
 
 	bsg_reply->reply_payload_rcv_len = 0;
 
-	pm_runtime_get_sync(hba->dev);
+	ufshcd_rpm_get_sync(hba);
 
 	msgcode = bsg_request->msgcode;
 	switch (msgcode) {

@@ -106,7 +106,7 @@ static int ufs_bsg_request(struct bsg_job *job)
 		ret = ufs_bsg_alloc_desc_buffer(hba, job, &desc_buff,
 						&desc_len, desc_op);
 		if (ret) {
-			pm_runtime_put_sync(hba->dev);
+			ufshcd_rpm_put_sync(hba);
 			goto out;
 		}
 

@@ -138,7 +138,7 @@ static int ufs_bsg_request(struct bsg_job *job)
 		break;
 	}
 
-	pm_runtime_put_sync(hba->dev);
+	ufshcd_rpm_put_sync(hba);
 
 	if (!desc_buff)
 		goto out;
drivers/scsi/ufs/ufshcd-pci.c +5 -31
index 23ee828..e6c334b 100644
@@ -410,29 +410,6 @@ static int ufshcd_pci_resume(struct device *dev)
 	return ufshcd_system_resume(dev_get_drvdata(dev));
 }
 
-/**
- * ufshcd_pci_poweroff - suspend-to-disk poweroff function
- * @dev: pointer to PCI device handle
- *
- * Returns 0 if successful
- * Returns non-zero otherwise
- */
-static int ufshcd_pci_poweroff(struct device *dev)
-{
-	struct ufs_hba *hba = dev_get_drvdata(dev);
-	int spm_lvl = hba->spm_lvl;
-	int ret;
-
-	/*
-	 * For poweroff we need to set the UFS device to PowerDown mode.
-	 * Force spm_lvl to ensure that.
-	 */
-	hba->spm_lvl = 5;
-	ret = ufshcd_system_suspend(hba);
-	hba->spm_lvl = spm_lvl;
-	return ret;
-}
-
 #endif /* !CONFIG_PM_SLEEP */
 
 #ifdef CONFIG_PM

@@ -533,17 +510,14 @@ ufshcd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 }
 
 static const struct dev_pm_ops ufshcd_pci_pm_ops = {
-#ifdef CONFIG_PM_SLEEP
-	.suspend	= ufshcd_pci_suspend,
-	.resume		= ufshcd_pci_resume,
-	.freeze		= ufshcd_pci_suspend,
-	.thaw		= ufshcd_pci_resume,
-	.poweroff	= ufshcd_pci_poweroff,
-	.restore	= ufshcd_pci_resume,
-#endif
 	SET_RUNTIME_PM_OPS(ufshcd_pci_runtime_suspend,
 			   ufshcd_pci_runtime_resume,
 			   ufshcd_pci_runtime_idle)
+	SET_SYSTEM_SLEEP_PM_OPS(ufshcd_pci_suspend, ufshcd_pci_resume)
+#ifdef CONFIG_PM_SLEEP
+	.prepare	= ufshcd_suspend_prepare,
+	.complete	= ufshcd_resume_complete,
+#endif
 };
 
 static const struct pci_device_id ufshcd_pci_tbl[] = {
drivers/scsi/ufs/ufshcd.c +511 -208

1,051 lines not on this page · raw message

drivers/scsi/ufs/ufshcd.h +21 -0
index 5eb66a8..3a8dc0d 100644
@@ -72,6 +72,8 @@ enum ufs_event_type {
 	UFS_EVT_LINK_STARTUP_FAIL,
 	UFS_EVT_RESUME_ERR,
 	UFS_EVT_SUSPEND_ERR,
+	UFS_EVT_WL_SUSP_ERR,
+	UFS_EVT_WL_RES_ERR,
 
 	/* abnormal events */
 	UFS_EVT_DEV_RESET,

@@ -807,6 +809,7 @@ struct ufs_hba {
 	struct list_head clk_list_head;
 
 	bool wlun_dev_clr_ua;
+	bool wlun_rpmb_clr_ua;
 
 	/* Number of requests aborts */
 	int req_abort_count;

@@ -846,6 +849,7 @@ struct ufs_hba {
 	struct delayed_work debugfs_ee_work;
 	u32 debugfs_ee_rate_limit_ms;
 #endif
+	u32 luns_avail;
 };
 
 /* Returns true if clocks can be gated. Otherwise false */

@@ -1105,6 +1109,8 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 			     enum query_opcode desc_op);
 
 int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable);
+int ufshcd_suspend_prepare(struct device *dev);
+void ufshcd_resume_complete(struct device *dev);
 
 /* Wrapper functions for safely calling variant operations */
 static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)

@@ -1309,4 +1315,19 @@ static inline int ufshcd_update_ee_usr_mask(struct ufs_hba *hba,
 					&hba->ee_drv_mask, set, clr);
 }
 
+static inline int ufshcd_rpm_get_sync(struct ufs_hba *hba)
+{
+	return pm_runtime_get_sync(&hba->sdev_ufs_device->sdev_gendev);
+}
+
+static inline int ufshcd_rpm_put_sync(struct ufs_hba *hba)
+{
+	return pm_runtime_put_sync(&hba->sdev_ufs_device->sdev_gendev);
+}
+
+static inline int ufshcd_rpm_put(struct ufs_hba *hba)
+{
+	return pm_runtime_put(&hba->sdev_ufs_device->sdev_gendev);
+}
+
 #endif /* End of Header */
include/trace/events/ufs.h +20 -0
index 1cb6f1a..599739e 100644
@@ -246,6 +246,26 @@ DEFINE_EVENT(ufshcd_template, ufshcd_init,
 		      int dev_state, int link_state),
 	     TP_ARGS(dev_name, err, usecs, dev_state, link_state));
 
+DEFINE_EVENT(ufshcd_template, ufshcd_wl_suspend,
+	     TP_PROTO(const char *dev_name, int err, s64 usecs,
+		      int dev_state, int link_state),
+	     TP_ARGS(dev_name, err, usecs, dev_state, link_state));
+
+DEFINE_EVENT(ufshcd_template, ufshcd_wl_resume,
+	     TP_PROTO(const char *dev_name, int err, s64 usecs,
+		      int dev_state, int link_state),
+	     TP_ARGS(dev_name, err, usecs, dev_state, link_state));
+
+DEFINE_EVENT(ufshcd_template, ufshcd_wl_runtime_suspend,
+	     TP_PROTO(const char *dev_name, int err, s64 usecs,
+		      int dev_state, int link_state),
+	     TP_ARGS(dev_name, err, usecs, dev_state, link_state));
+
+DEFINE_EVENT(ufshcd_template, ufshcd_wl_runtime_resume,
+	     TP_PROTO(const char *dev_name, int err, s64 usecs,
+		      int dev_state, int link_state),
+	     TP_ARGS(dev_name, err, usecs, dev_state, link_state));
+
 TRACE_EVENT(ufshcd_command,
 	TP_PROTO(const char *dev_name, enum ufs_trace_str_t str_t,
 		 unsigned int tag, u32 doorbell, int transfer_len, u32 intr,
Signature · 1 line
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
author
#11 lore raw

PATCH v20 2/2 ufs: sysfs: Resume the proper scsi device

In reply to Asutosh Das

Resumes the actual scsi device the unit descriptor of which is being accessed instead of the hba alone.

Reviewed-by Adrian Hunter <[email protected]>
Reviewed-by Can Guo <[email protected]>
Signed-off-by Asutosh Das <[email protected]>
---
 drivers/scsi/ufs/ufs-sysfs.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)
drivers/scsi/ufs/ufs-sysfs.c +12 -12
index d7c3cff..4d9d4d8 100644
@@ -245,9 +245,9 @@ static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
 		goto out;
 	}
 
-	pm_runtime_get_sync(hba->dev);
+	ufshcd_rpm_get_sync(hba);
 	res = ufshcd_wb_toggle(hba, wb_enable);
-	pm_runtime_put_sync(hba->dev);
+	ufshcd_rpm_put_sync(hba);
 out:
 	up(&hba->host_sem);
 	return res < 0 ? res : count;

@@ -297,10 +297,10 @@ static ssize_t ufs_sysfs_read_desc_param(struct ufs_hba *hba,
 		goto out;
 	}
 
-	pm_runtime_get_sync(hba->dev);
+	ufshcd_rpm_get_sync(hba);
 	ret = ufshcd_read_desc_param(hba, desc_id, desc_index,
 				param_offset, desc_buf, param_size);
-	pm_runtime_put_sync(hba->dev);
+	ufshcd_rpm_put_sync(hba);
 	if (ret) {
 		ret = -EINVAL;
 		goto out;

@@ -678,7 +678,7 @@ static ssize_t _name##_show(struct device *dev,				\
 		up(&hba->host_sem);					\
 		return -ENOMEM;						\
 	}								\
-	pm_runtime_get_sync(hba->dev);					\
+	ufshcd_rpm_get_sync(hba);					\
 	ret = ufshcd_query_descriptor_retry(hba,			\
 		UPIU_QUERY_OPCODE_READ_DESC, QUERY_DESC_IDN_DEVICE,	\
 		0, 0, desc_buf, &desc_len);				\

@@ -695,7 +695,7 @@ static ssize_t _name##_show(struct device *dev,				\
 		goto out;						\
 	ret = sysfs_emit(buf, "%s\n", desc_buf);			\
 out:									\
-	pm_runtime_put_sync(hba->dev);					\
+	ufshcd_rpm_put_sync(hba);					\
 	kfree(desc_buf);						\
 	up(&hba->host_sem);						\
 	return ret;							\

@@ -744,10 +744,10 @@ static ssize_t _name##_show(struct device *dev,				\
 	}								\
 	if (ufshcd_is_wb_flags(QUERY_FLAG_IDN##_uname))			\
 		index = ufshcd_wb_get_query_index(hba);			\
-	pm_runtime_get_sync(hba->dev);					\
+	ufshcd_rpm_get_sync(hba);					\
 	ret = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_READ_FLAG,	\
 		QUERY_FLAG_IDN##_uname, index, &flag);			\
-	pm_runtime_put_sync(hba->dev);					\
+	ufshcd_rpm_put_sync(hba);					\
 	if (ret) {							\
 		ret = -EINVAL;						\
 		goto out;						\

@@ -813,10 +813,10 @@ static ssize_t _name##_show(struct device *dev,				\
 	}								\
 	if (ufshcd_is_wb_attrs(QUERY_ATTR_IDN##_uname))			\
 		index = ufshcd_wb_get_query_index(hba);			\
-	pm_runtime_get_sync(hba->dev);					\
+	ufshcd_rpm_get_sync(hba);					\
 	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,	\
 		QUERY_ATTR_IDN##_uname, index, 0, &value);		\
-	pm_runtime_put_sync(hba->dev);					\
+	ufshcd_rpm_put_sync(hba);					\
 	if (ret) {							\
 		ret = -EINVAL;						\
 		goto out;						\

@@ -964,10 +964,10 @@ static ssize_t dyn_cap_needed_attribute_show(struct device *dev,
 		goto out;
 	}
 
-	pm_runtime_get_sync(hba->dev);
+	ufshcd_rpm_get_sync(hba);
 	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,
 		QUERY_ATTR_IDN_DYN_CAP_NEEDED, lun, 0, &value);
-	pm_runtime_put_sync(hba->dev);
+	ufshcd_rpm_put_sync(hba);
 	if (ret) {
 		ret = -EINVAL;
 		goto out;
Signature · 1 line
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
#2 lore raw

Re: [PATCH v20 1/2] scsi: ufs: Enable power management for wlun

In reply to Asutosh Das
Quoted 40 lines · Asutosh Das

During runtime-suspend of ufs host, the scsi devices are already suspended and so are the queues associated with them. But the ufs host sends SSU (START_STOP_UNIT) to wlun
during its runtime-suspend.
During the process blk_queue_enter checks if the queue is not in suspended state. If so, it waits for the queue to resume, and never comes out of it.
The commit
(d55d15a33: scsi: block: Do not accept any requests while suspended) adds the check if the queue is in suspended state in blk_queue_enter().

Call trace:
 __switch_to+0x174/0x2c4
 __schedule+0x478/0x764
 schedule+0x9c/0xe0
 blk_queue_enter+0x158/0x228
 blk_mq_alloc_request+0x40/0xa4
 blk_get_request+0x2c/0x70
 __scsi_execute+0x60/0x1c4
 ufshcd_set_dev_pwr_mode+0x124/0x1e4
 ufshcd_suspend+0x208/0x83c
 ufshcd_runtime_suspend+0x40/0x154
 ufshcd_pltfrm_runtime_suspend+0x14/0x20
 pm_generic_runtime_suspend+0x28/0x3c
 __rpm_callback+0x80/0x2a4
 rpm_suspend+0x308/0x614
 rpm_idle+0x158/0x228
 pm_runtime_work+0x84/0xac
 process_one_work+0x1f0/0x470
 worker_thread+0x26c/0x4c8
 kthread+0x13c/0x320
 ret_from_fork+0x10/0x18

Fix this by registering ufs device wlun as a scsi driver and registering it for block runtime-pm. Also make this as a supplier for all other luns. That way, this device wlun suspends after all the consumers and resumes after
hba resumes. This also registers a new scsi driver for rpmb wlun. This new driver is mostly used to clear rpmb uac.

Quoted 8 lines

Fixed smatch warnings:

Reported-by kernel test robot <[email protected]>
Reported-by Dan Carpenter <[email protected]>
Co-developed-by Can Guo <[email protected]>
Signed-off-by Can Guo <[email protected]>
Signed-off-by Asutosh Das <[email protected]>

---

I came across 3 issues while testing. See comments below.

<SNIP>

drivers/scsi/ufs/ufshcd.c static void ufshcd_clk_scaling_suspend(struct ufs_hba *hba, bool suspend) · line 5800

Quoted 7 lines
drivers/scsi/ufs/ufshcd.c +1 -2
@@ -5753,12 +5797,13 @@ static void ufshcd_clk_scaling_suspend(struct ufs_hba *hba, bool suspend)
 
 static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
 {
-	pm_runtime_get_sync(hba->dev);
-	if (pm_runtime_status_suspended(hba->dev) || hba->is_sys_suspended) {
+	ufshcd_rpm_get_sync(hba);

hba->sdev_ufs_device could be NULL.
Need to add a check for that in ufshcd_err_handling_should_stop()

Quoted 11 lines
drivers/scsi/ufs/ufshcd.c +3 -1
+	if (pm_runtime_status_suspended(&hba->sdev_ufs_device->sdev_gendev) ||
+	    hba->is_sys_suspended) {
 		enum ufs_pm_op pm_op;
 
 		/*
-		 * Don't assume anything of pm_runtime_get_sync(), if
+		 * Don't assume anything of resume, if
 		 * resume fails, irq and clocks can be OFF, and powers
 		 * can be OFF or in LPM.
 		 */

drivers/scsi/ufs/ufshcd.c static void ufshcd_err_handling_unprepare(struct ufs_hba *hba) · line 5841

Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +0 -0
@@ -5794,7 +5839,7 @@ static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
 	if (ufshcd_is_clkscaling_supported(hba))
 		ufshcd_clk_scaling_suspend(hba, false);
 	ufshcd_clear_ua_wluns(hba);

ufshcd_clear_ua_wluns() deadlocks trying to clear UFS_UPIU_RPMB_WLUN if sdev_rpmb is suspended and sdev_ufs_device is suspending. e.g. ufshcd_wl_suspend() is waiting on host_sem while ufshcd_err_handler() is running, at which point sdev_rpmb has already suspended.

Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +1 -1
-	pm_runtime_put(hba->dev);
+	ufshcd_rpm_put(hba);
 }

<SNIP>

Quoted 41 lines
drivers/scsi/ufs/ufshcd.c +41 -0
+void ufshcd_resume_complete(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	ufshcd_rpm_put(hba);
+}
+EXPORT_SYMBOL_GPL(ufshcd_resume_complete);
+
+int ufshcd_suspend_prepare(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct device *ufs_dev = &hba->sdev_ufs_device->sdev_gendev;
+	enum ufs_dev_pwr_mode spm_pwr_mode;
+	enum uic_link_state spm_link_state;
+	unsigned long flags;
+	bool rpm_state_ok;
+
+	/*
+	 * SCSI assumes that runtime-pm and system-pm for scsi drivers
+	 * are same. And it doesn't wake up the device for system-suspend
+	 * if it's runtime suspended. But ufs doesn't follow that.
+	 * The rpm-lvl and spm-lvl can be different in ufs.
+	 * However, if the current_{pwr_mode, link_state} is same as the
+	 * desired_{pwr_mode, link_state}, there's no need to rpm resume
+	 * the device.
+	 * Refer ufshcd_resume_complete()
+	 */
+	pm_runtime_get_noresume(ufs_dev);
+
+	spin_lock_irqsave(&ufs_dev->power.lock, flags);
+
+	spm_pwr_mode = ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl);
+	spm_link_state = ufs_get_pm_lvl_to_link_pwr_state(hba->spm_lvl);
+
+	rpm_state_ok = pm_runtime_suspended(ufs_dev) &&
+		hba->curr_dev_pwr_mode == spm_pwr_mode &&
+		hba->uic_link_state == spm_link_state &&
+		!hba->dev_info.b_rpm_dev_flush_capable;
+
+	spin_unlock_irqrestore(&ufs_dev->power.lock, flags);
+

drivers/scsi/ufs/ufshcd.c int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) · line 9621

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +10 -0
+	if (!rpm_state_ok) {
+		int ret = pm_runtime_resume(ufs_dev);
+
+		if (ret < 0 && ret != -EACCES) {
+			pm_runtime_put(ufs_dev);
+			return ret;
+		}
+	}
+	return 0;
+}

Unfortunately this does not work because SCSI PM forcibly sets the sdevs to runtime active after system resume. Really we should change SCSI PM to call the driver's .prepare / .complete then we could use direct complete, but let's leave that for now and go back to before, but allowing for errors and !hba->sdev_ufs_device. e.g.

void ufshcd_resume_complete(struct device *dev)

{
	struct ufs_hba *hba = dev_get_drvdata(dev);

	if (hba->complete_put) {
		hba->complete_put = false;
		ufshcd_rpm_put(hba);
	}
}

EXPORT_SYMBOL_GPL(ufshcd_resume_complete);

int ufshcd_suspend_prepare(struct device *dev)

{
	struct ufs_hba *hba = dev_get_drvdata(dev);
	int ret;

	if (!hba->sdev_ufs_device)
		return 0;

	ret = ufshcd_rpm_get_sync(hba);
	if (ret < 0 && ret != -EACCES) {
		ufshcd_rpm_put(hba);
		return ret;
	}
	hba->complete_put = true;
	return 0;
}

EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare);

author
#3 lore raw

Re: [PATCH v20 1/2] scsi: ufs: Enable power management for wlun

Quoted 9 lines · Adrian Hunter
Quoted 5 lines · Asutosh Das
Co-developed-by Can Guo <[email protected]>
Signed-off-by Can Guo <[email protected]>
Signed-off-by Asutosh Das <[email protected]>

---

I came across 3 issues while testing. See comments below.

Hi Adrian
Thanks for the comments.

Quoted 12 lines

<SNIP>

drivers/scsi/ufs/ufshcd.c static void ufshcd_err_handling_unprepare(struct ufs_hba *hba) · line 5841

Quoted 4 lines
drivers/scsi/ufs/ufshcd.c +0 -0
@@ -5794,7 +5839,7 @@ static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
  	if (ufshcd_is_clkscaling_supported(hba))
  		ufshcd_clk_scaling_suspend(hba, false);
  	ufshcd_clear_ua_wluns(hba);

ufshcd_clear_ua_wluns() deadlocks trying to clear UFS_UPIU_RPMB_WLUN if sdev_rpmb is suspended and sdev_ufs_device is suspending. e.g. ufshcd_wl_suspend() is waiting on host_sem while ufshcd_err_handler() is running, at which point sdev_rpmb has already suspended.

Umm, I didn't understand this deadlock.
When you say, sdev_rpmb is suspended, does it mean runtime_suspended?
sdev_ufs_device is suspending - this can't be runtime_suspending, while ufshcd_err_handling_unprepare is running.

If you've a call-stack of this deadlock, please can you share it with me. I'll also try to reproduce this.

I'll address the other comments in the next version.

Thank you!

Quoted 8 lines
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +1 -1
-	pm_runtime_put(hba->dev);
+	ufshcd_rpm_put(hba);
  }

<SNIP>

Quoted 2 lines
(quoted diff) +2 -0
+void ufshcd_resume_complete(struct device *dev)
+{
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project
#4 lore raw

Re: [PATCH v20 1/2] scsi: ufs: Enable power management for wlun

Quoted 24 lines · Asutosh Das (asd)
Quoted 9 lines · Adrian Hunter
Quoted 5 lines · Asutosh Das
Co-developed-by Can Guo <[email protected]>
Signed-off-by Can Guo <[email protected]>
Signed-off-by Asutosh Das <[email protected]>

---

I came across 3 issues while testing.  See comments below.

Hi Adrian
Thanks for the comments.

Quoted 12 lines

<SNIP>

drivers/scsi/ufs/ufshcd.c static void ufshcd_err_handling_unprepare(struct ufs_hba *hba) · line 5841

Quoted 4 lines
drivers/scsi/ufs/ufshcd.c +0 -0
@@ -5794,7 +5839,7 @@ static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
       if (ufshcd_is_clkscaling_supported(hba))
           ufshcd_clk_scaling_suspend(hba, false);
       ufshcd_clear_ua_wluns(hba);

ufshcd_clear_ua_wluns() deadlocks trying to clear UFS_UPIU_RPMB_WLUN if sdev_rpmb is suspended and sdev_ufs_device is suspending. e.g. ufshcd_wl_suspend() is waiting on host_sem while ufshcd_err_handler() is running, at which point sdev_rpmb has already suspended.

Quoted 5 lines

Umm, I didn't understand this deadlock.
When you say, sdev_rpmb is suspended, does it mean runtime_suspended?
sdev_ufs_device is suspending - this can't be runtime_suspending, while ufshcd_err_handling_unprepare is running.

If you've a call-stack of this deadlock, please can you share it with me. I'll also try to reproduce this.

Yes it is system suspend. sdev_rpmb has suspended, sdev_ufs_device is waiting on host_sem. ufshcd_err_handler() holds host_sem. ufshcd_clear_ua_wlun(UFS_UPIU_RPMB_WLUN) gets stuck. I will get some call-stacks.

Quoted 15 lines

I'll address the other comments in the next version.

Thank you!

Quoted 8 lines
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +1 -1
-    pm_runtime_put(hba->dev);
+    ufshcd_rpm_put(hba);
   }

<SNIP>

Quoted 2 lines
(quoted diff) +2 -0
+void ufshcd_resume_complete(struct device *dev)
+{
#5 lore raw

Re: [PATCH v20 1/2] scsi: ufs: Enable power management for wlun

Quoted 29 lines · Adrian Hunter
Quoted 28 lines · Asutosh Das (asd)
Quoted 9 lines · Adrian Hunter
Quoted 5 lines · Asutosh Das
Co-developed-by Can Guo <[email protected]>
Signed-off-by Can Guo <[email protected]>
Signed-off-by Asutosh Das <[email protected]>

---

I came across 3 issues while testing.  See comments below.

Hi Adrian
Thanks for the comments.

Quoted 12 lines

<SNIP>

drivers/scsi/ufs/ufshcd.c static void ufshcd_err_handling_unprepare(struct ufs_hba *hba) · line 5841

Quoted 4 lines
drivers/scsi/ufs/ufshcd.c +0 -0
@@ -5794,7 +5839,7 @@ static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
       if (ufshcd_is_clkscaling_supported(hba))
           ufshcd_clk_scaling_suspend(hba, false);
       ufshcd_clear_ua_wluns(hba);

ufshcd_clear_ua_wluns() deadlocks trying to clear UFS_UPIU_RPMB_WLUN if sdev_rpmb is suspended and sdev_ufs_device is suspending. e.g. ufshcd_wl_suspend() is waiting on host_sem while ufshcd_err_handler() is running, at which point sdev_rpmb has already suspended.

Umm, I didn't understand this deadlock.
When you say, sdev_rpmb is suspended, does it mean runtime_suspended?
sdev_ufs_device is suspending - this can't be runtime_suspending, while ufshcd_err_handling_unprepare is running.

Quoted 5 lines
Quoted 1 line

If you've a call-stack of this deadlock, please can you share it with me. I'll also try to reproduce this.

Yes it is system suspend. sdev_rpmb has suspended, sdev_ufs_device is waiting on host_sem. ufshcd_err_handler() holds host_sem. ufshcd_clear_ua_wlun(UFS_UPIU_RPMB_WLUN) gets stuck. I will get some call-stacks.

Here are the call stacks

[   34.094321] Workqueue: ufs_eh_wq_0 ufshcd_err_handler
[   34.094788] Call Trace:
[   34.095281]  __schedule+0x275/0x6c0
[   34.095743]  schedule+0x41/0xa0
[   34.096240]  blk_queue_enter+0x10d/0x230
[   34.096693]  ? wait_woken+0x70/0x70
[   34.097167]  blk_mq_alloc_request+0x53/0xc0
[   34.097610]  blk_get_request+0x1e/0x60
[   34.098053]  __scsi_execute+0x3c/0x260
[   34.098529]  ufshcd_clear_ua_wlun.cold+0xa6/0x14b
[   34.098977]  ufshcd_clear_ua_wluns.part.0+0x4d/0x92
[   34.099456]  ufshcd_err_handler+0x97a/0x9ff
[   34.099902]  process_one_work+0x1cc/0x360
[   34.100384]  worker_thread+0x45/0x3b0
[   34.100851]  ? process_one_work+0x360/0x360
[   34.101308]  kthread+0xf6/0x130
[   34.101728]  ? kthread_park+0x80/0x80
[   34.102186]  ret_from_fork+0x1f/0x30
[   34.640751] task:kworker/u10:9   state:D stack:14528 pid:  255 ppid:     2 flags:0x00004000
[   34.641253] Workqueue: events_unbound async_run_entry_fn
[   34.641722] Call Trace:
[   34.642217]  __schedule+0x275/0x6c0
[   34.642683]  schedule+0x41/0xa0
[   34.643179]  schedule_timeout+0x18b/0x290
[   34.643645]  ? del_timer_sync+0x30/0x30
[   34.644131]  __down_timeout+0x6b/0xc0
[   34.644568]  ? ufshcd_clkscale_enable_show+0x20/0x20
[   34.645014]  ? async_schedule_node_domain+0x17d/0x190
[   34.645496]  down_timeout+0x42/0x50
[   34.645947]  ufshcd_wl_suspend+0x79/0xa0
[   34.646432]  ? scmd_printk+0x100/0x100
[   34.646917]  scsi_bus_suspend_common+0x56/0xc0
[   34.647405]  ? scsi_bus_freeze+0x10/0x10
[   34.647858]  dpm_run_callback+0x45/0x110
[   34.648347]  __device_suspend+0x117/0x460
[   34.648788]  async_suspend+0x16/0x90
[   34.649251]  async_run_entry_fn+0x26/0x110
[   34.649676]  process_one_work+0x1cc/0x360
[   34.650137]  worker_thread+0x45/0x3b0
[   34.650563]  ? process_one_work+0x360/0x360
[   34.650994]  kthread+0xf6/0x130
[   34.651455]  ? kthread_park+0x80/0x80
[   34.651882]  ret_from_fork+0x1f/0x30
Quoted 17 lines
Quoted 16 lines

I'll address the other comments in the next version.

Thank you!

Quoted 8 lines
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +1 -1
-    pm_runtime_put(hba->dev);
+    ufshcd_rpm_put(hba);
   }

<SNIP>

Quoted 2 lines
(quoted diff) +2 -0
+void ufshcd_resume_complete(struct device *dev)
+{
author
#6 lore raw

Re: [PATCH v20 1/2] scsi: ufs: Enable power management for wlun

Quoted 30 lines · Adrian Hunter
Quoted 29 lines · Adrian Hunter
Quoted 28 lines · Asutosh Das (asd)
Quoted 9 lines · Adrian Hunter
Quoted 5 lines · Asutosh Das
Co-developed-by Can Guo <[email protected]>
Signed-off-by Can Guo <[email protected]>
Signed-off-by Asutosh Das <[email protected]>

---

I came across 3 issues while testing.  See comments below.

Hi Adrian
Thanks for the comments.

Quoted 12 lines

<SNIP>

drivers/scsi/ufs/ufshcd.c static void ufshcd_err_handling_unprepare(struct ufs_hba *hba) · line 5841

Quoted 4 lines
drivers/scsi/ufs/ufshcd.c +0 -0
@@ -5794,7 +5839,7 @@ static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
       if (ufshcd_is_clkscaling_supported(hba))
           ufshcd_clk_scaling_suspend(hba, false);
       ufshcd_clear_ua_wluns(hba);

ufshcd_clear_ua_wluns() deadlocks trying to clear UFS_UPIU_RPMB_WLUN if sdev_rpmb is suspended and sdev_ufs_device is suspending. e.g. ufshcd_wl_suspend() is waiting on host_sem while ufshcd_err_handler() is running, at which point sdev_rpmb has already suspended.

Umm, I didn't understand this deadlock.
When you say, sdev_rpmb is suspended, does it mean runtime_suspended?
sdev_ufs_device is suspending - this can't be runtime_suspending, while ufshcd_err_handling_unprepare is running.

Quoted 6 lines
Quoted 5 lines
Quoted 1 line

If you've a call-stack of this deadlock, please can you share it with me. I'll also try to reproduce this.

Yes it is system suspend. sdev_rpmb has suspended, sdev_ufs_device is waiting on host_sem. ufshcd_err_handler() holds host_sem. ufshcd_clear_ua_wlun(UFS_UPIU_RPMB_WLUN) gets stuck. I will get some call-stacks.

Hi Adrian,

Thanks for the call stacks.
From the current information, I can't say for sure why it'd get stuck in blk_queue_enter().

I tried reproducing this issue on my setup yesterday but couldn't.
Here's what I did:
1. sdev_rpmb is RPM_SUSPENDED, checked before initiating system suspend
2. sdev_ufs_device is RPM_RESUMED
3. I triggered system suspend (echo mem > /sys/power/state) and scheduled the error handler from ufshcd_wl_suspend().
4. Waited until error handler ran and then ufshcd_wl_suspend() blocks on host_sem.
5. The ufshcd_clear_wa_wlun(UFS_UPIU_RPMB_WLUN) went through fine.

Do you've some specific steps to reproduce this or a script, perhaps? If so, please can you share it with me. I will try again. My test environment is in 5.10 kernel with Android, I suppose that should be ok though.

Thanks
-asd

Quoted 67 lines

Here are the call stacks

[   34.094321] Workqueue: ufs_eh_wq_0 ufshcd_err_handler
[   34.094788] Call Trace:
[   34.095281]  __schedule+0x275/0x6c0
[   34.095743]  schedule+0x41/0xa0
[   34.096240]  blk_queue_enter+0x10d/0x230
[   34.096693]  ? wait_woken+0x70/0x70
[   34.097167]  blk_mq_alloc_request+0x53/0xc0
[   34.097610]  blk_get_request+0x1e/0x60
[   34.098053]  __scsi_execute+0x3c/0x260
[   34.098529]  ufshcd_clear_ua_wlun.cold+0xa6/0x14b
[   34.098977]  ufshcd_clear_ua_wluns.part.0+0x4d/0x92
[   34.099456]  ufshcd_err_handler+0x97a/0x9ff
[   34.099902]  process_one_work+0x1cc/0x360
[   34.100384]  worker_thread+0x45/0x3b0
[   34.100851]  ? process_one_work+0x360/0x360
[   34.101308]  kthread+0xf6/0x130
[   34.101728]  ? kthread_park+0x80/0x80
[   34.102186]  ret_from_fork+0x1f/0x30
[   34.640751] task:kworker/u10:9   state:D stack:14528 pid:  255 ppid:     2 flags:0x00004000
[   34.641253] Workqueue: events_unbound async_run_entry_fn
[   34.641722] Call Trace:
[   34.642217]  __schedule+0x275/0x6c0
[   34.642683]  schedule+0x41/0xa0
[   34.643179]  schedule_timeout+0x18b/0x290
[   34.643645]  ? del_timer_sync+0x30/0x30
[   34.644131]  __down_timeout+0x6b/0xc0
[   34.644568]  ? ufshcd_clkscale_enable_show+0x20/0x20
[   34.645014]  ? async_schedule_node_domain+0x17d/0x190
[   34.645496]  down_timeout+0x42/0x50
[   34.645947]  ufshcd_wl_suspend+0x79/0xa0
[   34.646432]  ? scmd_printk+0x100/0x100
[   34.646917]  scsi_bus_suspend_common+0x56/0xc0
[   34.647405]  ? scsi_bus_freeze+0x10/0x10
[   34.647858]  dpm_run_callback+0x45/0x110
[   34.648347]  __device_suspend+0x117/0x460
[   34.648788]  async_suspend+0x16/0x90
[   34.649251]  async_run_entry_fn+0x26/0x110
[   34.649676]  process_one_work+0x1cc/0x360
[   34.650137]  worker_thread+0x45/0x3b0
[   34.650563]  ? process_one_work+0x360/0x360
[   34.650994]  kthread+0xf6/0x130
[   34.651455]  ? kthread_park+0x80/0x80
[   34.651882]  ret_from_fork+0x1f/0x30
Quoted 17 lines
Quoted 16 lines

I'll address the other comments in the next version.

Thank you!

Quoted 8 lines
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +1 -1
-    pm_runtime_put(hba->dev);
+    ufshcd_rpm_put(hba);
   }

<SNIP>

Quoted 2 lines
(quoted diff) +2 -0
+void ufshcd_resume_complete(struct device *dev)
+{
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project
#7 lore raw

Re: [PATCH v20 1/2] scsi: ufs: Enable power management for wlun

Quoted 33 lines · Asutosh Das (asd)
Quoted 32 lines · Adrian Hunter
Quoted 31 lines · Adrian Hunter
Quoted 29 lines · Asutosh Das (asd)
Quoted 9 lines · Adrian Hunter
Quoted 5 lines · Asutosh Das
Co-developed-by Can Guo <[email protected]>
Signed-off-by Can Guo <[email protected]>
Signed-off-by Asutosh Das <[email protected]>

---

I came across 3 issues while testing.  See comments below.

Hi Adrian
Thanks for the comments.

Quoted 12 lines

<SNIP>

drivers/scsi/ufs/ufshcd.c static void ufshcd_err_handling_unprepare(struct ufs_hba *hba) · line 5841

Quoted 4 lines
drivers/scsi/ufs/ufshcd.c +0 -0
@@ -5794,7 +5839,7 @@ static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
        if (ufshcd_is_clkscaling_supported(hba))
            ufshcd_clk_scaling_suspend(hba, false);
        ufshcd_clear_ua_wluns(hba);

ufshcd_clear_ua_wluns() deadlocks trying to clear UFS_UPIU_RPMB_WLUN if sdev_rpmb is suspended and sdev_ufs_device is suspending. e.g. ufshcd_wl_suspend() is waiting on host_sem while ufshcd_err_handler() is running, at which point sdev_rpmb has already suspended.

Umm, I didn't understand this deadlock.
When you say, sdev_rpmb is suspended, does it mean runtime_suspended?
sdev_ufs_device is suspending - this can't be runtime_suspending, while ufshcd_err_handling_unprepare is running.

If you've a call-stack of this deadlock, please can you share it with me. I'll also try to reproduce this.

Quoted 8 lines
Quoted 4 lines
Quoted 3 lines

Yes it is system suspend. sdev_rpmb has suspended, sdev_ufs_device is waiting on host_sem. ufshcd_err_handler() holds host_sem. ufshcd_clear_ua_wlun(UFS_UPIU_RPMB_WLUN) gets stuck. I will get some call-stacks.

Hi Adrian,

Thanks for the call stacks.
From the current information, I can't say for sure why it'd get stuck in blk_queue_enter().

I presume SCSI is leaving the RPMB WLUN device runtime suspended and consequently the queue status is RPM_SUSPENDED

Quoted 10 lines

I tried reproducing this issue on my setup yesterday but couldn't.
Here's what I did:
1. sdev_rpmb is RPM_SUSPENDED, checked before initiating system suspend
2. sdev_ufs_device is RPM_RESUMED
3. I triggered system suspend (echo mem > /sys/power/state) and scheduled the error handler from ufshcd_wl_suspend().
4. Waited until error handler ran and then ufshcd_wl_suspend() blocks on host_sem.
5. The ufshcd_clear_wa_wlun(UFS_UPIU_RPMB_WLUN) went through fine.

Do you've some specific steps to reproduce this or a script, perhaps? If so, please can you share it with me. I will try again.

I was using a device that gives occasional errors, but I will what see I can do.

Quoted 74 lines

My test environment is in 5.10 kernel with Android, I suppose that should be ok though.

Thanks
-asd

Quoted 67 lines

Here are the call stacks

[   34.094321] Workqueue: ufs_eh_wq_0 ufshcd_err_handler [   34.094788] Call Trace:
[   34.095281]  __schedule+0x275/0x6c0
[   34.095743]  schedule+0x41/0xa0
[   34.096240]  blk_queue_enter+0x10d/0x230
[   34.096693]  ? wait_woken+0x70/0x70
[   34.097167]  blk_mq_alloc_request+0x53/0xc0 [   34.097610]  blk_get_request+0x1e/0x60
[   34.098053]  __scsi_execute+0x3c/0x260
[   34.098529]  ufshcd_clear_ua_wlun.cold+0xa6/0x14b [   34.098977]  ufshcd_clear_ua_wluns.part.0+0x4d/0x92 [   34.099456]  ufshcd_err_handler+0x97a/0x9ff [   34.099902]  process_one_work+0x1cc/0x360
[   34.100384]  worker_thread+0x45/0x3b0
[   34.100851]  ? process_one_work+0x360/0x360 [   34.101308]  kthread+0xf6/0x130
[   34.101728]  ? kthread_park+0x80/0x80
[   34.102186]  ret_from_fork+0x1f/0x30

[   34.640751] task:kworker/u10:9   state:D stack:14528 pid:  255 ppid:     2 flags:0x00004000 [   34.641253] Workqueue: events_unbound async_run_entry_fn
[   34.641722] Call Trace:
[   34.642217]  __schedule+0x275/0x6c0
[   34.642683]  schedule+0x41/0xa0
[   34.643179]  schedule_timeout+0x18b/0x290
[   34.643645]  ? del_timer_sync+0x30/0x30
[   34.644131]  __down_timeout+0x6b/0xc0
[   34.644568]  ? ufshcd_clkscale_enable_show+0x20/0x20
[   34.645014]  ? async_schedule_node_domain+0x17d/0x190
[   34.645496]  down_timeout+0x42/0x50
[   34.645947]  ufshcd_wl_suspend+0x79/0xa0
[   34.646432]  ? scmd_printk+0x100/0x100
[   34.646917]  scsi_bus_suspend_common+0x56/0xc0
[   34.647405]  ? scsi_bus_freeze+0x10/0x10
[   34.647858]  dpm_run_callback+0x45/0x110
[   34.648347]  __device_suspend+0x117/0x460
[   34.648788]  async_suspend+0x16/0x90
[   34.649251]  async_run_entry_fn+0x26/0x110
[   34.649676]  process_one_work+0x1cc/0x360
[   34.650137]  worker_thread+0x45/0x3b0
[   34.650563]  ? process_one_work+0x360/0x360
[   34.650994]  kthread+0xf6/0x130
[   34.651455]  ? kthread_park+0x80/0x80
[   34.651882]  ret_from_fork+0x1f/0x30

Quoted 17 lines
Quoted 16 lines

I'll address the other comments in the next version.

Thank you!

Quoted 8 lines
Quoted 3 lines
drivers/scsi/ufs/ufshcd.c +1 -1
-    pm_runtime_put(hba->dev);
+    ufshcd_rpm_put(hba);
    }

<SNIP>

Quoted 2 lines
(quoted diff) +2 -0
+void ufshcd_resume_complete(struct device *dev)
+{
#8 lore raw

Re: [PATCH v20 1/2] scsi: ufs: Enable power management for wlun

Quoted 54 lines · Adrian Hunter
Quoted 33 lines · Asutosh Das (asd)
Quoted 32 lines · Adrian Hunter
Quoted 31 lines · Adrian Hunter
Quoted 29 lines · Asutosh Das (asd)
Quoted 9 lines · Adrian Hunter
Quoted 5 lines · Asutosh Das
Co-developed-by Can Guo <[email protected]>
Signed-off-by Can Guo <[email protected]>
Signed-off-by Asutosh Das <[email protected]>

---

I came across 3 issues while testing.  See comments below.

Hi Adrian
Thanks for the comments.

Quoted 12 lines

<SNIP>

Review static void ufshcd_err_handling_unprepare(struct ufs_hba *hba) · line 5841

Quoted 4 lines
(quoted diff) +0 -0
@@ -5794,7 +5839,7 @@ static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
        if (ufshcd_is_clkscaling_supported(hba))
            ufshcd_clk_scaling_suspend(hba, false);
        ufshcd_clear_ua_wluns(hba);

ufshcd_clear_ua_wluns() deadlocks trying to clear UFS_UPIU_RPMB_WLUN if sdev_rpmb is suspended and sdev_ufs_device is suspending. e.g. ufshcd_wl_suspend() is waiting on host_sem while ufshcd_err_handler() is running, at which point sdev_rpmb has already suspended.

Umm, I didn't understand this deadlock.
When you say, sdev_rpmb is suspended, does it mean runtime_suspended?
sdev_ufs_device is suspending - this can't be runtime_suspending, while ufshcd_err_handling_unprepare is running.

If you've a call-stack of this deadlock, please can you share it with me. I'll also try to reproduce this.

Quoted 8 lines
Quoted 4 lines
Quoted 3 lines

Yes it is system suspend. sdev_rpmb has suspended, sdev_ufs_device is waiting on host_sem. ufshcd_err_handler() holds host_sem. ufshcd_clear_ua_wlun(UFS_UPIU_RPMB_WLUN) gets stuck. I will get some call-stacks.

Hi Adrian,

Thanks for the call stacks.
From the current information, I can't say for sure why it'd get stuck in blk_queue_enter().

I presume SCSI is leaving the RPMB WLUN device runtime suspended and consequently the queue status is RPM_SUSPENDED

Quoted 9 lines

I tried reproducing this issue on my setup yesterday but couldn't.
Here's what I did:
1. sdev_rpmb is RPM_SUSPENDED, checked before initiating system suspend
2. sdev_ufs_device is RPM_RESUMED
3. I triggered system suspend (echo mem > /sys/power/state) and scheduled the error handler from ufshcd_wl_suspend().
4. Waited until error handler ran and then ufshcd_wl_suspend() blocks on host_sem.
5. The ufshcd_clear_wa_wlun(UFS_UPIU_RPMB_WLUN) went through fine.

Quoted 3 lines
Quoted 1 line

Do you've some specific steps to reproduce this or a script, perhaps? If so, please can you share it with me. I will try again.

I was using a device that gives occasional errors, but I will what see I can do.

I have attached 2 patches to reproduce the issue, and the test script I used. Note it is using down_timeout() so it doesn't lock up completely. The kernel messages are:

[   79.385456] ufshcd_wl_suspend: doing ufshcd_schedule_eh_work
[   79.385511] ufshcd_wl_suspend: sleeping 1000 ms
[   79.386916] ufshcd_err_handler: start
[   79.386979] ufshcd_err_handler: got sem
[   79.387053] ufshcd_err_handling_unprepare: start
[   79.387302] ufshcd_clear_ua_wluns: before ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN)
[   80.435878] ufshcd_wl_suspend: trying to get sem
[   85.683876] ufshcd_wl_suspend: failed to get sem
[   85.683993] ufs_device_wlun 0:0:0:49488: PM: failed to suspend async: error -22
[   85.686901] ufshcd_clear_ua_wluns: after ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN), ret 0
[   85.687025] ufshcd_err_handling_unprepare: finish
[   85.687090] ufshcd_err_handler: finish
Quoted 1 line

From c21ea993b28bec894f96c0c7b387f9688b01900f Mon Sep 17 00:00:00 2001

From: Adrian Hunter <[email protected]>
Date: Tue, 20 Apr 2021 14:33:03 +0300
Subject: [PATCH 1/2] changes suggested since v20

Signed-off-by Adrian Hunter <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 50 ++++++++++-----------------------------
 drivers/scsi/ufs/ufshcd.h |  1 +
 2 files changed, 14 insertions(+), 37 deletions(-)
drivers/scsi/ufs/ufshcd.c +13 -37
index eb36f753469d..5be5b2472db0 100644
@@ -5844,7 +5844,7 @@ static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
 
 static inline bool ufshcd_err_handling_should_stop(struct ufs_hba *hba)
 {
-	return (!hba->is_powered || hba->shutting_down ||
+	return (!hba->is_powered || hba->shutting_down || !hba->sdev_ufs_device ||
 		hba->ufshcd_state == UFSHCD_STATE_ERROR ||
 		(!(hba->saved_err || hba->saved_uic_err || hba->force_reset ||
 		   ufshcd_is_link_broken(hba))));

@@ -9572,51 +9572,27 @@ void ufshcd_resume_complete(struct device *dev)
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);
 
-	ufshcd_rpm_put(hba);
+	if (hba->complete_put) {
+		hba->complete_put = false;
+		ufshcd_rpm_put(hba);
+	}
 }
 EXPORT_SYMBOL_GPL(ufshcd_resume_complete);
 
 int ufshcd_suspend_prepare(struct device *dev)
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);
-	struct device *ufs_dev = &hba->sdev_ufs_device->sdev_gendev;
-	enum ufs_dev_pwr_mode spm_pwr_mode;
-	enum uic_link_state spm_link_state;
-	unsigned long flags;
-	bool rpm_state_ok;
-
-	/*
-	 * SCSI assumes that runtime-pm and system-pm for scsi drivers
-	 * are same. And it doesn't wake up the device for system-suspend
-	 * if it's runtime suspended. But ufs doesn't follow that.
-	 * The rpm-lvl and spm-lvl can be different in ufs.
-	 * However, if the current_{pwr_mode, link_state} is same as the
-	 * desired_{pwr_mode, link_state}, there's no need to rpm resume
-	 * the device.
-	 * Refer ufshcd_resume_complete()
-	 */
-	pm_runtime_get_noresume(ufs_dev);
-
-	spin_lock_irqsave(&ufs_dev->power.lock, flags);
-
-	spm_pwr_mode = ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl);
-	spm_link_state = ufs_get_pm_lvl_to_link_pwr_state(hba->spm_lvl);
-
-	rpm_state_ok = pm_runtime_suspended(ufs_dev) &&
-		hba->curr_dev_pwr_mode == spm_pwr_mode &&
-		hba->uic_link_state == spm_link_state &&
-		!hba->dev_info.b_rpm_dev_flush_capable;
-
-	spin_unlock_irqrestore(&ufs_dev->power.lock, flags);
+	int ret;
 
-	if (!rpm_state_ok) {
-		int ret = pm_runtime_resume(ufs_dev);
+	if (!hba->sdev_ufs_device)
+		return 0;
 
-		if (ret < 0 && ret != -EACCES) {
-			pm_runtime_put(ufs_dev);
-			return ret;
-		}
+	ret = ufshcd_rpm_get_sync(hba);
+	if (ret < 0 && ret != -EACCES) {
+		ufshcd_rpm_put(hba);
+		return ret;
 	}
+	hba->complete_put = true;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare);
drivers/scsi/ufs/ufshcd.h +1 -0
index 3a8dc0dfae05..fd5be083e168 100644
@@ -850,6 +850,7 @@ struct ufs_hba {
 	u32 debugfs_ee_rate_limit_ms;
 #endif
 	u32 luns_avail;
+	bool complete_put;
 };
 
 /* Returns true if clocks can be gated. Otherwise false */
Signature · 1 line
2.25.1
Quoted 1 line

From 2b9aae51ac5b1f1b13fdab5570dceefbbdc9682b Mon Sep 17 00:00:00 2001

From: Adrian Hunter <[email protected]>
Date: Fri, 23 Apr 2021 08:56:18 +0300
Subject: [PATCH 2/2] instrumentation to reproduce issue

Signed-off-by Adrian Hunter <[email protected]>
---
 drivers/scsi/ufs/ufshcd.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)
drivers/scsi/ufs/ufshcd.c +22 -4
index 5be5b2472db0..32fc4e3991ff 100644
@@ -5834,12 +5834,14 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
 
 static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
 {
+	pr_err("%s: start\n", __func__);
 	ufshcd_scsi_unblock_requests(hba);
 	ufshcd_release(hba);
 	if (ufshcd_is_clkscaling_supported(hba))
 		ufshcd_clk_scaling_suspend(hba, false);
 	ufshcd_clear_ua_wluns(hba);
 	ufshcd_rpm_put(hba);
+	pr_err("%s: finish\n", __func__);
 }
 
 static inline bool ufshcd_err_handling_should_stop(struct ufs_hba *hba)

@@ -5919,11 +5921,14 @@ static void ufshcd_err_handler(struct work_struct *work)
 	int tag;
 	bool needs_reset = false, needs_restore = false;
 
+	pr_err("%s: start\n", __func__);
+
 	hba = container_of(work, struct ufs_hba, eh_work);
 
 	down(&hba->host_sem);
+	pr_err("%s: got sem\n", __func__);
 	spin_lock_irqsave(hba->host->host_lock, flags);
-	if (ufshcd_err_handling_should_stop(hba)) {
+	if (0 && ufshcd_err_handling_should_stop(hba)) {
 		if (hba->ufshcd_state != UFSHCD_STATE_ERROR)
 			hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
 		spin_unlock_irqrestore(hba->host->host_lock, flags);

@@ -6095,6 +6100,7 @@ static void ufshcd_err_handler(struct work_struct *work)
 	ufshcd_clear_eh_in_progress(hba);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 	ufshcd_err_handling_unprepare(hba);
+	pr_err("%s: finish\n", __func__);
 	up(&hba->host_sem);
 }
 

@@ -7872,12 +7878,15 @@ static int ufshcd_clear_ua_wluns(struct ufs_hba *hba)
 {
 	int ret = 0;
 
-	if (!hba->wlun_dev_clr_ua)
+	if (0 && !hba->wlun_dev_clr_ua)
 		goto out;
 
 	ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_UFS_DEVICE_WLUN);
-	if (!ret)
+	if (!ret) {
+		pr_err("%s: before ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN)\n", __func__);
 		ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
+		pr_err("%s: after ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN), ret %d\n", __func__, ret);
+	}
 	if (!ret)
 		hba->wlun_dev_clr_ua = false;
 out:

@@ -8983,7 +8992,16 @@ static int ufshcd_wl_suspend(struct device *dev)
 	ktime_t start = ktime_get();
 
 	hba = shost_priv(sdev->host);
-	down(&hba->host_sem);
+	pr_err("%s: doing ufshcd_schedule_eh_work\n", __func__);
+	ufshcd_schedule_eh_work(hba);
+	pr_err("%s: sleeping 1000 ms\n", __func__);
+	msleep(1000);
+	pr_err("%s: trying to get sem\n", __func__);
+	if (down_timeout(&hba->host_sem, msecs_to_jiffies(5000))) {
+		pr_err("%s: failed to get sem\n", __func__);
+		return -EINVAL;
+	}
+	pr_err("%s: got sem\n", __func__);
 
 	if (pm_runtime_suspended(dev))
 		goto out;
Signature · 1 line
2.25.1
#9 lore raw

Re: [PATCH v20 1/2] scsi: ufs: Enable power management for wlun

Quoted 63 lines · Adrian Hunter
Quoted 54 lines · Adrian Hunter
Quoted 33 lines · Asutosh Das (asd)
Quoted 32 lines · Adrian Hunter
Quoted 31 lines · Adrian Hunter
Quoted 29 lines · Asutosh Das (asd)
Quoted 9 lines · Adrian Hunter
Quoted 5 lines · Asutosh Das
Co-developed-by Can Guo <[email protected]>
Signed-off-by Can Guo <[email protected]>
Signed-off-by Asutosh Das <[email protected]>

---

I came across 3 issues while testing.  See comments below.

Hi Adrian
Thanks for the comments.

Quoted 12 lines

<SNIP>

drivers/scsi/ufs/ufshcd.c static void ufshcd_err_handling_unprepare(struct ufs_hba *hba) · line 5841

Quoted 4 lines
drivers/scsi/ufs/ufshcd.c +0 -0
@@ -5794,7 +5839,7 @@ static void ufshcd_err_handling_unprepare(struct ufs_hba *hba)
        if (ufshcd_is_clkscaling_supported(hba))
            ufshcd_clk_scaling_suspend(hba, false);
        ufshcd_clear_ua_wluns(hba);

ufshcd_clear_ua_wluns() deadlocks trying to clear UFS_UPIU_RPMB_WLUN if sdev_rpmb is suspended and sdev_ufs_device is suspending. e.g. ufshcd_wl_suspend() is waiting on host_sem while ufshcd_err_handler() is running, at which point sdev_rpmb has already suspended.

Umm, I didn't understand this deadlock.
When you say, sdev_rpmb is suspended, does it mean runtime_suspended?
sdev_ufs_device is suspending - this can't be runtime_suspending, while ufshcd_err_handling_unprepare is running.

If you've a call-stack of this deadlock, please can you share it with me. I'll also try to reproduce this.

Quoted 8 lines
Quoted 4 lines
Quoted 3 lines

Yes it is system suspend. sdev_rpmb has suspended, sdev_ufs_device is waiting on host_sem. ufshcd_err_handler() holds host_sem. ufshcd_clear_ua_wlun(UFS_UPIU_RPMB_WLUN) gets stuck. I will get some call-stacks.

Hi Adrian,

Thanks for the call stacks.
From the current information, I can't say for sure why it'd get stuck in blk_queue_enter().

I presume SCSI is leaving the RPMB WLUN device runtime suspended and consequently the queue status is RPM_SUSPENDED

Quoted 9 lines

I tried reproducing this issue on my setup yesterday but couldn't.
Here's what I did:
1. sdev_rpmb is RPM_SUSPENDED, checked before initiating system suspend
2. sdev_ufs_device is RPM_RESUMED
3. I triggered system suspend (echo mem > /sys/power/state) and scheduled the error handler from ufshcd_wl_suspend().
4. Waited until error handler ran and then ufshcd_wl_suspend() blocks on host_sem.
5. The ufshcd_clear_wa_wlun(UFS_UPIU_RPMB_WLUN) went through fine.

Quoted 3 lines
Quoted 1 line

Do you've some specific steps to reproduce this or a script, perhaps? If so, please can you share it with me. I will try again.

I was using a device that gives occasional errors, but I will what see I can do.

I have attached 2 patches to reproduce the issue, and the test script I used. Note it is using down_timeout() so it doesn't lock up completely. The kernel messages are:

Quoted 14 lines
[   79.385456] ufshcd_wl_suspend: doing ufshcd_schedule_eh_work
[   79.385511] ufshcd_wl_suspend: sleeping 1000 ms
[   79.386916] ufshcd_err_handler: start
[   79.386979] ufshcd_err_handler: got sem
[   79.387053] ufshcd_err_handling_unprepare: start
[   79.387302] ufshcd_clear_ua_wluns: before ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN)
[   80.435878] ufshcd_wl_suspend: trying to get sem
[   85.683876] ufshcd_wl_suspend: failed to get sem
[   85.683993] ufs_device_wlun 0:0:0:49488: PM: failed to suspend async: error -22
[   85.686901] ufshcd_clear_ua_wluns: after ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN), ret 0
[   85.687025] ufshcd_err_handling_unprepare: finish
[   85.687090] ufshcd_err_handler: finish

I think we also need to runtime resume RPMB WLUN before system suspend. e.g.

+static int ufshcd_rpmb_rpm_get_sync(struct ufs_hba *hba) +{
+ return pm_runtime_get_sync(&hba->sdev_rpmb->sdev_gendev);
+}
+
+static int ufshcd_rpmb_rpm_put(struct ufs_hba *hba) +{
+ return pm_runtime_put(&hba->sdev_rpmb->sdev_gendev);
+}
+
void ufshcd_resume_complete(struct device *dev) {
struct ufs_hba *hba = dev_get_drvdata(dev);

+ if (hba->rpmb_complete_put) {
+ hba->rpmb_complete_put = false;
+ ufshcd_rpmb_rpm_put(hba);
+ }
if (hba->complete_put) {
hba->complete_put = false; ufshcd_rpm_put(hba);
@@ -9611,6 +9625,11 @@ int ufshcd_suspend_prepare(struct device *dev)
return ret;
}
hba->complete_put = true;
+
+ if (hba->sdev_rpmb) {
+ ufshcd_rpmb_rpm_get_sync(hba);
+ hba->rpmb_complete_put = true;
+ }
return 0;
}

That also avoids another issue: if RPMB WLUN is runtime suspended at system resume, we have to skip clearing UAC, but SCSI PM will force the runtime status to RPM_ACTIVE after system resume, so the UAC never gets cleared in that case.

Furthermore, it seems better not to report errors from RPMB resume and instead let the error handler sort it out. So, with the above change, we can simplify a bit:

-static int ufshcd_rpmb_runtime_resume(struct device *dev) -{
- struct ufs_hba *hba = wlun_dev_to_hba(dev);
-
- if (hba->sdev_rpmb)
- return ufshcd_clear_rpmb_uac(hba);
- return 0;
-}
-
static int ufshcd_rpmb_resume(struct device *dev) {
struct ufs_hba *hba = wlun_dev_to_hba(dev);

- if (hba->sdev_rpmb && !pm_runtime_suspended(dev))
- return ufshcd_clear_rpmb_uac(hba);
+ if (hba->sdev_rpmb)
+ ufshcd_clear_rpmb_uac(hba);
return 0;
}

static const struct dev_pm_ops ufs_rpmb_pm_ops = {
- SET_RUNTIME_PM_OPS(NULL, ufshcd_rpmb_runtime_resume, NULL)
+ SET_RUNTIME_PM_OPS(NULL, ufshcd_rpmb_resume, NULL)
SET_SYSTEM_SLEEP_PM_OPS(NULL, ufshcd_rpmb_resume)
};

author
#10 lore raw

Re: [PATCH v20 1/2] scsi: ufs: Enable power management for wlun

Quoted 3 lines · Adrian Hunter
Quoted 2 lines

Hi Adrian,
Thanks for the help.
I made the changes and tried to reproduce it.
My setup becomes non-responsive and resets.
I don't think it's related to this issue though.

I hadn't set rpm_lvl and spm_lvl to 0 while testing the other day.
Setting those to 0, it proceeds further and now it becomes unresponsive. So I still don't clearly see the issue that you're seeing. However, I'd make the suggested changes and push it in next version.

Quoted 68 lines

I think we also need to runtime resume RPMB WLUN before system suspend. e.g.

(quoted diff) +19 -0
+static int ufshcd_rpmb_rpm_get_sync(struct ufs_hba *hba)
+{
+	return pm_runtime_get_sync(&hba->sdev_rpmb->sdev_gendev);
+}
+
+static int ufshcd_rpmb_rpm_put(struct ufs_hba *hba)
+{
+	return pm_runtime_put(&hba->sdev_rpmb->sdev_gendev);
+}
+
  void ufshcd_resume_complete(struct device *dev)
  {
  	struct ufs_hba *hba = dev_get_drvdata(dev);
 
+	if (hba->rpmb_complete_put) {
+		hba->rpmb_complete_put = false;
+		ufshcd_rpmb_rpm_put(hba);
+	}
  	if (hba->complete_put) {
  		hba->complete_put = false;
  		ufshcd_rpm_put(hba);

@@ -9611,6 +9625,11 @@ int ufshcd_suspend_prepare(struct device *dev)
  		return ret;
  	}
  	hba->complete_put = true;
+
+	if (hba->sdev_rpmb) {
+		ufshcd_rpmb_rpm_get_sync(hba);
+		hba->rpmb_complete_put = true;
+	}
  	return 0;
  }

That also avoids another issue: if RPMB WLUN is runtime suspended at system resume, we have to skip clearing UAC, but SCSI PM will force the runtime status to RPM_ACTIVE after system resume, so the UAC never gets cleared in that case.

Furthermore, it seems better not to report errors from RPMB resume and instead let the error handler sort it out. So, with the above change, we can simplify a bit:

(quoted diff) +3 -12
-static int ufshcd_rpmb_runtime_resume(struct device *dev)
-{
-	struct ufs_hba *hba = wlun_dev_to_hba(dev);
-
-	if (hba->sdev_rpmb)
-		return ufshcd_clear_rpmb_uac(hba);
-	return 0;
-}
-
  static int ufshcd_rpmb_resume(struct device *dev)
  {
  	struct ufs_hba *hba = wlun_dev_to_hba(dev);
 
-	if (hba->sdev_rpmb && !pm_runtime_suspended(dev))
-		return ufshcd_clear_rpmb_uac(hba);
+	if (hba->sdev_rpmb)
+		ufshcd_clear_rpmb_uac(hba);
  	return 0;
  }
 
  static const struct dev_pm_ops ufs_rpmb_pm_ops = {
-	SET_RUNTIME_PM_OPS(NULL, ufshcd_rpmb_runtime_resume, NULL)
+	SET_RUNTIME_PM_OPS(NULL, ufshcd_rpmb_resume, NULL)
  	SET_SYSTEM_SLEEP_PM_OPS(NULL, ufshcd_rpmb_resume)
  };
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project