Re: [PATCH v1 3/6] ata: libata-scsi: improve service action support in ata_scsi_report_supported_opcodes()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ata_scsi_supported_cmd_has_sa() and ata_supported_cmds incorrectly handle service actions of value 0, breaking REPORT SUPPORTED OPERATION CODES.
--

commit 976ae28e21b5ac80b95af84b459ff3561417568d
Author: Damien Le Moal <[email protected]>

ata: libata-scsi: improve service action support in ata_scsi_report_supported_opcodes()

This commit updates ata_scsi_report_supported_opcodes() to take service actions
into consideration. It adds ata_scsi_supported_cmd_has_sa() to check if an
opcode has service actions and uses both opcode and service action to look up
supported commands.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 380ca3886a777..e432e521ad318 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
[ ... ]
> @@ -3641,32 +3641,46 @@ static const struct ata_scsi_cmd ata_supported_cmds[] = {
>  	{ ZBC_OUT,		16,	ZO_RESET_WRITE_POINTER },
>  };
>  
> -static const struct ata_scsi_cmd *ata_scsi_get_supported_cmd(u8 op)
> +static const struct ata_scsi_cmd *ata_scsi_get_supported_cmd(u8 op, u16 sa)
>  {
>  	const struct ata_scsi_cmd *cmd;
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(ata_supported_cmds); i++) {
>  		cmd = &ata_supported_cmds[i];
> -		if (cmd->op == op)
> +		if (cmd->op == op && cmd->sa == sa)

[Severity: Medium]
Does this strict service action matching cause issues for VARIABLE_LENGTH_CMD?

Looking at the ata_supported_cmds array, VARIABLE_LENGTH_CMD uses service
actions (e.g., ATA_32 = 0x1FF0) but is mapped with sa = 0. Will this cause the
matching here to fail for valid service actions under reporting option 3?

>  			return cmd;
>  	}
>  
>  	return NULL;
>  }
>  
> +static bool ata_scsi_supported_cmd_has_sa(u8 op)
> +{
> +	const struct ata_scsi_cmd *cmd;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ata_supported_cmds); i++) {
> +		cmd = &ata_supported_cmds[i];
> +		if (cmd->op == op)
> +			return cmd->sa != 0;

[Severity: Medium]
Will this incorrectly handle service actions with a value of 0?

Because 0 is a valid service action (e.g., ZBC_IN uses ZI_REPORT_ZONES = 0),
this condition will incorrectly cause ata_scsi_supported_cmd_has_sa() to
return false for ZBC_IN.

> +	}
> +
> +	return false;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3