[PATCH] nvme: Invalidate dcache before submitting admin cmd

[André Przywara]

On 07/06/2020 12:22, Jagan Teki wrote:

Hi,

(CC: ing Mark)

Without looking to deep, I think invalidating the cache might be the
right thing to do, but the rationale or at least the wording of it seems
somehow flawed:

> Some architecture like ARM Cortex A53, A72 would need

Please don't mix the term "architecture" with actual implementations.
And the problem is more general, I would say. This *might* be a case
here where this is due to speculation, and then I would expect it to
only show on an A72, for instance. I guess this is about NVMe on RK3399?
Does U-Boot run on an A53 or an A72 core?

> to invalidate dcache to sync the cache with the memory
> contents before flushing the cache to memory.

That is somewhat confusing. What does "sync" and "flush" mean here?
AFAIK only "clean" and "invalidate" are clearly defined terms.
The command buffer is cleaned to DRAM in nvme_submit_cmd(), so this is
purely about the buffer for the *return* data, in which case I would
expect this being a pure invalidation problem.

This issue looks like the case described on slide 30 and 31 here:
https://elinux.org/images/6/6d/Rutland2.pdf

> The NVME here submitting the admin command using dma_addr
> to the memory without prior cache invalidation. This causing
> dma_addr is pointing to an invalid location in the memory

This does not sound right: dma_addr is always pointing to the right
location in memory.
The actual reason this fixes something might be that (some) cache lines
of the DMA buffer were dirty and were evicted *before* the existing
invalidation, but *after* the DMA transfer. That sounds unlikely in an
U-Boot context, though, but would match the case described in Mark's slides.

So there might be more to this. Are we missing a barrier here? Should we
use coherent buffers (memory mapped as un-cached) for the return DMA in
the first place (but I don't think U-Boot provides those)?

> and found the sane nvme_ctrl result.
> 
> Below example shows the nvme disk scan result improper result
> 
> => nvme scan
> nvme_get_info_from_identify: nn = 544502629, vwc = 100,
> sn = dev_0T, mn = `�\�, fr = t_part, mdts = 105
> 
> So, invalidating the cache before submitting the admin command
> makes the dma_addr points to a valid location in the memory.

If this shows up already, I think we should address all the comments in
the driver where it says we should do cache maintenance. And it makes me
wonder how this actually works today ...

Cheers,
Andre.

> 
> Cc: Andre Przywara <andre.przywara at arm.com> 
> Reported-by: Suniel Mahesh <sunil at amarulasolutions.com>
> Signed-off-by: Michael Trimarchi <michael at amarulasolutions.com>
> Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> Tested-by: Suniel Mahesh <sunil at amarulasolutions.com>
> ---
>  drivers/nvme/nvme.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c
> index 0357aba7f1..fc64d93ab8 100644
> --- a/drivers/nvme/nvme.c
> +++ b/drivers/nvme/nvme.c
> @@ -466,6 +466,9 @@ int nvme_identify(struct nvme_dev *dev, unsigned nsid,
>  
>  	c.identify.cns = cpu_to_le32(cns);
>  
> +	invalidate_dcache_range(dma_addr,
> +				dma_addr + sizeof(struct nvme_id_ctrl));
> +
>  	ret = nvme_submit_admin_cmd(dev, &c, NULL);
>  	if (!ret)
>  		invalidate_dcache_range(dma_addr,
>