| Message ID | 20210902101818.4132-4-adrian.hunter@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: ufs: Let devices remain runtime suspended during system suspend | expand |
On 9/2/2021 3:18 AM, 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 patches have cleared the way for that to happen: > scsi: ufs: Fix runtime PM dependencies getting broken > scsi: ufs: Fix error handler clear ua deadlock > > So amend the logic accordingly. > > Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/scsi/ufs/ufshcd.c | 45 ++++++++++++++++++++++++++++----------- > drivers/scsi/ufs/ufshcd.h | 11 +++++++++- > 2 files changed, 43 insertions(+), 13 deletions(-) > Hi Adrian, Thanks for the change. > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 57ed4b93b949..8e799e47e095 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -9722,13 +9722,29 @@ void ufshcd_resume_complete(struct device *dev) > ufshcd_rpm_put(hba); > hba->complete_put = false; > } > - if (hba->rpmb_complete_put) { > - ufshcd_rpmb_rpm_put(hba); > - hba->rpmb_complete_put = false; > - } > } > EXPORT_SYMBOL_GPL(ufshcd_resume_complete); > > +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; > + In the current ufshcd_suspend(), there's a ufshcd_vops_suspend(). That's invoked for different pm_ops independent of the rpm_lvl and spm_lvl. I'm not sure if any vendor driver does different things for diff pm_op. Perhaps something to check. > + 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) > { > struct ufs_hba *hba = dev_get_drvdata(dev); > @@ -9741,17 +9757,22 @@ 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 (!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; > } > - if (hba->sdev_rpmb) { > - ufshcd_rpmb_rpm_get_sync(hba); > - hba->rpmb_complete_put = true; > - } > return 0; > } > EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare); > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index 4723f27a55d1..149803d60ecb 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -915,7 +915,6 @@ struct ufs_hba { > #endif > u32 luns_avail; > bool complete_put; > - bool rpmb_complete_put; > }; > > /* Returns true if clocks can be gated. Otherwise false */ > @@ -1383,6 +1382,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 2/09/21 8:07 pm, Asutosh Das (asd) wrote: > On 9/2/2021 3:18 AM, 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 patches have cleared the way for that to happen: >> scsi: ufs: Fix runtime PM dependencies getting broken >> scsi: ufs: Fix error handler clear ua deadlock >> >> So amend the logic accordingly. >> >> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >> --- >> drivers/scsi/ufs/ufshcd.c | 45 ++++++++++++++++++++++++++++----------- >> drivers/scsi/ufs/ufshcd.h | 11 +++++++++- >> 2 files changed, 43 insertions(+), 13 deletions(-) >> > Hi Adrian, > Thanks for the change. > >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index 57ed4b93b949..8e799e47e095 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -9722,13 +9722,29 @@ void ufshcd_resume_complete(struct device *dev) >> ufshcd_rpm_put(hba); >> hba->complete_put = false; >> } >> - if (hba->rpmb_complete_put) { >> - ufshcd_rpmb_rpm_put(hba); >> - hba->rpmb_complete_put = false; >> - } >> } >> EXPORT_SYMBOL_GPL(ufshcd_resume_complete); >> +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; >> + > In the current ufshcd_suspend(), there's a ufshcd_vops_suspend(). > That's invoked for different pm_ops independent of the rpm_lvl and spm_lvl. > I'm not sure if any vendor driver does different things for diff pm_op. > Perhaps something to check. > Good point. The logic was that way before "scsi: ufs: core: Enable power management for wlu" which was first in v5.14, so drivers should expect the behaviour of this patch. Checking shows only ufs-hisi does something different but it sets different rpm_lvl and spm_lvl so wouldn't see any change. However I am sending a V2 patch that makes it explicit.
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 57ed4b93b949..8e799e47e095 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -9722,13 +9722,29 @@ void ufshcd_resume_complete(struct device *dev) ufshcd_rpm_put(hba); hba->complete_put = false; } - if (hba->rpmb_complete_put) { - ufshcd_rpmb_rpm_put(hba); - hba->rpmb_complete_put = false; - } } EXPORT_SYMBOL_GPL(ufshcd_resume_complete); +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) { struct ufs_hba *hba = dev_get_drvdata(dev); @@ -9741,17 +9757,22 @@ 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 (!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; } - if (hba->sdev_rpmb) { - ufshcd_rpmb_rpm_get_sync(hba); - hba->rpmb_complete_put = true; - } return 0; } EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare); diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 4723f27a55d1..149803d60ecb 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -915,7 +915,6 @@ struct ufs_hba { #endif u32 luns_avail; bool complete_put; - bool rpmb_complete_put; }; /* Returns true if clocks can be gated. Otherwise false */ @@ -1383,6 +1382,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);
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 patches have cleared the way for that to happen: scsi: ufs: Fix runtime PM dependencies getting broken scsi: ufs: Fix error handler clear ua deadlock So amend the logic accordingly. Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> --- drivers/scsi/ufs/ufshcd.c | 45 ++++++++++++++++++++++++++++----------- drivers/scsi/ufs/ufshcd.h | 11 +++++++++- 2 files changed, 43 insertions(+), 13 deletions(-)