| Message ID | 20221201094358.20700-1-peter.wang@mediatek.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] ufs: core: wlun suspend SSU/enter hibern8 fail recovery | expand |
On 12/1/22 01:43, peter.wang@mediatek.com wrote: > @@ -9049,6 +9050,20 @@ 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)) { Please remove the superfluous parentheses from the above statement. > + /* > + * If return err in suspend flow, IO will hang. > + * Trigger error handler and break suspend for > + * error recovery. > + */ > + spin_lock_irqsave(hba->host->host_lock, flags); __ufshcd_wl_suspend() is allowed to sleep. Please change spin_lock_irqsave() into spin_lock_irq(). > + hba->force_reset = true; > + ufshcd_schedule_eh_work(hba); > + spin_unlock_irqrestore(hba->host->host_lock, > + flags); > + > + ret = -EBUSY; Why is the value of 'ret' changed into -EBUSY? Can the above code be left out? > @@ -9060,6 +9075,20 @@ 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. > + */ > + spin_lock_irqsave(hba->host->host_lock, flags); > + hba->force_reset = true; > + ufshcd_schedule_eh_work(hba); > + spin_unlock_irqrestore(hba->host->host_lock, > + flags); > + > + ret = -EBUSY; > + } Same comments as above for this code block. Thanks, Bart.
On Thu, 2022-12-01 at 10:44 -0800, Bart Van Assche wrote: > On 12/1/22 01:43, peter.wang@mediatek.com wrote: > > @@ -9049,6 +9050,20 @@ 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)) { > > Please remove the superfluous parentheses from the above statement. > Hi Bart, Okay, will change next version. > > + /* > > + * If return err in suspend flow, IO > > will hang. > > + * Trigger error handler and break > > suspend for > > + * error recovery. > > + */ > > + spin_lock_irqsave(hba->host->host_lock, > > flags); > > __ufshcd_wl_suspend() is allowed to sleep. Please change > spin_lock_irqsave() into spin_lock_irq(). > > > + hba->force_reset = true; > > + ufshcd_schedule_eh_work(hba); > > + spin_unlock_irqrestore(hba->host- > > >host_lock, > > + flags); > > + > > + ret = -EBUSY; > > Why is the value of 'ret' changed into -EBUSY? Can the above code be > left out? > Because in runtime pm framework, return EBUSY is okay for next time runtime suspend retry. Return ETIMEOUT/EIO or other then EBUSY will keep error code in veriable "runtime_error". (supplier first then consumer resume block by supplier also get runtime_error too). Finally, lead to IO hang. > > @@ -9060,6 +9075,20 @@ 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. > > + */ > > + spin_lock_irqsave(hba->host->host_lock, flags); > > + hba->force_reset = true; > > + ufshcd_schedule_eh_work(hba); > > + spin_unlock_irqrestore(hba->host->host_lock, > > + flags); > > + > > + ret = -EBUSY; > > + } > > Same comments as above for this code block. > > Thanks, > > Bart. Okay, will change next version. Thanks BR Peter >
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index b1f59a5fe632..8e5f7d9fe7c2 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -8970,6 +8970,7 @@ static int __ufshcd_wl_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op) enum ufs_pm_level pm_lvl; enum ufs_dev_pwr_mode req_dev_pwr_mode; enum uic_link_state req_link_state; + unsigned long flags; hba->pm_op_in_progress = true; if (pm_op != UFS_SHUTDOWN_PM) { @@ -9049,6 +9050,20 @@ 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. + */ + spin_lock_irqsave(hba->host->host_lock, flags); + hba->force_reset = true; + ufshcd_schedule_eh_work(hba); + spin_unlock_irqrestore(hba->host->host_lock, + flags); + + ret = -EBUSY; + } if (ret) goto enable_scaling; } @@ -9060,6 +9075,20 @@ 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. + */ + spin_lock_irqsave(hba->host->host_lock, flags); + hba->force_reset = true; + ufshcd_schedule_eh_work(hba); + spin_unlock_irqrestore(hba->host->host_lock, + flags); + + ret = -EBUSY; + } if (ret) goto set_dev_active;