Re: [PATCH v5 2/2] ufs: mcq: poll OCS after sq cleanup for device management commands

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ufs: mcq: race between OCS poll in ufshcd_mcq_sq_cleanup() and CQE completion
--

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

ufs: mcq: poll OCS after sq cleanup for device management commands

This commit extends ufshcd_mcq_sq_cleanup() to poll the UTRD header.ocs
field for reserved (device management) requests after SQ_CUS is set, so
that cleanup waits for the OCS update before returning.

> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
> index 25e392f8ede2b..8857775586311 100644
> --- a/drivers/ufs/core/ufs-mcq.c
> +++ b/drivers/ufs/core/ufs-mcq.c
> @@ -613,6 +613,24 @@ int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag)
>  			 __func__, id,
>  			 FIELD_GET(SQ_ICU_ERR_CODE_MASK, readl(reg)));
>  
> +	/*
> +	 * For device management commands, there is no order guaranteed between
> +	 * OCS update and CUS update, which is why we need to wait here. SCSI
> +	 * commands are excluded because the OCS in their UTRD is not updated
> +	 * from the CQE.
> +	 */
> +	if (!err && blk_mq_is_reserved_rq(rq)) {
> +		enum utp_ocs ocs;
> +
> +		err = read_poll_timeout(READ_ONCE, ocs,
> +					ocs != OCS_INVALID_COMMAND_STATUS, 20,
> +					MCQ_POLL_US, false,
> +					lrbp->utr_descriptor_ptr->header.ocs);
> +		if (err)
> +			dev_err(hba->dev, "%s: OCS not updated. hwq=%d, tag=%d err=%d\n",
> +				__func__, id, task_tag, err);
> +	}

[Severity: High]
Can polling lrbp->utr_descriptor_ptr->header.ocs without synchronizing
with hwq->cq_lock race with ufshcd_compl_one_cqe()?

In MCQ mode, the UFS controller writes the CQE to hwq->cqe_base_addr
rather than updating lrbp->utr_descriptor_ptr->header.ocs in host memory.
The UTRD header.ocs field is updated in software by the CQ completion
handler under hwq->cq_lock:

drivers/ufs/core/ufshcd.c:ufshcd_compl_one_cqe() {
	...
	if (blk_mq_is_reserved_rq(scsi_cmd_to_rq(cmd))) {
		if (cqe) {
			ocs = cqe->overall_status & MASK_OCS;
			lrbp->utr_descriptor_ptr->header.ocs = ocs;
		} else {
			ocs = lrbp->utr_descriptor_ptr->header.ocs;
		}
		ufshcd_add_query_upiu_trace(
			hba,
			ocs == OCS_SUCCESS ? UFS_QUERY_COMP : UFS_QUERY_ERR,
			(struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
		cmd->result = 0;
	}
	/* Do not touch lrbp after scsi_done() has been called. */
	scsi_done(cmd);
}

Because ufshcd_compl_one_cqe() writes lrbp->utr_descriptor_ptr->header.ocs
before tracing lrbp->ucd_rsp_ptr, setting cmd->result = 0, and calling
scsi_done(cmd), ufshcd_mcq_sq_cleanup() can observe the updated ocs and
return 0 to scmd_eh_abort_handler() while ufshcd_compl_one_cqe() is still
running on another CPU.

When scmd_eh_abort_handler() receives SUCCESS from the abort path:

drivers/scsi/scsi_error.c:scmd_eh_abort_handler() {
	...
	set_host_byte(scmd, DID_TIME_OUT);
	...
	scsi_finish_command(scmd);
}

Two races can then occur against the in-progress ufshcd_compl_one_cqe():

1. If ufshcd_compl_one_cqe() executes cmd->result = 0 after
   scmd_eh_abort_handler() calls set_host_byte(scmd, DID_TIME_OUT), the
   timeout status in cmd->result is overwritten with 0 before
   scsi_io_completion() inspects it, causing blk_execute_rq() in
   ufshcd_issue_dev_cmd() to see BLK_STS_OK instead of a timeout.

2. If scsi_finish_command() completes the request first,
   ufshcd_exec_dev_cmd() releases the reserved command via
   ufshcd_put_dev_mgmt_cmd() and a retry loop such as
   ufshcd_query_flag_retry() can immediately reallocate the single
   reserved request and submit the retry via blk_execute_rq(), which
   clears SCMD_STATE_COMPLETE in scsi_prepare_cmd(). When the delayed
   ufshcd_compl_one_cqe() from the first command then reaches
   scsi_done(cmd), it can prematurely complete the newly submitted retry
   command while it is still in flight in hardware.

Should ufshcd_mcq_sq_cleanup() synchronize with hwq->cq_lock (or poll CQEs
under hwq->cq_lock) so that ufshcd_compl_one_cqe() has completely finished
before cleanup returns?

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