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

[...]