[PATCH] nvme: invalidate correct memory range after read
The current code invalidates the range after the read buffer since the buffer pointer gets incremented in the read loop. Use a temporary pointer to make sure we have a pristine pointer to invalidate the correct memory range after read.
Fixes: 704e040a51d2 ("nvme: Apply cache operations on the DMA buffers") Signed-off-by: Stefan Agner stefan@agner.ch ---
drivers/nvme/nvme.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index f6465ea7f4..354513ad30 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -743,6 +743,7 @@ static ulong nvme_blk_rw(struct udevice *udev, lbaint_t blknr, u64 prp2; u64 total_len = blkcnt << desc->log2blksz; u64 temp_len = total_len; + void *temp_buffer = buffer;
u64 slba = blknr; u16 lbas = 1 << (dev->max_transfer_shift - ns->lba_shift); @@ -770,19 +771,19 @@ static ulong nvme_blk_rw(struct udevice *udev, lbaint_t blknr, }
if (nvme_setup_prps(dev, &prp2, - lbas << ns->lba_shift, (ulong)buffer)) + lbas << ns->lba_shift, (ulong)temp_buffer)) return -EIO; c.rw.slba = cpu_to_le64(slba); slba += lbas; c.rw.length = cpu_to_le16(lbas - 1); - c.rw.prp1 = cpu_to_le64((ulong)buffer); + c.rw.prp1 = cpu_to_le64((ulong)temp_buffer); c.rw.prp2 = cpu_to_le64(prp2); status = nvme_submit_sync_cmd(dev->queues[NVME_IO_Q], &c, NULL, IO_TIMEOUT); if (status) break; temp_len -= (u32)lbas << ns->lba_shift; - buffer += lbas << ns->lba_shift; + temp_buffer += lbas << ns->lba_shift; }
if (read)
On Tue, 28 Sep 2021 10:01:40 +0200 Stefan Agner stefan@agner.ch wrote:
The current code invalidates the range after the read buffer since the buffer pointer gets incremented in the read loop. Use a temporary pointer to make sure we have a pristine pointer to invalidate the correct memory range after read.
Ah, good catch! This looks good, just one nit below:
Fixes: 704e040a51d2 ("nvme: Apply cache operations on the DMA buffers") Signed-off-by: Stefan Agner stefan@agner.ch
drivers/nvme/nvme.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index f6465ea7f4..354513ad30 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -743,6 +743,7 @@ static ulong nvme_blk_rw(struct udevice *udev, lbaint_t blknr, u64 prp2; u64 total_len = blkcnt << desc->log2blksz; u64 temp_len = total_len;
- void *temp_buffer = buffer;
You could make this an unsigned long (or better uintptr_t), then lose all the casts below and avoid the void ptr arithmetic.
But it works either way, so:
Reviewed-by: Andre Przywara andre.przywara@arm.com
Cheers, Andre
u64 slba = blknr; u16 lbas = 1 << (dev->max_transfer_shift - ns->lba_shift); @@ -770,19 +771,19 @@ static ulong nvme_blk_rw(struct udevice *udev, lbaint_t blknr, }
if (nvme_setup_prps(dev, &prp2,
lbas << ns->lba_shift, (ulong)buffer))
lbas << ns->lba_shift, (ulong)temp_buffer)) return -EIO;
c.rw.prp1 = cpu_to_le64((ulong)buffer);
c.rw.prp1 = cpu_to_le64((ulong)temp_buffer);
buffer += lbas << ns->lba_shift;
temp_buffer += lbas << ns->lba_shift;}
if (read)
On 2021-09-30 18:09, Andre Przywara wrote:
On Tue, 28 Sep 2021 10:01:40 +0200 Stefan Agner stefan@agner.ch wrote:
The current code invalidates the range after the read buffer since the buffer pointer gets incremented in the read loop. Use a temporary pointer to make sure we have a pristine pointer to invalidate the correct memory range after read.
Ah, good catch!
It did actually create issues in real world where sometimes it would just not recognize a file system properly when using the ls command.
This looks good, just one nit below:
Fixes: 704e040a51d2 ("nvme: Apply cache operations on the DMA buffers") Signed-off-by: Stefan Agner stefan@agner.ch
drivers/nvme/nvme.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index f6465ea7f4..354513ad30 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -743,6 +743,7 @@ static ulong nvme_blk_rw(struct udevice *udev, lbaint_t blknr, u64 prp2; u64 total_len = blkcnt << desc->log2blksz; u64 temp_len = total_len;
- void *temp_buffer = buffer;
You could make this an unsigned long (or better uintptr_t), then lose all the casts below and avoid the void ptr arithmetic.
Ok, I'll send an update shortly.
But it works either way, so:
Reviewed-by: Andre Przywara andre.przywara@arm.com
Thanks for your review.
-- Stefan
Cheers, Andre
u64 slba = blknr; u16 lbas = 1 << (dev->max_transfer_shift - ns->lba_shift); @@ -770,19 +771,19 @@ static ulong nvme_blk_rw(struct udevice *udev, lbaint_t blknr, }
if (nvme_setup_prps(dev, &prp2,
lbas << ns->lba_shift, (ulong)buffer))
lbas << ns->lba_shift, (ulong)temp_buffer)) return -EIO;
c.rw.prp1 = cpu_to_le64((ulong)buffer);
c.rw.prp1 = cpu_to_le64((ulong)temp_buffer);
buffer += lbas << ns->lba_shift;
temp_buffer += lbas << ns->lba_shift;}
if (read)
participants (2)
-
Andre Przywara -
Stefan Agner