Re: [PATCH v2] ufs: core: wlun suspend SSU/enter hibern8 fail recovery

["Peter Wang (王信友)" <peter.wang@mediatek.com>]

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


>