Skip to contentSkip to conversation

Enable power management for ufs wlun

PATCH Superseded Asutosh Das 6 participants 10 messages Apr 14, 2021 – Apr 16, 2021

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

v18 of 21 Superseded Latest is v21 →

lore
Share & tools
lore

Asutosh Das

author
#0 lore raw

v18 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

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          | 692 +++++++++++++++++++++++++------------
 drivers/scsi/ufs/ufshcd.h          |  21 ++
 include/trace/events/ufs.h         |  20 ++
 14 files changed, 561 insertions(+), 268 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 v18 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          | 692 +++++++++++++++++++++++++------------
 drivers/scsi/ufs/ufshcd.h          |  21 ++
 include/trace/events/ufs.h         |  20 ++
 13 files changed, 549 insertions(+), 256 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..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..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..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 +480 -212

1,025 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.
build bot
#2 lore raw

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

I love your patch! Perhaps something to improve:

Hi Asutosh,

I love your patch! Perhaps something to improve:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next next-20210414] [cannot apply to tip/perf/core v5.12-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Asutosh-Das/Enable-power-management-for-ufs-wlun/20210415-030146
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: x86_64-randconfig-a002-20210414 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 9829f5e6b1bca9b61efc629770d28bb9014dec45)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# https://github.com/0day-ci/linux/commit/9bad6c1c707d2a9974aa90e3399e510ec06c77f2
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Asutosh-Das/Enable-power-management-for-ufs-wlun/20210415-030146 git checkout 9bad6c1c707d2a9974aa90e3399e510ec06c77f2
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

>> drivers/scsi/ufs/ufshcd.c:9136:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
           if (pm_runtime_suspended(hba->dev))
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/scsi/ufs/ufshcd.c:9141:50: note: uninitialized use occurs here
           trace_ufshcd_system_suspend(dev_name(hba->dev), ret,
                                                           ^~~
   drivers/scsi/ufs/ufshcd.c:9136:2: note: remove the 'if' if its condition is always false
           if (pm_runtime_suspended(hba->dev))
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/scsi/ufs/ufshcd.c:9133:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   drivers/scsi/ufs/ufshcd.c:9160:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
           if (pm_runtime_suspended(hba->dev))
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/scsi/ufs/ufshcd.c:9166:49: note: uninitialized use occurs here
           trace_ufshcd_system_resume(dev_name(hba->dev), ret,
                                                          ^~~
   drivers/scsi/ufs/ufshcd.c:9160:2: note: remove the 'if' if its condition is always false
           if (pm_runtime_suspended(hba->dev))
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/scsi/ufs/ufshcd.c:9157:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   drivers/scsi/ufs/ufshcd.c:9679:12: warning: unused function 'ufshcd_rpmb_resume' [-Wunused-function]
   static int ufshcd_rpmb_resume(struct device *dev)
              ^

3 warnings generated.

vim +9136 drivers/scsi/ufs/ufshcd.c

9121
9122 /**
9123 * ufshcd_system_suspend - system suspend routine
9124 * @hba: per adapter instance
9125 *
9126 * Check the description of ufshcd_suspend() function for more details.
9127 * Also check the description of __ufshcd_wl_suspend().
9128 *
9129 * Returns 0 for success and non-zero for failure
9130 */
9131 int ufshcd_system_suspend(struct ufs_hba *hba)
9132 {
9133 int ret;
9134 ktime_t start = ktime_get();
9135

> 9136		if (pm_runtime_suspended(hba->dev))

9137 goto out;
9138
9139 ret = ufshcd_suspend(hba);
9140 out:
9141 trace_ufshcd_system_suspend(dev_name(hba->dev), ret,
9142 ktime_to_us(ktime_sub(ktime_get(), start)),
9143 hba->curr_dev_pwr_mode, hba->uic_link_state);
9144 return ret;
9145 }
9146 EXPORT_SYMBOL(ufshcd_system_suspend);
9147

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

#3 lore raw

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

In reply to Asutosh Das
Quoted 33 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
Quoted 5 lines

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.

Can you also explain the new driver for RPMB WLUN and UAC changes here?

And also mention that the driver will now always be runtime resumed before system suspend.

Quoted 5 lines
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 inline void ufshcd_get_lu_power_on_wp_status(struct ufs_hba *hba, · line 4834

Quoted 12 lines
drivers/scsi/ufs/ufshcd.c +12 -0
+static void ufshcd_setup_links(struct ufs_hba *hba, struct scsi_device *sdev)
+{
+	struct device_link *link;
+
+	/*
+	 * device wlun is the supplier & rest of the luns are consumers
+	 * This ensures that device wlun suspends after all other luns.
+	 */
+	if (hba->sdev_ufs_device) {
+		link = device_link_add(&sdev->sdev_gendev,
+				       &hba->sdev_ufs_device->sdev_gendev,
+				       DL_FLAG_PM_RUNTIME|DL_FLAG_RPM_ACTIVE);

"|" could be surrounded by spaces i.e.

				       DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
Quoted 16 lines
drivers/scsi/ufs/ufshcd.c +16 -0
+		if (!link) {
+			dev_err(&sdev->sdev_gendev, "Failed establishing link - %s\n",
+				dev_name(&hba->sdev_ufs_device->sdev_gendev));
+			return;
+		}
+		hba->luns_avail--;
+		/* Ignore REPORT_LUN wlun probing */
+		if (hba->luns_avail == 1) {
+			ufshcd_rpm_put(hba);
+			return;
+		}
+	} else {
+		/* device wlun is probed */
+		hba->luns_avail--;
+	}
+}

<SNIP>

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

Quoted 13 lines
drivers/scsi/ufs/ufshcd.c +2 -7
@@ -8916,42 +8906,214 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 	if (hba->ee_usr_mask)
 		ufshcd_write_ee_control(hba);
 
-	hba->clk_gating.is_suspended = false;
-
-	if (ufshcd_is_clkscaling_supported(hba))
-		ufshcd_clk_scaling_suspend(hba, false);
-
-	/* Enable Auto-Hibernate if configured */
-	ufshcd_auto_hibern8_enable(hba);
+	if (hba->clk_scaling.is_allowed)
+		ufshcd_resume_clkscaling(hba);

We don't use clks, regulators, clk gating, clk scaling, but the original code looks more correct because hba->clk_scaling.is_allowed will have been set to false by ufshcd_clk_scaling_suspend(hba, true) in __ufshcd_wl_suspend().

<SNIP>

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

Quoted 6 lines
drivers/scsi/ufs/ufshcd.c +6 -0
+#ifdef CONFIG_PM_SLEEP
+static int ufshcd_wl_suspend(struct device *dev)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct ufs_hba *hba;
+	int ret;

For below:

	int ret = 0;

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

Quoted 4 lines
drivers/scsi/ufs/ufshcd.c +4 -0
+	ktime_t start = ktime_get();
+
+	hba = shost_priv(sdev->host);
+	down(&hba->host_sem);

If we get here with dev runtime suspended, then skip __ufshcd_wl_suspend() i.e.

	if (pm_runtime_suspended(dev))
		goto out;

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

Quoted 7 lines
drivers/scsi/ufs/ufshcd.c +7 -0
+	ret = __ufshcd_wl_suspend(hba, UFS_SYSTEM_PM);
+	if (ret) {
+		dev_err(&sdev->sdev_gendev, "%s failed: %d\n", __func__,  ret);
+		up(&hba->host_sem);
+	} else {
+		hba->is_sys_suspended = true;
+	}

out:

	if (!ret)
		hba->is_sys_suspended = true;
Quoted 7 lines
drivers/scsi/ufs/ufshcd.c +7 -0
+
+	trace_ufshcd_wl_suspend(dev_name(dev), ret,
+		ktime_to_us(ktime_sub(ktime_get(), start)),
+		hba->curr_dev_pwr_mode, hba->uic_link_state);
+
+	return ret;
+}

<SNIP>

#4 lore raw

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

In reply to Asutosh Das

Hi Asutosh,

url:    https://github.com/0day-ci/linux/commits/Asutosh-Das/Enable-power-management-for-ufs-wlun/20210415-030146
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: i386-randconfig-m021-20210415 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>
Reported-by Dan Carpenter <[email protected]>

New smatch warnings:
drivers/scsi/ufs/ufshcd.c:9017 ufshcd_wl_resume() error: potentially dereferencing uninitialized 'hba'. drivers/scsi/ufs/ufshcd.c:9141 ufshcd_system_suspend() error: uninitialized symbol 'ret'. drivers/scsi/ufs/ufshcd.c:9166 ufshcd_system_resume() error: uninitialized symbol 'ret'.

Old smatch warnings:
drivers/scsi/ufs/ufshcd.c:5112 ufshcd_uic_cmd_compl() error: we previously assumed 'hba->active_uic_cmd' could be null (see line 5100)

vim +/hba +9017 drivers/scsi/ufs/ufshcd.c

9bad6c1c707d2a Asutosh Das        2021-04-14  8998  
9bad6c1c707d2a Asutosh Das        2021-04-14  8999  static int ufshcd_wl_resume(struct device *dev)
9bad6c1c707d2a Asutosh Das        2021-04-14  9000  {
9bad6c1c707d2a Asutosh Das        2021-04-14  9001  	struct scsi_device *sdev = to_scsi_device(dev);
9bad6c1c707d2a Asutosh Das        2021-04-14  9002  	struct ufs_hba *hba;
                                                                        ^^^
9bad6c1c707d2a Asutosh Das        2021-04-14  9003  	int ret = 0;
9bad6c1c707d2a Asutosh Das        2021-04-14  9004  	ktime_t start = ktime_get();
9bad6c1c707d2a Asutosh Das        2021-04-14  9005  
9bad6c1c707d2a Asutosh Das        2021-04-14  9006  	if (pm_runtime_suspended(dev))
9bad6c1c707d2a Asutosh Das        2021-04-14  9007  		goto out;
                                                                ^^^^^^^^
9bad6c1c707d2a Asutosh Das        2021-04-14  9008  
9bad6c1c707d2a Asutosh Das        2021-04-14  9009  	hba = shost_priv(sdev->host);
9bad6c1c707d2a Asutosh Das        2021-04-14  9010  
9bad6c1c707d2a Asutosh Das        2021-04-14  9011  	ret = __ufshcd_wl_resume(hba, UFS_SYSTEM_PM);
9bad6c1c707d2a Asutosh Das        2021-04-14  9012  	if (ret)
9bad6c1c707d2a Asutosh Das        2021-04-14  9013  		dev_err(&sdev->sdev_gendev, "%s failed: %d\n", __func__, ret);
9bad6c1c707d2a Asutosh Das        2021-04-14  9014  out:
9bad6c1c707d2a Asutosh Das        2021-04-14  9015  	trace_ufshcd_wl_resume(dev_name(dev), ret,
9bad6c1c707d2a Asutosh Das        2021-04-14  9016  		ktime_to_us(ktime_sub(ktime_get(), start)),
9bad6c1c707d2a Asutosh Das        2021-04-14 @9017  		hba->curr_dev_pwr_mode, hba->uic_link_state);
                                                                ^^^^^^^^^^^^^^^^^^^^^^

Uninitialized.

9bad6c1c707d2a Asutosh Das        2021-04-14  9018  	if (!ret)
9bad6c1c707d2a Asutosh Das        2021-04-14  9019  		hba->is_sys_suspended = false;
9bad6c1c707d2a Asutosh Das        2021-04-14  9020  	up(&hba->host_sem);
9bad6c1c707d2a Asutosh Das        2021-04-14  9021  	return ret;
9bad6c1c707d2a Asutosh Das        2021-04-14  9022  }

[ snip ]

57d104c153d3d6 Subhash Jadavani   2014-09-25  9131  int ufshcd_system_suspend(struct ufs_hba *hba)
7a3e97b0dc4bba Santosh Yaraganavi 2012-02-29  9132  {
9bad6c1c707d2a Asutosh Das        2021-04-14  9133  	int ret;
7ff5ab47363334 Subhash Jadavani   2016-12-22  9134  	ktime_t start = ktime_get();
57d104c153d3d6 Subhash Jadavani   2014-09-25  9135  
9bad6c1c707d2a Asutosh Das        2021-04-14  9136  	if (pm_runtime_suspended(hba->dev))
57d104c153d3d6 Subhash Jadavani   2014-09-25  9137  		goto out;
                                                                ^^^^^^^^
3b1d05807a9a68 Vinayak Holikatti  2013-02-25  9138  
9bad6c1c707d2a Asutosh Das        2021-04-14  9139  	ret = ufshcd_suspend(hba);
57d104c153d3d6 Subhash Jadavani   2014-09-25  9140  out:
7ff5ab47363334 Subhash Jadavani   2016-12-22 @9141  	trace_ufshcd_system_suspend(dev_name(hba->dev), ret,
                                                                                                        ^^^

"ret" uninitialized.

7ff5ab47363334 Subhash Jadavani   2016-12-22  9142  		ktime_to_us(ktime_sub(ktime_get(), start)),
73eba2be9203c0 Subhash Jadavani   2017-01-10  9143  		hba->curr_dev_pwr_mode, hba->uic_link_state);
57d104c153d3d6 Subhash Jadavani   2014-09-25  9144  	return ret;
57d104c153d3d6 Subhash Jadavani   2014-09-25  9145  }
57d104c153d3d6 Subhash Jadavani   2014-09-25  9146  EXPORT_SYMBOL(ufshcd_system_suspend);
57d104c153d3d6 Subhash Jadavani   2014-09-25  9147  
57d104c153d3d6 Subhash Jadavani   2014-09-25  9148  /**
57d104c153d3d6 Subhash Jadavani   2014-09-25  9149   * ufshcd_system_resume - system resume routine
57d104c153d3d6 Subhash Jadavani   2014-09-25  9150   * @hba: per adapter instance
57d104c153d3d6 Subhash Jadavani   2014-09-25  9151   *
57d104c153d3d6 Subhash Jadavani   2014-09-25  9152   * Returns 0 for success and non-zero for failure
57d104c153d3d6 Subhash Jadavani   2014-09-25  9153   */
57d104c153d3d6 Subhash Jadavani   2014-09-25  9154  
57d104c153d3d6 Subhash Jadavani   2014-09-25  9155  int ufshcd_system_resume(struct ufs_hba *hba)
66ec6d59407baf Sujit Reddy Thumma 2013-07-30  9156  {
9bad6c1c707d2a Asutosh Das        2021-04-14  9157  	int ret;
7ff5ab47363334 Subhash Jadavani   2016-12-22  9158  	ktime_t start = ktime_get();
7ff5ab47363334 Subhash Jadavani   2016-12-22  9159  
9bad6c1c707d2a Asutosh Das        2021-04-14  9160  	if (pm_runtime_suspended(hba->dev))
7ff5ab47363334 Subhash Jadavani   2016-12-22  9161  		goto out;
9bad6c1c707d2a Asutosh Das        2021-04-14  9162  
9bad6c1c707d2a Asutosh Das        2021-04-14  9163  	ret = ufshcd_resume(hba);
9bad6c1c707d2a Asutosh Das        2021-04-14  9164  
7ff5ab47363334 Subhash Jadavani   2016-12-22  9165  out:
7ff5ab47363334 Subhash Jadavani   2016-12-22 @9166  	trace_ufshcd_system_resume(dev_name(hba->dev), ret,
                                                                                                       ^^^

ret uninitialized

7ff5ab47363334 Subhash Jadavani   2016-12-22  9167  		ktime_to_us(ktime_sub(ktime_get(), start)),
73eba2be9203c0 Subhash Jadavani   2017-01-10  9168  		hba->curr_dev_pwr_mode, hba->uic_link_state);
9bad6c1c707d2a Asutosh Das        2021-04-14  9169  
7ff5ab47363334 Subhash Jadavani   2016-12-22  9170  	return ret;
57d104c153d3d6 Subhash Jadavani   2014-09-25  9171  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

#5 lore raw

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

In reply to Asutosh Das
Quoted 1 line · Asutosh Das

[ ... ]

Patches sent to the SCSI mailing list should not have a "scsi: " prefix in the subject. That prefix is inserted before any SCSI patches go into Martin's tree.

Quoted 24 lines
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,
 };

[ ... ]

Quoted 16 lines
drivers/scsi/ufs/ufs-exynos.c +2 -0
@@ -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 +0 -0
index 0aa5813..d463b44 100644

drivers/scsi/ufs/ufs-hisi.c static const struct dev_pm_ops ufs_hisi_pm_ops = { · line 579

Quoted 6 lines
drivers/scsi/ufs/ufs-hisi.c +2 -0
@@ -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,
 };

A minor comment about source code formatting: please make sure that the equality signs are aligned in struct dev_pm_ops definitions.

drivers/scsi/ufs/ufs-hisi.c static const struct dev_pm_ops ufs_hisi_pm_ops = {

Quoted 10 lines
drivers/scsi/ufs/ufs-hisi.c +10 -0
+static inline bool is_rpmb_wlun(struct scsi_device *sdev)
+{
+	return (sdev->lun == ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN));
+}
+
+static inline bool is_device_wlun(struct scsi_device *sdev)
+{
+	return (sdev->lun ==
+		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_UFS_DEVICE_WLUN));
+}

The Linux kernel coding style requires not to surround expressions with parentheses in return statements.

Quoted 23 lines

/**
+ * ufshcd_setup_links - associate link b/w device wlun and other luns
+ * @sdev: pointer to SCSI device
+ * @hba: pointer to ufs hba
+ */

drivers/scsi/ufs/ufs-hisi.c +18 -0
+static void ufshcd_setup_links(struct ufs_hba *hba, struct scsi_device *sdev)
+{
+	struct device_link *link;
+
+	/*
+	 * device wlun is the supplier & rest of the luns are consumers
+	 * This ensures that device wlun suspends after all other luns.
+	 */
+	if (hba->sdev_ufs_device) {
+		link = device_link_add(&sdev->sdev_gendev,
+				       &hba->sdev_ufs_device->sdev_gendev,
+				       DL_FLAG_PM_RUNTIME|DL_FLAG_RPM_ACTIVE);
+		if (!link) {
+			dev_err(&sdev->sdev_gendev, "Failed establishing link - %s\n",
+				dev_name(&hba->sdev_ufs_device->sdev_gendev));
+			return;
+		}
+		hba->luns_avail--;

drivers/scsi/ufs/ufs-hisi.c

Quoted 10 lines
drivers/scsi/ufs/ufs-hisi.c +10 -0
+		/* Ignore REPORT_LUN wlun probing */
+		if (hba->luns_avail == 1) {
+			ufshcd_rpm_put(hba);
+			return;
+		}
+	} else {
+		/* device wlun is probed */
+		hba->luns_avail--;
+	}
+}

Please add a comment that explains that it is assumed that the WLUNs are scanned before the other LUNs.

Quoted 6 lines
drivers/scsi/ufs/ufs-hisi.c +0 -2
@@ -4862,8 +4913,13 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
 	blk_queue_update_dma_pad(q, PRDT_DATA_BYTE_COUNT_PAD - 1);
 	if (hba->quirks & UFSHCD_QUIRK_ALIGN_SG_WITH_PAGE_SIZE)
 		blk_queue_update_dma_alignment(q, PAGE_SIZE - 1);
-
-	if (ufshcd_is_rpm_autosuspend_allowed(hba))

drivers/scsi/ufs/ufs-hisi.c static int ufshcd_slave_configure(struct scsi_device *sdev) · line 4925

Quoted 10 lines
drivers/scsi/ufs/ufs-hisi.c +7 -0
+	/*
+	 * Block runtime-pm until all consumers are added.
+	 * Refer ufshcd_setup_links().
+	 */
+	if (is_device_wlun(sdev))
+		pm_runtime_get_noresume(&sdev->sdev_gendev);
+	else if (ufshcd_is_rpm_autosuspend_allowed(hba))
 		sdev->rpm_autosuspend = 1;
 
 	ufshcd_crypto_setup_rq_keyslot_manager(hba, q);

The following code is executed before ufshcd_async_scan() is called:

	dev = hba->dev;
	[ ... ]
	/* Hold auto suspend until async scan completes */
	pm_runtime_get_sync(dev);

and the following code occurs in ufshcd_add_lus():

	pm_runtime_put_sync(hba->dev);

Isn't that sufficient to postpone enabling of runtime PM until LUN scanning has finished? Or in other words, is adding a pm_runtime_get_noresume() call in ufshcd_slave_configure() really necessary?

Quoted 6 lines
drivers/scsi/ufs/ufs-hisi.c +0 -2
@@ -4979,15 +5035,9 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 			 */
 			if (!hba->pm_op_in_progress &&
 			    !ufshcd_eh_in_progress(hba) &&
-			    ufshcd_is_exception_event(lrbp->ucd_rsp_ptr) &&
-			    schedule_work(&hba->eeh_work)) {

drivers/scsi/ufs/ufs-hisi.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) · line 5040

Quoted 10 lines
drivers/scsi/ufs/ufs-hisi.c +3 -7
-				/*
-				 * Prevent suspend once eeh_work is scheduled
-				 * to avoid deadlock between ufshcd_suspend
-				 * and exception event handler.
-				 */
-				pm_runtime_get_noresume(hba->dev);
-			}
+			    ufshcd_is_exception_event(lrbp->ucd_rsp_ptr))
+				/* Flushed in suspend */
+				schedule_work(&hba->eeh_work);

What makes it safe to leave out the above pm_runtime_get_noresume() call?

Thanks,

Bart.

#6 lore raw

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

Hi Bart!

Quoted 3 lines

Patches sent to the SCSI mailing list should not have a "scsi: " prefix in the subject. That prefix is inserted before any SCSI patches go into Martin's tree.

This doesn't actually matter. My script will add the prefix if it's not present.

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
author
#7 lore raw

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

In reply to Bart Van Assche
Quoted 3 lines · Bart Van Assche
Quoted 1 line · Asutosh Das

[ ... ]

Hi Bart,
Thanks for the comments. I will fix the comments in the next version.

Quoted 7 lines

The following code is executed before ufshcd_async_scan() is called:

	dev = hba->dev;
	[ ... ]
	/* Hold auto suspend until async scan completes */
	pm_runtime_get_sync(dev);

That would only keep the hba runtime resumed. At this point of time the luns are not detected yet.

Quoted 8 lines

and the following code occurs in ufshcd_add_lus():

	pm_runtime_put_sync(hba->dev);

Isn't that sufficient to postpone enabling of runtime PM until LUN scanning has finished? Or in other words, is adding a pm_runtime_get_noresume() call in ufshcd_slave_configure() really necessary?

Yes, because the supplier (device wlun) may be suspended otherwise in scsi_sysfs_add_sdev().

Quoted 10 lines
Quoted 10 lines
drivers/scsi/ufs/ufshcd.c +0 -6
@@ -4979,15 +5035,9 @@ ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
  			 */
  			if (!hba->pm_op_in_progress &&
  			    !ufshcd_eh_in_progress(hba) &&
-			    ufshcd_is_exception_event(lrbp->ucd_rsp_ptr) &&
-			    schedule_work(&hba->eeh_work)) {
-				/*
-				 * Prevent suspend once eeh_work is scheduled
-				 * to avoid deadlock between ufshcd_suspend
-				 * and exception event handler.
Quoted 9 lines

drivers/scsi/ufs/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) · line 5040

Quoted 6 lines
drivers/scsi/ufs/ufshcd.c +3 -3
-				 */
-				pm_runtime_get_noresume(hba->dev);
-			}
+			    ufshcd_is_exception_event(lrbp->ucd_rsp_ptr))
+				/* Flushed in suspend */
+				schedule_work(&hba->eeh_work);

What makes it safe to leave out the above pm_runtime_get_noresume() call?

The __ufshcd_wl_suspend() would flush this work so that it doesn't run after suspend.

Quoted 4 lines

Thanks,

Bart.

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

PATCH v18 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 | 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.
Reviewed-by on v18 2/2
#9 lore raw

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

Quoted 5 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]>
Reviewed-by Adrian Hunter <[email protected]>
Quoted 91 lines
---
 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;