Re: [PATCH v5 34/40] target: use 16-bits defined sense codes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Incomplete refactoring leaves hard-coded ASC and ASCQ values, violating the commit claim.

Pre-existing issues:
- [High] Pending Unit Attention incorrectly returns SAM_STAT_BUSY instead of CHECK_CONDITION when UA interlock control is enabled.
--

commit ccd814e94e89b6a0920259ca5ec40d06ac0cee69
Author: Damien Le Moal <dlemoal@kernel.org>

target: use 16-bits defined sense codes

Refactor the target core code to use the 16-bits sense codes for building
sense and replace all hard-coded additional sense codes and additional
sense code qualifiers with the enum values defined in
include/scsi/scsi_sense.h. This helps with code clarity as the sense codes
being processed are easier to test and self-documented.

[Severity: Medium]
The commit message states this patch will "replace all hard-coded additional
sense codes and additional sense code qualifiers with the enum values". 
Is this refactoring complete?

Looking at the unmodified code in target_core_transport.c,
sense_detail_table still has hard-coded values like 0x25. Similarly,
core_scsi3_ua_allocate() in target_core_ua.c still checks for
ua_p->ua_asc == 0x29. Should these remaining hard-coded values be updated
to use the 16-bit enum values to fully align with the commit description?

> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
[ ... ]
> @@ -3566,21 +3567,19 @@ static void translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
>  
>  	key = sd->key;
>  	if (reason == TCM_CHECK_CONDITION_UNIT_ATTENTION) {
> -		if (!core_scsi3_ua_for_check_condition(cmd, &key, &asc,
> -						       &ascq)) {
> +		if (!core_scsi3_ua_for_check_condition(cmd, &key, &code)) {
>  			cmd->scsi_status = SAM_STAT_BUSY;
>  			return;
>  		}

[Severity: High]
This is a pre-existing issue, but does returning SAM_STAT_BUSY here cause an
infinite I/O retry loop if the target is configured with UA interlock?

In core_scsi3_ua_for_check_condition() below, when dev_ua_intlck_clear
is false, the loop breaks early and leaves the head variable set to 1.
This causes core_scsi3_ua_for_check_condition() to return false, which in
turn leads translate_sense_reason() to return SAM_STAT_BUSY instead of
properly formatting the CHECK_CONDITION with the pending Unit Attention
sense data.

Should this be reporting the CHECK_CONDITION instead of spinning on BUSY?

> diff --git a/drivers/target/target_core_ua.c b/drivers/target/target_core_ua.c
> --- a/drivers/target/target_core_ua.c
> +++ b/drivers/target/target_core_ua.c
[ ... ]
> @@ -232,8 +230,7 @@ bool core_scsi3_ua_for_check_condition(struct se_cmd *cmd, u8 *key, u8 *asc,
>  		 * clearing it.
>  		 */
>  		if (!dev_ua_intlck_clear) {
> -			*asc = ua->ua_asc;
> -			*ascq = ua->ua_ascq;
> +			*code = scsi_sense_code(ua->ua_asc, ua->ua_ascq);
>  			break;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907024343.1046387-1-dlemoal@kernel.org?part=34