Re: [PATCH v8 1/2] scsi: ufs: Enable power management for wlun
On 1/03/21 8:10 pm, Asutosh Das wrote: > On Mon, Mar 01 2021 at 05:23 -0800, Adrian Hunter wrote: >> On 26/02/21 1:37 am, Asutosh Das wrote: >>> @@ -8901,43 +9125,14 @@ static int ufshcd_resume(struct ufs_hba *hba, enum >>> ufs_pm_op pm_op) >>> goto vendor_suspend; >>> } >> >> The ufshcd_reset_and_restore() in ufshcd_resume() will also change the power >> mode of the UFS device to active. Until the UFS device is also resumed and >> then suspended, it will not return to a low power mode. >> >> > Umm, sorry, I didn't understand this comment. > Say, the UFS device was reset in ufshcd_reset_and_restore() it'd be a hardware > reset and the UFS device would move to Powered On mode and then to Active > power > mode, when it is ready to begin initialization. And from this state it should > move to all other legal states. > Before entering system suspend ufshcd_system_suspend(), the ufs device is > runtime resumed in ufshcd_suspend_prepare(). > > Please can you explain a bit more on this issue that you see? Say you runtime resume the host controller, and ufshcd_reset_and_restore() makes the UFS device active, but the UFS device is still runtime suspended. Example: Add a debugfs file to show the current power mode: diff --git a/drivers/scsi/ufs/ufs-debugfs.c b/drivers/scsi/ufs/ufs-debugfs.c index dee98dc72d29..700b88df0866 100644 --- a/drivers/scsi/ufs/ufs-debugfs.c +++ b/drivers/scsi/ufs/ufs-debugfs.c @@ -48,6 +48,7 @@ void ufs_debugfs_hba_init(struct ufs_hba *hba) { hba->debugfs_root = debugfs_create_dir(dev_name(hba->dev), ufs_debugfs_root); debugfs_create_file("stats", 0400, hba->debugfs_root, hba, &ufs_debugfs_stats_fops); + debugfs_create_u32("curr_dev_pwr_mode", 0400, hba->debugfs_root, (u32 *)&hba->curr_dev_pwr_mode); } void ufs_debugfs_hba_exit(struct ufs_hba *hba) # grep -H . /sys/bus/pci/drivers/ufshcd/\:00\:12.5/rpm* /sys/bus/pci/drivers/ufshcd/:00:12.5/rpm_lvl:6 /sys/bus/pci/drivers/ufshcd/:00:12.5/rpm_target_dev_state:DEEPSLEEP /sys/bus/pci/drivers/ufshcd/:00:12.5/rpm_target_link_state:OFF # cat /sys/kernel/debug/ufshcd/\:00\:12.5/curr_dev_pwr_mode 4 # echo on > /sys/devices/pci:00/:00:12.5/power/control # cat /sys/kernel/debug/ufshcd/\:00\:12.5/curr_dev_pwr_mode 1 # grep -H . /sys/bus/pci/drivers/ufshcd/\:00\:12.5/host*/target*/*/power/runtime_status /sys/bus/pci/drivers/ufshcd/:00:12.5/host1/target1:0:0/1:0:0:0/power/runtime_status:suspended /sys/bus/pci/drivers/ufshcd/:00:12.5/host1/target1:0:0/1:0:0:49456/power/runtime_status:suspended /sys/bus/pci/drivers/ufshcd/:00:12.5/host1/target1:0:0/1:0:0:49476/power/runtime_status:suspended /sys/bus/pci/drivers/ufshcd/:00:12.5/host1/target1:0:0/1:0:0:49488/power/runtime_status:suspended So UFS devices is runtime suspended and should in DeepSleep, but it is active.
Re: [PATCH v8 1/2] scsi: ufs: Enable power management for wlun
On Mon, Mar 01 2021 at 05:23 -0800, Adrian Hunter wrote: Hi A couple of minor things, but also a potential issue with when link state transitions are done. Please see comments below. On 26/02/21 1:37 am, Asutosh Das wrote: Hi Adrian Thanks for your comments. 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 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 Signed-off-by: Can Guo Signed-off-by: Asutosh Das --- drivers/scsi/ufs/cdns-pltfrm.c | 2 + drivers/scsi/ufs/tc-dwc-g210-pci.c | 2 + drivers/scsi/ufs/ufs-exynos.c | 2 + drivers/scsi/ufs/ufs-hisi.c| 2 + drivers/scsi/ufs/ufs-mediatek.c| 2 + drivers/scsi/ufs/ufs-qcom.c| 2 + drivers/scsi/ufs/ufshcd-pci.c | 26 +- drivers/scsi/ufs/ufshcd.c | 540 ++--- drivers/scsi/ufs/ufshcd.h | 7 + include/trace/events/ufs.h | 20 ++ 10 files changed, 483 insertions(+), 122 deletions(-) diff --git a/drivers/scsi/ufs/cdns-pltfrm.c b/drivers/scsi/ufs/cdns-pltfrm.c index 149391f..3e70c23 100644 --- a/drivers/scsi/ufs/cdns-pltfrm.c +++ b/drivers/scsi/ufs/cdns-pltfrm.c @@ -319,6 +319,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 = { diff --git a/drivers/scsi/ufs/tc-dwc-g210-pci.c b/drivers/scsi/ufs/tc-dwc-g210-pci.c index 67a6a61..b01db12 100644 --- a/drivers/scsi/ufs/tc-dwc-g210-pci.c +++ b/drivers/scsi/ufs/tc-dwc-g210-pci.c @@ -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[] = { diff --git a/drivers/scsi/ufs/ufs-exynos.c b/drivers/scsi/ufs/ufs-exynos.c index 267943a1..45c0b02 100644 --- a/drivers/scsi/ufs/ufs-exynos.c +++ b/drivers/scsi/ufs/ufs-exynos.c @@ -1268,6 +1268,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 = { diff --git a/drivers/scsi/ufs/ufs-hisi.c b/drivers/scsi/ufs/ufs-hisi.c index 0aa5813..d463b44 100644 --- a/drivers/scsi/ufs/ufs-hisi.c +++ b/drivers/scsi/ufs/ufs-hisi.c @@ -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 = { diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c index c55202b..df1eabb 100644 --- a/drivers/scsi/ufs/ufs-mediatek.c +++ b/drivers/scsi/ufs/ufs-mediatek.c @@ -1097,6 +1097,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
Re: [PATCH v8 1/2] scsi: ufs: Enable power management for wlun
Hi A couple of minor things, but also a potential issue with when link state transitions are done. Please see comments below. On 26/02/21 1:37 am, Asutosh Das wrote: > 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 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 > Signed-off-by: Can Guo > Signed-off-by: Asutosh Das > --- > drivers/scsi/ufs/cdns-pltfrm.c | 2 + > drivers/scsi/ufs/tc-dwc-g210-pci.c | 2 + > drivers/scsi/ufs/ufs-exynos.c | 2 + > drivers/scsi/ufs/ufs-hisi.c| 2 + > drivers/scsi/ufs/ufs-mediatek.c| 2 + > drivers/scsi/ufs/ufs-qcom.c| 2 + > drivers/scsi/ufs/ufshcd-pci.c | 26 +- > drivers/scsi/ufs/ufshcd.c | 540 > ++--- > drivers/scsi/ufs/ufshcd.h | 7 + > include/trace/events/ufs.h | 20 ++ > 10 files changed, 483 insertions(+), 122 deletions(-) > > diff --git a/drivers/scsi/ufs/cdns-pltfrm.c b/drivers/scsi/ufs/cdns-pltfrm.c > index 149391f..3e70c23 100644 > --- a/drivers/scsi/ufs/cdns-pltfrm.c > +++ b/drivers/scsi/ufs/cdns-pltfrm.c > @@ -319,6 +319,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 = { > diff --git a/drivers/scsi/ufs/tc-dwc-g210-pci.c > b/drivers/scsi/ufs/tc-dwc-g210-pci.c > index 67a6a61..b01db12 100644 > --- a/drivers/scsi/ufs/tc-dwc-g210-pci.c > +++ b/drivers/scsi/ufs/tc-dwc-g210-pci.c > @@ -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[] = { > diff --git a/drivers/scsi/ufs/ufs-exynos.c b/drivers/scsi/ufs/ufs-exynos.c > index 267943a1..45c0b02 100644 > --- a/drivers/scsi/ufs/ufs-exynos.c > +++ b/drivers/scsi/ufs/ufs-exynos.c > @@ -1268,6 +1268,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 = { > diff --git a/drivers/scsi/ufs/ufs-hisi.c b/drivers/scsi/ufs/ufs-hisi.c > index 0aa5813..d463b44 100644 > --- a/drivers/scsi/ufs/ufs-hisi.c > +++ b/drivers/scsi/ufs/ufs-hisi.c > @@ -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 = { > diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c > index c55202b..df1eabb 100644 > --- a/drivers/scsi/ufs/ufs-mediatek.c > +++ b/drivers/scsi/ufs/ufs-mediatek.c > @@ -1097,6 +1097,8 @@ static const struct dev_pm_ops ufs_mtk_pm_ops = { > .runtime_suspend = ufshcd_pltfrm_runtime_suspend, > .runtime_resume = ufshcd_pltfrm_run
[PATCH v8 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 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 Signed-off-by: Can Guo Signed-off-by: Asutosh Das --- drivers/scsi/ufs/cdns-pltfrm.c | 2 + drivers/scsi/ufs/tc-dwc-g210-pci.c | 2 + drivers/scsi/ufs/ufs-exynos.c | 2 + drivers/scsi/ufs/ufs-hisi.c| 2 + drivers/scsi/ufs/ufs-mediatek.c| 2 + drivers/scsi/ufs/ufs-qcom.c| 2 + drivers/scsi/ufs/ufshcd-pci.c | 26 +- drivers/scsi/ufs/ufshcd.c | 540 ++--- drivers/scsi/ufs/ufshcd.h | 7 + include/trace/events/ufs.h | 20 ++ 10 files changed, 483 insertions(+), 122 deletions(-) diff --git a/drivers/scsi/ufs/cdns-pltfrm.c b/drivers/scsi/ufs/cdns-pltfrm.c index 149391f..3e70c23 100644 --- a/drivers/scsi/ufs/cdns-pltfrm.c +++ b/drivers/scsi/ufs/cdns-pltfrm.c @@ -319,6 +319,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 = { diff --git a/drivers/scsi/ufs/tc-dwc-g210-pci.c b/drivers/scsi/ufs/tc-dwc-g210-pci.c index 67a6a61..b01db12 100644 --- a/drivers/scsi/ufs/tc-dwc-g210-pci.c +++ b/drivers/scsi/ufs/tc-dwc-g210-pci.c @@ -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[] = { diff --git a/drivers/scsi/ufs/ufs-exynos.c b/drivers/scsi/ufs/ufs-exynos.c index 267943a1..45c0b02 100644 --- a/drivers/scsi/ufs/ufs-exynos.c +++ b/drivers/scsi/ufs/ufs-exynos.c @@ -1268,6 +1268,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 = { diff --git a/drivers/scsi/ufs/ufs-hisi.c b/drivers/scsi/ufs/ufs-hisi.c index 0aa5813..d463b44 100644 --- a/drivers/scsi/ufs/ufs-hisi.c +++ b/drivers/scsi/ufs/ufs-hisi.c @@ -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 = { diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c index c55202b..df1eabb 100644 --- a/drivers/scsi/ufs/ufs-mediatek.c +++ b/drivers/scsi/ufs/ufs-mediatek.c @@ -1097,6 +1097,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 = { diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c index f97d7b0..9aa098a 100644 --- a/drivers/scsi/ufs/ufs-qc