Re: Return type of clean_and_invalidate_dcache_va_range

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 2024-02-12 09:26, Jan Beulich wrote:
> 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
> 

I agree. I took this helper as an example, but e.g. 
invalidate_dcache_va_range returns -EOPNOTSUPP on x86 and it's only used 
in common/grant_table.
Perhaps the signatures should remain as is for consistency, especially 
given the remark below about the other architectures, and this would 
entail a deviation.

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

Should I CC them in this thread?

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)