[PATCH v1] ufs: core: wlun suspend SSU fail recovery

[PATCH v1] ufs: core: wlun suspend SSU fail recovery

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

When SSU fail in wlun suspend flow, trigger error handlder and
return busy to break the suspend.
If not, wlun runtime pm status become error and the consumer will
stuck in runtime suspend status.

Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/core/ufshcd.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index b1f59a5fe632..2f2d3d5d8684 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,8 +9050,21 @@ 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)
+			if (ret) {
+				/*
+				 * If retrun 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;
 				goto enable_scaling;
+			}
 		}
 	}
 
-- 
2.18.0

Re: [PATCH v1] ufs: core: wlun suspend SSU fail recovery

[Adrian Hunter]

On 1/11/22 16:24, peter.wang@mediatek.com wrote:
> From: Peter Wang <peter.wang@mediatek.com>
> 
> When SSU fail in wlun suspend flow, trigger error handlder and

handlder -> handler

Why / how does SSU fail?

> return busy to break the suspend.
> If not, wlun runtime pm status become error and the consumer will
> stuck in runtime suspend status.
> 
> Signed-off-by: Peter Wang <peter.wang@mediatek.com>
> ---
>  drivers/ufs/core/ufshcd.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index b1f59a5fe632..2f2d3d5d8684 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,8 +9050,21 @@ 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)
> +			if (ret) {
> +				/*
> +				 * If retrun err in suspend flow, IO will hang.

retrun -> return

> +				 * 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);

__ufshcd_wl_suspend() is also used for shutdown and poweroff
where scheduling EH is not appropriate.

> +				spin_unlock_irqrestore(hba->host->host_lock,
> +					flags);
> +
> +				ret = -EBUSY;
>  				goto enable_scaling;
> +			}
>  		}
>  	}
>  

Re: [PATCH v1] ufs: core: wlun suspend SSU fail recovery

[Bart Van Assche]

On 11/30/22 02:17, Adrian Hunter wrote:
> On 1/11/22 16:24, peter.wang@mediatek.com wrote:
>> From: Peter Wang <peter.wang@mediatek.com>
>>
>> When SSU fail in wlun suspend flow, trigger error handlder and
> 
> handlder -> handler
> 
> Why / how does SSU fail?

I'm not sure but the issue that Peter is trying to fix with this patch 
may already have been fixed by my patch series "Fix a deadlock in the 
UFS driver".

Thanks,

Bart.

Re: [PATCH v1] ufs: core: wlun suspend SSU fail recovery

[Peter Wang (王信友)]

On Wed, 2022-11-30 at 09:49 -0800, Bart Van Assche wrote:
> On 11/30/22 02:17, Adrian Hunter wrote:
> > On 1/11/22 16:24, peter.wang@mediatek.com wrote:
> > > From: Peter Wang <peter.wang@mediatek.com>
> > > 
> > > When SSU fail in wlun suspend flow, trigger error handlder and
> > 
> > handlder -> handler
> > 
> > Why / how does SSU fail?
> 
> I'm not sure but the issue that Peter is trying to fix with this
> patch 
> may already have been fixed by my patch series "Fix a deadlock in
> the 
> UFS driver".
> 
> Thanks,
> 
> Bart.
> 

Hi Bart,

Yes, it may also can fix by 
"[v4,07/10] scsi: ufs: Try harder to change the power mode"

But I am not sure if any outher corner case SSU will got antother err
which retry canoot fix? (ex, device always return same error sense code
which host is not expect to receive again, again and again let retry
sill fail)

By the way, in wl suspend flow, not only SSU, but also enter
hibern8(0x17) could got fail. I think it is better fix in both fail
case.
Hera is our 0x17 timeout fail log and runtime suspend retrun timeout.

473-c(1), 21140.355276017,31742,18, rs, ret=-110, time_us= 2892755,
pwr_mode=2, link_status=3
472-u(1), 21139.829423708,31742, 7,0x17,arg1=0x0,arg2=0x0,arg3=0x0,	
0
471-r(1), 21139.829361861,    0,
1,0x1b,t=46,db:0x       0,is:0x   80000,crypt:0,0,lba=         0,len=  
  -1,	1738384
470-r(1), 21139.827623708,31742,
0,0x1b,t=46,db:0x       0,is:0x   80000,crypt:0,0,lba=         0,len=  
  -1,	0


Thanks.
Peter

Re: [PATCH v1] ufs: core: wlun suspend SSU fail recovery

[Daniil Lunev]

On Thu, Dec 1, 2022 at 4:50 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 11/30/22 02:17, Adrian Hunter wrote:
> > On 1/11/22 16:24, peter.wang@mediatek.com wrote:
> >> From: Peter Wang <peter.wang@mediatek.com>
> >>
> >> When SSU fail in wlun suspend flow, trigger error handlder and
> >
> > handlder -> handler
> >
> > Why / how does SSU fail?
>
> I'm not sure but the issue that Peter is trying to fix with this patch
> may already have been fixed by my patch series "Fix a deadlock in the
> UFS driver".

We observe a similar failure scenario when a device tries to change
power mode (e.g. due to autosuspend) while "purge" is in progress. It
appears that in this case the "pm_only" counter is increased on the
device queue prior to the attempt of entering a runtime suspended
state, but not reduced upon it failing due to the device being busy
with an ongoing purge. That leads to a deadlock of subsequent IO
operations to the device.

--Daniil

Re: [PATCH v1] ufs: core: wlun suspend SSU fail recovery

[Peter Wang (王信友)]

On Fri, 2022-12-02 at 10:32 +1100, Daniil Lunev wrote:
> On Thu, Dec 1, 2022 at 4:50 AM Bart Van Assche <bvanassche@acm.org>
> wrote:
> > 
> > On 11/30/22 02:17, Adrian Hunter wrote:
> > > On 1/11/22 16:24, peter.wang@mediatek.com wrote:
> > > > From: Peter Wang <peter.wang@mediatek.com>
> > > > 
> > > > When SSU fail in wlun suspend flow, trigger error handlder and
> > > 
> > > handlder -> handler
> > > 
> > > Why / how does SSU fail?
> > 
> > I'm not sure but the issue that Peter is trying to fix with this
> > patch
> > may already have been fixed by my patch series "Fix a deadlock in
> > the
> > UFS driver".
> 
> We observe a similar failure scenario when a device tries to change
> power mode (e.g. due to autosuspend) while "purge" is in progress. It
> appears that in this case the "pm_only" counter is increased on the
> device queue prior to the attempt of entering a runtime suspended
> state, but not reduced upon it failing due to the device being busy
> with an ongoing purge. That leads to a deadlock of subsequent IO
> operations to the device.
> 
> --Daniil

Hi Daniil,

May I have a question, which dievce you use in this failure scenario?  
I think it is same issue due to SSU fail, and wlun devcie pm enter
error state. So the cunsomer(scsi lu device) is block in suspend state
and connot resume to reduce pm_only lead to IO hang.

Thanks.
BR
Peter

Re: [PATCH v1] ufs: core: wlun suspend SSU fail recovery

[Daniil Lunev]

> May I have a question, which dievce you use in this failure scenario?
> I think it is same issue due to SSU fail, and wlun devcie pm enter
> error state. So the cunsomer(scsi lu device) is block in suspend state
> and connot resume to reduce pm_only lead to IO hang.
>
> Thanks.
> BR
> Peter

I don't think it is device specific. I am pretty sure it is the same
problem, for it traces to the same origin and your patch fixes the
issue (though with a lot of log spam which is a bit unfortunate, but
we can live with that)

--Daniil

Re: [PATCH v1] ufs: core: wlun suspend SSU fail recovery

[Peter Wang (王信友)]

Hi Daniil,

Thanks your feedback. 
I am glad to hear your problem can be solved by this patch.
Please also help have a review of my v3 patch

https://patchwork.kernel.org/project/linux-scsi/patch/20221202073532.7884-1-peter.wang@mediatek.com/

Thanks.
BR
Peter



On Mon, 2022-12-05 at 07:02 +1100, Daniil Lunev wrote:
> > May I have a question, which dievce you use in this failure
> > scenario?
> > I think it is same issue due to SSU fail, and wlun devcie pm enter
> > error state. So the cunsomer(scsi lu device) is block in suspend
> > state
> > and connot resume to reduce pm_only lead to IO hang.
> > 
> > Thanks.
> > BR
> > Peter
> 
> I don't think it is device specific. I am pretty sure it is the same
> problem, for it traces to the same origin and your patch fixes the
> issue (though with a lot of log spam which is a bit unfortunate, but
> we can live with that)
> 
> --Daniil

Re: [PATCH v1] ufs: core: wlun suspend SSU fail recovery

[Peter Wang (王信友)]

On Wed, 2022-11-30 at 12:17 +0200, Adrian Hunter wrote:
> On 1/11/22 16:24, peter.wang@mediatek.com wrote:
> > From: Peter Wang <peter.wang@mediatek.com>
> > 
> > When SSU fail in wlun suspend flow, trigger error handlder and
> 
> handlder -> handler

will fix next versio, thanks.

> 
> Why / how does SSU fail?
> 

Unknow, it is seldom and we cannot catch fail the failure scenarios. We
suspect is maybe the device problem which response wrong sense code.
So, I think the driver is better to cover this error.
Here is our cmd histroy log with 3 times SSU fail, and retrun busy to
do error handler.

 26-c(1),  2576.085601525, 9396,18, rs, ret=-16, time_us=   41136,
pwr_mode=1, link_status=1
 25-r(1),  2576.085027371, 9396,
1,0x1b,t=53,db:0x       0,is:0x   80000,crypt:0,0,lba=         0,len=  
  -1,	31308
 24-r(1),  2576.084996910, 9396,
0,0x1b,t=53,db:0x       0,is:0x   80000,crypt:0,0,lba=         0,len=  
  -1,	0
 23-r(1),  2576.084919679, 9396,
1,0x1b,t=52,db:0x       0,is:0x   80000,crypt:0,0,lba=         0,len=  
  -1,	32846
 22-r(1),  2576.084887679, 9396,
0,0x1b,t=52,db:0x       0,is:0x   80000,crypt:0,0,lba=         0,len=  
  -1,	0
 21-r(1),  2576.084704295,    0,
1,0x1b,t=55,db:0x       0,is:0x   80000,crypt:0,0,lba=         0,len=  
  -1,	39777539
 20-r(1),  2576.044927679, 9396,
0,0x1b,t=55,db:0x       0,is:0x   80000,crypt:0,0,lba=         0,len=  
  -1,	0


> > return busy to break the suspend.
> > If not, wlun runtime pm status become error and the consumer will
> > stuck in runtime suspend status.
> > 
> > Signed-off-by: Peter Wang <peter.wang@mediatek.com>
> > ---
> >  drivers/ufs/core/ufshcd.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index b1f59a5fe632..2f2d3d5d8684 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,8 +9050,21 @@ 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)
> > +			if (ret) {
> > +				/*
> > +				 * If retrun err in suspend flow, IO
> > will hang.
> 
> retrun -> return

will fix next version, thanks

> 
> > +				 * 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);
> 
> __ufshcd_wl_suspend() is also used for shutdown and poweroff
> where scheduling EH is not appropriate.

Will check pm_op and bypass UFS_SHUTDOWN_PM next version, thanks.

> 
> > +				spin_unlock_irqrestore(hba->host-
> > >host_lock,
> > +					flags);
> > +
> > +				ret = -EBUSY;
> >  				goto enable_scaling;
> > +			}
> >  		}
> >  	}
> >  
> 
>