[PATCH v1] ufs: mcq: fix dev cmd abort path and add OCS poll after sq cleanup

[PATCH v1] ufs: mcq: fix dev cmd abort path and add OCS poll after sq cleanup

[Kiwoong Kim]

In MCQ mode, ufshcd_mcq_abort() falls through to ufshcd_try_to_abort_task()
for all timed-out commands. However, device management commands (query, NOP)
use reserved requests and are not SCSI I/O commands. The ABORT TASK task
management function is designed for SCSI I/O UPIUs and is not applicable to
device management UPIUs.

Fix this by checking ufshcd_is_scsi_cmd() in ufshcd_try_to_abort_task(). When
the request is a reserved (device management) command, skip the TM path and
call ufshcd_clear_cmd() directly instead.

Additionally, extend ufshcd_mcq_sq_cleanup() to poll the OCS field in the
UTRD after a successful SQ_CUS poll. UFSHCI does not specify the order between
the OCS update and the SQ_CUS update, so OCS may still be stale when cleanup
returns. Without this poll, a late response to a timed-out query can arrive
while a retry is already in flight, leaving the host with two query contexts
and raising IS.UTPES.

The poll waits until OCS is no longer OCS_INVALID_COMMAND_STATUS or
MCQ_POLL_US elapses. Any non-zero OCS after the poll is reported as
-ETIMEDOUT.

--
Here is the timeline of an actual case for this that happened with kernel 6.18.
1. The host sent a query but the device didn't respond with it for a long time.

[ 7384.156142] [4:  kworker/u40:5:10671] exynos-ufs 38300000.ufs: ufshcd_mcq_sq_cleanup, hwq 0: cleanup return code (RTC) 0
[ 7384.156154] [4:  kworker/u40:5:10671] exynos-ufs 38300000.ufs: ufshcd_query_flag: Sending flag query for idn 14 failed, err = -11

2. So after the query was timed out, the driver did SQ cleanup.
Then the host updated CQ update and raised IS.CQES,
Unfortunately, complete() was called the driver's ISR afterward.

3. The driver started to retried the query. In the meantime, it set OCS to 0xF.
Then the driver didn't wait for completion due to the previous complete() call,
and just checked OCS.

[ 7384.156163] [4:  kworker/u40:5:10671] exynos-ufs 38300000.ufs: ufshcd_query_flag: Sending flag query for idn 14 failed, err = 15

5. The driver retried the query again, but the device didn't return anything,
so the query was also timed-out.

[ 7385.660108] [3:  kworker/u40:5:10671] exynos-ufs 38300000.ufs: ufshcd_query_flag: Sending flag query for idn 14 failed, err = -11

6. A few seconds later, the device returned the response of the 1st query,
but since the host was holding two query contexts and couldn't handle it,
it raised IS.UTPES.
---
 drivers/ufs/core/ufs-mcq.c | 24 +++++++++++++++++++++++-
 drivers/ufs/core/ufshcd.c  |  4 ++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 8106d55f4041..091ae67d1dde 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -603,12 +603,34 @@ int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag)
 	if (err)
 		dev_err(hba->dev, "%s: failed. hwq=%d, tag=%d err=%d\n",
 			__func__, id, task_tag, err);
-	else
+	else {
+		enum utp_ocs ocs;
+		ktime_t start = ktime_get();
+
 		dev_info(hba->dev,
 			 "%s, hwq %d: cleanup return code (RTC) %ld\n",
 			 __func__, id,
 			 FIELD_GET(SQ_ICU_ERR_CODE_MASK, readl(reg)));
 
+		/*
+		 * There is no order guaranteed between OCS update and CUS update,
+		 * which is why we need to wait here.
+		 */
+		do {
+			unsigned long flags;
+
+			spin_lock_irqsave(&hwq->cq_lock, flags);
+			ocs = READ_ONCE(lrbp->utr_descriptor_ptr->header.ocs);
+			spin_unlock_irqrestore(&hwq->cq_lock, flags);
+			if (ocs != OCS_INVALID_COMMAND_STATUS)
+				break;
+			usleep_range(20, 50);
+		} while (ktime_us_delta(ktime_get(), start) < MCQ_POLL_US);
+
+		if (ocs)
+			err = -ETIMEDOUT;
+	}
+
 	if (ufshcd_mcq_sq_start(hba, hwq))
 		err = -ETIMEDOUT;
 
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index bcc2512bf34b..7fc80926eeec 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8046,6 +8046,9 @@ int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag)
 
 	lrbp = scsi_cmd_priv(cmd);
 
+	if (!ufshcd_is_scsi_cmd(cmd))
+		goto clear_cmd;
+
 	for (poll_cnt = 100; poll_cnt; poll_cnt--) {
 		err = ufshcd_issue_tm_cmd(hba, lrbp->lun, tag, UFS_QUERY_TASK,
 					  &resp);
@@ -8091,6 +8094,7 @@ int ufshcd_try_to_abort_task(struct ufs_hba *hba, int tag)
 		return err;
 	}
 
+clear_cmd:
 	err = ufshcd_clear_cmd(hba, tag);
 	if (err)
 		dev_err(hba->dev, "%s: Failed clearing cmd at tag %d, err %d\n",
-- 
2.43.0

Re: [PATCH v1] ufs: mcq: fix dev cmd abort path and add OCS poll after sq cleanup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ufs: mcq: polling unupdated header.ocs in ufshcd_mcq_sq_cleanup() breaks SCSI cleanup
- [High] ufs: mcq: ufshcd_mcq_sq_cleanup() treats OCS_ABORTED as -ETIMEDOUT and wedges EH
--

commit 1f3e43d50c19634050bcd7e46bc9f38e9ee41667
Author: Kiwoong Kim <kwmad.kim@samsung.com>

ufs: mcq: fix dev cmd abort path and add OCS poll after sq cleanup

This commit skips task management commands in ufshcd_try_to_abort_task()
when aborting reserved device management commands and goes directly to
ufshcd_clear_cmd(). It also adds a polling loop in ufshcd_mcq_sq_cleanup()
waiting for header.ocs to change from OCS_INVALID_COMMAND_STATUS after
SQ_CUS is set, returning -ETIMEDOUT if ocs is non-zero.

> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
> index 25e392f8ede2b..1a38aecd126b3 100644
> --- a/drivers/ufs/core/ufs-mcq.c
> +++ b/drivers/ufs/core/ufs-mcq.c
> @@ -607,12 +607,34 @@ int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag)
>  	if (err)
>  		dev_err(hba->dev, "%s: failed. hwq=%d, tag=%d err=%d\n",
>  			__func__, id, task_tag, err);
> -	else
> +	else {
> +		enum utp_ocs ocs;
> +		ktime_t start = ktime_get();
> +
>  		dev_info(hba->dev,
>  			 "%s, hwq %d: cleanup return code (RTC) %ld\n",
>  			 __func__, id,
>  			 FIELD_GET(SQ_ICU_ERR_CODE_MASK, readl(reg)));
>  
> +		/*
> +		 * There is no order guaranteed between OCS update and CUS update,
> +		 * which is why we need to wait here.
> +		 */
> +		do {
> +			unsigned long flags;
> +
> +			spin_lock_irqsave(&hwq->cq_lock, flags);
> +			ocs = READ_ONCE(lrbp->utr_descriptor_ptr->header.ocs);
> +			spin_unlock_irqrestore(&hwq->cq_lock, flags);
> +			if (ocs != OCS_INVALID_COMMAND_STATUS)
> +				break;
> +			usleep_range(20, 50);
> +		} while (ktime_us_delta(ktime_get(), start) < MCQ_POLL_US);

[Severity: High]
Does this loop ever see lrbp->utr_descriptor_ptr->header.ocs change when
ufshcd_mcq_sq_cleanup() is called for a regular SCSI I/O command?

In MCQ mode, hardware writes the completion status into cqe->overall_status
in the completion queue entry rather than lrbp->utr_descriptor_ptr->header.ocs.
In software, ufshcd_compl_one_cqe() only copies cqe->overall_status into
lrbp->utr_descriptor_ptr->header.ocs when !ufshcd_is_scsi_cmd(cmd):

drivers/ufs/core/ufshcd.c:ufshcd_compl_one_cqe() {
	if (ufshcd_is_scsi_cmd(cmd)) {
		if (unlikely(ufshcd_should_inform_monitor(hba, cmd)))
			ufshcd_update_monitor(hba, cmd);
		ufshcd_add_command_trace(hba, cmd, UFS_CMD_COMP);
		cmd->result = ufshcd_transfer_rsp_status(hba, cmd, cqe);
		ufshcd_release_scsi_cmd(hba, cmd);
	} else {
		if (cqe) {
			ocs = cqe->overall_status & MASK_OCS;
			lrbp->utr_descriptor_ptr->header.ocs = ocs;
		} else {
			ocs = lrbp->utr_descriptor_ptr->header.ocs;
		}
		...
	}
}

In addition, callers such as ufshcd_clear_lu_cmds() and ufshcd_abort_all()
only call ufshcd_mcq_poll_cqe_lock() after ufshcd_clear_cmd() ->
ufshcd_mcq_sq_cleanup() returns 0, and poll queues or controllers with
UFSHCD_QUIRK_MCQ_BROKEN_INTR do not process CQEs via interrupts while this
loop is waiting.

For any SCSI command cleaned up via ufshcd_mcq_abort() ->
ufshcd_try_to_abort_task() -> ufshcd_clear_cmd() -> ufshcd_mcq_sq_cleanup()
or ufshcd_clear_lu_cmds() -> ufshcd_clear_cmd() -> ufshcd_mcq_sq_cleanup(),
won't header.ocs remain OCS_INVALID_COMMAND_STATUS for the full 500 ms
(MCQ_POLL_US) while holding hwq->sq_mutex with the SQ stopped, and then
always fail with -ETIMEDOUT?

> +
> +		if (ocs)
> +			err = -ETIMEDOUT;
> +	}

[Severity: High]
When SQ cleanup (SQ_ICU) aborts an outstanding transfer request, the host
controller posts a CQE with overall_status set to OCS_ABORTED (0x06) rather
than OCS_SUCCESS (0x00). Once ufshcd_compl_one_cqe() writes OCS_ABORTED to
lrbp->utr_descriptor_ptr->header.ocs, the poll loop exits because ocs is no
longer OCS_INVALID_COMMAND_STATUS, and this check then sets err = -ETIMEDOUT
because ocs is non-zero.

When a reserved device management command times out in ufshcd_exec_dev_cmd()
-> ufshcd_issue_dev_cmd() -> blk_execute_rq(), the timeout path runs:

scsi_timeout()
  -> scmd_eh_abort_handler()
    -> ufshcd_abort()
      -> ufshcd_mcq_abort()
        -> ufshcd_try_to_abort_task()
          -> ufshcd_clear_cmd()
            -> ufshcd_mcq_sq_cleanup()

In ufshcd_mcq_abort(), a non-zero return from ufshcd_try_to_abort_task() is
treated as an abort failure:

drivers/ufs/core/ufs-mcq.c:ufshcd_mcq_abort() {
	...
	err = ufshcd_try_to_abort_task(hba, tag);
	if (err) {
		dev_err(hba->dev, "%s: device abort failed %d\n", __func__, err);
		lrbp->req_abort_skip = true;
		return FAILED;
	}
	return SUCCESS;
}

When ufshcd_mcq_abort() returns FAILED to scmd_eh_abort_handler(), the
command is not completed for the caller to retry; instead,
scmd_eh_abort_handler() passes the reserved command to scsi_eh_scmd_add(),
which triggers WARN_ON_ONCE(!test_bit(SCMD_STATE_INFLIGHT, &scmd->state))
and escalates to SCSI error recovery while the caller is still blocked inside
blk_execute_rq() holding hba->dev_cmd.lock and hba->clk_scaling_lock via
ufshcd_dev_man_lock().

Should ufshcd_mcq_sq_cleanup() only return -ETIMEDOUT when ocs is still
OCS_INVALID_COMMAND_STATUS after the poll (rather than on any non-zero ocs
such as OCS_ABORTED), so that a successful SQ cleanup returns 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030083311.602909-1-kwmad.kim@samsung.com?part=1