[PATCH] nvme: Invalidate dcache before submitting admin cmd

[Michael Nazzareno Trimarchi]

Hi

On Mon, Jun 8, 2020 at 3:52 PM André Przywara <andre.przywara at arm.com> wrote:
>
> 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.
>

We will adjust commit message. The idea is that when we invalidate after the DMA
transfer, a cache flush happen for the invalidation itself

> 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.
>

Correct

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

Thank you for point out to the slide

> > 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.

Does it not depend on how invalidation work in the particular CPU?

>
> 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)?

Right, I don't find a better way and yes this is  general problem in uboot

Michael


>
> > 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,
> >
>


-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |