Re: endianness and sparse warnings

[Geert Uytterhoeven]

On Fri, 29 Aug 2008, Harvey Harrison wrote:
> On Fri, 2008-08-29 at 16:54 +0200, Geert Uytterhoeven wrote:
> > With `make  C=1 CF="-D__CHECK_ENDIAN__"', you can let sparse check for bad
> > handling of endian-annotated data.
> > 
> > Unfortunately several of the accessors for endian-annotated data do not cause
> > sparse warnings.
> 
> I'll try and give some background on why the unaligned versions are implemented
> this way.
> 
> The get_unaligned helpers were meant to replace two kinds of use (using le16 as
> an example)
> 
> char *ptr;
> 
> 1 - le16_to_cpu(get_unaligned((__le16 *)ptr))
> 2 - u16 val = ptr[0] | (ptr[1] << 8)
> 
> The places where 1 was replaced with the unaligned helpers would have been fine
> with an annotated version as it already had the cast to a proper type.
> 
> The places where 2 was replaced would have required a new cast to __le16 *.
> 
> Lots of places that were using 2 are drivers that have some data area pointed
> to by a char * and they are grabbing values from there at known offsets,
> for these users, the need for extra casting was quite ugly and it was known
> exactly how many bytes and in what endianness you are reading as it is
> right in the function name so I thought it would be ok to omit the annotation
> on the parameter.
> 
> u16 foo, bar;
> char *my_data;
> 
> foo = get_unaligned_le16((__le16 *)my_data); /* if unaligned helpers were annotated */
> bar = get_unaligned_le16(my_data);	/* current version */

Indeed, if you have a void/char *, this works, and there's not much annotation
you can do here.

But please consider this case:

    struct xxx {
	u8 a;
	__le16 b;
    } __attribute__ ((packed));

    struct xxx *p;
    u16 foo = get_unaligned_le16(&p->b);

Here you could have typechecking.
Note that a `get_unaligned_le()' that handles different sizes automatically
would work.

> > Summarized:
> >   - [bl]e{16,32,64}_to_cpu() is OK
> >   - [bl]e{16,32,64}_to_cpup() (aka get_aligned_[bl]e{16,32,64}() ;-) is OK
> >   - get_unaligned_[bl]e{16,32,64} is not OK
> >   - __get_unaligned_[bl]e() is partially OK, as long as you don't use it on
> >     non-annotated data, but
> >       o it's meant for internal use only
> >       o it incorrectly causes sparse warnings when assigning the resulting
> >         value to a non-annotated variable
> 
> Almost... __get_unaligned_le16 etc are _never_ to be used...as some arches
> choose to use memmove-based implementations, and on arches where unaligned
> access is OK, they don't exist _at_all_.

So perhaps we want a public get_unaligned_[bl]e() for the case above?

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village  Da Vincilaan 7-D1  B-1935 Zaventem  Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@xxxxxxxxxxx
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 Â RPR Brussels
Fortis  BIC GEBABEBB  IBAN BE41293037680010