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

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

Well, if a device reports an invalid logical sector size, non-functional
TRIM is probably the least of the problems :D :D

Anyway, we could replace the min() with a min_not_zero() to silence this
warning.


> 
> 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.

Well, as mentioned above, this can only happen for broken devices that does
not report a valid logical sector size.

Anyway, modifying ata_dsm_trim_pages() to use min_not_zero() should ensure
that the concern raised in this review comment can no longer happen.


Kind regards,
Niklas