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

On Thu, Jul 02, 2026 at 06:53:20AM +0000, [email protected] wrote:
> 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?

No, AFAICT, libata should not expose WRITE SAME (16) support, regardless if
TRIM is supported or not.


In ata_scsi_write_same_xlat(), we have:

/*
 * We only allow sending this command through the block layer,
 * as it modifies the DATA OUT buffer, which would corrupt user
 * memory for SG_IO commands.
 */
if (unlikely(blk_rq_is_passthrough(scsi_cmd_to_rq(scmd))))
	goto invalid_opcode;

This means that we will always reject a WRITE SAME (16) passthrough command.

Additionally, in ata_scsi_sdev_config(), we have sdev->no_write_same = 1;


Thus the only way we can reach ata_scsi_write_same_xlat() is via the block
layer.

Block layer commands that can reach ata_scsi_write_same_xlat():

REQ_OP_WRITE_ZEROES:
In sd.c, sd_setup_write_zeroes_cmnd() we have:
if (sdp->no_write_same) return BLK_STS_TARGET;

So REQ_OP_WRITE_ZEROES will not reach ata_scsi_write_same_xlat().


REQ_OP_DISCARD:
In sd.c, REQ_OP_DISCARD builds a WRITE_SAME_16 with the UNMAP bit and never
consults no_write_same. provisioning_mode == SD_LBP_WS16 is chosen by
sd_discard_mode() purely from the LBP/thin-provisioning bits
(lbpme, lbpws - both of which libata advertises for a TRIM device), again
independent of no_write_same.

So a libata TRIM device ends up with no_write_same = 1 and
provisioning_mode = SD_LBP_WS16 simultaneously, and every discard is a
WRITE SAME(16) UNMAP that lands in ata_scsi_write_same_xlat() -> DSM TRIM.

That's exactly the intended arrangement: no_write_same turns off the
"write a pattern" use while leaving discard working.

This seems correct as ATA has no way to splat an arbitrary pattern across
many blocks in one command.


Kind regards,
Niklas