Re: Return type of clean_and_invalidate_dcache_va_range

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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)