[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.