Re: [PATCH] scsi: ufs: core: Clear power-on UNIT ATTENTION on BIWIN devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] scsi: ufs: unhandled broken OCS quirk in ufshcd_clear_unit_attention()
- [Medium] scsi: ufs: core: ufshcd_issue_devman_upiu_cmd() hardcodes lun 0 in lrbp
--

commit 02e1f4aa450245801c780ec99fe1072a3f8e77c3
Author: Alexey Charkov <alchark@flipper.net>

scsi: ufs: core: Clear power-on UNIT ATTENTION on BIWIN devices

This patch adds a device quirk for BIWIN UFS devices (models starting with
BWU2A05) to clear pending power-on unit attention conditions across all
logical units after a host reset and restore by issuing a TEST_UNIT_READY
command via the device management slot.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ee643830a57a2..d52275a813fd3 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c

[ ... ]

> @@ -8265,6 +8268,55 @@ static void ufshcd_process_probe_result(struct ufs_hba *hba,
>  			  hba->curr_dev_pwr_mode, hba->uic_link_state);
>  }
>  
> +/**
> + * ufshcd_clear_unit_attention - clear the power-on UNIT ATTENTION of each LU
> + * @hba: per adapter instance
> + *
> + * Send a TEST UNIT READY to each logical unit to clear the power-on UNIT
> + * ATTENTION left by a device reset, for devices that keep reporting it to
> + * WRITE commands instead. Receiving the UNIT ATTENTION is what clears it, so
> + * only transport failures matter.
> + *
> + * Use the device management slot rather than the logical units' request
> + * queues: those are blocked during error handling, and during a runtime
> + * resume of the device W-LU its runtime suspended consumers cannot take
> + * requests until it has resumed.
> + */
> +static void ufshcd_clear_unit_attention(struct ufs_hba *hba)
> +{
> +	struct utp_upiu_req req_upiu, rsp_upiu;
> +	struct scsi_device *sdev;
> +	int err;
> +
> +	shost_for_each_device(sdev, hba->host) {
> +		if (scsi_is_wlun(sdev->lun))
> +			continue;
> +
> +		req_upiu = (struct utp_upiu_req){
> +			.header = {
> +				.transaction_code = UPIU_TRANSACTION_COMMAND,
> +				.lun = ufshcd_scsi_to_upiu_lun(sdev->lun),
> +				.command_set_type = UPIU_COMMAND_SET_TYPE_SCSI,
> +			},
> +			.sc.cdb[0] = TEST_UNIT_READY,
> +		};
> +
> +		ufshcd_dev_man_lock(hba);
> +		err = ufshcd_issue_devman_upiu_cmd(hba, &req_upiu, &rsp_upiu,
> +						   NULL, NULL,
> +						   DEV_CMD_TYPE_SCSI,
> +						   UPIU_QUERY_OPCODE_NOP);

[Severity: Medium]
When ufshcd_clear_unit_attention() sends a DEV_CMD_TYPE_SCSI command to a
non-zero LUN, ufshcd_issue_devman_upiu_cmd() still passes a hardcoded LUN of
0 to ufshcd_setup_dev_cmd():

drivers/ufs/core/ufshcd.c:ufshcd_issue_devman_upiu_cmd() {
	...
	tag = scsi_cmd_to_rq(cmd)->tag;

	ufshcd_setup_dev_cmd(hba, cmd, cmd_type, 0, tag);
	...
}

This leaves lrbp->lun set to 0 even though req_upiu.header.lun targets a
non-zero LUN.

If the command times out during error recovery (for example via
ufshcd_err_handler() -> ufshcd_reset_and_restore() ->
ufshcd_host_reset_and_restore() -> ufshcd_clear_unit_attention() when
hba->pm_op_in_progress is false), scmd_eh_abort_handler() invokes
ufshcd_abort() -> ufshcd_mcq_abort() -> ufshcd_try_to_abort_task(), which
uses lrbp->lun to issue UFS_QUERY_TASK and UFS_ABORT_TASK:

drivers/ufs/core/ufshcd.c:ufshcd_try_to_abort_task() {
	...
	for (poll_cnt = 100; poll_cnt; poll_cnt--) {
		err = ufshcd_issue_tm_cmd(hba, lrbp->lun, tag, UFS_QUERY_TASK,
					  &resp);
	...
}

And ufshcd_clear_cmd() -> ufshcd_mcq_sq_cleanup() also uses lrbp->lun to
build the nexus written to REG_SQCTI:

drivers/ufs/core/ufs-mcq.c:ufshcd_mcq_sq_cleanup() {
	...
	/* SQCTI = EXT_IID, IID, LUN, Task Tag */
	nexus = lrbp->lun << 8 | task_tag;
	opr_sqd_base = mcq_opr_base(hba, OPR_SQD, id);
	writel(nexus, opr_sqd_base + REG_SQCTI);
	...
}

Should ufshcd_issue_devman_upiu_cmd() pass req_upiu->header.lun instead of 0
to ufshcd_setup_dev_cmd() so that task abort and MCQ queue cleanup target the
right LUN?

> +		ufshcd_dev_man_unlock(hba);
> +		if (err) {
> +			dev_warn(hba->dev,
> +				 "TEST UNIT READY to LUN %llu failed: %d\n",
> +				 sdev->lun, err);
> +			scsi_device_put(sdev);
> +			break;
> +		}

[Severity: Medium]
What happens here on host controllers with UFSHCD_QUIRK_BROKEN_OCS_FATAL_ERROR
(such as ufs-exynos)?

When a logical unit has a pending unit attention condition, the device
responds to TEST_UNIT_READY with a Response UPIU having header.response set
to TARGET_FAILURE and header.status set to SAM_STAT_CHECK_CONDITION. On
controllers with UFSHCD_QUIRK_BROKEN_OCS_FATAL_ERROR, hardware reports
OCS_FATAL_ERROR in the UTRD/CQE whenever the Response UPIU has a non-zero
response or status field.

For normal SCSI commands, ufshcd_transfer_rsp_status() overrides ocs back to
OCS_SUCCESS:

drivers/ufs/core/ufshcd.c:ufshcd_transfer_rsp_status() {
	...
	if (hba->quirks & UFSHCD_QUIRK_BROKEN_OCS_FATAL_ERROR) {
		if (lrbp->ucd_rsp_ptr->header.response ||
		    lrbp->ucd_rsp_ptr->header.status)
			ocs = OCS_SUCCESS;
	}
	...
}

However, ufshcd_issue_devman_upiu_cmd() uses a reserved device management
command (!ufshcd_is_scsi_cmd(cmd)). In ufshcd_compl_one_cqe(), the reserved
command path bypasses ufshcd_transfer_rsp_status() and leaves
lrbp->utr_descriptor_ptr->header.ocs as OCS_FATAL_ERROR, which
ufshcd_issue_dev_cmd() then returns directly:

drivers/ufs/core/ufshcd.c:ufshcd_issue_dev_cmd() {
	...
	rq->timeout = timeout;
	sts = blk_execute_rq(rq, true);
	if (sts != BLK_STS_OK)
		return blk_status_to_errno(sts);
	return lrbp->utr_descriptor_ptr->header.ocs;
}

Wouldn't this cause ufshcd_clear_unit_attention() to see OCS_FATAL_ERROR on
the first LUN that reports unit attention, log a warning, and break out of
the loop before clearing the remaining LUNs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028-biwin-tua-quirk-v1-1-767a12c9b9cc@flipper.net?part=1