fix abort defect

ufshcd_abort_all forcibly aborts all on-going commands and the host controller will automatically fill in the OCS field of the corresponding response with OCS_ABORTED based on different working modes.

MCQ mode: aborts a command using SQ cleanup, The host controller will post a Completion Queue entry with OCS = ABORTED.

SDB mode: aborts a command using UTRLCLR. Task Management response which means a Transfer Request was aborted.

drivers/ufs/core/ufshcd.c +8 -0

@@ -7561,6 +7551,21 @@ int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag)
  		goto out;
  	}
 
+	/*
+	 * When the host software receives a "FUNCTION COMPLETE", set flag
+	 * to requeue command after receive response with OCS_ABORTED
+	 * SDB mode: UTRLCLR Task Management response which means a Transfer
+	 *           Request was aborted.
+	 * MCQ mode: Host will post to CQ with OCS_ABORTED after SQ cleanup
+	 *
+	 * This flag is set because error handler ufshcd_abort_all forcibly

External email : Please do not click links or open attachments until you have verified the sender or the content.

Quoted 2 lines · On 9/11/24 2:56 AM, [email protected] wrote:

ufshcd_abort_all forcibly aborts all on-going commands and the host controller will automatically fill in the OCS field of the

corresponding

Quoted 7 lines

response with OCS_ABORTED based on different working modes.

MCQ mode: aborts a command using SQ cleanup, The host controller will post a Completion Queue entry with OCS = ABORTED.

SDB mode: aborts a command using UTRLCLR. Task Management response which means a Transfer Request was aborted.

I think this is incorrect. The UFSHCI specification does not require a
host controller to set the OCS field if a SCSI command is aborted by the
ABORT TMF nor if its resources are freed by writing into the UTRLCLR register.

Review ufshcd_transfer_rsp_status(struct ufs_hba

Quoted 1 line

(quoted diff) +0 -0

@@ -5404,7 +5405,10 @@ ufshcd_transfer_rsp_status(struct ufs_hba

*hba, struct ufshcd_lrb *lrbp,

drivers/ufs/core/ufshcd.c ufshcd_transfer_rsp_status(struct ufs_hba *hba, struct ufshcd_lrb *lrbp, · line 5412

Quoted 9 lines

drivers/ufs/core/ufshcd.c +4 -1

  }
  break;
  case OCS_ABORTED:
-result |= DID_ABORT << 16;
+if (lrbp->abort_initiated_by_eh)
+result |= DID_REQUEUE << 16;
+else
+result |= DID_ABORT << 16;
  break;

Is the above change necessary? ufshcd_abort_one() aborts commands by submitting an ABORT TMF. Hence, ufshcd_transfer_rsp_status() won't be called if aborting the command succeeds.

Review int ufshcd_try_to_abort_task(struct ufs_hba

Quoted 1 line

(quoted diff) +0 -0

@@ -7561,6 +7551,21 @@ int ufshcd_try_to_abort_task(struct ufs_hba

*hba, int tag)

drivers/ufs/core/ufshcd.c int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag)

Quoted 7 lines

drivers/ufs/core/ufshcd.c +4 -0

  goto out;
  }
 
+/*
+ * When the host software receives a "FUNCTION COMPLETE", set flag
+ * to requeue command after receive response with OCS_ABORTED
+ * SDB mode: UTRLCLR Task Management response which means a

Transfer

Review int ufshcd_try_to_abort_task(struct ufs_hba · line 7559

Quoted 2 lines

(quoted diff) +2 -0

+ *           Request was aborted.
+ * MCQ mode: Host will post to CQ with OCS_ABORTED after SQ

cleanup

Review int ufshcd_try_to_abort_task(struct ufs_hba · line 7561

Quoted 2 lines

(quoted diff) +2 -0

+ *
+ * This flag is set because error handler ufshcd_abort_all

forcibly

Review int ufshcd_try_to_abort_task(struct ufs_hba · line 7563

Quoted 2 lines

(quoted diff) +2 -0

+ * aborts all commands, and the host controller will automatically
+ * fill in the OCS field of the corresponding response with

OCS_ABORTED.

Review int ufshcd_try_to_abort_task(struct ufs_hba · line 7564

Quoted 1 line

(quoted diff) +1 -0

+ * Therefore, upon receiving this response, it needs to be

requeued.

drivers/ufs/core/ufshcd.c int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag) · line 7571

Quoted 7 lines

drivers/ufs/core/ufshcd.c +4 -0

+ */
+if (!err && ufshcd_eh_in_progress(hba))
+lrbp->abort_initiated_by_eh = true;
+
  err = ufshcd_clear_cmd(hba, tag);
  if (err)
  dev_err(hba->dev, "%s: Failed clearing cmd at tag %d, err %d\n",

The above change will cause lrbp->abort_initiated_by_eh to be set not only if the UFS error handler decides to abort a command but also if the
SCSI core decides to abort a command. I think this is wrong.

Is this patch 2/2 necessary or is patch 1/2 perhaps sufficient?

Thanks,

Bart.

On Wed, 2024-09-11 at 12:37 -0700, Bart Van Assche wrote:

So at this time, the device will not have a corresponding response coming back. The host controller will automatically fill in the response for the corresponding command based on the results of SQ cleanup (MCQ) or UTRLCLR (DBR, mediatek), with the COS content being ABORTED by interrupt.

Quoted 4 lines

The above change will cause lrbp->abort_initiated_by_eh to be set not only if the UFS error handler decides to abort a command but also if the
SCSI core decides to abort a command. I think this is wrong.

Sorry, I might have missed something, but I didn't see scsi abort (ufshcd_abort) calling ufshcd_set_eh_in_progress. So, during a SCSI abort, ufshcd_eh_in_progress(hba) should return false and not set this flag, right?

Additionally, SCSI abort (ufshcd_abort) will have different return values for MCQ mode and DBR mode when the device does not respond with a response.
MCQ mode will receivce OCS_ABORTED (nullify)

	case OCS_ABORTED:
		result |= DID_ABORT << 16;
		break;

But DBR mode, OCS won't change, it is 0x0F

	case OCS_INVALID_COMMAND_STATUS:
		result |= DID_REQUEUE << 16;
		break;

In this case, should we also return DID_ABORT for DBR mode?

External email : Please do not click links or open attachments until you have verified the sender or the content.

Quoted 1 line · Peter Wang (王信友)

On Wed, 2024-09-11 at 12:37 -0700, Bart Van Assche wrote:

[ ... ]

Quoted 5 lines

So at this time, the device will not have a corresponding response coming back. The host controller will automatically fill in the response for the corresponding command based on the results of SQ cleanup (MCQ) or UTRLCLR (DBR, mediatek), with the COS content being ABORTED by interrupt.

I don't think so. In SDB mode, writing into the UTRLCLR register does NOT cause the ABORTED status to be written into the OCS field. Even if
there would be UFSHCI controllers that do this, the UFSHCI specification
does not require this behavior and hence the UFS driver should not assume this behavior.

Quoted 1 line

Quoted 1 line

The above change will cause lrbp->abort_initiated_by_eh to be set

not

Quoted 1 line

Quoted 1 line

only if the UFS error handler decides to abort a command but also

if

Quoted 7 lines

Quoted 2 lines

the
SCSI core decides to abort a command. I think this is wrong.

Sorry, I might have missed something, but I didn't see scsi abort (ufshcd_abort) calling ufshcd_set_eh_in_progress. So, during a SCSI abort, ufshcd_eh_in_progress(hba) should return false and not set this flag, right?

I think you are right so please ignore my comment above.

Quoted 7 lines

Additionally, SCSI abort (ufshcd_abort) will have different return values for MCQ mode and DBR mode when the device does not respond with a response.
MCQ mode will receivce OCS_ABORTED (nullify)
case OCS_ABORTED:
result |= DID_ABORT << 16;
break;

No. ufshcd_abort() submits an abort TMF and the OCS status is not modified if the abort TMF succeeds.

Quoted 5 lines

But DBR mode, OCS won't change, it is 0x0F
case OCS_INVALID_COMMAND_STATUS:
result |= DID_REQUEUE << 16;
break;
In this case, should we also return DID_ABORT for DBR mode?

The above code comes from ufshcd_transfer_rsp_status(), isn't it? ufshcd_transfer_rsp_status() should not be called if the SCSI core aborts a SCSI command (ufshcd_abort()). It is not allowed to call scsi_done() from a SCSI abort handler like ufshcd_abort(). SCSI abort handlers must return SUCCESS, FAILED or FAST_IO_FAIL and let the SCSI core decide whether to complete or whether to resubmit the SCSI command.

Thanks,

Bart.