[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 at 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);
>  }
>