2008-06-24 12:54:25

by Geert Uytterhoeven

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

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 <[email protected]>
> > ---
> > 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: [email protected]
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


2008-06-25 02:35:48

by Harvey Harrison

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

On Tue, 2008-06-24 at 14:54 +0200, Geert Uytterhoeven wrote:
> >
> > 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'm starting to come around to the typechecking argument. This would
also be a chance to fix the argument ordering in put_analigned_XXXX
that was noticed by others. As there are already some existing users
in-tree, we could transition gradually by:

1) Introduce typed versions of get/put_unaligned_XXXX, that implies the
byteswap better:
u16 load_unaligned_le16(__le16 *)
void store_unaligned_le16(__le16 *, u16)

Then the aligned helpers could be:
le16_to_cpup -> aligned equivalent of load_unaligned_le16
store_le16(__le16 *, u16)

Implemented as (to allow constant folding)
#define store_le16(ptr, val) (*(__le16 *)(ptr) = cpu_to_le16((u16)(val)))

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

It is not meant for generic use, it is just there as a helper for each
arch to wire up it's get_unaligned() macro depending on its endianness,
so each arch doesn't wire up its own version that may or may not have
the size checking.

Anything I missed?

Harvey

2008-06-25 11:20:28

by Geert Uytterhoeven

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

On Tue, 24 Jun 2008, Harvey Harrison wrote:
> On Tue, 2008-06-24 at 14:54 +0200, Geert Uytterhoeven wrote:
> > > 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'm starting to come around to the typechecking argument. This would
> also be a chance to fix the argument ordering in put_analigned_XXXX
> that was noticed by others. As there are already some existing users
> in-tree, we could transition gradually by:
>
> 1) Introduce typed versions of get/put_unaligned_XXXX, that implies the
> byteswap better:
> u16 load_unaligned_le16(__le16 *)
> void store_unaligned_le16(__le16 *, u16)

OK

> Then the aligned helpers could be:
> le16_to_cpup -> aligned equivalent of load_unaligned_le16
> store_le16(__le16 *, u16)
>
> Implemented as (to allow constant folding)
> #define store_le16(ptr, val) (*(__le16 *)(ptr) = cpu_to_le16((u16)(val)))

Again, no typechecking in store_le16(), due to the explicit cast.

> > 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.
>
> It is not meant for generic use, it is just there as a helper for each
> arch to wire up it's get_unaligned() macro depending on its endianness,
> so each arch doesn't wire up its own version that may or may not have
> the size checking.

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: [email protected]
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