[PATCH 01/21] lib: add byteorder helpers for the aligned case

[PATCH 01/21] lib: add byteorder helpers for the aligned case

[Harvey Harrison]

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@gmail.com>
---
 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);
+}
+
+static inline u32 get_le32(void *ptr)
+{
+	return le32_to_cpu(*(__le32 *)ptr);
+}
+
+static inline u64 get_le64(void *ptr)
+{
+	return le64_to_cpu(*(__le64 *)ptr);
+}
+
+static inline u16 get_be16(void *ptr)
+{
+	return be16_to_cpu(*(__be16 *)ptr);
+}
+
+static inline u32 get_be32(void *ptr)
+{
+	return be32_to_cpu(*(__be32 *)ptr);
+}
+
+static inline u64 get_be64(void *ptr)
+{
+	return be64_to_cpu(*(__be64 *)ptr);
+}
+
+static inline void put_le16(u16 val, void *ptr)
+{
+	*(__le16 *)ptr = cpu_to_le16(val);
+}
+
+static inline void put_le32(u32 val, void *ptr)
+{
+	*(__le32 *)ptr = cpu_to_le32(val);
+}
+
+static inline void put_le64(u64 val, void *ptr)
+{
+	*(__le64 *)ptr = cpu_to_le64(val);
+}
+
+static inline void put_be16(u16 val, void *ptr)
+{
+	*(__be16 *)ptr = cpu_to_be16(val);
+}
+
+static inline void put_be32(u32 val, void *ptr)
+{
+	*(__be32 *)ptr = cpu_to_be32(val);
+}
+
+static inline void put_be64(u64 val, void *ptr)
+{
+	*(__be64 *)ptr = cpu_to_be64(val);
+}
+
 /*
  * They have to be macros in order to do the constant folding
  * correctly - if the argument passed into a inline function
-- 
1.5.5.1.570.g26b5e

Re: [PATCH 01/21] lib: add byteorder helpers for the aligned case

[Pavel Machek]

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@gmail.com>
> ---
>  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?

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2822 bytes --]

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@gmail.com>
> > ---
> >  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@sonycom.com
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

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

[Harvey Harrison]

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

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

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2685 bytes --]

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:   Geert.Uytterhoeven@sonycom.com
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