Re: [PATCH v4 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] The primary target core sense table still uses hard-coded hex values despite the patch's stated goals.

Pre-existing issues:
- [High] Incorrect handling of Unit Attention interlock control causes an infinite loop of SAM_STAT_BUSY responses.
--

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

target: use 16-bits defined sense codes

This commit refactors the target core code to use 16-bit sense codes.
It replaces hard-coded additional sense codes and qualifiers with the
enum values defined in include/scsi/scsi_sense.h to improve clarity.

> 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]
Does this patch miss updating the sense_detail_table in
drivers/target/target_core_transport.c?

Looking at the source, sense_detail_table still contains hard-coded hex
values for asc and ascq (such as 0x25 for LOGICAL UNIT NOT SUPPORTED),
which seems to contradict the stated intent to replace all of them.

> 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;
>  		}

[ ... ]

> 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;
>  		}

[Severity: High]
This is a pre-existing issue, but does this early break cause an infinite
loop of busy responses when emulate_ua_intlck_ctrl is not zero?

When dev_ua_intlck_clear is false, the loop in
core_scsi3_ua_for_check_condition() breaks before setting head = 0.
This causes the function to return false since head is initialized to 1.

In translate_sense_reason(), receiving false causes it to set the status
to SAM_STAT_BUSY instead of properly reporting the unit attention.

The initiator will retry, hit the exact same uncleared unit attention,
and receive a busy response indefinitely.

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