[PATCH] nvme: Elaborate on cache maintenance operation in get/set_features

[PATCH] nvme: Elaborate on cache maintenance operation in get/set_features

[Andre Przywara]

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

[PATCH] nvme: Elaborate on cache maintenance operation in get/set_features

[Michael Nazzareno Trimarchi]

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
>


-- 
Michael Nazzareno Trimarchi
Amarula Solutions BV
COO Co-Founder
Cruquiuskade 47 Amsterdam 1018 AM NL
T. +31(0)851119172
M. +39(0)3479132170
[`as] https://www.amarulasolutions.com

[PATCH] nvme: Elaborate on cache maintenance operation in get/set_features

[Bin Meng]

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>

[PATCH] nvme: Elaborate on cache maintenance operation in get/set_features

[Marek Vasut]

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

[...]

[PATCH] nvme: Elaborate on cache maintenance operation in get/set_features

[Andre Przywara]

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);  
> 
> [...]

[PATCH] nvme: Elaborate on cache maintenance operation in get/set_features

[Marek Vasut]

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.

[PATCH] nvme: Elaborate on cache maintenance operation in get/set_features

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210319/30130803/attachment.sig>