Skip to contentSkip to conversation

Enable power management for ufs wlun

PATCH Superseded Asutosh Das 3 participants 9 messages Apr 8, 2021 – Apr 11, 2021

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

v17 of 21 Superseded Latest is v21 →

lore
Share & tools
lore

Asutosh Das

author
#0 lore raw

v17 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

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       |  30 +-
 drivers/scsi/ufs/ufs_bsg.c         |   6 +-
 drivers/scsi/ufs/ufshcd-pci.c      |  36 +--
 drivers/scsi/ufs/ufshcd.c          | 642 ++++++++++++++++++++++++++-----------
 drivers/scsi/ufs/ufshcd.h          |   6 +
 include/trace/events/ufs.h         |  20 ++
 14 files changed, 526 insertions(+), 244 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 v17 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.

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          | 642 ++++++++++++++++++++++++++-----------
 drivers/scsi/ufs/ufshcd.h          |   6 +
 include/trace/events/ufs.h         |  20 ++
 13 files changed, 509 insertions(+), 231 deletions(-)
drivers/scsi/ufs/cdns-pltfrm.c +2 -0
index 13d9204..b9105e4 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..b01db12 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..1e8d92b 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);
+	scsi_autopm_get_device(hba->sdev_ufs_device);
 	return 0;
 }
 
 static void ufs_debugfs_put_user_access(struct ufs_hba *hba)
 __releases(&hba->host_sem)
 {
-	pm_runtime_put_sync(hba->dev);
+	scsi_autopm_put_device(hba->sdev_ufs_device);
 	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..49a918c 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..d463b44 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..cbb5a90 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);
+	scsi_autopm_get_device(hba->sdev_ufs_device);
 
 	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);
+			scsi_autopm_put_device(hba->sdev_ufs_device);
 			goto out;
 		}
 

@@ -138,7 +138,7 @@ static int ufs_bsg_request(struct bsg_job *job)
 		break;
 	}
 
-	pm_runtime_put_sync(hba->dev);
+	scsi_autopm_put_device(hba->sdev_ufs_device);
 
 	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 +455 -187

980 lines not on this page · raw message

drivers/scsi/ufs/ufshcd.h +6 -0
index 5eb66a8..40436e3 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)
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
#7 lore raw

PATCH v17 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 Can Guo <[email protected]>
Signed-off-by Asutosh Das <[email protected]>
---
 drivers/scsi/ufs/ufs-sysfs.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)
drivers/scsi/ufs/ufs-sysfs.c +17 -13
index d7c3cff..fa57bac 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);
+	scsi_autopm_get_device(hba->sdev_ufs_device);
 	res = ufshcd_wb_toggle(hba, wb_enable);
-	pm_runtime_put_sync(hba->dev);
+	scsi_autopm_put_device(hba->sdev_ufs_device);
 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);
+	scsi_autopm_get_device(hba->sdev_ufs_device);
 	ret = ufshcd_read_desc_param(hba, desc_id, desc_index,
 				param_offset, desc_buf, param_size);
-	pm_runtime_put_sync(hba->dev);
+	scsi_autopm_put_device(hba->sdev_ufs_device);
 	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);					\
+	scsi_autopm_get_device(hba->sdev_ufs_device);			\
 	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);					\
+	scsi_autopm_put_device(hba->sdev_ufs_device);			\
 	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);					\
+	scsi_autopm_get_device(hba->sdev_ufs_device);			\
 	ret = ufshcd_query_flag(hba, UPIU_QUERY_OPCODE_READ_FLAG,	\
 		QUERY_FLAG_IDN##_uname, index, &flag);			\
-	pm_runtime_put_sync(hba->dev);					\
+	scsi_autopm_put_device(hba->sdev_ufs_device);			\
 	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);					\
+	scsi_autopm_get_device(hba->sdev_ufs_device);			\
 	ret = ufshcd_query_attr(hba, UPIU_QUERY_OPCODE_READ_ATTR,	\
 		QUERY_ATTR_IDN##_uname, index, 0, &value);		\
-	pm_runtime_put_sync(hba->dev);					\
+	scsi_autopm_put_device(hba->sdev_ufs_device);			\
 	if (ret) {							\
 		ret = -EINVAL;						\
 		goto out;						\

@@ -899,11 +899,15 @@ static ssize_t _pname##_show(struct device *dev,			\
 	struct scsi_device *sdev = to_scsi_device(dev);			\
 	struct ufs_hba *hba = shost_priv(sdev->host);			\
 	u8 lun = ufshcd_scsi_to_upiu_lun(sdev->lun);			\
+	int ret;							\
 	if (!ufs_is_valid_unit_desc_lun(&hba->dev_info, lun,		\
 				_duname##_DESC_PARAM##_puname))		\
 		return -EINVAL;						\
-	return ufs_sysfs_read_desc_param(hba, QUERY_DESC_IDN_##_duname,	\
+	scsi_autopm_get_device(sdev);					\
+	ret = ufs_sysfs_read_desc_param(hba, QUERY_DESC_IDN_##_duname,	\
 		lun, _duname##_DESC_PARAM##_puname, buf, _size);	\
+	scsi_autopm_put_device(sdev);					\
+	return ret;							\
 }									\
 static DEVICE_ATTR_RO(_pname)
 

@@ -964,10 +968,10 @@ static ssize_t dyn_cap_needed_attribute_show(struct device *dev,
 		goto out;
 	}
 
-	pm_runtime_get_sync(hba->dev);
+	scsi_autopm_get_device(hba->sdev_ufs_device);
 	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);
+	scsi_autopm_put_device(hba->sdev_ufs_device);
 	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 v17 1/2] scsi: ufs: Enable power management for wlun

In reply to Asutosh Das

Hi Asutosh Das,

Quoted 43 lines

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.

Co-developed-by Can Guo <[email protected]>
Signed-off-by Can Guo <[email protected]>
Signed-off-by Asutosh Das <[email protected]>

---

Quoted 14 lines
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          | 642 ++++++++++++++++++++++++++-----------
drivers/scsi/ufs/ufshcd.h          |   6 +
include/trace/events/ufs.h         |  20 ++
13 files changed, 509 insertions(+), 231 deletions(-)

In this patch, you changed pm_runtime_{get, put}_sync to scsi_autopm_{get, put}_device. But, scsi_autopm_get_device() calls pm_runtime_put_sync() in case of error of pm_runtime_get_sync(). So, pm_runtime_put_sync() can be called twice if scsi_autopm_get_device has error.

Thanks,
Daejun

#3 lore raw

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

Quoted 60 lines · Daejun Park

Hi Asutosh Das,

Quoted 43 lines

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.

Co-developed-by Can Guo <[email protected]>
Signed-off-by Can Guo <[email protected]>
Signed-off-by Asutosh Das <[email protected]>

---

Quoted 14 lines
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          | 642 ++++++++++++++++++++++++++-----------
drivers/scsi/ufs/ufshcd.h          |   6 +
include/trace/events/ufs.h         |  20 ++
13 files changed, 509 insertions(+), 231 deletions(-)
Quoted 4 lines

In this patch, you changed pm_runtime_{get, put}_sync to scsi_autopm_{get, put}_device. But, scsi_autopm_get_device() calls pm_runtime_put_sync() in case of error of pm_runtime_get_sync(). So, pm_runtime_put_sync() can be called twice if scsi_autopm_get_device has error.

Also it might be tidy to make wrappers e.g.

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(struct ufs_hba *hba)

{
    return pm_runtime_put(&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);
}

And also consider matching: e.g.

	pm_runtime_put(hba->dev)	to	ufshcd_rpm_put(hba)
	pm_runtime_put_sync(hba->dev)	to	ufshcd_rpm_put_sync(hba)
author
#4 lore raw

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

Quoted 79 lines · Adrian Hunter
Quoted 60 lines · Daejun Park

Hi Asutosh Das,

Quoted 43 lines

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.

Co-developed-by Can Guo <[email protected]>
Signed-off-by Can Guo <[email protected]>
Signed-off-by Asutosh Das <[email protected]>

---

Quoted 14 lines
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          | 642 ++++++++++++++++++++++++++-----------
drivers/scsi/ufs/ufshcd.h          |   6 +
include/trace/events/ufs.h         |  20 ++
13 files changed, 509 insertions(+), 231 deletions(-)
Quoted 4 lines

In this patch, you changed pm_runtime_{get, put}_sync to scsi_autopm_{get, put}_device. But, scsi_autopm_get_device() calls pm_runtime_put_sync() in case of error of pm_runtime_get_sync(). So, pm_runtime_put_sync() can be called twice if scsi_autopm_get_device has error.

Also it might be tidy to make wrappers e.g.

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(struct ufs_hba *hba)

{
     return pm_runtime_put(&hba->sdev_ufs_device->sdev_gendev);
}

static inline int ufshcd_rpm_put_sync(struct ufs_hba *hba)

Quoted 11 lines
{
     return pm_runtime_put_sync(&hba->sdev_ufs_device->sdev_gendev);
}

And also consider matching: e.g.

	pm_runtime_put(hba->dev)	to	ufshcd_rpm_put(hba)
	pm_runtime_put_sync(hba->dev)	to	ufshcd_rpm_put_sync(hba)

Ok, I'll push the changes shortly.

Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project
#5 lore raw

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

Quoted 85 lines · Asutosh Das (asd)
Quoted 84 lines · Adrian Hunter
Quoted 60 lines · Daejun Park

Hi Asutosh Das,

Quoted 56 lines

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.

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          | 642 ++++++++++++++++++++++++++----------- drivers/scsi/ufs/ufshcd.h          |   6 + include/trace/events/ufs.h         |  20 ++

Quoted 1 line
13 files changed, 509 insertions(+), 231 deletions(-)
Quoted 4 lines

In this patch, you changed pm_runtime_{get, put}_sync to scsi_autopm_{get, put}_device. But, scsi_autopm_get_device() calls pm_runtime_put_sync() in case of error of pm_runtime_get_sync(). So, pm_runtime_put_sync() can be called twice if scsi_autopm_get_device has error.

Also it might be tidy to make wrappers e.g.

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(struct ufs_hba *hba) {
     return pm_runtime_put(&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); }

And also consider matching: e.g.

Quoted 8 lines
Quoted 5 lines

    pm_runtime_put(hba->dev)    to    ufshcd_rpm_put(hba) pm_runtime_put_sync(hba->dev)    to    ufshcd_rpm_put_sync(hba)

Ok, I'll push the changes shortly.

I think I will have some more comments, so you could wait if you want.

#8 lore raw

Re: [PATCH v17 2/2] ufs: sysfs: Resume the proper scsi device

In reply to Asutosh Das
Quoted 13 lines · Asutosh Das

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

Reviewed-by Can Guo <[email protected]>
Signed-off-by Asutosh Das <[email protected]>
---
 drivers/scsi/ufs/ufs-sysfs.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)
drivers/scsi/ufs/ufs-sysfs.c +0 -0
index d7c3cff..fa57bac 100644

<SNIP>

Quoted 4 lines
drivers/scsi/ufs/ufs-sysfs.c +0 -0
@@ -899,11 +899,15 @@ static ssize_t _pname##_show(struct device *dev,			\
 	struct scsi_device *sdev = to_scsi_device(dev);			\
 	struct ufs_hba *hba = shost_priv(sdev->host);			\
 	u8 lun = ufshcd_scsi_to_upiu_lun(sdev->lun);			\

drivers/scsi/ufs/ufs-sysfs.c static ssize_t _pname##_show(struct device *dev, \ · line 910

Quoted 10 lines
drivers/scsi/ufs/ufs-sysfs.c +5 -1
+	int ret;							\
 	if (!ufs_is_valid_unit_desc_lun(&hba->dev_info, lun,		\
 				_duname##_DESC_PARAM##_puname))		\
 		return -EINVAL;						\
-	return ufs_sysfs_read_desc_param(hba, QUERY_DESC_IDN_##_duname,	\
+	scsi_autopm_get_device(sdev);					\
+	ret = ufs_sysfs_read_desc_param(hba, QUERY_DESC_IDN_##_duname,	\
 		lun, _duname##_DESC_PARAM##_puname, buf, _size);	\
+	scsi_autopm_put_device(sdev);					\
+	return ret;							\

I am not sure why this change is needed. It it is needed, please add a comment explaining.

#6 lore raw

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

In reply to Asutosh Das
Quoted 43 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.

Co-developed-by Can Guo <[email protected]>
Signed-off-by Can Guo <[email protected]>
Signed-off-by Asutosh Das <[email protected]>

---

<SNIP>

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

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +2 -2
@@ -5815,14 +5857,14 @@ static void ufshcd_recover_pm_error(struct ufs_hba *hba)
 
 	hba->is_sys_suspended = false;
 	/*
-	 * Set RPM status of hba device to RPM_ACTIVE,
+	 * Set RPM status of wlun device to RPM_ACTIVE,
 	 * this also clears its runtime error.
 	 */
-	ret = pm_runtime_set_active(hba->dev);
+	ret = pm_runtime_set_active(&hba->sdev_ufs_device->sdev_gendev);

It may be that it was hba->dev with a runtime error. So, also need here:

	if (ret)
		ret = pm_runtime_set_active(hba->dev);
Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +1 -1
@@ -8671,7 +8701,7 @@ static int ufshcd_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 = 1;
+	hba->pm_op_in_progress = true;
 	if (!ufshcd_is_shutdown_pm(pm_op)) {
 		pm_lvl = ufshcd_is_runtime_pm(pm_op) ?
 			 hba->rpm_lvl : hba->spm_lvl;

drivers/scsi/ufs/ufshcd.c static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) · line 8727

Quoted 5 lines
drivers/scsi/ufs/ufshcd.c +1 -1
@@ -8694,17 +8724,17 @@ static int ufshcd_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 
 	if (req_dev_pwr_mode == UFS_ACTIVE_PWR_MODE &&
 			req_link_state == UIC_LINK_ACTIVE_STATE) {
-		goto disable_clks;
+		goto enable_scaling;

Previously "goto disable_clks" would go to the vendor callback ufshcd_vops_suspend(), but now it is skipped.

Quoted 13 lines
drivers/scsi/ufs/ufshcd.c +2 -2
 	}
 
 	if ((req_dev_pwr_mode == hba->curr_dev_pwr_mode) &&
 	    (req_link_state == hba->uic_link_state))
-		goto enable_gating;
+		goto enable_scaling;
 
 	/* UFS device & link must be active before we enter in this function */
 	if (!ufshcd_is_ufs_dev_active(hba) || !ufshcd_is_link_active(hba)) {
 		ret = -EINVAL;
-		goto enable_gating;
+		goto enable_scaling;
 	}

<SNIP>

drivers/scsi/ufs/ufshcd.c static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) · line 9044

Quoted 11 lines
drivers/scsi/ufs/ufshcd.c +11 -0
+/**
+ * ufshcd_suspend - helper function for suspend operations
+ * @hba: per adapter instance
+ *
+ * This function will put disable irqs, turn off clocks
+ * and set vreg and hba-vreg in lpm mode.
+ * Also check the description of __ufshcd_wl_suspend().
+ */
+static void ufshcd_suspend(struct ufs_hba *hba)
+{
+	hba->pm_op_in_progress = 1;

Seems like pm_op_in_progress wouldn't be needed in this function, nor ufshcd_resume().

You could probably simplify the callers slighly by putting the "if (!hbs->is_powered) return" check here and in ufshcd_resume().

Quoted 61 lines
drivers/scsi/ufs/ufshcd.c +43 -0
+
+	/*
+	 * Disable the host irq as host controller as there won't be any
+	 * host controller transaction expected till resume.
+	 */
 	ufshcd_disable_irq(hba);
 	ufshcd_setup_clocks(hba, false);
 	if (ufshcd_is_clkgating_allowed(hba)) {

@@ -8948,6 +9054,43 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 		trace_ufshcd_clk_gating(dev_name(hba->dev),
 					hba->clk_gating.state);
 	}
+
+	ufshcd_vreg_set_lpm(hba);
+	/* Put the host controller in low power mode if possible */
+	ufshcd_hba_vreg_set_lpm(hba);
+	hba->pm_op_in_progress = 0;
+}
+
+/**
+ * ufshcd_resume - helper function for resume operations
+ * @hba: per adapter instance
+ *
+ * This function basically turns on the regulators, clocks and
+ * irqs of the hba.
+ * Also check the description of __ufshcd_wl_resume().
+ *
+ * Returns 0 for success and non-zero for failure
+ */
+static int ufshcd_resume(struct ufs_hba *hba)
+{
+	int ret;
+
+	hba->pm_op_in_progress = 1;
+
+	ufshcd_hba_vreg_set_hpm(hba);
+	ret = ufshcd_vreg_set_hpm(hba);
+	if (ret)
+		goto out;
+
+	/* Make sure clocks are enabled before accessing controller */
+	ret = ufshcd_setup_clocks(hba, true);
+	if (ret)
+		goto disable_vreg;
+
+	/* enable the host irq as host controller would be active soon */
+	ufshcd_enable_irq(hba);
+	goto out;
+
 disable_vreg:
 	ufshcd_vreg_set_lpm(hba);
 out:

@@ -8962,6 +9105,7 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
  * @hba: per adapter instance
  *
  * Check the description of ufshcd_suspend() function for more details.
+ * Also check the description of __ufshcd_wl_suspend().
  *
  * Returns 0 for success and non-zero for failure
  */

drivers/scsi/ufs/ufshcd.c int ufshcd_system_suspend(struct ufs_hba *hba) · line 9131

Quoted 6 lines
drivers/scsi/ufs/ufshcd.c +0 -0
@@ -8982,21 +9126,7 @@ int ufshcd_system_suspend(struct ufs_hba *hba)
 	if ((ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl) ==
 	     hba->curr_dev_pwr_mode) &&
 	    (ufs_get_pm_lvl_to_link_pwr_state(hba->spm_lvl) ==
 	     hba->uic_link_state) &&
 	     !hba->dev_info.b_rpm_dev_flush_capable)
 		goto out;

The logic above can be removed.

Quoted 4 lines
drivers/scsi/ufs/ufshcd.c +0 -3
-	if (pm_runtime_suspended(hba->dev)) {
-		/*
-		 * UFS device and/or UFS link low power states during runtime

drivers/scsi/ufs/ufshcd.c int ufshcd_system_suspend(struct ufs_hba *hba) · line 8997

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +0 -10
-		 * suspend seems to be different than what is expected during
-		 * system suspend. Hence runtime resume the devic & link and
-		 * let the system suspend low power states to take effect.
-		 * TODO: If resume takes longer time, we might have optimize
-		 * it in future by not resuming everything if possible.
-		 */
-		ret = ufshcd_runtime_resume(hba);
-		if (ret)
-			goto out;
-	}

System suspend/resume and runtime suspend/resume are identical for hba->dev, so the logic becomes:

	if (pm_runtime_suspended(hba->dev))
		goto out;
Quoted 9 lines
drivers/scsi/ufs/ufshcd.c +1 -2
-
-	ret = ufshcd_suspend(hba, UFS_SYSTEM_PM);
+	ufshcd_suspend(hba);

out:

drivers/scsi/ufs/ufshcd.c +0 -0
 	trace_ufshcd_system_suspend(dev_name(hba->dev), ret,
 		ktime_to_us(ktime_sub(ktime_get(), start)),

@@ -9018,25 +9148,20 @@ EXPORT_SYMBOL(ufshcd_system_suspend);
 
 int ufshcd_system_resume(struct ufs_hba *hba)

drivers/scsi/ufs/ufshcd.c EXPORT_SYMBOL(ufshcd_system_suspend); · line 9153

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +1 -6
 {
-	int ret = 0;
 	ktime_t start = ktime_get();
 
-	if (!hba->is_powered || pm_runtime_suspended(hba->dev))
-		/*
-		 * Let the runtime resume take care of resuming
-		 * if runtime suspended.
-		 */
+	if (!hba->is_powered)

System suspend/resume and runtime suspend/resume are identical for hba->dev, so the original logic is fine here.

Quoted 4 lines
drivers/scsi/ufs/ufshcd.c +1 -1
 		goto out;
 	else
-		ret = ufshcd_resume(hba, UFS_SYSTEM_PM);
+		ufshcd_resume(hba);

drivers/scsi/ufs/ufshcd.c EXPORT_SYMBOL(ufshcd_system_suspend); · line 9163

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +3 -3
 out:
-	trace_ufshcd_system_resume(dev_name(hba->dev), ret,
+	trace_ufshcd_system_resume(dev_name(hba->dev), 0,
 		ktime_to_us(ktime_sub(ktime_get(), start)),
 		hba->curr_dev_pwr_mode, hba->uic_link_state);
-	if (!ret)
-		hba->is_sys_suspended = false;
+
+	hba->is_sys_suspended = false;
 	up(&hba->host_sem);

It seems likely that hba->host_sem down / up should be in ufshcd_wl_suspend() / ufshcd_wl_resume() respectively, instead of here.

Ditto hba->is_sys_suspended

Quoted 119 lines
drivers/scsi/ufs/ufshcd.c +15 -32
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL(ufshcd_system_resume);
 

@@ -9045,23 +9170,23 @@ EXPORT_SYMBOL(ufshcd_system_resume);
  * @hba: per adapter instance
  *
  * Check the description of ufshcd_suspend() function for more details.
+ * Also check the description of __ufshcd_wl_suspend().
  *
  * Returns 0 for success and non-zero for failure
  */
 int ufshcd_runtime_suspend(struct ufs_hba *hba)
 {
-	int ret = 0;
 	ktime_t start = ktime_get();
 
 	if (!hba->is_powered)
 		goto out;
 	else
-		ret = ufshcd_suspend(hba, UFS_RUNTIME_PM);
+		ufshcd_suspend(hba);
 out:
-	trace_ufshcd_runtime_suspend(dev_name(hba->dev), ret,
+	trace_ufshcd_runtime_suspend(dev_name(hba->dev), 0,
 		ktime_to_us(ktime_sub(ktime_get(), start)),
 		hba->curr_dev_pwr_mode, hba->uic_link_state);
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL(ufshcd_runtime_suspend);
 

@@ -9069,37 +9194,25 @@ EXPORT_SYMBOL(ufshcd_runtime_suspend);
  * ufshcd_runtime_resume - runtime resume routine
  * @hba: per adapter instance
  *
- * This function basically brings the UFS device, UniPro link and controller
+ * This function basically brings controller
  * to active state. Following operations are done in this function:
  *
  * 1. Turn on all the controller related clocks
- * 2. Bring the UniPro link out of Hibernate state
- * 3. If UFS device is in sleep state, turn ON VCC rail and bring the UFS device
- *    to active state.
- * 4. If auto-bkops is enabled on the device, disable it.
- *
- * So following would be the possible power state after this function return
- * successfully:
- *	S1: UFS device in Active state with VCC rail ON
- *	    UniPro link in Active state
- *	    All the UFS/UniPro controller clocks are ON
- *
- * Returns 0 for success and non-zero for failure
+ * 2. Turn ON VCC rail
  */
 int ufshcd_runtime_resume(struct ufs_hba *hba)
 {
-	int ret = 0;
 	ktime_t start = ktime_get();
 
 	if (!hba->is_powered)
 		goto out;
 	else
-		ret = ufshcd_resume(hba, UFS_RUNTIME_PM);
+		ufshcd_resume(hba);
 out:
-	trace_ufshcd_runtime_resume(dev_name(hba->dev), ret,
+	trace_ufshcd_runtime_resume(dev_name(hba->dev), 0,
 		ktime_to_us(ktime_sub(ktime_get(), start)),
 		hba->curr_dev_pwr_mode, hba->uic_link_state);
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL(ufshcd_runtime_resume);
 

@@ -9113,18 +9226,13 @@ EXPORT_SYMBOL(ufshcd_runtime_idle);
  * ufshcd_shutdown - shutdown routine
  * @hba: per adapter instance
  *
- * This function would power off both UFS device and UFS link.
+ * This function would turn off both UFS device and UFS hba
+ * regulators. It would also disable clocks.
  *
  * Returns 0 always to allow force shutdown even in case of errors.
  */
 int ufshcd_shutdown(struct ufs_hba *hba)
 {
-	int ret = 0;
-
-	down(&hba->host_sem);
-	hba->shutting_down = true;
-	up(&hba->host_sem);
-
 	if (!hba->is_powered)
 		goto out;
 

@@ -9133,10 +9241,8 @@ int ufshcd_shutdown(struct ufs_hba *hba)
 
 	pm_runtime_get_sync(hba->dev);
 
-	ret = ufshcd_suspend(hba, UFS_SHUTDOWN_PM);
+	ufshcd_suspend(hba);
 out:
-	if (ret)
-		dev_err(hba->dev, "%s failed, err %d\n", __func__, ret);
 	hba->is_powered = false;
 	/* allow force shutdown even in case of errors */
 	return 0;

@@ -9150,6 +9256,8 @@ EXPORT_SYMBOL(ufshcd_shutdown);
  */
 void ufshcd_remove(struct ufs_hba *hba)
 {
+	if (hba->sdev_ufs_device)
+		scsi_autopm_get_device(hba->sdev_ufs_device);
 	ufs_bsg_remove(hba);
 	ufs_sysfs_remove_nodes(hba->dev);
 	blk_cleanup_queue(hba->tmf_queue);

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

Quoted 8 lines
drivers/scsi/ufs/ufshcd.c +5 -0
@@ -9453,15 +9561,175 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 }
 EXPORT_SYMBOL_GPL(ufshcd_init);
 
+void ufshcd_resume_complete(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	pm_runtime_put_noidle(&hba->sdev_ufs_device->sdev_gendev);

pm_runtime_put() instead of pm_runtime_put_noidle() would be more correct here.

Quoted 8 lines
drivers/scsi/ufs/ufshcd.c +8 -0
+}
+EXPORT_SYMBOL_GPL(ufshcd_resume_complete);
+
+int ufshcd_suspend_prepare(struct device *dev)
+{
+	struct ufs_hba *hba = dev_get_drvdata(dev);
+
+	/*

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

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +10 -0
+	 * 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.
+	 * Force it to honor system-suspend.
+	 */
+	scsi_autopm_get_device(hba->sdev_ufs_device);
+	/* Refer ufshcd_resume_complete() */
+	pm_runtime_get_noresume(&hba->sdev_ufs_device->sdev_gendev);
+	scsi_autopm_put_device(hba->sdev_ufs_device);

This could simply be:

	pm_runtime_get_sync(&hba->sdev_ufs_device->sdev_gendev);

or new wrapper.

Quoted 129 lines
drivers/scsi/ufs/ufshcd.c +126 -0
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare);
+
+#ifdef CONFIG_PM_SLEEP
+static int ufshcd_wl_poweroff(struct device *dev)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct ufs_hba *hba = shost_priv(sdev->host);
+
+	__ufshcd_wl_suspend(hba, UFS_SHUTDOWN_PM);
+	return 0;
+}
+#endif
+
+static int ufshcd_wl_probe(struct device *dev)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+
+	if (!is_device_wlun(sdev))
+		return -ENODEV;
+
+	blk_pm_runtime_init(sdev->request_queue, dev);
+	pm_runtime_set_autosuspend_delay(dev, 0);
+	pm_runtime_allow(dev);
+
+	return  0;
+}
+
+static int ufshcd_wl_remove(struct device *dev)
+{
+	pm_runtime_forbid(dev);
+	return 0;
+}
+
+static const struct dev_pm_ops ufshcd_wl_pm_ops = {
+#ifdef CONFIG_PM_SLEEP
+	.suspend = ufshcd_wl_suspend,
+	.resume = ufshcd_wl_resume,
+	.freeze = ufshcd_wl_suspend,
+	.thaw = ufshcd_wl_resume,
+	.poweroff = ufshcd_wl_poweroff,
+	.restore = ufshcd_wl_resume,
+#endif
+	SET_RUNTIME_PM_OPS(ufshcd_wl_runtime_suspend, ufshcd_wl_runtime_resume, NULL)
+};
+
+/**
+ * ufs_dev_wlun_template - describes ufs device wlun
+ * ufs-device wlun - used to send pm commands
+ * All luns are consumers of ufs-device wlun.
+ *
+ * Currently, no sd driver is present for wluns.
+ * Hence the no specific pm operations are performed.
+ * With ufs design, SSU should be sent to ufs-device wlun.
+ * Hence register a scsi driver for ufs wluns only.
+ */
+static struct scsi_driver ufs_dev_wlun_template = {
+	.gendrv = {
+		.name = "ufs_device_wlun",
+		.owner = THIS_MODULE,
+		.probe = ufshcd_wl_probe,
+		.remove = ufshcd_wl_remove,
+		.pm = &ufshcd_wl_pm_ops,
+		.shutdown = ufshcd_wl_shutdown,
+	},
+};
+
+static int ufshcd_rpmb_probe(struct device *dev)
+{
+	return is_rpmb_wlun(to_scsi_device(dev)) ? 0 : -ENODEV;
+}
+
+static inline int ufshcd_clear_rpmb_uac(struct ufs_hba *hba)
+{
+	int ret = 0;
+
+	if (!hba->wlun_rpmb_clr_ua)
+		return 0;
+	ret = ufshcd_clear_ua_wlun(hba, UFS_UPIU_RPMB_WLUN);
+	if (!ret)
+		hba->wlun_rpmb_clr_ua = 0;
+	return ret;
+}
+
+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);
+	return 0;
+}
+
+static const struct dev_pm_ops ufs_rpmb_pm_ops = {
+	SET_RUNTIME_PM_OPS(NULL, ufshcd_rpmb_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(NULL, ufshcd_rpmb_resume)
+};
+
+/**
+ * Describes the ufs rpmb wlun.
+ * Used only to send uac.
+ */
+static struct scsi_driver ufs_rpmb_wlun_template = {
+	.gendrv = {
+		.name = "ufs_rpmb_wlun",
+		.owner = THIS_MODULE,
+		.probe = ufshcd_rpmb_probe,
+		.pm = &ufs_rpmb_pm_ops,
+	},
+};
+
 static int __init ufshcd_core_init(void)
 {
+	int ret;
+
 	ufs_debugfs_init();
+
+	ret = scsi_register_driver(&ufs_dev_wlun_template.gendrv);
+	if (ret) {

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

Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +9 -0
+		ufs_debugfs_exit();
+		return ret;
+	}
+	ret = scsi_register_driver(&ufs_rpmb_wlun_template.gendrv);
+	if (ret) {
+		ufs_debugfs_exit();
+		scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
+		return ret;
+	}
 	return 0;

A separated error path is preferred i.e.

	ret = scsi_register_driver(&ufs_dev_wlun_template.gendrv);
	if (ret)
		goto debugfs_exit;

	ret = scsi_register_driver(&ufs_rpmb_wlun_template.gendrv);
	if (ret)
		goto unregister;

return 0;

unregister:

	scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
debugfs_exit:
	ufs_debugfs_exit();
	return ret;
Quoted 80 lines
drivers/scsi/ufs/ufshcd.c +2 -0
 }
 
 static void __exit ufshcd_core_exit(void)
 {
 	ufs_debugfs_exit();
+	scsi_unregister_driver(&ufs_rpmb_wlun_template.gendrv);
+	scsi_unregister_driver(&ufs_dev_wlun_template.gendrv);
 }
 
 module_init(ufshcd_core_init);
drivers/scsi/ufs/ufshcd.h +6 -0
index 5eb66a8..40436e3 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)
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,