[PATCH] nvme: add cache flush in get/set_features

[PATCH] nvme: add cache flush in get/set_features

[Neil Armstrong]

On Amlogic G12A platforms, the NVME probe timeouts at get/set_feature(),
adding a cache flush solves the timeout.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 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 5d6331ad34..44c00a0309 100644
--- a/drivers/nvme/nvme.c
+++ b/drivers/nvme/nvme.c
@@ -487,11 +487,11 @@ int nvme_get_features(struct nvme_dev *dev, unsigned fid, unsigned nsid,
 	c.features.nsid = cpu_to_le32(nsid);
 	c.features.prp1 = cpu_to_le64(dma_addr);
 	c.features.fid = cpu_to_le32(fid);
-
 	/*
-	 * TODO: add cache invalidate operation when the size of
+	 * TODO: add better cache invalidate operation when the size of
 	 * the DMA buffer is known
 	 */
+	invalidate_dcache_all();
 
 	return nvme_submit_admin_cmd(dev, &c, result);
 }
@@ -508,9 +508,10 @@ int nvme_set_features(struct nvme_dev *dev, unsigned fid, unsigned dword11,
 	c.features.dword11 = cpu_to_le32(dword11);
 
 	/*
-	 * TODO: add cache flush operation when the size of
+	 * TODO: add better cache flush operation when the size of
 	 * the DMA buffer is known
 	 */
+	invalidate_dcache_all();
 
 	return nvme_submit_admin_cmd(dev, &c, result);
 }
-- 
2.25.1

[PATCH] nvme: add cache flush in get/set_features

[Bin Meng]

Hi Neil,

On Fri, Feb 26, 2021 at 10:13 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> On Amlogic G12A platforms, the NVME probe timeouts at get/set_feature(),
> adding a cache flush solves the timeout.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/nvme/nvme.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>

Does below patch fix your issue?
http://patchwork.ozlabs.org/project/uboot/patch/20210208133154.12645-1-andre.przywara at arm.com/

REgards,
Bin

[PATCH] nvme: add cache flush in get/set_features

[André Przywara]

On 26/02/2021 14:13, Neil Armstrong wrote:

Hi,

> On Amlogic G12A platforms, the NVME probe timeouts at get/set_feature(),
> adding a cache flush solves the timeout.

I am puzzled how this is supposed to work ...

> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  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 5d6331ad34..44c00a0309 100644
> --- a/drivers/nvme/nvme.c
> +++ b/drivers/nvme/nvme.c
> @@ -487,11 +487,11 @@ int nvme_get_features(struct nvme_dev *dev, unsigned fid, unsigned nsid,
>  	c.features.nsid = cpu_to_le32(nsid);
>  	c.features.prp1 = cpu_to_le64(dma_addr);
>  	c.features.fid = cpu_to_le32(fid);
> -
>  	/*
> -	 * TODO: add cache invalidate operation when the size of
> +	 * TODO: add better cache invalidate operation when the size of
>  	 * the DMA buffer is known
>  	 */
> +	invalidate_dcache_all();

Why is this? Isn't it totally dangerous, because we kill all the
information in dirty cache lines? We have extra checks in place to
prevent invalidating extra cache lines, when invalidating a single
buffer, but this is blanketly killing all of the cache?

And just ignoring for a minute that cache operations by set/way are
mostly wrong anyway? They are just meant to initialise the cache after
power state changes.

But more importantly: I don't see a single user of nvme_get_features()
in the tree? So this would never be called?

>  
>  	return nvme_submit_admin_cmd(dev, &c, result);
>  }
> @@ -508,9 +508,10 @@ int nvme_set_features(struct nvme_dev *dev, unsigned fid, unsigned dword11,
>  	c.features.dword11 = cpu_to_le32(dword11);
>  
>  	/*
> -	 * TODO: add cache flush operation when the size of
> +	 * TODO: add better cache flush operation when the size of
>  	 * the DMA buffer is known
>  	 */
> +	invalidate_dcache_all();

Same comment as above, first part: Dangerous and mostly wrong.
Besides: the comment speaks of "flush", not invalidate. So would be
extra wrong.
Also: there is exactly one caller in the whole tree, in this very same
file. And this one is passing a dma_addr of 0, apparently because it
doesn't actually use any buffer, instead passes the single piece of
information (the queue count) in the dword11 field.

So how is this supposed to work?

And if this seems to fix something, how?

Cheers,
Andre

>  
>  	return nvme_submit_admin_cmd(dev, &c, result);
>  }
> 

[PATCH] nvme: add cache flush in get/set_features

[Neil Armstrong]

Hi Andre, Bin,

On 26/02/2021 16:22, Andr? Przywara wrote:
> On 26/02/2021 14:13, Neil Armstrong wrote:
> 
> Hi,
> 
[..]

> 
> And if this seems to fix something, how?

Good question... sorry for the noise my patch is totally wrong, but it fixed something somehow.

But, "nvme: Always invalidate whole cqes[] array" fixes (correctly) the issue !

Thanks,

Neil

> 
> Cheers,
> Andre
> 
>>  
>>  	return nvme_submit_admin_cmd(dev, &c, result);
>>  }
>>
> 

[PATCH] nvme: add cache flush in get/set_features

[Andre Przywara]

On Fri, 26 Feb 2021 17:11:07 +0100
Neil Armstrong <narmstrong@baylibre.com> wrote:

Hi Neil,

> On 26/02/2021 16:22, Andr? Przywara wrote:
> > On 26/02/2021 14:13, Neil Armstrong wrote:
> > 
> > Hi,
> >   
> [..]
> 
> > 
> > And if this seems to fix something, how?  
> 
> Good question... sorry for the noise my patch is totally wrong, but it fixed something somehow.

No worries, the comments sound indeed promising!
I made a patch to clarify the situation.

> But, "nvme: Always invalidate whole cqes[] array" fixes (correctly) the issue !

That great to know! Thanks for trying this!

Cheers,
Andre

> >>  
> >>  	return nvme_submit_admin_cmd(dev, &c, result);
> >>  }
> >>  
> >   
>