| Message ID | 20221208061037.24313-1-peter.wang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] ufs: core: wlun suspend SSU/enter hibern8 fail recovery | expand |
Hi Peter, Please update the patch version and bring all tags (review, ack ...etc.) to your new patch : ) On Thu, Dec 8, 2022 at 2:17 PM <peter.wang@mediatek.com> wrote: > > From: Peter Wang <peter.wang@mediatek.com> > > When SSU/enter hibern8 fail in wlun suspend flow, trigger error > handler and return busy to break the suspend. > If not, wlun runtime pm status become error and the consumer will > stuck in runtime suspend status. > > Fixes: b294ff3e3449 ("scsi: ufs: core: Enable power management for wlun") > Cc: stable@vger.kernel.org > Signed-off-by: Peter Wang <peter.wang@mediatek.com> > --- > drivers/ufs/core/ufshcd.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index b1f59a5fe632..c91d58d1486a 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -106,6 +106,13 @@ > 16, 4, buf, __len, false); \ > } while (0) > > +#define ufshcd_force_error_recovery(hba) do { \ > + spin_lock_irq(hba->host->host_lock); \ > + hba->force_reset = true; \ > + ufshcd_schedule_eh_work(hba); \ > + spin_unlock_irq(hba->host->host_lock); \ > +} while (0) > + > int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len, > const char *prefix) > { > @@ -9049,6 +9056,15 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > > if (!hba->dev_info.b_rpm_dev_flush_capable) { > ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode); > + if (ret && pm_op != UFS_SHUTDOWN_PM) { > + /* > + * If return err in suspend flow, IO will hang. > + * Trigger error handler and break suspend for > + * error recovery. > + */ > + ufshcd_force_error_recovery(hba); > + ret = -EBUSY; > + } > if (ret) > goto enable_scaling; > } > @@ -9060,6 +9076,15 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > */ > check_for_bkops = !ufshcd_is_ufs_dev_deepsleep(hba); > ret = ufshcd_link_state_transition(hba, req_link_state, check_for_bkops); > + if (ret && pm_op != UFS_SHUTDOWN_PM) { > + /* > + * If return err in suspend flow, IO will hang. > + * Trigger error handler and break suspend for > + * error recovery. > + */ > + ufshcd_force_error_recovery(hba); > + ret = -EBUSY; > + } > if (ret) > goto set_dev_active; > > -- > 2.18.0 >
On 8/12/22 08:10, peter.wang@mediatek.com wrote: > From: Peter Wang <peter.wang@mediatek.com> > > When SSU/enter hibern8 fail in wlun suspend flow, trigger error > handler and return busy to break the suspend. > If not, wlun runtime pm status become error and the consumer will > stuck in runtime suspend status. > > Fixes: b294ff3e3449 ("scsi: ufs: core: Enable power management for wlun") > Cc: stable@vger.kernel.org > Signed-off-by: Peter Wang <peter.wang@mediatek.com> > --- > drivers/ufs/core/ufshcd.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index b1f59a5fe632..c91d58d1486a 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -106,6 +106,13 @@ > 16, 4, buf, __len, false); \ > } while (0) > > +#define ufshcd_force_error_recovery(hba) do { \ > + spin_lock_irq(hba->host->host_lock); \ > + hba->force_reset = true; \ > + ufshcd_schedule_eh_work(hba); \ > + spin_unlock_irq(hba->host->host_lock); \ > +} while (0) Thanks for separating it out, but there is no reason to make this a macro, so it should be a function, because functions offer nicer structure and less chance of surprises. It need not be an inline function either because the compiler can determine that, and this is not a hot path. i.e. static void ufshcd_force_error_recovery(struct ufs_hba *hba) { spin_lock_irq(hba->host->host_lock); hba->force_reset = true; ufshcd_schedule_eh_work(hba); spin_unlock_irq(hba->host->host_lock); } > + > int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len, > const char *prefix) > { > @@ -9049,6 +9056,15 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > > if (!hba->dev_info.b_rpm_dev_flush_capable) { > ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode); > + if (ret && pm_op != UFS_SHUTDOWN_PM) { > + /* > + * If return err in suspend flow, IO will hang. > + * Trigger error handler and break suspend for > + * error recovery. > + */ > + ufshcd_force_error_recovery(hba); > + ret = -EBUSY; > + } > if (ret) > goto enable_scaling; > } > @@ -9060,6 +9076,15 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) > */ > check_for_bkops = !ufshcd_is_ufs_dev_deepsleep(hba); > ret = ufshcd_link_state_transition(hba, req_link_state, check_for_bkops); > + if (ret && pm_op != UFS_SHUTDOWN_PM) { > + /* > + * If return err in suspend flow, IO will hang. > + * Trigger error handler and break suspend for > + * error recovery. > + */ > + ufshcd_force_error_recovery(hba); > + ret = -EBUSY; > + } > if (ret) > goto set_dev_active; >
On Thu, 2022-12-08 at 08:41 +0200, Adrian Hunter wrote: > On 8/12/22 08:10, peter.wang@mediatek.com wrote: > > From: Peter Wang <peter.wang@mediatek.com> > > > > When SSU/enter hibern8 fail in wlun suspend flow, trigger error > > handler and return busy to break the suspend. > > If not, wlun runtime pm status become error and the consumer will > > stuck in runtime suspend status. > > > > Fixes: b294ff3e3449 ("scsi: ufs: core: Enable power management for > > wlun") > > Cc: stable@vger.kernel.org > > Signed-off-by: Peter Wang <peter.wang@mediatek.com> > > --- > > drivers/ufs/core/ufshcd.c | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > > index b1f59a5fe632..c91d58d1486a 100644 > > --- a/drivers/ufs/core/ufshcd.c > > +++ b/drivers/ufs/core/ufshcd.c > > @@ -106,6 +106,13 @@ > > 16, 4, buf, __len, > > false); \ > > } while (0) > > > > +#define ufshcd_force_error_recovery(hba) do > > { \ > > + spin_lock_irq(hba->host- > > >host_lock); \ > > + hba->force_reset = > > true; \ > > + ufshcd_schedule_eh_work(hba); > > \ > > + spin_unlock_irq(hba->host- > > >host_lock); \ > > +} while (0) > > Thanks for separating it out, but there is no reason to make this > a macro, so it should be a function, because functions offer nicer > structure and less chance of surprises. It need not be an inline > function either because the compiler can determine that, and this > is not a hot path. i.e. > > static void ufshcd_force_error_recovery(struct ufs_hba *hba) > { > spin_lock_irq(hba->host->host_lock); > hba->force_reset = true; > ufshcd_schedule_eh_work(hba); > spin_unlock_irq(hba->host->host_lock); > } > Hi Adrian, Okay, will change to static function next version, Thank you very much. :) Peter > > + > > int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t > > len, > > const char *prefix) > > { > > @@ -9049,6 +9056,15 @@ static int __ufshcd_wl_suspend(struct > > ufs_hba *hba, enum ufs_pm_op pm_op) > > > > if (!hba->dev_info.b_rpm_dev_flush_capable) { > > ret = ufshcd_set_dev_pwr_mode(hba, > > req_dev_pwr_mode); > > + if (ret && pm_op != UFS_SHUTDOWN_PM) { > > + /* > > + * If return err in suspend flow, IO > > will hang. > > + * Trigger error handler and break > > suspend for > > + * error recovery. > > + */ > > + ufshcd_force_error_recovery(hba); > > + ret = -EBUSY; > > + } > > if (ret) > > goto enable_scaling; > > } > > @@ -9060,6 +9076,15 @@ static int __ufshcd_wl_suspend(struct > > ufs_hba *hba, enum ufs_pm_op pm_op) > > */ > > check_for_bkops = !ufshcd_is_ufs_dev_deepsleep(hba); > > ret = ufshcd_link_state_transition(hba, req_link_state, > > check_for_bkops); > > + if (ret && pm_op != UFS_SHUTDOWN_PM) { > > + /* > > + * If return err in suspend flow, IO will hang. > > + * Trigger error handler and break suspend for > > + * error recovery. > > + */ > > + ufshcd_force_error_recovery(hba); > > + ret = -EBUSY; > > + } > > if (ret) > > goto set_dev_active; > > > >
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index b1f59a5fe632..c91d58d1486a 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -106,6 +106,13 @@ 16, 4, buf, __len, false); \ } while (0) +#define ufshcd_force_error_recovery(hba) do { \ + spin_lock_irq(hba->host->host_lock); \ + hba->force_reset = true; \ + ufshcd_schedule_eh_work(hba); \ + spin_unlock_irq(hba->host->host_lock); \ +} while (0) + int ufshcd_dump_regs(struct ufs_hba *hba, size_t offset, size_t len, const char *prefix) { @@ -9049,6 +9056,15 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) if (!hba->dev_info.b_rpm_dev_flush_capable) { ret = ufshcd_set_dev_pwr_mode(hba, req_dev_pwr_mode); + if (ret && pm_op != UFS_SHUTDOWN_PM) { + /* + * If return err in suspend flow, IO will hang. + * Trigger error handler and break suspend for + * error recovery. + */ + ufshcd_force_error_recovery(hba); + ret = -EBUSY; + } if (ret) goto enable_scaling; } @@ -9060,6 +9076,15 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) */ check_for_bkops = !ufshcd_is_ufs_dev_deepsleep(hba); ret = ufshcd_link_state_transition(hba, req_link_state, check_for_bkops); + if (ret && pm_op != UFS_SHUTDOWN_PM) { + /* + * If return err in suspend flow, IO will hang. + * Trigger error handler and break suspend for + * error recovery. + */ + ufshcd_force_error_recovery(hba); + ret = -EBUSY; + } if (ret) goto set_dev_active;