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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ata_dsm_trim_pages() does not enforce a minimum value for logical sector sizes, causing max_pages to evaluate to 0 for non-compliant devices. This permanently disables TRIM for devices that previously worked.
--

commit cdc4dda768e65a6957c51f439c9f87c5c94b90d7
Author: Niklas Cassel <[email protected]>

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

This commit modifies DSM TRIM to emit a payload sized up to the maximum 
number of pages the device can accept, instead of just a single 512-byte 
page. It also builds the descriptor directly into the WRITE SAME data-out 
buffer via an atomic sg_miter mapping.

Reviewed-by: Hannes Reinecke <[email protected]>
Signed-off-by: Niklas Cassel <[email protected]>

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 429b03a080717..82342351b1c99 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c

[ ... ]

> @@ -2198,6 +2198,39 @@ 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 the device does not specify a limit, assume only a single page. */
> +	if (!max_pages)
> +		max_pages = 1;
> +
> +	pages = min(pages, max_pages);
> +
> +	return pages;
> +}

[Severity: Medium]
What happens here if a buggy device reports an invalid logical sector size 
less than 512 (e.g., 0)?

If sector_size is 0, pages will evaluate to 0. Since min(pages, max_pages)
is used, ata_dsm_trim_pages() will return 0. 

While the SCSI disk layer typically corrects invalid 0-byte sector sizes 
dynamically to keep the device functional, this raw uncorrected IDENTIFY 
data calculation bypasses that correction.

[ ... ]

> @@ -3523,7 +3568,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 > max_pages * ATA_MAX_TRIM_RNUM * (u64)U16_MAX) {
>  		fp = 2;
>  		goto invalid_fld;
>  	}

[Severity: Medium]
Does this unintentionally break TRIM operations for non-compliant devices?

If ata_dsm_trim_pages() returns 0 as discussed above, max_pages is 0 here.
Because the Linux block layer prevents 0-length discard bios, n_block will
strictly be > 0. 

This makes the condition n_block > 0 unconditionally true for any valid 
TRIM request. The request will safely be rejected via goto invalid_fld, but
this permanently fails TRIM operations for a device that might have previously
worked.

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