[PATCH v2 0/2] ufs: core: fix err handler mcq abort defect

[PATCH v2 0/2] ufs: core: fix err handler mcq abort defect

[peter.wang]

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

This series fixes ufshcd_err_handler abort defect in mcq mode.

V2:
 - Change patch description typo.

Peter Wang (2):
  ufs: core: complete scsi command after release
  ufs: core: force reset after mcq abort all

 drivers/ufs/core/ufshcd.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
2.45.2

[PATCH v2 1/2] ufs: core: complete scsi command after release

[peter.wang]

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(-)

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);
 	}
 
-- 
2.45.2

Re: [PATCH v2 1/2] ufs: core: complete scsi command after release

[Bart Van Assche]

On 8/25/24 8:45 PM, peter.wang@mediatek.com wrote:
> 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.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH v2 2/2] ufs: core: force reset after mcq abort all

[peter.wang]

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

In mcq mode gerneal case, cq (head/tail) pointer is same as
sq pointer (head/tail) if the hwq is empty.

But if command send to device and abort it,
no response return and cq point will less than sq.
In this case will have unpredictable error.
This patch force reset for this case.

Below is error log
[   34.976612][    C3] ufshcd-mtk 112b0000.ufshci: OCS error from controller = 3 for tag 19

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

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 4bcd4e5b62bd..d9ef8f0279da 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -6519,6 +6519,8 @@ static bool ufshcd_abort_all(struct ufs_hba *hba)
 	/* Complete the requests that are cleared by s/w */
 	ufshcd_complete_requests(hba, false);
 
+	if (is_mcq_enabled(hba))
+		return true;
 	return ret != 0;
 }
 
-- 
2.45.2

Re: [PATCH v2 2/2] ufs: core: force reset after mcq abort all

[Bart Van Assche]

On 8/25/24 8:45 PM, peter.wang@mediatek.com wrote:
> From: Peter Wang <peter.wang@mediatek.com>
> 
> In mcq mode gerneal case, cq (head/tail) pointer is same as

Please capitalize "MCQ" and please fix the spelling of "general".

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 4bcd4e5b62bd..d9ef8f0279da 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -6519,6 +6519,8 @@ static bool ufshcd_abort_all(struct ufs_hba *hba)
>   	/* Complete the requests that are cleared by s/w */
>   	ufshcd_complete_requests(hba, false);
>   
> +	if (is_mcq_enabled(hba))
> +		return true;
>   	return ret != 0;
>   }

Please add a comment above the new if-test that explains why that code
is present otherwise it will be hard to understand why that statement
has been introduced.

Thanks,

Bart.

Re: [PATCH v2 2/2] ufs: core: force reset after mcq abort all

[Peter Wang (王信友)]

On Tue, 2024-08-27 at 11:55 -0400, 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/25/24 8:45 PM, peter.wang@mediatek.com wrote:
> > From: Peter Wang <peter.wang@mediatek.com>
> > 
> > In mcq mode gerneal case, cq (head/tail) pointer is same as
> 
> Please capitalize "MCQ" and please fix the spelling of "general".
> 
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index 4bcd4e5b62bd..d9ef8f0279da 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -6519,6 +6519,8 @@ static bool ufshcd_abort_all(struct ufs_hba
> *hba)
> >   /* Complete the requests that are cleared by s/w */
> >   ufshcd_complete_requests(hba, false);
> >   
> > +if (is_mcq_enabled(hba))
> > +return true;
> >   return ret != 0;
> >   }
> 
> Please add a comment above the new if-test that explains why that
> code
> is present otherwise it will be hard to understand why that statement
> has been introduced.
> 
> Thanks,
> 
> Bart.

Hi Bart,

Sorry, I need to abandon this patch series because I've discovered 
the reason why the aborted command hasn't returned.
And this patch 93e6c0e19d5b ("scsi: ufs: core: Clear cmd if abort
succeeds in MCQ mode")
should reverted.

I will submit another patch to fix this issue.

Thanks.
Peter

>