Re: [PATCH v2 09/12] scsi: ufs: Simplify ufshcd_abort_all()

[Bart Van Assche <bvanassche@xxxxxxx>]

On 11/14/23 22:57, Peter Wang (王信友) wrote:
> On Thu, 2023-07-27 at 12:41 -0700, Bart Van Assche wrote:
> > Unify the MCQ and legacy code paths. This patch reworks code
> > introduced by
> > commit ab248643d3d6 ("scsi: ufs: core: Add error handling for MCQ
> > mode").
> >
> > Cc: Bao D. Nguyen <quic_nguyenb@xxxxxxxxxxx>
> > Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> > ---
> >   drivers/ufs/core/ufshcd.c | 46 +++++++++++++++++------------------
> > ----
> >   1 file changed, 20 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index c0031cf8855c..bf76ea59ba6c 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -6387,6 +6387,22 @@ static bool
> > ufshcd_is_pwr_mode_restore_needed(struct ufs_hba *hba)
> >   	return false;
> >   }
> >   
> > +static bool ufshcd_abort_one(struct request *rq, void *priv)
> > +{
> > +	int *ret = priv;
> > +	u32 tag = rq->tag;
> > +	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
> > +	struct scsi_device *sdev = cmd->device;
> > +	struct Scsi_Host *shost = sdev->host;
> > +	struct ufs_hba *hba = shost_priv(shost);
> > +
> > +	*ret = ufshcd_try_to_abort_task(hba, tag);
> > +	dev_err(hba->dev, "Aborting tag %d / CDB %#02x %s\n", tag,
> > +		hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1,
> > +		*ret ? "failed" : "succeeded");
> > +	return *ret == 0;
> > +}
> > +
> >   /**
> >    * ufshcd_abort_all - Abort all pending commands.
> >    * @hba: Host bus adapter pointer.
> > @@ -6395,34 +6411,12 @@ static bool
> > ufshcd_is_pwr_mode_restore_needed(struct ufs_hba *hba)
> >    */
> >   static bool ufshcd_abort_all(struct ufs_hba *hba)
> >   {
> > -	int tag, ret;
> > +	int tag, ret = 0;
> >   
> > -	if (is_mcq_enabled(hba)) {
> > -		struct ufshcd_lrb *lrbp;
> > -		int tag;
> > +	blk_mq_tagset_busy_iter(&hba->host->tag_set, ufshcd_abort_one,
> > &ret);
> > +	if (ret)
> > +		goto out;
> >   
> > -		for (tag = 0; tag < hba->nutrs; tag++) {
> > -			lrbp = &hba->lrb[tag];
> > -			if (!ufshcd_cmd_inflight(lrbp->cmd))
> > -				continue;
> > -			ret = ufshcd_try_to_abort_task(hba, tag);
> > -			dev_err(hba->dev, "Aborting tag %d / CDB %#02x
> > %s\n", tag,
> > -				hba->lrb[tag].cmd ? hba->lrb[tag].cmd-
> > > cmnd[0] : -1,
> > -				ret ? "failed" : "succeeded");
> > -			if (ret)
> > -				goto out;
> > -		}
> > -	} else {
> > -		/* Clear pending transfer requests */
> > -		for_each_set_bit(tag, &hba->outstanding_reqs, hba-
> > > nutrs) {
> > -			ret = ufshcd_try_to_abort_task(hba, tag);
> > -			dev_err(hba->dev, "Aborting tag %d / CDB %#02x
> > %s\n", tag,
> > -				hba->lrb[tag].cmd ? hba->lrb[tag].cmd-
> > > cmnd[0] : -1,
> > -				ret ? "failed" : "succeeded");
> > -			if (ret)
> > -				goto out;
> > -		}
> > -	}
> >   	/* Clear pending task management requests */
> >   	for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs) {
> >   		ret = ufshcd_clear_tm_cmd(hba, tag);
>
> Hi Bart,
>
> Previous ufshcd_try_to_abort_task retrun fail will break tag iterate
> and return true to tell caller need reset.
> But this patch only return last tag ufshcd_try_to_abort_task return
> value, if some tag abort fail and last tag abort success, will not
> retrun true to tell caller need reset, am I right?

Hi Peter,

blk_mq_tagset_busy_iter() keeps iterating as long as the callback function
that has been passed as the second argument returns true. ufshcd_abort_one()
returns false if ufshcd_try_to_abort_task() fails.

The behavior that a host reset is triggered if ufshcd_try_to_abort_task()
fails has been preserved.

Bart.