diff mbox series

[v1,1/2] ufs: core: complete scsi command after release

Message ID 20240823100707.6699-2-peter.wang@mediatek.com (mailing list archive)
State Superseded
Headers show
Series ufs: core: fix err handler mcq abort defect | expand

Commit Message

Peter Wang (王信友) Aug. 23, 2024, 10:07 a.m. UTC 
From: Peter Wang <peter.wang@mediatek.com>

When the error handler successfully aborts a MCQ request,
it only releases the command and does not notify the SCSI layer.
This may cause another abort after 30 seconds timeout.
This patch notifies the SCSI layer to requeue the request.

Below is error log
[   14.183804][   T74] ufshcd-mtk 112b0000.ufshci: ufshcd_err_handler started; HBA state eh_non_fatal; powered 1; shutting down 0; saved_err = 4; saved_uic_err = 64; force_reset = 0
[   14.256164][   T74] ufshcd-mtk 112b0000.ufshci: ufshcd_try_to_abort_task: cmd pending in the device. tag = 19
[   14.257511][   T74] ufshcd-mtk 112b0000.ufshci: Aborting tag 19 / CDB 0x35 succeeded
[   34.287949][    T8] ufshcd-mtk 112b0000.ufshci: ufshcd_abort: Device abort task at tag 19
[   34.290514][    T8] ufshcd-mtk 112b0000.ufshci: ufshcd_mcq_abort: skip abort. cmd at tag 19 already completed.

Fixes:93e6c0e19d5b ("scsi: ufs: core: Clear cmd if abort succeeds in MCQ mode")
Cc: <stable@vger.kernel.org> 6.6.x

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

Comments

Bart Van Assche Aug. 23, 2024, 4:20 p.m. UTC | #1 
On 8/23/24 3:07 AM, peter.wang@mediatek.com wrote:
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 0b3d0c8e0dda..4bcd4e5b62bd 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -6482,8 +6482,12 @@ static bool ufshcd_abort_one(struct request *rq, void *priv)
>   		if (!hwq)
>   			return 0;
>   		spin_lock_irqsave(&hwq->cq_lock, flags);
> -		if (ufshcd_cmd_inflight(lrbp->cmd))
> +		if (ufshcd_cmd_inflight(lrbp->cmd)) {
> +			struct scsi_cmnd *cmd = lrbp->cmd;
> +			set_host_byte(cmd, DID_REQUEUE);
>   			ufshcd_release_scsi_cmd(hba, lrbp);
> +			scsi_done(cmd);
> +		}
>   		spin_unlock_irqrestore(&hwq->cq_lock, flags);
>   	}

Hmm ... isn't the ufshcd_complete_requests() call in ufshcd_abort_all()
expected to complete these commands? Can the above change lead to
scsi_done() being called twice, something that is not allowed?

Bart.
Peter Wang (王信友) Aug. 26, 2024, 3:41 a.m. UTC | #2 
On Fri, 2024-08-23 at 09:20 -0700, Bart Van Assche wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On 8/23/24 3:07 AM, peter.wang@mediatek.com wrote:
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index 0b3d0c8e0dda..4bcd4e5b62bd 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -6482,8 +6482,12 @@ static bool ufshcd_abort_one(struct request
> *rq, void *priv)
> >   if (!hwq)
> >   return 0;
> >   spin_lock_irqsave(&hwq->cq_lock, flags);
> > -if (ufshcd_cmd_inflight(lrbp->cmd))
> > +if (ufshcd_cmd_inflight(lrbp->cmd)) {
> > +struct scsi_cmnd *cmd = lrbp->cmd;
> > +set_host_byte(cmd, DID_REQUEUE);
> >   ufshcd_release_scsi_cmd(hba, lrbp);
> > +scsi_done(cmd);
> > +}
> >   spin_unlock_irqrestore(&hwq->cq_lock, flags);
> >   }
> 
> Hmm ... isn't the ufshcd_complete_requests() call in
> ufshcd_abort_all()
> expected to complete these commands? Can the above change lead to
> scsi_done() being called twice, something that is not allowed?
> 
> Bart.
> 

Hi Bart,

ufshcd_complete_requests call in ufshcd_abort_all() with 
force_compl = false in mcq mode, which only check cq is empty or not.
For already abort cmd, no response will come back and no cq 
slot insert by hardware, which we need release this cmd and scsi done,
same as force_compl = true does.

Thanks
Peter
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0b3d0c8e0dda..4bcd4e5b62bd 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -6482,8 +6482,12 @@  static bool ufshcd_abort_one(struct request *rq, void *priv)
 		if (!hwq)
 			return 0;
 		spin_lock_irqsave(&hwq->cq_lock, flags);
-		if (ufshcd_cmd_inflight(lrbp->cmd))
+		if (ufshcd_cmd_inflight(lrbp->cmd)) {
+			struct scsi_cmnd *cmd = lrbp->cmd;
+			set_host_byte(cmd, DID_REQUEUE);
 			ufshcd_release_scsi_cmd(hba, lrbp);
+			scsi_done(cmd);
+		}
 		spin_unlock_irqrestore(&hwq->cq_lock, flags);
 	}