Re: Return type of clean_and_invalidate_dcache_va_range

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.02.2024 11:17, Julien Grall wrote:
> Hi,
> 
> On 09/02/2024 22:02, Stefano Stabellini wrote:
>> On Fri, 9 Feb 2024, Nicola Vetrini wrote:
>>> Hi all,
>>>
>>> In the context of violations of MISRA C:2012 Rule 17.7: "The value returned 
>>> by
>>> a function having non-void return type shall be used", I was looking at the
>>> function "clean_and_invalidate_dcache_va_range". It has the following
>>> signature on both arm and x86:
>>>
>>> static inline int clean_and_invalidate_dcache_va_range
>>>      (const void *p, unsigned long size)
>>>
>>> The commit that introduced it for Arm ~9 years ago (71d64afe3e12: "arm: 
>>> return
>>> int from *_dcache_va_range") [1] mentions that on Arm it can't fail, but
>>> supposedly it can on x86.
>>>
>>> However, as far as I can tell, for both arch-es the implementation now 
>>> always
>>> returns 0 [2][3], so perhaps the mention of -EOPNOTSUPP for x86 is no longer
>>> true (I wasn't able to reconstruct if there was a time at which this was 
>>> true,
>>> even in the same commit that changed the return type to int).
>>>
>>> The question is: should the return type be void, since it appears that every
>>> user is ignoring the returned value (violating the rule), except the one in
>>> common/grant_table.c [4]?
>>
>> Looking at the implementation on both ARM and x86, I am in favor of
>> changing the return type to void
> I think we need some consistency between all the cache flush helpers 
> (clean_and_invalidate_dcache_va_range, invalidate_dcache_va_range() and 
> clean_dcache_va_range()). They should all return a values or not return any.

+1

> That said, we have two other architectures in development. Are we saying 
> this helpers will not need to (initially) return -EOPNOTSUPP?

For "(initially)" that's not an issue - such a stub can as well be filled
for BUG_ON("unimplemented"). The question there is what the ultimate
implementations are going to look like.

Jan