| Message ID | cover.1618529652.git.asutoshd@codeaurora.org |
|---|---|
| Headers | show |
| Series | Enable power management for ufs wlun | expand |
On 16/04/21 2:36 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 (START_STOP_UNIT) to wlun > during its runtime-suspend. > During the process blk_queue_enter checks if the queue is not in > suspended state. If so, it waits for the queue to resume, and never > comes out of it. > The commit > (d55d15a33: scsi: block: Do not accept any requests while suspended) > adds the check if the queue is in suspended state in blk_queue_enter(). > > Call trace: > __switch_to+0x174/0x2c4 > __schedule+0x478/0x764 > schedule+0x9c/0xe0 > blk_queue_enter+0x158/0x228 > blk_mq_alloc_request+0x40/0xa4 > blk_get_request+0x2c/0x70 > __scsi_execute+0x60/0x1c4 > ufshcd_set_dev_pwr_mode+0x124/0x1e4 > ufshcd_suspend+0x208/0x83c > ufshcd_runtime_suspend+0x40/0x154 > ufshcd_pltfrm_runtime_suspend+0x14/0x20 > pm_generic_runtime_suspend+0x28/0x3c > __rpm_callback+0x80/0x2a4 > rpm_suspend+0x308/0x614 > rpm_idle+0x158/0x228 > pm_runtime_work+0x84/0xac > process_one_work+0x1f0/0x470 > worker_thread+0x26c/0x4c8 > kthread+0x13c/0x320 > ret_from_fork+0x10/0x18 > > Fix this by registering ufs device wlun as a scsi driver and > registering it for block runtime-pm. Also make this as a > supplier for all other luns. That way, this device wlun > suspends after all the consumers and resumes after > hba resumes. > This also registers a new scsi driver for rpmb wlun. > This new driver is mostly used to clear rpmb uac. > With this design, the driver would always be runtime resumed > before system suspend. I thought some more about that and I think we can still support allowing runtime suspend to work with system suspend, without too much difficulty. See ufshcd_suspend_prepare() below. > > Fixed smatch warnings: > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Co-developed-by: Can Guo <cang@codeaurora.org> > Signed-off-by: Can Guo <cang@codeaurora.org> > Signed-off-by: Asutosh Das <asutoshd@codeaurora.org> > --- <SNIP> > -static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) > +static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) > { > int ret; > - enum uic_link_state old_link_state; > + enum uic_link_state old_link_state = hba->uic_link_state; > > - hba->pm_op_in_progress = 1; > - old_link_state = hba->uic_link_state; > - > - 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); > + hba->pm_op_in_progress = true; > > /* > * Call vendor specific resume callback. As these callbacks may access > @@ -8868,7 +8858,7 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) > */ > ret = ufshcd_vops_resume(hba, pm_op); > if (ret) > - goto disable_irq_and_vops_clks; > + goto out; > > /* For DeepSleep, the only supported option is to have the link off */ > WARN_ON(ufshcd_is_ufs_dev_deepsleep(hba) && !ufshcd_is_link_off(hba)); > @@ -8916,42 +8906,219 @@ 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); > + ufshcd_resume_clkscaling(hba); This still doesn't look right. ufshcd_resume_clkscaling() doesn't update hba->clk_scaling.is_allowed whereas ufshcd_clk_scaling_suspend() does. > > if (hba->dev_info.b_rpm_dev_flush_capable) { > hba->dev_info.b_rpm_dev_flush_capable = false; > cancel_delayed_work(&hba->rpm_dev_flush_recheck_work); > } > > - ufshcd_clear_ua_wluns(hba); > - > - /* Schedule clock gating in case of no access to UFS device yet */ > - ufshcd_release(hba); > - > + /* Enable Auto-Hibernate if configured */ > + ufshcd_auto_hibern8_enable(hba); > goto out; > > set_old_link_state: > ufshcd_link_state_transition(hba, old_link_state, 0); > vendor_suspend: > ufshcd_vops_suspend(hba, pm_op); > -disable_irq_and_vops_clks: > +out: > + if (ret) > + ufshcd_update_evt_hist(hba, UFS_EVT_WL_RES_ERR, (u32)ret); > + hba->clk_gating.is_suspended = false; > + ufshcd_release(hba); > + hba->pm_op_in_progress = false; > + return ret; > +} <SNIP> > +void ufshcd_resume_complete(struct device *dev) > +{ > + struct ufs_hba *hba = dev_get_drvdata(dev); > + > + ufshcd_rpm_put(hba); > +} > +EXPORT_SYMBOL_GPL(ufshcd_resume_complete); > + > +int ufshcd_suspend_prepare(struct device *dev) > +{ > + struct ufs_hba *hba = dev_get_drvdata(dev); > + > + /* > + * 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. > + * Refer ufshcd_resume_complete() > + */ > + ufshcd_rpm_get_sync(hba); > + > + return 0; > +} I think we can support allowing runtime suspend to work with system suspend. ufshcd_resume_complete() remains the same, and ufshcd_suspend_prepare() is like this: /* * 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. * However UFS doesn't follow that. The rpm-lvl and spm-lvl can be different in * UFS, so special care is needed. * Refer also ufshcd_resume_complete() */ int ufshcd_suspend_prepare(struct device *dev) { struct ufs_hba *hba = dev_get_drvdata(dev); struct device *ufs_dev = &hba->sdev_ufs_device->sdev_gendev; enum ufs_dev_pwr_mode spm_pwr_mode; enum uic_link_state spm_link_state; unsigned long flags; bool rpm_state_ok; /* * First prevent runtime suspend. Note this does not prevent runtime * resume e.g. pm_runtime_get_sync() will still do the right thing. */ pm_runtime_get_noresume(ufs_dev); /* Now check if the rpm state is ok to use for spm */ spin_lock_irqsave(&ufs_dev->power.lock, flags); spm_pwr_mode = ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl); spm_link_state = ufs_get_pm_lvl_to_link_pwr_state(hba->spm_lvl); rpm_state_ok = pm_runtime_suspended(ufs_dev) && hba->curr_dev_pwr_mode == spm_pwr_mode && hba->uic_link_state == spm_link_state && !hba->dev_info.b_rpm_dev_flush_capable; spin_unlock_irqrestore(&ufs_dev->power.lock, flags); /* If is isn't, do a runtime resume */ if (!rpm_state_ok) pm_runtime_resume(ufs_dev); return 0; }
On 16/04/21 12:22 pm, Adrian Hunter wrote: > On 16/04/21 2:36 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 (START_STOP_UNIT) to wlun >> during its runtime-suspend. >> During the process blk_queue_enter checks if the queue is not in >> suspended state. If so, it waits for the queue to resume, and never >> comes out of it. >> The commit >> (d55d15a33: scsi: block: Do not accept any requests while suspended) >> adds the check if the queue is in suspended state in blk_queue_enter(). >> >> Call trace: >> __switch_to+0x174/0x2c4 >> __schedule+0x478/0x764 >> schedule+0x9c/0xe0 >> blk_queue_enter+0x158/0x228 >> blk_mq_alloc_request+0x40/0xa4 >> blk_get_request+0x2c/0x70 >> __scsi_execute+0x60/0x1c4 >> ufshcd_set_dev_pwr_mode+0x124/0x1e4 >> ufshcd_suspend+0x208/0x83c >> ufshcd_runtime_suspend+0x40/0x154 >> ufshcd_pltfrm_runtime_suspend+0x14/0x20 >> pm_generic_runtime_suspend+0x28/0x3c >> __rpm_callback+0x80/0x2a4 >> rpm_suspend+0x308/0x614 >> rpm_idle+0x158/0x228 >> pm_runtime_work+0x84/0xac >> process_one_work+0x1f0/0x470 >> worker_thread+0x26c/0x4c8 >> kthread+0x13c/0x320 >> ret_from_fork+0x10/0x18 >> >> Fix this by registering ufs device wlun as a scsi driver and >> registering it for block runtime-pm. Also make this as a >> supplier for all other luns. That way, this device wlun >> suspends after all the consumers and resumes after >> hba resumes. >> This also registers a new scsi driver for rpmb wlun. >> This new driver is mostly used to clear rpmb uac. >> With this design, the driver would always be runtime resumed >> before system suspend. > > I thought some more about that and I think we can still support > allowing runtime suspend to work with system suspend, without > too much difficulty. See ufshcd_suspend_prepare() below. > >> >> Fixed smatch warnings: >> Reported-by: kernel test robot <lkp@intel.com> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >> >> Co-developed-by: Can Guo <cang@codeaurora.org> >> Signed-off-by: Can Guo <cang@codeaurora.org> >> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org> >> --- > > <SNIP> > >> -static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) >> +static int __ufshcd_wl_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) >> { >> int ret; >> - enum uic_link_state old_link_state; >> + enum uic_link_state old_link_state = hba->uic_link_state; >> >> - hba->pm_op_in_progress = 1; >> - old_link_state = hba->uic_link_state; >> - >> - 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); >> + hba->pm_op_in_progress = true; >> >> /* >> * Call vendor specific resume callback. As these callbacks may access >> @@ -8868,7 +8858,7 @@ static int ufshcd_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) >> */ >> ret = ufshcd_vops_resume(hba, pm_op); >> if (ret) >> - goto disable_irq_and_vops_clks; >> + goto out; >> >> /* For DeepSleep, the only supported option is to have the link off */ >> WARN_ON(ufshcd_is_ufs_dev_deepsleep(hba) && !ufshcd_is_link_off(hba)); >> @@ -8916,42 +8906,219 @@ 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); >> + ufshcd_resume_clkscaling(hba); > > This still doesn't look right. ufshcd_resume_clkscaling() > doesn't update hba->clk_scaling.is_allowed whereas > ufshcd_clk_scaling_suspend() does. > >> >> if (hba->dev_info.b_rpm_dev_flush_capable) { >> hba->dev_info.b_rpm_dev_flush_capable = false; >> cancel_delayed_work(&hba->rpm_dev_flush_recheck_work); >> } >> >> - ufshcd_clear_ua_wluns(hba); >> - >> - /* Schedule clock gating in case of no access to UFS device yet */ >> - ufshcd_release(hba); >> - >> + /* Enable Auto-Hibernate if configured */ >> + ufshcd_auto_hibern8_enable(hba); >> goto out; >> >> set_old_link_state: >> ufshcd_link_state_transition(hba, old_link_state, 0); >> vendor_suspend: >> ufshcd_vops_suspend(hba, pm_op); >> -disable_irq_and_vops_clks: >> +out: >> + if (ret) >> + ufshcd_update_evt_hist(hba, UFS_EVT_WL_RES_ERR, (u32)ret); >> + hba->clk_gating.is_suspended = false; >> + ufshcd_release(hba); >> + hba->pm_op_in_progress = false; >> + return ret; >> +} > > <SNIP> > >> +void ufshcd_resume_complete(struct device *dev) >> +{ >> + struct ufs_hba *hba = dev_get_drvdata(dev); >> + >> + ufshcd_rpm_put(hba); >> +} >> +EXPORT_SYMBOL_GPL(ufshcd_resume_complete); >> + >> +int ufshcd_suspend_prepare(struct device *dev) >> +{ >> + struct ufs_hba *hba = dev_get_drvdata(dev); >> + >> + /* >> + * 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. >> + * Refer ufshcd_resume_complete() >> + */ >> + ufshcd_rpm_get_sync(hba); >> + >> + return 0; >> +} > > I think we can support allowing runtime suspend to work with > system suspend. ufshcd_resume_complete() remains the same, > and ufshcd_suspend_prepare() is like this: > > > /* > * 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. > * However UFS doesn't follow that. The rpm-lvl and spm-lvl can be different in > * UFS, so special care is needed. > * Refer also ufshcd_resume_complete() > */ > int ufshcd_suspend_prepare(struct device *dev) > { > struct ufs_hba *hba = dev_get_drvdata(dev); > struct device *ufs_dev = &hba->sdev_ufs_device->sdev_gendev; > enum ufs_dev_pwr_mode spm_pwr_mode; > enum uic_link_state spm_link_state; > unsigned long flags; > bool rpm_state_ok; > > /* > * First prevent runtime suspend. Note this does not prevent runtime > * resume e.g. pm_runtime_get_sync() will still do the right thing. > */ > pm_runtime_get_noresume(ufs_dev); > > /* Now check if the rpm state is ok to use for spm */ > spin_lock_irqsave(&ufs_dev->power.lock, flags); > > spm_pwr_mode = ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl); > spm_link_state = ufs_get_pm_lvl_to_link_pwr_state(hba->spm_lvl); > > rpm_state_ok = pm_runtime_suspended(ufs_dev) && > hba->curr_dev_pwr_mode == spm_pwr_mode && > hba->uic_link_state == spm_link_state && > !hba->dev_info.b_rpm_dev_flush_capable; > > spin_unlock_irqrestore(&ufs_dev->power.lock, flags); > > /* If is isn't, do a runtime resume */ > if (!rpm_state_ok) > pm_runtime_resume(ufs_dev); But we should return an error if runtime resume fails. if (!rpm_state_ok) { int ret = pm_runtime_resume(ufs_dev); if (ret < 0 && ret != EACCES) { pm_runtime_put(ufs_dev); return ret; } } > > return 0; > } >
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 v18 -> v19: - Addressed Adrian's comments - Fixed smatch warnings v17 -> v18: - Addressed Adrian's comments v16 -> v17: - Addressed Adrian's & Daejun's comments v15 -> v16: - Brought back the missing changes * Added scsi_autopm_[get/put] to ufs_debugfs[get/put]_user_access() * Fix ufshcd_wl_poweroff() v14 -> v15: - Rebased on 5.13/scsi-staging v13 -> v14: - Addressed Adrian's comments * Rebased it on top of scsi-next * Added scsi_autopm_[get/put] to ufs_debugfs[get/put]_user_access() * Resume the device in ufshcd_remove() * Unregister ufs_rpmb_wlun before ufs_dev_wlun * hba->shutting_down moved to ufshcd_wl_shutdown() v12 -> v13: - Addressed Adrian's comments * Paired pm_runtime_get_noresume() with pm_runtime_put() * no rpm_autosuspend for ufs device wlun * Moved runtime-pm init functionality to ufshcd_wl_probe() - Addressed Bart's comments * Expanded abbrevs in commit message v11 -> v12: - Addressed Adrian's comments * Fixed ahit for Mediatek driver * Fixed error handling in ufshcd_core_init() * Tested this patch and the issue is still seen. v10 -> v11: - Fixed supplier suspending before consumer race - Addressed Adrian's comments * Added proper resume/suspend cb to ufshcd_auto_hibern8_update() * Cosmetic changes to ufshcd-pci.c * Cleaned up ufshcd_system_suspend() * Added ufshcd_debugfs_eh_exit to ufshcd_core_init() v9 -> v10: - Addressed Adrian's comments * Moved suspend/resume vops to __ufshcd_wl_[suspend/resume]() * Added correct resume in ufs_bsg v8 -> v9: - Addressed Adrian's comments * Moved link transition to __ufshcd_wl_[suspend/resume]() * Fixed the other minor comments v7 -> v8: - Addressed Adrian's comments * Removed separate autosuspend delay for ufs-device lun * Fixed the ee handler getting scheduled during pm * Always runtime resume in suspend_prepare() * Added CONFIG_PM_SLEEP where needed v6 -> v7: * Resume the ufs device before shutting it down v5 -> v6: - Addressed Adrian's comments * Added complete() cb * Added suspend_prepare() and complete() to all drivers * Moved suspend_prepare() and complete() to ufshcd * .poweroff() uses ufhcd_wl_poweroff() * Removed several forward declarations * Moved scsi_register_driver() to ufshcd_core_init() v4 -> v5: - Addressed Adrian's comments * Used the rpmb driver contributed by Adrian * Runtime-resume the ufs device during suspend to honor spm-lvl * Unregister the scsi_driver in ufshcd_remove() * Currently shutdown() puts the ufs device to power-down mode so, just removed ufshcd_pci_poweroff() * Quiesce the scsi device during shutdown instead of remove v3 RFC -> v4: - Addressed Bart's comments * Except that I didn't get any checkpatch failures - Addressed Avri's comments - Addressed Adrian's comments * Added a check for deepsleep power mode * Removed a couple of forward declarations * Didn't separate the scsi drivers because in rpmb case it just sends uac in resume and it seemed pretty neat to me. - Added sysfs changes to resume the devices before accessing Asutosh Das (2): scsi: ufs: Enable power management for wlun ufs: sysfs: Resume the proper scsi device drivers/scsi/ufs/cdns-pltfrm.c | 2 + drivers/scsi/ufs/tc-dwc-g210-pci.c | 2 + drivers/scsi/ufs/ufs-debugfs.c | 6 +- drivers/scsi/ufs/ufs-debugfs.h | 2 +- drivers/scsi/ufs/ufs-exynos.c | 2 + drivers/scsi/ufs/ufs-hisi.c | 2 + drivers/scsi/ufs/ufs-mediatek.c | 12 +- drivers/scsi/ufs/ufs-qcom.c | 2 + drivers/scsi/ufs/ufs-sysfs.c | 24 +- drivers/scsi/ufs/ufs_bsg.c | 6 +- drivers/scsi/ufs/ufshcd-pci.c | 36 +- drivers/scsi/ufs/ufshcd.c | 691 ++++++++++++++++++++++++++----------- drivers/scsi/ufs/ufshcd.h | 21 ++ include/trace/events/ufs.h | 20 ++ 14 files changed, 563 insertions(+), 265 deletions(-)