byteorder helpers and void * (was: Re: [PATCH 01/21] lib: addbyteorder helpers for the aligned case)

[Geert Uytterhoeven]

On Fri, 23 May 2008, Pavel Machek wrote:
> On Tue 2008-05-20 11:05:21, Harvey Harrison wrote:
> > Some users know the pointer they are writeing to are aligned,
> > rather than doing *(__le16 *)ptr = cpu_to_le16(val) add helpers
> > wrapping this up that have the same convention as put_unaligned_le/be.
> > 
> > Although the get_be/le versions duplicate the le16_to_cpup functionality
> > add them anyway as it makes this a complete api and start work to
> > eliminate {le|be}{16|32|64}_to_cpup uses (not many).
> > 
> > This makes the api look like:
> > 
> > get_unaligned_le16
> > put_unaligned_le16
> > 
> > get_le16
> > put_le16
> > 
> > With explicit alignment constraints.
> > 
> > Signed-off-by: Harvey Harrison <harvey.harrison@xxxxxxxxx>
> > ---
> >  include/linux/byteorder/generic.h |   60 +++++++++++++++++++++++++++++++++++++
> >  1 files changed, 60 insertions(+), 0 deletions(-)
> > 
> > diff --git a/include/linux/byteorder/generic.h b/include/linux/byteorder/generic.h
> > index 0846e6b..1f0c07e 100644
> > --- a/include/linux/byteorder/generic.h
> > +++ b/include/linux/byteorder/generic.h
> > @@ -119,6 +119,66 @@
> >  #define cpu_to_be16s __cpu_to_be16s
> >  #define be16_to_cpus __be16_to_cpus
> >  
> > +static inline u16 get_le16(void *ptr)
> > +{
> > +	return le16_to_cpu(*(__le16 *)ptr);
> > +}
> > +
> 
> Document the fact that void * passed in needs to be 16-bit aligned?

Why not let it just take a __le16 *? Because in many use-cases the pointer just
points to an array of bytes?

For the unaligned case, e.g. get_unaligned_le16(), I can understand a bit the
rationale about using void * (a typical use-case is accessing a little endian
16-bit value in the middle of an arrays of bytes).

However, a disadvantage is that you remove the ability of the compiler to check
for using the wrong accessor in a (packed for the unaligned case) struct, e.g.

    struct {
	u8 pad;
	__le16 val;			/* 16-bit value */
    } __attribute ((packed)) s;

    x = get_unaligned_le32(&s.val);	/* oops, 32-bit access */

I noticed there's also a __get_unaligned_le(), which uses compile-time
detection of the pointer time, to make sure the correct accessor is used.
Do you intend this to be used by generic code? It's function name starts
with double underscore, indicating otherwise.

Thanks!

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre
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/

Sony Technology and Software Centre Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7  B-1840 Londerzeel  Belgium
VAT BE 0413.825.160 Â RPR Brussels
Fortis 293-0376800-10 GEBA-BE-BB