lib: add byteorder helpers for the aligned case

PATCH In review Harvey Harrison 3 participants 5 messages May 20, 2008 – Jun 25, 2008

Share & tools

Harvey Harrison

author

2008-05-20 18:05 UTC #0 lore raw

PATCH 1/21 lib: add byteorder helpers for the aligned case

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(-)

include/linux/byteorder/generic.h +60 -0

index 0846e6b..1f0c07e 100644
@@ -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

Signature · 1 line

1.5.5.1.570.g26b5e

Pavel Machek

2008-05-23 13:22 UTC #1 lore raw

Quoted 28 lines · 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 <[email protected]>
---
 include/linux/byteorder/generic.h |   60 +++++++++++++++++++++++++++++++++++++
 1 files changed, 60 insertions(+), 0 deletions(-)
include/linux/byteorder/generic.h +0 -0
index 0846e6b..1f0c07e 100644

include/linux/byteorder/generic.h line 126

Quoted 8 lines

include/linux/byteorder/generic.h +5 -0

@@ -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?

Signature · 2 lines

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

Geert Uytterhoeven

2008-06-24 12:54 UTC #2 lore raw

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

Quoted 32 lines · Pavel Machek

Quoted 31 lines · 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 <[email protected]>
---
 include/linux/byteorder/generic.h |   60 +++++++++++++++++++++++++++++++++++++
 1 files changed, 60 insertions(+), 0 deletions(-)
include/linux/byteorder/generic.h +0 -0
index 0846e6b..1f0c07e 100644
@@ -119,6 +119,66 @@
 #define cpu_to_be16s __cpu_to_be16s
 #define be16_to_cpus __be16_to_cpus
 

Quoted 7 lines

include/linux/byteorder/generic.h line 126
Quoted 5 lines
include/linux/byteorder/generic.h +5 -0
+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

Harvey Harrison

author

2008-06-25 02:35 UTC #3 lore raw

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

Quoted 13 lines · Geert Uytterhoeven

Quoted 2 lines
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.

Quoted 7 lines

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

Quoted 4 lines

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

Geert Uytterhoeven

2008-06-25 11:20 UTC #4 lore raw

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

Quoted 26 lines · Harvey Harrison

Quoted 12 lines · Geert Uytterhoeven
Quoted 1 line
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.
Quoted 7 lines
    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:

Quoted 4 lines

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)

Quoted 6 lines

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.

Quoted 9 lines

Quoted 4 lines
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/