| Message ID | 20211027130614.406985-2-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | scsi: ufs: Let devices remain runtime suspended during system suspend | expand |
On 27/10/2021 16:06, Adrian Hunter wrote: > If the UFS Device WLUN is runtime suspended and is in the same power > mode, link state and b_rpm_dev_flush_capable (BKOP or WB buffer flush etc) > state, then it can remain runtime suspended instead of being runtime > resumed and then system suspended. > > The following patch has cleared the way for that to happen: > scsi: core: pm: Only runtime resume if necessary > > So amend the logic accordingly. > > Note, the ufs-hisi driver uses different RPM and SPM, but it is made > explicit by a new parameter to suspend prepare. > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > Reviewed-by: Asutosh Das <asutoshd@codeaurora.org> Any more comments on this? > --- > drivers/scsi/ufs/ufs-hisi.c | 8 ++++++- > drivers/scsi/ufs/ufshcd.c | 45 ++++++++++++++++++++++++++++++++----- > drivers/scsi/ufs/ufshcd.h | 11 +++++++++ > 3 files changed, 58 insertions(+), 6 deletions(-) > > diff --git a/drivers/scsi/ufs/ufs-hisi.c b/drivers/scsi/ufs/ufs-hisi.c > index 8c7e8d321746..ab1a7ebd89b1 100644 > --- a/drivers/scsi/ufs/ufs-hisi.c > +++ b/drivers/scsi/ufs/ufs-hisi.c > @@ -396,6 +396,12 @@ static int ufs_hisi_pwr_change_notify(struct ufs_hba *hba, > return ret; > } > > +static int ufs_hisi_suspend_prepare(struct device *dev) > +{ > + /* RPM and SPM are different. Refer ufs_hisi_suspend() */ > + return __ufshcd_suspend_prepare(dev, false); > +} > + > static int ufs_hisi_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op, > enum ufs_notify_change_status status) > { > @@ -578,7 +584,7 @@ static int ufs_hisi_remove(struct platform_device *pdev) > static const struct dev_pm_ops ufs_hisi_pm_ops = { > SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume) > SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL) > - .prepare = ufshcd_suspend_prepare, > + .prepare = ufs_hisi_suspend_prepare, > .complete = ufshcd_resume_complete, > }; > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 04cb67995750..115ea16f5a22 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -9707,7 +9707,27 @@ void ufshcd_resume_complete(struct device *dev) > } > EXPORT_SYMBOL_GPL(ufshcd_resume_complete); > > -int ufshcd_suspend_prepare(struct device *dev) > +static bool ufshcd_rpm_ok_for_spm(struct ufs_hba *hba) > +{ > + struct device *dev = &hba->sdev_ufs_device->sdev_gendev; > + enum ufs_dev_pwr_mode dev_pwr_mode; > + enum uic_link_state link_state; > + unsigned long flags; > + bool res; > + > + spin_lock_irqsave(&dev->power.lock, flags); > + dev_pwr_mode = ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl); > + link_state = ufs_get_pm_lvl_to_link_pwr_state(hba->spm_lvl); > + res = pm_runtime_suspended(dev) && > + hba->curr_dev_pwr_mode == dev_pwr_mode && > + hba->uic_link_state == link_state && > + !hba->dev_info.b_rpm_dev_flush_capable; > + spin_unlock_irqrestore(&dev->power.lock, flags); > + > + return res; > +} > + > +int __ufshcd_suspend_prepare(struct device *dev, bool rpm_ok_for_spm) > { > struct ufs_hba *hba = dev_get_drvdata(dev); > int ret; > @@ -9719,15 +9739,30 @@ int ufshcd_suspend_prepare(struct device *dev) > * Refer ufshcd_resume_complete() > */ > if (hba->sdev_ufs_device) { > - ret = ufshcd_rpm_get_sync(hba); > - if (ret < 0 && ret != -EACCES) { > - ufshcd_rpm_put(hba); > - return ret; > + /* Prevent runtime suspend */ > + ufshcd_rpm_get_noresume(hba); > + /* > + * Check if already runtime suspended in same state as system > + * suspend would be. > + */ > + if (!rpm_ok_for_spm || !ufshcd_rpm_ok_for_spm(hba)) { > + /* RPM state is not ok for SPM, so runtime resume */ > + ret = ufshcd_rpm_resume(hba); > + if (ret < 0 && ret != -EACCES) { > + ufshcd_rpm_put(hba); > + return ret; > + } > } > hba->complete_put = true; > } > return 0; > } > +EXPORT_SYMBOL_GPL(__ufshcd_suspend_prepare); > + > +int ufshcd_suspend_prepare(struct device *dev) > +{ > + return __ufshcd_suspend_prepare(dev, true); > +} > EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare); > > #ifdef CONFIG_PM_SLEEP > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index 3f5dc6732fe1..b9492f300bd1 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -1199,6 +1199,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba, > > int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable); > int ufshcd_suspend_prepare(struct device *dev); > +int __ufshcd_suspend_prepare(struct device *dev, bool rpm_ok_for_spm); > void ufshcd_resume_complete(struct device *dev); > > /* Wrapper functions for safely calling variant operations */ > @@ -1408,6 +1409,16 @@ static inline int ufshcd_rpm_put_sync(struct ufs_hba *hba) > return pm_runtime_put_sync(&hba->sdev_ufs_device->sdev_gendev); > } > > +static inline void ufshcd_rpm_get_noresume(struct ufs_hba *hba) > +{ > + pm_runtime_get_noresume(&hba->sdev_ufs_device->sdev_gendev); > +} > + > +static inline int ufshcd_rpm_resume(struct ufs_hba *hba) > +{ > + return pm_runtime_resume(&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); >
On 12/11/2021 09:09, Adrian Hunter wrote: > On 27/10/2021 16:06, Adrian Hunter wrote: >> If the UFS Device WLUN is runtime suspended and is in the same power >> mode, link state and b_rpm_dev_flush_capable (BKOP or WB buffer flush etc) >> state, then it can remain runtime suspended instead of being runtime >> resumed and then system suspended. >> >> The following patch has cleared the way for that to happen: >> scsi: core: pm: Only runtime resume if necessary >> >> So amend the logic accordingly. >> >> Note, the ufs-hisi driver uses different RPM and SPM, but it is made >> explicit by a new parameter to suspend prepare. >> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >> Reviewed-by: Asutosh Das <asutoshd@codeaurora.org> > > Any more comments on this? Seems like there are no objections, so can this be applied for 5.17 ? > >> --- >> drivers/scsi/ufs/ufs-hisi.c | 8 ++++++- >> drivers/scsi/ufs/ufshcd.c | 45 ++++++++++++++++++++++++++++++++----- >> drivers/scsi/ufs/ufshcd.h | 11 +++++++++ >> 3 files changed, 58 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/scsi/ufs/ufs-hisi.c b/drivers/scsi/ufs/ufs-hisi.c >> index 8c7e8d321746..ab1a7ebd89b1 100644 >> --- a/drivers/scsi/ufs/ufs-hisi.c >> +++ b/drivers/scsi/ufs/ufs-hisi.c >> @@ -396,6 +396,12 @@ static int ufs_hisi_pwr_change_notify(struct ufs_hba *hba, >> return ret; >> } >> >> +static int ufs_hisi_suspend_prepare(struct device *dev) >> +{ >> + /* RPM and SPM are different. Refer ufs_hisi_suspend() */ >> + return __ufshcd_suspend_prepare(dev, false); >> +} >> + >> static int ufs_hisi_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op, >> enum ufs_notify_change_status status) >> { >> @@ -578,7 +584,7 @@ static int ufs_hisi_remove(struct platform_device *pdev) >> static const struct dev_pm_ops ufs_hisi_pm_ops = { >> SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume) >> SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL) >> - .prepare = ufshcd_suspend_prepare, >> + .prepare = ufs_hisi_suspend_prepare, >> .complete = ufshcd_resume_complete, >> }; >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index 04cb67995750..115ea16f5a22 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -9707,7 +9707,27 @@ void ufshcd_resume_complete(struct device *dev) >> } >> EXPORT_SYMBOL_GPL(ufshcd_resume_complete); >> >> -int ufshcd_suspend_prepare(struct device *dev) >> +static bool ufshcd_rpm_ok_for_spm(struct ufs_hba *hba) >> +{ >> + struct device *dev = &hba->sdev_ufs_device->sdev_gendev; >> + enum ufs_dev_pwr_mode dev_pwr_mode; >> + enum uic_link_state link_state; >> + unsigned long flags; >> + bool res; >> + >> + spin_lock_irqsave(&dev->power.lock, flags); >> + dev_pwr_mode = ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl); >> + link_state = ufs_get_pm_lvl_to_link_pwr_state(hba->spm_lvl); >> + res = pm_runtime_suspended(dev) && >> + hba->curr_dev_pwr_mode == dev_pwr_mode && >> + hba->uic_link_state == link_state && >> + !hba->dev_info.b_rpm_dev_flush_capable; >> + spin_unlock_irqrestore(&dev->power.lock, flags); >> + >> + return res; >> +} >> + >> +int __ufshcd_suspend_prepare(struct device *dev, bool rpm_ok_for_spm) >> { >> struct ufs_hba *hba = dev_get_drvdata(dev); >> int ret; >> @@ -9719,15 +9739,30 @@ int ufshcd_suspend_prepare(struct device *dev) >> * Refer ufshcd_resume_complete() >> */ >> if (hba->sdev_ufs_device) { >> - ret = ufshcd_rpm_get_sync(hba); >> - if (ret < 0 && ret != -EACCES) { >> - ufshcd_rpm_put(hba); >> - return ret; >> + /* Prevent runtime suspend */ >> + ufshcd_rpm_get_noresume(hba); >> + /* >> + * Check if already runtime suspended in same state as system >> + * suspend would be. >> + */ >> + if (!rpm_ok_for_spm || !ufshcd_rpm_ok_for_spm(hba)) { >> + /* RPM state is not ok for SPM, so runtime resume */ >> + ret = ufshcd_rpm_resume(hba); >> + if (ret < 0 && ret != -EACCES) { >> + ufshcd_rpm_put(hba); >> + return ret; >> + } >> } >> hba->complete_put = true; >> } >> return 0; >> } >> +EXPORT_SYMBOL_GPL(__ufshcd_suspend_prepare); >> + >> +int ufshcd_suspend_prepare(struct device *dev) >> +{ >> + return __ufshcd_suspend_prepare(dev, true); >> +} >> EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare); >> >> #ifdef CONFIG_PM_SLEEP >> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h >> index 3f5dc6732fe1..b9492f300bd1 100644 >> --- a/drivers/scsi/ufs/ufshcd.h >> +++ b/drivers/scsi/ufs/ufshcd.h >> @@ -1199,6 +1199,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba, >> >> int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable); >> int ufshcd_suspend_prepare(struct device *dev); >> +int __ufshcd_suspend_prepare(struct device *dev, bool rpm_ok_for_spm); >> void ufshcd_resume_complete(struct device *dev); >> >> /* Wrapper functions for safely calling variant operations */ >> @@ -1408,6 +1409,16 @@ static inline int ufshcd_rpm_put_sync(struct ufs_hba *hba) >> return pm_runtime_put_sync(&hba->sdev_ufs_device->sdev_gendev); >> } >> >> +static inline void ufshcd_rpm_get_noresume(struct ufs_hba *hba) >> +{ >> + pm_runtime_get_noresume(&hba->sdev_ufs_device->sdev_gendev); >> +} >> + >> +static inline int ufshcd_rpm_resume(struct ufs_hba *hba) >> +{ >> + return pm_runtime_resume(&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); >> >
On Wed, 2021-10-27 at 16:06 +0300, Adrian Hunter wrote: > If the UFS Device WLUN is runtime suspended and is in the same power > > mode, link state and b_rpm_dev_flush_capable (BKOP or WB buffer flush > etc) > > state, then it can remain runtime suspended instead of being runtime > > resumed and then system suspended. > > > > The following patch has cleared the way for that to happen: > > scsi: core: pm: Only runtime resume if necessary > > > > So amend the logic accordingly. > > > > Note, the ufs-hisi driver uses different RPM and SPM, but it is made > > explicit by a new parameter to suspend prepare. > > > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > > Reviewed-by: Asutosh Das <asutoshd@codeaurora.org> Reviewed-by: Bean Huo <beanhuo@micron.com>
Adrian,
> Seems like there are no objections, so can this be applied for 5.17 ?
Applied to 5.17/scsi-staging, thanks!
diff --git a/drivers/scsi/ufs/ufs-hisi.c b/drivers/scsi/ufs/ufs-hisi.c index 8c7e8d321746..ab1a7ebd89b1 100644 --- a/drivers/scsi/ufs/ufs-hisi.c +++ b/drivers/scsi/ufs/ufs-hisi.c @@ -396,6 +396,12 @@ static int ufs_hisi_pwr_change_notify(struct ufs_hba *hba, return ret; } +static int ufs_hisi_suspend_prepare(struct device *dev) +{ + /* RPM and SPM are different. Refer ufs_hisi_suspend() */ + return __ufshcd_suspend_prepare(dev, false); +} + static int ufs_hisi_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op, enum ufs_notify_change_status status) { @@ -578,7 +584,7 @@ static int ufs_hisi_remove(struct platform_device *pdev) static const struct dev_pm_ops ufs_hisi_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume) SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL) - .prepare = ufshcd_suspend_prepare, + .prepare = ufs_hisi_suspend_prepare, .complete = ufshcd_resume_complete, }; diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 04cb67995750..115ea16f5a22 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -9707,7 +9707,27 @@ void ufshcd_resume_complete(struct device *dev) } EXPORT_SYMBOL_GPL(ufshcd_resume_complete); -int ufshcd_suspend_prepare(struct device *dev) +static bool ufshcd_rpm_ok_for_spm(struct ufs_hba *hba) +{ + struct device *dev = &hba->sdev_ufs_device->sdev_gendev; + enum ufs_dev_pwr_mode dev_pwr_mode; + enum uic_link_state link_state; + unsigned long flags; + bool res; + + spin_lock_irqsave(&dev->power.lock, flags); + dev_pwr_mode = ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl); + link_state = ufs_get_pm_lvl_to_link_pwr_state(hba->spm_lvl); + res = pm_runtime_suspended(dev) && + hba->curr_dev_pwr_mode == dev_pwr_mode && + hba->uic_link_state == link_state && + !hba->dev_info.b_rpm_dev_flush_capable; + spin_unlock_irqrestore(&dev->power.lock, flags); + + return res; +} + +int __ufshcd_suspend_prepare(struct device *dev, bool rpm_ok_for_spm) { struct ufs_hba *hba = dev_get_drvdata(dev); int ret; @@ -9719,15 +9739,30 @@ int ufshcd_suspend_prepare(struct device *dev) * Refer ufshcd_resume_complete() */ if (hba->sdev_ufs_device) { - ret = ufshcd_rpm_get_sync(hba); - if (ret < 0 && ret != -EACCES) { - ufshcd_rpm_put(hba); - return ret; + /* Prevent runtime suspend */ + ufshcd_rpm_get_noresume(hba); + /* + * Check if already runtime suspended in same state as system + * suspend would be. + */ + if (!rpm_ok_for_spm || !ufshcd_rpm_ok_for_spm(hba)) { + /* RPM state is not ok for SPM, so runtime resume */ + ret = ufshcd_rpm_resume(hba); + if (ret < 0 && ret != -EACCES) { + ufshcd_rpm_put(hba); + return ret; + } } hba->complete_put = true; } return 0; } +EXPORT_SYMBOL_GPL(__ufshcd_suspend_prepare); + +int ufshcd_suspend_prepare(struct device *dev) +{ + return __ufshcd_suspend_prepare(dev, true); +} EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare); #ifdef CONFIG_PM_SLEEP diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 3f5dc6732fe1..b9492f300bd1 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -1199,6 +1199,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba, int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable); int ufshcd_suspend_prepare(struct device *dev); +int __ufshcd_suspend_prepare(struct device *dev, bool rpm_ok_for_spm); void ufshcd_resume_complete(struct device *dev); /* Wrapper functions for safely calling variant operations */ @@ -1408,6 +1409,16 @@ static inline int ufshcd_rpm_put_sync(struct ufs_hba *hba) return pm_runtime_put_sync(&hba->sdev_ufs_device->sdev_gendev); } +static inline void ufshcd_rpm_get_noresume(struct ufs_hba *hba) +{ + pm_runtime_get_noresume(&hba->sdev_ufs_device->sdev_gendev); +} + +static inline int ufshcd_rpm_resume(struct ufs_hba *hba) +{ + return pm_runtime_resume(&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);