| Message ID | 20210302154343.6080-1-andre.przywara@arm.com |
|---|---|
| State | Accepted |
| Commit | 4c498796891a26a7283130f367a346096a6ccce7 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | nvme: Elaborate on cache maintenance operation in get/set_features | expand |
Hi On Tue, Mar 2, 2021 at 4:43 PM Andre Przywara <andre.przywara@arm.com> wrote: > > At the moment the nvme_get_features() and nvme_set_features() functions > carry a (somewhat misleading) comment about missing cache maintenance. > > As it turns out, nvme_get_features() has no caller at all in the tree, > and nvme_set_features' only user doesn't use a DMA buffer. > > Mention that in the comment, and leave some breadcrumbs for the future, > should those functions attract more users. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > Hi, > > just extending the comments as this caused some confusion lately, and to > keep the pointer to the NVMe spec I checked. > > Cheers, > Andre > > drivers/nvme/nvme.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c > index 5d6331ad346..3ff3d5de08e 100644 > --- a/drivers/nvme/nvme.c > +++ b/drivers/nvme/nvme.c > @@ -481,6 +481,7 @@ int nvme_get_features(struct nvme_dev *dev, unsigned fid, unsigned nsid, > dma_addr_t dma_addr, u32 *result) > { > struct nvme_command c; > + int ret; > > memset(&c, 0, sizeof(c)); > c.features.opcode = nvme_admin_get_features; > @@ -488,12 +489,20 @@ int nvme_get_features(struct nvme_dev *dev, unsigned fid, unsigned nsid, > c.features.prp1 = cpu_to_le64(dma_addr); > c.features.fid = cpu_to_le32(fid); > > + ret = nvme_submit_admin_cmd(dev, &c, result); > + > /* > - * TODO: add cache invalidate operation when the size of > - * the DMA buffer is known > + * TODO: Add some cache invalidation when a DMA buffer is involved > + * in the request, here and before the command gets submitted. The > + * buffer size varies by feature, also some features use a different > + * field in the command packet to hold the buffer address. > + * Section 5.21.1 (Set Features command) in the NVMe specification > + * details the buffer requirements for each feature. > + * > + * At the moment there is no user of this function. > */ > > - return nvme_submit_admin_cmd(dev, &c, result); > + return ret; > } > > int nvme_set_features(struct nvme_dev *dev, unsigned fid, unsigned dword11, > @@ -508,8 +517,14 @@ 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 > - * the DMA buffer is known > + * TODO: Add a cache clean (aka flush) operation when a DMA buffer is > + * involved in the request. The buffer size varies by feature, also > + * some features use a different field in the command packet to hold > + * the buffer address. Section 5.21.1 (Set Features command) in the > + * NVMe specification details the buffer requirements for each > + * feature. > + * At the moment the only user of this function is not using > + * any DMA buffer at all. > */ > > return nvme_submit_admin_cmd(dev, &c, result); Reviewed-By: Michael Trimarchi <michael@amarulasolutions.com> > -- > 2.17.5 >
On Tue, Mar 2, 2021 at 11:43 PM Andre Przywara <andre.przywara@arm.com> wrote: > > At the moment the nvme_get_features() and nvme_set_features() functions > carry a (somewhat misleading) comment about missing cache maintenance. > > As it turns out, nvme_get_features() has no caller at all in the tree, > and nvme_set_features' only user doesn't use a DMA buffer. > > Mention that in the comment, and leave some breadcrumbs for the future, > should those functions attract more users. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > Hi, > > just extending the comments as this caused some confusion lately, and to > keep the pointer to the NVMe spec I checked. > > Cheers, > Andre > > drivers/nvme/nvme.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
On 3/2/21 4:43 PM, Andre Przywara wrote: [...] > @@ -488,12 +489,20 @@ int nvme_get_features(struct nvme_dev *dev, unsigned fid, unsigned nsid, > c.features.prp1 = cpu_to_le64(dma_addr); > c.features.fid = cpu_to_le32(fid); > > + ret = nvme_submit_admin_cmd(dev, &c, result); > + > /* > - * TODO: add cache invalidate operation when the size of > - * the DMA buffer is known > + * TODO: Add some cache invalidation when a DMA buffer is involved > + * in the request, here and before the command gets submitted. The > + * buffer size varies by feature, also some features use a different > + * field in the command packet to hold the buffer address. > + * Section 5.21.1 (Set Features command) in the NVMe specification > + * details the buffer requirements for each feature. > + * > + * At the moment there is no user of this function. If there is no user, remove the function. > */ > > - return nvme_submit_admin_cmd(dev, &c, result); > + return ret; > } > > int nvme_set_features(struct nvme_dev *dev, unsigned fid, unsigned dword11, > @@ -508,8 +517,14 @@ 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 > - * the DMA buffer is known > + * TODO: Add a cache clean (aka flush) operation when a DMA buffer is The operation is called flush in U-Boot, cache clean is ARM-specific terminology, avoid it in generic drivers. > + * involved in the request. The buffer size varies by feature, also > + * some features use a different field in the command packet to hold > + * the buffer address. Section 5.21.1 (Set Features command) in the > + * NVMe specification details the buffer requirements for each > + * feature. It would be far more useful to explain the buffer requirements here than to point to an unknown version of some spec, which could very well be re-enumerated in the next version. > + * At the moment the only user of this function is not using > + * any DMA buffer at all. > */ > > return nvme_submit_admin_cmd(dev, &c, result); [...]
On Wed, 3 Mar 2021 03:42:40 +0100 Marek Vasut <marex@denx.de> wrote: Hi, > On 3/2/21 4:43 PM, Andre Przywara wrote: > > [...] > > > @@ -488,12 +489,20 @@ int nvme_get_features(struct nvme_dev *dev, unsigned fid, unsigned nsid, > > c.features.prp1 = cpu_to_le64(dma_addr); > > c.features.fid = cpu_to_le32(fid); > > > > + ret = nvme_submit_admin_cmd(dev, &c, result); > > + > > /* > > - * TODO: add cache invalidate operation when the size of > > - * the DMA buffer is known > > + * TODO: Add some cache invalidation when a DMA buffer is involved > > + * in the request, here and before the command gets submitted. The > > + * buffer size varies by feature, also some features use a different > > + * field in the command packet to hold the buffer address. > > + * Section 5.21.1 (Set Features command) in the NVMe specification > > + * details the buffer requirements for each feature. > > + * > > + * At the moment there is no user of this function. > > If there is no user, remove the function. Well, this function was apparently introduced for a reason, and it's exported. So I am just trying to clarify on the comment here. I'd leave it to people more familiar with NVMe and its implementation in U-Boot to remove something. > > */ > > > > - return nvme_submit_admin_cmd(dev, &c, result); > > + return ret; > > } > > > > int nvme_set_features(struct nvme_dev *dev, unsigned fid, unsigned dword11, > > @@ -508,8 +517,14 @@ 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 > > - * the DMA buffer is known > > + * TODO: Add a cache clean (aka flush) operation when a DMA buffer is > The operation is called flush in U-Boot, cache clean is ARM-specific > terminology, avoid it in generic drivers. > > > + * involved in the request. The buffer size varies by feature, also > > + * some features use a different field in the command packet to hold > > + * the buffer address. Section 5.21.1 (Set Features command) in the > > + * NVMe specification details the buffer requirements for each > > + * feature. > > It would be far more useful to explain the buffer requirements here than > to point to an unknown version of some spec, which could very well be > re-enumerated in the next version. I originally wanted to, but didn't for two reasons: - It's not trivial, since the buffer usage seems to differ between the features. This would require to enumerate all the features involved, and then to include all the special cases. I would probably mess this up, so I didn't want to introduce untested and hard-to-verify information. - There is little point in adding specific information when we will never need it. The spec is the authority, anyone can look that up. Yes, the enumeration might change, that's why I added the section name, hoping that people adding a feature can piece things together. The numbering was valid for v1.3 and is still in the latest v1.4b, so I can just add that version number to make this less ambiguous. Cheers, Andre > > > + * At the moment the only user of this function is not using > > + * any DMA buffer at all. > > */ > > > > return nvme_submit_admin_cmd(dev, &c, result); > > [...]
On 3/3/21 12:13 PM, Andre Przywara wrote: [...] >>> @@ -488,12 +489,20 @@ int nvme_get_features(struct nvme_dev *dev, unsigned fid, unsigned nsid, >>> c.features.prp1 = cpu_to_le64(dma_addr); >>> c.features.fid = cpu_to_le32(fid); >>> >>> + ret = nvme_submit_admin_cmd(dev, &c, result); >>> + >>> /* >>> - * TODO: add cache invalidate operation when the size of >>> - * the DMA buffer is known >>> + * TODO: Add some cache invalidation when a DMA buffer is involved >>> + * in the request, here and before the command gets submitted. The >>> + * buffer size varies by feature, also some features use a different >>> + * field in the command packet to hold the buffer address. >>> + * Section 5.21.1 (Set Features command) in the NVMe specification >>> + * details the buffer requirements for each feature. >>> + * >>> + * At the moment there is no user of this function. >> >> If there is no user, remove the function. > > Well, this function was apparently introduced for a reason, and it's > exported. So I am just trying to clarify on the comment here. I'd leave > it to people more familiar with NVMe and its implementation in U-Boot > to remove something. No need to keep dead code around. >>> */ >>> >>> - return nvme_submit_admin_cmd(dev, &c, result); >>> + return ret; >>> } >>> >>> int nvme_set_features(struct nvme_dev *dev, unsigned fid, unsigned dword11, >>> @@ -508,8 +517,14 @@ 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 >>> - * the DMA buffer is known >>> + * TODO: Add a cache clean (aka flush) operation when a DMA buffer is >> The operation is called flush in U-Boot, cache clean is ARM-specific >> terminology, avoid it in generic drivers. >> >>> + * involved in the request. The buffer size varies by feature, also >>> + * some features use a different field in the command packet to hold >>> + * the buffer address. Section 5.21.1 (Set Features command) in the >>> + * NVMe specification details the buffer requirements for each >>> + * feature. >> >> It would be far more useful to explain the buffer requirements here than >> to point to an unknown version of some spec, which could very well be >> re-enumerated in the next version. > > I originally wanted to, but didn't for two reasons: > - It's not trivial, since the buffer usage seems to differ between the > features. This would require to enumerate all the features involved, > and then to include all the special cases. I would probably mess this > up, so I didn't want to introduce untested and hard-to-verify > information. > - There is little point in adding specific information when we will > never need it. The spec is the authority, anyone can look that up. > > Yes, the enumeration might change, that's why I added the section name, > hoping that people adding a feature can piece things together. > The numbering was valid for v1.3 and is still in the latest v1.4b, so I > can just add that version number to make this less ambiguous. Yes please. Some link to the spec would also help. Not listing which spec exactly you refer to and in which revision makes the comment useless.
On Tue, Mar 02, 2021 at 03:43:43PM +0000, Andre Przywara wrote: > At the moment the nvme_get_features() and nvme_set_features() functions > carry a (somewhat misleading) comment about missing cache maintenance. > > As it turns out, nvme_get_features() has no caller at all in the tree, > and nvme_set_features' only user doesn't use a DMA buffer. > > Mention that in the comment, and leave some breadcrumbs for the future, > should those functions attract more users. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > Reviewed-By: Michael Trimarchi <michael@amarulasolutions.com> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> Applied to u-boot/master, thanks!
diff --git a/drivers/nvme/nvme.c b/drivers/nvme/nvme.c index 5d6331ad346..3ff3d5de08e 100644 --- a/drivers/nvme/nvme.c +++ b/drivers/nvme/nvme.c @@ -481,6 +481,7 @@ int nvme_get_features(struct nvme_dev *dev, unsigned fid, unsigned nsid, dma_addr_t dma_addr, u32 *result) { struct nvme_command c; + int ret; memset(&c, 0, sizeof(c)); c.features.opcode = nvme_admin_get_features; @@ -488,12 +489,20 @@ int nvme_get_features(struct nvme_dev *dev, unsigned fid, unsigned nsid, c.features.prp1 = cpu_to_le64(dma_addr); c.features.fid = cpu_to_le32(fid); + ret = nvme_submit_admin_cmd(dev, &c, result); + /* - * TODO: add cache invalidate operation when the size of - * the DMA buffer is known + * TODO: Add some cache invalidation when a DMA buffer is involved + * in the request, here and before the command gets submitted. The + * buffer size varies by feature, also some features use a different + * field in the command packet to hold the buffer address. + * Section 5.21.1 (Set Features command) in the NVMe specification + * details the buffer requirements for each feature. + * + * At the moment there is no user of this function. */ - return nvme_submit_admin_cmd(dev, &c, result); + return ret; } int nvme_set_features(struct nvme_dev *dev, unsigned fid, unsigned dword11, @@ -508,8 +517,14 @@ 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 - * the DMA buffer is known + * TODO: Add a cache clean (aka flush) operation when a DMA buffer is + * involved in the request. The buffer size varies by feature, also + * some features use a different field in the command packet to hold + * the buffer address. Section 5.21.1 (Set Features command) in the + * NVMe specification details the buffer requirements for each + * feature. + * At the moment the only user of this function is not using + * any DMA buffer at all. */ return nvme_submit_admin_cmd(dev, &c, result);
At the moment the nvme_get_features() and nvme_set_features() functions carry a (somewhat misleading) comment about missing cache maintenance. As it turns out, nvme_get_features() has no caller at all in the tree, and nvme_set_features' only user doesn't use a DMA buffer. Mention that in the comment, and leave some breadcrumbs for the future, should those functions attract more users. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- Hi, just extending the comments as this caused some confusion lately, and to keep the pointer to the NVMe spec I checked. Cheers, Andre drivers/nvme/nvme.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)