| Message ID | 20210226141318.3882733-1-narmstrong@baylibre.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | nvme: add cache flush in get/set_features | expand |
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@arm.com/ REgards, Bin
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); > } >
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); >> } >> >
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); > >> } > >> > > >
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); }
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(-)