Re: [PATCH v2 2/5] ata: libata-scsi: refactor ata_scsi_report_supported_opcodes()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] WRITE_SAME_16 is unconditionally reported as supported via REPORT SUPPORTED OPERATION CODES, even for devices that do not support TRIM.
--

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

ata: libata-scsi: refactor ata_scsi_report_supported_opcodes()

This commit refactors ata_scsi_report_supported_opcodes() to use an array
of supported commands for easier modifications and extensions. It also adds
handling for WRITE_SAME_16 and VARIABLE_LENGTH_CMD to the list of commands
which libata-scsi translates.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 8811bfc373697..4f0ae44f59dd2 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
[ ... ]
> +static bool ata_scsi_cmd_is_supported(struct ata_device *dev, u8 op,
> +				      struct ata_scsi_cmd_support *sup)
> +{
> +	const struct ata_scsi_cmd *cmd;
> +
> +	/* First, see if we support the command. */
> +	cmd = ata_scsi_get_supported_cmd(op);
> +	if (!cmd)
> +		return false;
> +
> +	/* Now refine the support report depending on the device features. */
> +	memset(sup, 0, sizeof(*sup));
> +	switch (op) {
> +	case READ_16:
[ ... ]
> +	case ZBC_IN:
> +	case ZBC_OUT:
> +		return ata_dev_is_zoned(dev);
> +	case SECURITY_PROTOCOL_IN:
> +	case SECURITY_PROTOCOL_OUT:
> +		return dev->flags & ATA_DFLAG_TRUSTED;
> +	default:
> +		break;
> +	}
> +
> +	return true;
> +}

[Severity: Medium]
Should WRITE_SAME_16 be refined here to check for TRIM support?

The commit message mentions adding WRITE_SAME_16 to the supported array,
so it falls through to the default case in this switch statement and
unconditionally returns true.

However, looking at ata_scsi_write_same_xlat(), the command is unconditionally
rejected if the device lacks TRIM capabilities:

ata_scsi_write_same_xlat()
    ...
    if (!unmap || (dev->quirks & ATA_QUIRK_NOTRIM) ||
        !ata_id_has_trim(dev->id)) {
        fp = 1;
        bp = 3;
        goto invalid_fld;
    }

Does this cause the report supported operation codes query to incorrectly
inform the SCSI layer that WRITE_SAME_16 is supported on ATA devices that
do not actually support TRIM?

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