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