Patchwork [4/4] libata: Fix garbled Trim payload

login
register
mail settings
Submitter Martin K. Petersen
Date Nov. 21, 2009, 2:45 a.m.
Message ID <1258771524-26673-5-git-send-email-martin.petersen@oracle.com>
Download mbox | patch
Permalink /patch/38952/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Martin K. Petersen - Nov. 21, 2009, 2:45 a.m.
ata_set_lba_range_entries confused indexes into a u64 buffer with byte
offsets.  Fix this up and document the return value.

Update the libata write same translation to reflect this.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/ata/libata-scsi.c |    8 ++++----
 include/linux/ata.h       |    9 +++++----
 2 files changed, 9 insertions(+), 8 deletions(-)
Christoph Hellwig - Nov. 21, 2009, 10:47 a.m.
On Fri, Nov 20, 2009 at 09:45:24PM -0500, Martin K. Petersen wrote:
> ata_set_lba_range_entries confused indexes into a u64 buffer with byte
> offsets.  Fix this up and document the return value.
> 
> Update the libata write same translation to reflect this.

Once we fix up this beast we should also change the current max argument
to encode the buffer size, not the maximum number of entries which is a
rather confusing interface.

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martin K. Petersen - Nov. 21, 2009, 7:50 p.m.
>>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:

Christoph> On Fri, Nov 20, 2009 at 09:45:24PM -0500, Martin K. Petersen wrote:
>> ata_set_lba_range_entries confused indexes into a u64 buffer with
>> byte offsets.  Fix this up and document the return value.
>> 
>> Update the libata write same translation to reflect this.

Christoph> Once we fix up this beast we should also change the current
Christoph> max argument to encode the buffer size, not the maximum
Christoph> number of entries which is a rather confusing interface.

What threw me off yesterday was that `max' starts out being a cap on the
number of 8-byte entries in the buffer. And then towards the end it
suddenly turns into a number-of-bytes entity as a result of the ALIGN
macro.

I agree that we should fix this up to be consistent. I'll do that.

Patch

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 08d4ab7..0c496f8 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2950,7 +2950,7 @@  static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 	const u8 *cdb = scmd->cmnd;
 	u64 block;
 	u32 n_block;
-	u32 size;
+	u32 trim_sectors;
 	void *buf;
 
 	/* we may not issue DMA commands if no DMA mode is set */
@@ -2973,13 +2973,13 @@  static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 		goto invalid_fld;
 
 	buf = page_address(sg_page(scsi_sglist(scmd)));
-	size = ata_set_lba_range_entries(buf, 512 / 8, block, n_block);
+	trim_sectors = ata_set_lba_range_entries(buf, 512 / 8, block, n_block);
 
 	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 = trim_sectors >> 8;
+	tf->nsect = trim_sectors;
 	tf->command = ATA_CMD_DSM;
 	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE | ATA_TFLAG_LBA48 |
 		     ATA_TFLAG_WRITE;
diff --git a/include/linux/ata.h b/include/linux/ata.h
index b18b2bb..150c9dc 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -983,7 +983,8 @@  static inline void ata_id_to_hd_driveid(u16 *id)
 /*
  * Write up to 'max' LBA Range Entries to the buffer that will cover the
  * extent from sector to sector + count.  This is used for TRIM and for
- * ADD LBA(S) TO NV CACHE PINNED SET.
+ * ADD LBA(S) TO NV CACHE PINNED SET.  Returns number of 512-byte
+ * sectors required to encode the range.
  */
 static inline unsigned ata_set_lba_range_entries(void *_buffer, unsigned max,
 						u64 sector, unsigned long count)
@@ -1001,9 +1002,9 @@  static inline unsigned ata_set_lba_range_entries(void *_buffer, unsigned max,
 		sector += 0xffff;
 	}
 
-	max = ALIGN(i * 8, 512);
-	memset(buffer + i, 0, max - i * 8);
-	return max;
+	memset(&buffer[i], 0, (max - i) * 8);
+
+	return ALIGN(i * 8, 512);
 }
 
 static inline int is_multi_taskfile(struct ata_taskfile *tf)