[PATCH] byteorder: force in-place endian conversion to always evaluate args

[PATCH] byteorder: force in-place endian conversion to always evaluate args

[Harvey Harrison]

David Miller reported breakage in ide when the in-place byteorder helpers
were used as the macros do not always evaluate their args which led to
an infinite loop.

Just make them functions to ensure they always do so.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 include/linux/byteorder/big_endian.h    |   60 ++++++++++++++++++++++++------
 include/linux/byteorder/little_endian.h |   60 ++++++++++++++++++++++++------
 2 files changed, 96 insertions(+), 24 deletions(-)

diff --git a/include/linux/byteorder/big_endian.h b/include/linux/byteorder/big_endian.h
index 961ed4b..b53ccd0 100644
--- a/include/linux/byteorder/big_endian.h
+++ b/include/linux/byteorder/big_endian.h
@@ -88,18 +88,54 @@ static inline __u16 __be16_to_cpup(const __be16 *p)
 {
 	return (__force __u16)*p;
 }
-#define __cpu_to_le64s(x) __swab64s((x))
-#define __le64_to_cpus(x) __swab64s((x))
-#define __cpu_to_le32s(x) __swab32s((x))
-#define __le32_to_cpus(x) __swab32s((x))
-#define __cpu_to_le16s(x) __swab16s((x))
-#define __le16_to_cpus(x) __swab16s((x))
-#define __cpu_to_be64s(x) do {} while (0)
-#define __be64_to_cpus(x) do {} while (0)
-#define __cpu_to_be32s(x) do {} while (0)
-#define __be32_to_cpus(x) do {} while (0)
-#define __cpu_to_be16s(x) do {} while (0)
-#define __be16_to_cpus(x) do {} while (0)
+
+static inline void __cpu_to_le64s(__u64 *p)
+{
+	__swab64s(x);
+}
+
+static inline void __le64_to_cpus(__u64 *p)
+{
+	__swab64s(x);
+}
+
+static inline void __cpu_to_le32s(__u32 *p)
+{
+	__swab32s(x);
+}
+
+static inline void __le32_to_cpus(__u32 *p)
+{
+	__swab32s(x);
+}
+
+static inline void __cpu_to_le16s(__u16 *p)
+{
+	__swab16s(x);
+}
+
+static inline void __le16_to_cpus(__u16 *p)
+{
+	__swab16s(x);
+}
+
+static inline void __cpu_to_be64s(__u64 *p)
+{}
+
+static inline void __be64_to_cpus(__u64 *p)
+{}
+
+static inline void __cpu_to_be32s(__u32 *p)
+{}
+
+static inline void __be32_to_cpus(__u32 *p)
+{}
+
+static inline void __cpu_to_be16s(__u16 *p)
+{}
+
+static inline void __be16_to_cpus(__u16 *p)
+{}
 
 #ifdef __KERNEL__
 #include <linux/byteorder/generic.h>
diff --git a/include/linux/byteorder/little_endian.h b/include/linux/byteorder/little_endian.h
index 05dc7c3..5d12ff8 100644
--- a/include/linux/byteorder/little_endian.h
+++ b/include/linux/byteorder/little_endian.h
@@ -88,18 +88,54 @@ static inline __u16 __be16_to_cpup(const __be16 *p)
 {
 	return __swab16p((__u16 *)p);
 }
-#define __cpu_to_le64s(x) do {} while (0)
-#define __le64_to_cpus(x) do {} while (0)
-#define __cpu_to_le32s(x) do {} while (0)
-#define __le32_to_cpus(x) do {} while (0)
-#define __cpu_to_le16s(x) do {} while (0)
-#define __le16_to_cpus(x) do {} while (0)
-#define __cpu_to_be64s(x) __swab64s((x))
-#define __be64_to_cpus(x) __swab64s((x))
-#define __cpu_to_be32s(x) __swab32s((x))
-#define __be32_to_cpus(x) __swab32s((x))
-#define __cpu_to_be16s(x) __swab16s((x))
-#define __be16_to_cpus(x) __swab16s((x))
+
+static inline void __cpu_to_le64s(__u64 *p)
+{}
+
+static inline void __le64_to_cpus(__u64 *p)
+{}
+
+static inline void __cpu_to_le32s(__u32 *p)
+{}
+
+static inline void __le32_to_cpus(__u32 *p)
+{}
+
+static inline void __cpu_to_le16s(__u16 *p)
+{}
+
+static inline void __le16_to_cpus(__u16 *p)
+{}
+
+static inline void __cpu_to_be64s(__u64 *p)
+{
+	__swab64s(x);
+}
+
+static inline void __be64_to_cpus(__u64 *p)
+{
+	__swab64s(x);
+}
+
+static inline void __cpu_to_be32s(__u32 *p)
+{
+	__swab32s(x);
+}
+
+static inline void __be32_to_cpus(__u32 *p)
+{
+	__swab32s(x);
+}
+
+static inline void __cpu_to_be16s(__u16 *p)
+{
+	__swab16s(x);
+}
+
+static inline void __be16_to_cpus(__u16 *p)
+{
+	__swab16s(x);
+}
 
 #ifdef __KERNEL__
 #include <linux/byteorder/generic.h>
-- 
1.5.6.4.570.g052e

Re: [PATCH] byteorder: force in-place endian conversion to always evaluate args

[H. Peter Anvin]

Harvey Harrison wrote:
> David Miller reported breakage in ide when the in-place byteorder helpers
> were used as the macros do not always evaluate their args which led to
> an infinite loop.
> 
> Just make them functions to ensure they always do so.

> -#define __cpu_to_be64s(x) do {} while (0)

For what it's worth, the way to write a macro like this:

#define __cpu_to_be64s(x) ((void)(x))

	-hpa

Re: [PATCH] byteorder: force in-place endian conversion to always evaluate args

[Harvey Harrison]

On Fri, 2008-07-25 at 14:14 -0400, H. Peter Anvin wrote:
> Harvey Harrison wrote:
> > David Miller reported breakage in ide when the in-place byteorder helpers
> > were used as the macros do not always evaluate their args which led to
> > an infinite loop.
> > 
> > Just make them functions to ensure they always do so.
> 
> > -#define __cpu_to_be64s(x) do {} while (0)
> 
> For what it's worth, the way to write a macro like this:
> 
> #define __cpu_to_be64s(x) ((void)(x))

If you've looked at the byteorder rework I've done in -mm, these get
unified in a single include/linux/byteorder.h and look like:

static inline void __le16_to_cpus(__u16 *p)
{
#ifdef __BIG_ENDIAN
	__swab16s(p);
#endif
}

static inline void __be16_to_cpus(__u16 *p)
{
#ifdef __LITTLE_ENDIAN
	__swab16s(p);
#endif
}

...etc.

As you can now rely on __BIG/__LITTLE_ENDIAN being set reliably.

Harvey

Re: [PATCH] byteorder: force in-place endian conversion to always evaluate args

[Nish Aravamudan]

On 7/25/08, Harvey Harrison <harvey.harrison@gmail.com> wrote:
> David Miller reported breakage in ide when the in-place byteorder helpers
>  were used as the macros do not always evaluate their args which led to
>  an infinite loop.
>
>  Just make them functions to ensure they always do so.
>
>  Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
>  ---
>   include/linux/byteorder/big_endian.h    |   60 ++++++++++++++++++++++++------
>   include/linux/byteorder/little_endian.h |   60 ++++++++++++++++++++++++------
>   2 files changed, 96 insertions(+), 24 deletions(-)
>
>  diff --git a/include/linux/byteorder/big_endian.h b/include/linux/byteorder/big_endian.h
>  index 961ed4b..b53ccd0 100644
>  --- a/include/linux/byteorder/big_endian.h
>  +++ b/include/linux/byteorder/big_endian.h
>  @@ -88,18 +88,54 @@ static inline __u16 __be16_to_cpup(const __be16 *p)
>   {
>         return (__force __u16)*p;
>   }
>  -#define __cpu_to_le64s(x) __swab64s((x))
>  -#define __le64_to_cpus(x) __swab64s((x))
>  -#define __cpu_to_le32s(x) __swab32s((x))
>  -#define __le32_to_cpus(x) __swab32s((x))
>  -#define __cpu_to_le16s(x) __swab16s((x))
>  -#define __le16_to_cpus(x) __swab16s((x))
>  -#define __cpu_to_be64s(x) do {} while (0)
>  -#define __be64_to_cpus(x) do {} while (0)
>  -#define __cpu_to_be32s(x) do {} while (0)
>  -#define __be32_to_cpus(x) do {} while (0)
>  -#define __cpu_to_be16s(x) do {} while (0)
>  -#define __be16_to_cpus(x) do {} while (0)
>  +
>  +static inline void __cpu_to_le64s(__u64 *p)
>  +{
>  +       __swab64s(x);
>  +}

Shouldn't all the x's in the function bodies be p's? And I thought
David already posted a macro version of this change along the lines of
hpa's reply?

Thanks,
Nish

Re: [PATCH] byteorder: force in-place endian conversion to always evaluate args

[Harvey Harrison]

On Fri, 2008-07-25 at 11:22 -0700, Nish Aravamudan wrote:
> On 7/25/08, Harvey Harrison <harvey.harrison@gmail.com> wrote:
> > David Miller reported breakage in ide when the in-place byteorder helpers
> >  were used as the macros do not always evaluate their args which led to
> >  an infinite loop.
> >
> >  Just make them functions to ensure they always do so.
> >
> >  Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> >  ---
> >   include/linux/byteorder/big_endian.h    |   60 ++++++++++++++++++++++++------
> >   include/linux/byteorder/little_endian.h |   60 ++++++++++++++++++++++++------
> >   2 files changed, 96 insertions(+), 24 deletions(-)
> >
> >  diff --git a/include/linux/byteorder/big_endian.h b/include/linux/byteorder/big_endian.h
> >  index 961ed4b..b53ccd0 100644
> >  --- a/include/linux/byteorder/big_endian.h
> >  +++ b/include/linux/byteorder/big_endian.h
> >  @@ -88,18 +88,54 @@ static inline __u16 __be16_to_cpup(const __be16 *p)
> >   {
> >         return (__force __u16)*p;
> >   }
> >  -#define __cpu_to_le64s(x) __swab64s((x))
> >  -#define __le64_to_cpus(x) __swab64s((x))
> >  -#define __cpu_to_le32s(x) __swab32s((x))
> >  -#define __le32_to_cpus(x) __swab32s((x))
> >  -#define __cpu_to_le16s(x) __swab16s((x))
> >  -#define __le16_to_cpus(x) __swab16s((x))
> >  -#define __cpu_to_be64s(x) do {} while (0)
> >  -#define __be64_to_cpus(x) do {} while (0)
> >  -#define __cpu_to_be32s(x) do {} while (0)
> >  -#define __be32_to_cpus(x) do {} while (0)
> >  -#define __cpu_to_be16s(x) do {} while (0)
> >  -#define __be16_to_cpus(x) do {} while (0)
> >  +
> >  +static inline void __cpu_to_le64s(__u64 *p)
> >  +{
> >  +       __swab64s(x);
> >  +}
> 
> Shouldn't all the x's in the function bodies be p's? And I thought
> David already posted a macro version of this change along the lines of
> hpa's reply?

*sigh*

Yes they should, I sent the wrong mbox after this failed to compile
and fixed it.

Cheers,

Harvey

Re: [PATCH] byteorder: force in-place endian conversion to always evaluate args

[H. Peter Anvin]

Harvey Harrison wrote:
>> For what it's worth, the way to write a macro like this:
>>
>> #define __cpu_to_be64s(x) ((void)(x))
> 
> If you've looked at the byteorder rework I've done in -mm, these get
> unified in a single include/linux/byteorder.h and look like:

[inline functions]

What I meant with the above, was that it seems poorly understood how to 
write an empty "function-like" macro.

Obviously, inline functions don't have this problem, and is generally 
better anyway (type-safe even in the empty condition, for example.)

	-hpa

Re: [PATCH] byteorder: force in-place endian conversion to always evaluate args

[Christoph Hellwig]

On Fri, Jul 25, 2008 at 09:33:41AM -0700, Harvey Harrison wrote:
> David Miller reported breakage in ide when the in-place byteorder helpers
> were used as the macros do not always evaluate their args which led to
> an infinite loop.
> 
> Just make them functions to ensure they always do so.

Of course the best thing would be to kill these macros entirely.
In-place endianess conversions are bad idea.

Re: [PATCH] byteorder: force in-place endian conversion to always evaluate args

[Harvey Harrison]

On Sat, 2008-07-26 at 01:34 -0400, Christoph Hellwig wrote:
> On Fri, Jul 25, 2008 at 09:33:41AM -0700, Harvey Harrison wrote:
> > David Miller reported breakage in ide when the in-place byteorder helpers
> > were used as the macros do not always evaluate their args which led to
> > an infinite loop.
> > 
> > Just make them functions to ensure they always do so.
> 
> Of course the best thing would be to kill these macros entirely.
> In-place endianess conversions are bad idea.
> 

It's better than seeing this:
	some_u32 = cpu_to_le32(some_u32);

But agreed, the 's' versions of the byteswapping api are a pretty good
sign something could be done better.

Harvey