* [PATCH] nvme: invalidate correct memory range after read
@ 2021-09-28 8:01 Stefan Agner
2021-09-30 16:09 ` Andre Przywara
0 siblings, 1 reply; 3+ messages in thread
From: Stefan Agner @ 2021-09-28 8:01 UTC (permalink / raw)
To: u-boot, bmeng.cn; +Cc: nsaenz, mbrugger, m.szyprowski, s.nawrocki, Stefan Agner
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)
--
2.33.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] nvme: invalidate correct memory range after read
2021-09-28 8:01 [PATCH] nvme: invalidate correct memory range after read Stefan Agner
@ 2021-09-30 16:09 ` Andre Przywara
2021-10-04 9:14 ` Stefan Agner
0 siblings, 1 reply; 3+ messages in thread
From: Andre Przywara @ 2021-09-30 16:09 UTC (permalink / raw)
To: Stefan Agner
Cc: u-boot, bmeng.cn, nsaenz, mbrugger, m.szyprowski, s.nawrocki, Tom Rini
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.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)
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] nvme: invalidate correct memory range after read
2021-09-30 16:09 ` Andre Przywara
@ 2021-10-04 9:14 ` Stefan Agner
0 siblings, 0 replies; 3+ messages in thread
From: Stefan Agner @ 2021-10-04 9:14 UTC (permalink / raw)
To: Andre Przywara
Cc: u-boot, bmeng.cn, nsaenz, mbrugger, m.szyprowski, s.nawrocki, Tom Rini
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.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)
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-04 9:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-28 8:01 [PATCH] nvme: invalidate correct memory range after read Stefan Agner
2021-09-30 16:09 ` Andre Przywara
2021-10-04 9:14 ` Stefan Agner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).