Re: [PATCH v17 1/2] scsi: ufs: Enable power management for wlun
On 8/04/21 5:49 pm, 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 (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 > Signed-off-by: Can Guo > Signed-off-by: Asutosh Das > --- > @@ -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(>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); > @@ -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; > @@ -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. > } > > 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; > } > +/** > + * 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(). > + > + /* > + * 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
Re: [PATCH v17 1/2] scsi: ufs: Enable power management for wlun
On 9/04/21 8:15 pm, Asutosh Das (asd) wrote: > On 4/9/2021 3:07 AM, Adrian Hunter wrote: >> On 9/04/21 5:27 am, Daejun Park wrote: >>> Hi 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 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-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. >> >> 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(>sdev_ufs_device->sdev_gendev); >> } >> static inline int ufshcd_rpm_put(struct ufs_hba *hba) >> { >> return pm_runtime_put(>sdev_ufs_device->sdev_gendev); >> } >> >> static inline int ufshcd_rpm_put_sync(struct ufs_hba *hba) >> { >> return pm_runtime_put_sync(>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. > I think I will have some more comments, so you could wait if you want.
Re: [PATCH v17 1/2] scsi: ufs: Enable power management for wlun
On 4/9/2021 3:07 AM, Adrian Hunter wrote: On 9/04/21 5:27 am, Daejun Park wrote: Hi 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 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-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. 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(>sdev_ufs_device->sdev_gendev); } static inline int ufshcd_rpm_put(struct ufs_hba *hba) { return pm_runtime_put(>sdev_ufs_device->sdev_gendev); } static inline int ufshcd_rpm_put_sync(struct ufs_hba *hba) { return pm_runtime_put_sync(>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. -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, Linux Foundation Collaborative Project
Re: [PATCH v17 1/2] scsi: ufs: Enable power management for wlun
On 9/04/21 5:27 am, Daejun Park wrote: > Hi 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 >> 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-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. 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(>sdev_ufs_device->sdev_gendev); } static inline int ufshcd_rpm_put(struct ufs_hba *hba) { return pm_runtime_put(>sdev_ufs_device->sdev_gendev); } static inline int ufshcd_rpm_put_sync(struct ufs_hba *hba) { return pm_runtime_put_sync(>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)
RE: [PATCH v17 1/2] scsi: ufs: Enable power management for wlun
Hi 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 >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-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