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

[Niklas Cassel <cassel@kernel.org>]

On Wed, Jul 01, 2026 at 10:58:03PM +0000, sashiko-bot@kernel.org wrote:
> 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?

Well, from the spec:
"A value of 0000_0000h indicates that the maximum number of 512-byte blocks of LBA
Range Entries is not specified."

So will it be safe to send more than one 512-byte block?

We could cap it to one 512-byte block for devices that report a zero in
the MAX PAGES PER DSM COMMAND field, that is probably the safest, as we
have no idea to know the actual limit if the device does not report it.

(Note that the previous commit that claimed to add support for 4Kn
devices completely ignored the MAX PAGES PER DSM COMMAND field.)


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

Sure, can cast one of them to u64 to make sure that 64-bit arithmetic will be
used.


Kind regards,
Niklas