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