[PATCH 0/2] ata: DSM TRIM fix and improvements

[PATCH 0/2] ata: DSM TRIM fix and improvements

[Niklas Cassel]

Hello all,

This fixes DSM TRIM for 4Kn devices, which has ironically been broken
since commit ef2d7392c4ec ("libata: SCT Write Same / DSM Trim"), which
claimed to correct the DSM TRIM handling for 4Kn devices.


The first commit is a small fix that can be backported, which limits
the DSM TRIM to what we currently report in the Block Limits VPD page.
We currently report a single 512 byte page of entries, i.e. 64 Entries,
where each entry has a maximum of U16_MAX logical blocks.

The second commit:
-Improves the support by reading the MAX PAGES PER DSM COMMAND limit and
 populates the Block Limits VPD page accordingly.
-Modifies ata_scsi_write_same_xlat() to properly support up to this amount
 of pages.
-Removed the intermediate copying to ata_scsi_rbuf, writes to the scsi
 buffer directly.

Practically, sd still caps n_block at SD_MAX_WS16_BLOCKS
(~3 pages actually used on 4Kn), so while removing the limit on
ATA_SCSI_RBUF_SIZE (2048) - i.e. 4 pages), we will still be limited
by SD_MAX_WS16_BLOCKS. The main improvement is not longer taking the
ata_scsi_rbuf_lock and doing and extra copy from ata_scsi_rbuf.


Niklas Cassel (2):
  ata: libata-scsi: fix DSM TRIM for sector sizes larger than 2048 bytes
  ata: libata-scsi: scale DSM TRIM payload by MAX PAGES PER DSM COMMAND

 drivers/ata/libata-scsi.c | 134 ++++++++++++++++++++++++++------------
 include/linux/ata.h       |  13 ++++
 2 files changed, 105 insertions(+), 42 deletions(-)

-- 
2.54.0

[PATCH 1/2] ata: libata-scsi: fix DSM TRIM for sector sizes larger than 2048 bytes

[Niklas Cassel]

ata_scsi_write_same_xlat() translates a SCSI WRITE SAME command with the
UNMAP bit set into an ATA DATA SET MANAGEMENT TRIM command.  The TRIM
descriptor is built by ata_format_dsm_trim_descr() into the 2048-byte
ata_scsi_rbuf staging buffer, and the number of bytes copied is compared
against the logical sector size by the caller:

	size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block);
	if (size != len)		/* len == sdp->sector_size */
		goto invalid_param_len;

ata_format_dsm_trim_descr() clamps the copy length to ATA_SCSI_RBUF_SIZE
(2048).  On a device whose logical sector size exceeds that (e.g. a 4Kn
device, where sector_size == 4096) the function can never return more than
2048, while the caller expects it to return sector_size.  The comparison
therefore always fails, so every TRIM is rejected with "Parameter list
length error" and WARN_ON() splats on each attempt.  TRIM / discard is
thus completely broken on such devices.

The descriptor was incorrectly sized from the logical sector size.  A DSM
TRIM payload is a list of 512-byte pages, each holding up to
ATA_MAX_TRIM_RNUM (64) LBA Range Entries, and is independent of the logical
sector size.  The Block Limits VPD page already advertises a single such
page as the maximum WRITE SAME length (65535 * ATA_MAX_TRIM_RNUM logical
blocks), so the block layer never sends a request that needs more than one
page.

Emit exactly one 512-byte page, independent of the logical sector size,
and transfer only that page (COUNT == 1).  For a 512-byte-sector device
this is unchanged; devices with larger logical sectors now work instead of
failing every TRIM.

Fixes: ef2d7392c4ec ("libata: SCT Write Same / DSM Trim")
Cc: stable@vger.kernel.org
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-scsi.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index d54ec1631e9a..429b03a08071 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3447,17 +3447,13 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 static size_t ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 trmax,
 					u64 sector, u32 count)
 {
-	struct scsi_device *sdp = cmd->device;
-	size_t len = sdp->sector_size;
+	size_t len = ATA_SECT_SIZE;
 	size_t r;
 	__le64 *buf;
 	u32 i = 0;
 	unsigned long flags;
 
-	WARN_ON(len > ATA_SCSI_RBUF_SIZE);
-
-	if (len > ATA_SCSI_RBUF_SIZE)
-		len = ATA_SCSI_RBUF_SIZE;
+	BUILD_BUG_ON(ATA_SECT_SIZE > ATA_SCSI_RBUF_SIZE);
 
 	spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
 	buf = ((void *)ata_scsi_rbuf);
@@ -3492,13 +3488,11 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 {
 	struct ata_taskfile *tf = &qc->tf;
 	struct scsi_cmnd *scmd = qc->scsicmd;
-	struct scsi_device *sdp = scmd->device;
-	size_t len = sdp->sector_size;
 	struct ata_device *dev = qc->dev;
 	const u8 *cdb = scmd->cmnd;
 	u64 block;
 	u32 n_block;
-	const u32 trmax = len >> 3;
+	const u32 trmax = ATA_MAX_TRIM_RNUM;
 	u32 size;
 	u16 fp;
 	u8 bp = 0xff;
@@ -3542,13 +3536,13 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 		goto invalid_param_len;
 
 	/*
-	 * size must match sector size in bytes
-	 * For DATA SET MANAGEMENT TRIM in ACS-2 nsect (aka count)
-	 * is defined as number of 512 byte blocks to be transferred.
+	 * The TRIM descriptor is a single 512-byte page, which is the maximum
+	 * WRITE SAME length advertised in the Block Limits VPD page. For DATA
+	 * SET MANAGEMENT TRIM the COUNT field (aka nsect) is the number of
+	 * 512-byte blocks to be transferred.
 	 */
-
 	size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block);
-	if (size != len)
+	if (size != ATA_SECT_SIZE)
 		goto invalid_param_len;
 
 	if (ata_ncq_enabled(dev) && ata_fpdma_dsm_supported(dev)) {
@@ -3574,6 +3568,12 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 		     ATA_TFLAG_WRITE;
 
 	ata_qc_set_pc_nbytes(qc);
+	/*
+	 * The DSM TRIM payload is a single 512-byte page, which may be smaller
+	 * than the WRITE SAME data-out buffer (one logical block); only
+	 * transfer that page so the length matches the COUNT field.
+	 */
+	qc->nbytes = size;
 
 	return 0;
 
-- 
2.54.0

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

[Niklas Cassel]

DSM TRIM currently always emits a single 512-byte page of LBA Range Entries
(ATA_MAX_TRIM_RNUM == 64 ranges), regardless of how many pages the device
can accept in one DATA SET MANAGEMENT command.

The maximum is reported by MAX PAGES PER DSM COMMAND (IDENTIFY DEVICE word
105).  Honour it: size the TRIM descriptor as a whole number of 512-byte
pages, bounded by that limit and by the logical sector size (the WRITE SAME
data-out buffer is a single logical block).  Build and transfer only as
many pages as the request needs, and set the DSM COUNT field, qc->nbytes
and the maximum WRITE SAME length in the Block Limits VPD page accordingly.

Build the descriptor straight into the WRITE SAME data-out buffer using an
atomic sg_miter mapping, instead of staging it in the shared ata_scsi_rbuf
and copying it out.  This removes the global ata_scsi_rbuf_lock and a
memcpy from the TRIM path.

While commit 9379e6b8e0f9 ("libata: Safely overwrite attached page in WRITE
SAME xlat") replaced direct access to the data-out buffer with an
intermediate step that writes the entries in the ata_scsi_rbuf buffer, this
solution writes to the data-out buffer using sg_miter, which maps each
segment with kmap_atomic (SG_MITER_ATOMIC), so it's highmem- and
multi-segment-safe, and it's usable from the non-sleeping
command-submission path (unlike the page_address() access that
ata_scsi_rbuf originally replaced).

A 512-byte-sector device still uses a single page, so its behaviour is
unchanged.

Add ata_id_dsm_max_pages() to read IDENTIFY DEVICE word 105.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/libata-scsi.c | 132 ++++++++++++++++++++++++++------------
 include/linux/ata.h       |  13 ++++
 2 files changed, 104 insertions(+), 41 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 429b03a08071..c0525b86755d 100644
--- 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,
 	return get_unaligned_be16(&rbuf[2]) + 4;
 }
 
+/**
+ * ata_dsm_trim_pages - maximum DSM TRIM payload for a device, in 512-byte pages
+ * @dev: ATA device the DATA SET MANAGEMENT TRIM command will be sent to
+ *
+ * A DATA SET MANAGEMENT TRIM payload is a list of 512-byte pages, each holding
+ * up to ATA_MAX_TRIM_RNUM (64) LBA Range Entries; the format is page-based and
+ * unrelated to the logical sector size.
+ *
+ * The logical sector size still bounds it, though: the descriptor is written
+ * directly into the WRITE SAME data-out buffer, which sd sizes to a single
+ * logical block, so it can hold at most sector_size / 512 pages.
+ *
+ * Return: the maximum number of 512-byte pages a single translated WRITE SAME
+ * command may send to @dev (never less than one), that is the smaller of:
+ *   - MAX PAGES PER DSM COMMAND (IDENTIFY DEVICE word 105), when the device
+ *     reports a non-zero limit; and
+ *   - the logical sector size expressed in 512-byte pages (see above).
+ */
+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;
+}
+
 /**
  *	ata_scsiop_inq_b0 - Simulate INQUIRY VPD page B0, Block Limits
  *	@dev: Target device.
@@ -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;
 
 		if (dev->quirks & ATA_QUIRK_MAX_TRIM_128M)
 			max_blocks = 128 << (20 - SECTOR_SHIFT);
@@ -3426,14 +3457,14 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 /**
  * ata_format_dsm_trim_descr() - SATL Write Same to DSM Trim
  * @cmd: SCSI command being translated
- * @trmax: Maximum number of entries that will fit in sector_size bytes.
+ * @size: DSM TRIM payload size in bytes (a multiple of 512)
  * @sector: Starting sector
  * @count: Total Range of request in logical sectors
  *
  * Rewrite the WRITE SAME descriptor to be a DSM TRIM little-endian formatted
  * descriptor.
  *
- * Upto 64 entries of the format:
+ * The payload is a list of @size / 8 entries of the format:
  *   63:48 Range Length
  *   47:0  LBA
  *
@@ -3442,35 +3473,45 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
  *
  * NOTE: this is the same format as ADD LBA(S) TO NV CACHE PINNED SET
  *
- * Return: Number of bytes copied into sglist.
+ * The descriptor is written straight into the WRITE SAME data-out buffer;
+ * ata_dsm_trim_pages() guarantees @size does not exceed that buffer (one
+ * logical block). An atomic sg_miter mapping is used so this works from the
+ * command submission path and, unlike page_address(), copes with a high
+ * memory payload.
+ *
+ * Return: Number of bytes written into the data-out buffer.
  */
-static size_t ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, u32 trmax,
+static size_t ata_format_dsm_trim_descr(struct scsi_cmnd *cmd, size_t size,
 					u64 sector, u32 count)
 {
-	size_t len = ATA_SECT_SIZE;
-	size_t r;
-	__le64 *buf;
-	u32 i = 0;
-	unsigned long flags;
+	struct sg_mapping_iter miter;
+	size_t offset = 0;
+
+	sg_miter_start(&miter, scsi_sglist(cmd), scsi_sg_count(cmd),
+		       SG_MITER_TO_SG | SG_MITER_ATOMIC);
+	while (offset < size && sg_miter_next(&miter)) {
+		__le64 *buf = miter.addr;
+		size_t chunk = min_t(size_t, miter.length, size - offset);
+		unsigned int n = chunk / sizeof(__le64);
+		unsigned int i;
 
-	BUILD_BUG_ON(ATA_SECT_SIZE > ATA_SCSI_RBUF_SIZE);
+		for (i = 0; i < n; i++) {
+			u64 entry = 0;
 
-	spin_lock_irqsave(&ata_scsi_rbuf_lock, flags);
-	buf = ((void *)ata_scsi_rbuf);
-	memset(buf, 0, len);
-	while (i < trmax) {
-		u64 entry = sector |
-			((u64)(count > 0xffff ? 0xffff : count) << 48);
-		buf[i++] = __cpu_to_le64(entry);
-		if (count <= 0xffff)
-			break;
-		count -= 0xffff;
-		sector += 0xffff;
+			if (count) {
+				u32 rlen = min_t(u32, count, 0xffff);
+
+				entry = sector | ((u64)rlen << 48);
+				sector += rlen;
+				count -= rlen;
+			}
+			buf[i] = cpu_to_le64(entry);
+		}
+		offset += n * sizeof(__le64);
 	}
-	r = sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd), buf, len);
-	spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
+	sg_miter_stop(&miter);
 
-	return r;
+	return offset;
 }
 
 /**
@@ -3490,10 +3531,11 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 	struct scsi_cmnd *scmd = qc->scsicmd;
 	struct ata_device *dev = qc->dev;
 	const u8 *cdb = scmd->cmnd;
+	unsigned int max_pages = ata_dsm_trim_pages(dev);
+	unsigned int n_pages;
+	size_t size;
 	u64 block;
 	u32 n_block;
-	const u32 trmax = ATA_MAX_TRIM_RNUM;
-	u32 size;
 	u16 fp;
 	u8 bp = 0xff;
 	u8 unmap = cdb[1] & 0x8;
@@ -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) {
 		fp = 2;
 		goto invalid_fld;
 	}
@@ -3536,31 +3578,39 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 		goto invalid_param_len;
 
 	/*
-	 * The TRIM descriptor is a single 512-byte page, which is the maximum
-	 * WRITE SAME length advertised in the Block Limits VPD page. For DATA
-	 * SET MANAGEMENT TRIM the COUNT field (aka nsect) is the number of
-	 * 512-byte blocks to be transferred.
+	 * The DATA SET MANAGEMENT TRIM payload is a whole number of 512-byte
+	 * pages (each holding up to ATA_MAX_TRIM_RNUM LBA Range Entries),
+	 * independent of the logical sector size. Only use as many pages as
+	 * are needed to describe the request, capped at max_pages.
 	 */
-	size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block);
-	if (size != ATA_SECT_SIZE)
+	n_pages = DIV_ROUND_UP(DIV_ROUND_UP(n_block, U16_MAX),
+			       ATA_MAX_TRIM_RNUM);
+	n_pages = clamp(n_pages, 1U, max_pages);
+	size = (size_t)n_pages * ATA_SECT_SIZE;
+
+	if (ata_format_dsm_trim_descr(scmd, size, block, n_block) != size)
 		goto invalid_param_len;
 
+	/*
+	 * For DATA SET MANAGEMENT TRIM the COUNT field (aka nsect) is the
+	 * number of 512-byte pages to be transferred.
+	 */
 	if (ata_ncq_enabled(dev) && ata_fpdma_dsm_supported(dev)) {
 		/* Newer devices support queued TRIM commands */
 		tf->protocol = ATA_PROT_NCQ;
 		tf->command = ATA_CMD_FPDMA_SEND;
 		tf->hob_nsect = ATA_SUBCMD_FPDMA_SEND_DSM & 0x1f;
 		tf->nsect = qc->hw_tag << 3;
-		tf->hob_feature = (size / 512) >> 8;
-		tf->feature = size / 512;
+		tf->hob_feature = n_pages >> 8;
+		tf->feature = n_pages;
 
 		tf->auxiliary = 1;
 	} else {
 		tf->protocol = ATA_PROT_DMA;
 		tf->hob_feature = 0;
 		tf->feature = ATA_DSM_TRIM;
-		tf->hob_nsect = (size / 512) >> 8;
-		tf->nsect = size / 512;
+		tf->hob_nsect = n_pages >> 8;
+		tf->nsect = n_pages;
 		tf->command = ATA_CMD_DSM;
 	}
 
@@ -3569,9 +3619,9 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 
 	ata_qc_set_pc_nbytes(qc);
 	/*
-	 * The DSM TRIM payload is a single 512-byte page, which may be smaller
-	 * than the WRITE SAME data-out buffer (one logical block); only
-	 * transfer that page so the length matches the COUNT field.
+	 * The DSM TRIM payload (size) may be smaller than the WRITE SAME
+	 * data-out buffer (one logical block); only transfer the pages that
+	 * were actually built so the transfer length matches the COUNT field.
 	 */
 	qc->nbytes = size;
 
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 8fd48bcb2a46..d6361032a8af 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -75,6 +75,7 @@ enum {
 	ATA_ID_HW_CONFIG	= 93,
 	ATA_ID_SPG		= 98,
 	ATA_ID_LBA_CAPACITY_2	= 100,
+	ATA_ID_MAX_PAGES_PER_DSM	= 105,
 	ATA_ID_SECTOR_SIZE	= 106,
 	ATA_ID_WWN		= 108,
 	ATA_ID_LOGICAL_SECTOR_SIZE	= 117,	/* and 118 */
@@ -928,6 +929,18 @@ static inline bool ata_id_has_trim(const u16 *id)
 	return false;
 }
 
+static inline u16 ata_id_dsm_max_pages(const u16 *id)
+{
+	/*
+	 * IDENTIFY DEVICE word 105: MAX PAGES PER DSM COMMAND. Maximum number
+	 * of 512-byte pages of LBA Range Entries the device accepts in a
+	 * single DATA SET MANAGEMENT command. Zero means the device does not
+	 * report a limit. The field is reserved unless TRIM is supported, so
+	 * callers must gate on ata_id_has_trim().
+	 */
+	return id[ATA_ID_MAX_PAGES_PER_DSM];
+}
+
 static inline bool ata_id_has_zero_after_trim(const u16 *id)
 {
 	/* DSM supported, deterministic read, and read zero after trim set */
-- 
2.54.0

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

[sashiko-bot]

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

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

[Niklas Cassel]

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

Re: [PATCH 1/2] ata: libata-scsi: fix DSM TRIM for sector sizes larger than 2048 bytes

[Hannes Reinecke]

On 7/2/26 12:46 AM, Niklas Cassel wrote:
> ata_scsi_write_same_xlat() translates a SCSI WRITE SAME command with the
> UNMAP bit set into an ATA DATA SET MANAGEMENT TRIM command.  The TRIM
> descriptor is built by ata_format_dsm_trim_descr() into the 2048-byte
> ata_scsi_rbuf staging buffer, and the number of bytes copied is compared
> against the logical sector size by the caller:
> 
> 	size = ata_format_dsm_trim_descr(scmd, trmax, block, n_block);
> 	if (size != len)		/* len == sdp->sector_size */
> 		goto invalid_param_len;
> 
> ata_format_dsm_trim_descr() clamps the copy length to ATA_SCSI_RBUF_SIZE
> (2048).  On a device whose logical sector size exceeds that (e.g. a 4Kn
> device, where sector_size == 4096) the function can never return more than
> 2048, while the caller expects it to return sector_size.  The comparison
> therefore always fails, so every TRIM is rejected with "Parameter list
> length error" and WARN_ON() splats on each attempt.  TRIM / discard is
> thus completely broken on such devices.
> 
> The descriptor was incorrectly sized from the logical sector size.  A DSM
> TRIM payload is a list of 512-byte pages, each holding up to
> ATA_MAX_TRIM_RNUM (64) LBA Range Entries, and is independent of the logical
> sector size.  The Block Limits VPD page already advertises a single such
> page as the maximum WRITE SAME length (65535 * ATA_MAX_TRIM_RNUM logical
> blocks), so the block layer never sends a request that needs more than one
> page.
> 
> Emit exactly one 512-byte page, independent of the logical sector size,
> and transfer only that page (COUNT == 1).  For a 512-byte-sector device
> this is unchanged; devices with larger logical sectors now work instead of
> failing every TRIM.
> 
> Fixes: ef2d7392c4ec ("libata: SCT Write Same / DSM Trim")
> Cc: stable@vger.kernel.org
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/ata/libata-scsi.c | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@kernel.org>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

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

[Hannes Reinecke]

On 7/2/26 12:46 AM, Niklas Cassel wrote:
> DSM TRIM currently always emits a single 512-byte page of LBA Range Entries
> (ATA_MAX_TRIM_RNUM == 64 ranges), regardless of how many pages the device
> can accept in one DATA SET MANAGEMENT command.
> 
> The maximum is reported by MAX PAGES PER DSM COMMAND (IDENTIFY DEVICE word
> 105).  Honour it: size the TRIM descriptor as a whole number of 512-byte
> pages, bounded by that limit and by the logical sector size (the WRITE SAME
> data-out buffer is a single logical block).  Build and transfer only as
> many pages as the request needs, and set the DSM COUNT field, qc->nbytes
> and the maximum WRITE SAME length in the Block Limits VPD page accordingly.
> 
> Build the descriptor straight into the WRITE SAME data-out buffer using an
> atomic sg_miter mapping, instead of staging it in the shared ata_scsi_rbuf
> and copying it out.  This removes the global ata_scsi_rbuf_lock and a
> memcpy from the TRIM path.
> 
> While commit 9379e6b8e0f9 ("libata: Safely overwrite attached page in WRITE
> SAME xlat") replaced direct access to the data-out buffer with an
> intermediate step that writes the entries in the ata_scsi_rbuf buffer, this
> solution writes to the data-out buffer using sg_miter, which maps each
> segment with kmap_atomic (SG_MITER_ATOMIC), so it's highmem- and
> multi-segment-safe, and it's usable from the non-sleeping
> command-submission path (unlike the page_address() access that
> ata_scsi_rbuf originally replaced).
> 
> A 512-byte-sector device still uses a single page, so its behaviour is
> unchanged.
> 
> Add ata_id_dsm_max_pages() to read IDENTIFY DEVICE word 105.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>   drivers/ata/libata-scsi.c | 132 ++++++++++++++++++++++++++------------
>   include/linux/ata.h       |  13 ++++
>   2 files changed, 104 insertions(+), 41 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@kernel.org>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich