Re: [PATCH 2/2] ata: libata-scsi: scale DSM TRIM payload by MAX PAGES PER DSM COMMAND

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ATA DATA SET MANAGEMENT (TRIM) commands will fail on 4Kn devices that do not report a maximum DSM page limit, because the code defaults to sending up to 8 pages instead of the specification-mandated safe minimum of 1.
- [Medium] 32-bit integer overflow in the DSM TRIM maximum blocks calculation artificially clamps the block layer discard limit, causing performance regressions.
--

commit a387741dfad7fa23a80a62ebd7e1d57854a6d80e
Author: Niklas Cassel <cassel@kernel.org>

ata: libata-scsi: scale DSM TRIM payload by MAX PAGES PER DSM COMMAND

This commit updates the DSM TRIM command translation to scale the payload
size based on the MAX PAGES PER DSM COMMAND reported by the device. It
sizes the TRIM descriptor as a whole number of 512-byte pages, capped
by both the device limit and the logical sector size.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2198,6 +2198,36 @@ static unsigned int ata_scsiop_inq_89(struct ata_device *dev,

[ ... ]

> +static unsigned int ata_dsm_trim_pages(struct ata_device *dev)
> +{
> +	unsigned int sector_size = ata_id_logical_sector_size(dev->id);
> +	unsigned int max_pages = ata_id_dsm_max_pages(dev->id);
> +	unsigned int pages = sector_size / ATA_SECT_SIZE;
> +
> +	if (max_pages)
> +		pages = min(pages, max_pages);
> +
> +	return pages;
> +}

[Severity: High]
For 4Kn devices that report 0 for the maximum DSM limit (word 105), this
defaults to sending up to 8 pages (4096 / 512). Does this exceed the ATA
specification's guaranteed safe minimum of 1 page, potentially causing the
device to reject the command with an I/O error?

> @@ -2237,7 +2267,8 @@ static unsigned int ata_scsiop_inq_b0(struct ata_device *dev,
>  	 * with the unmap bit set.
>  	 */
>  	if (ata_id_has_trim(dev->id)) {
> -		u64 max_blocks = 65535 * ATA_MAX_TRIM_RNUM;
> +		unsigned int max_pages = ata_dsm_trim_pages(dev);
> +		u64 max_blocks = U16_MAX * max_pages * ATA_MAX_TRIM_RNUM;

[Severity: Medium]
When calculating max_blocks in ata_scsiop_inq_b0(), the multiplication involves
a 32-bit unsigned int (U16_MAX * max_pages * 64). If max_pages is greater
than 1024, can this expression overflow a 32-bit integer before being
zero-extended and assigned to the 64-bit max_blocks? This might artificially
truncate the block layer discard limit for devices supporting large TRIMs.

[ ... ]

> @@ -3523,7 +3565,7 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
>  		goto invalid_fld;
>  	}
>  	/* If the request is too large the cmd is invalid */
> -	if (n_block > 0xffff * trmax) {
> +	if (n_block > U16_MAX * max_pages * ATA_MAX_TRIM_RNUM) {

[Severity: Medium]
Similarly, in ata_scsi_write_same_xlat(), could this 32-bit arithmetic 
truncate the intended validation limit if the device reports a large 
maximum DSM page limit, inadvertently masking the mismatch from the truncated
limit above?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701224638.1835123-4-cassel@kernel.org?part=2