Re: [PATCH 19/22] x86: use the new byteorder headers

Re: [PATCH 19/22] x86: use the new byteorder headers

[Andrew Morton]

On Tue, 12 Aug 2008 17:27:14 -0700 Harvey Harrison <harvey.harrison@gmail.com> wrote:

> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> ---
> The prerequisite patches have landed in Linus' tree now.
> 
>  include/asm-x86/byteorder.h |   69 +++++++++++++++++--------------------------
>  1 files changed, 27 insertions(+), 42 deletions(-)
> 
> diff --git a/include/asm-x86/byteorder.h b/include/asm-x86/byteorder.h
> index e02ae2d..7187b3a 100644
> --- a/include/asm-x86/byteorder.h
> +++ b/include/asm-x86/byteorder.h
> @@ -4,26 +4,33 @@
>  #include <asm/types.h>
>  #include <linux/compiler.h>
>  
> -#ifdef __GNUC__
> +#define __LITTLE_ENDIAN
>  
> -#ifdef __i386__
> -
> -static inline __attribute_const__ __u32 ___arch__swab32(__u32 x)
> +static inline __attribute_const__ __u32 __arch_swab32(__u32 val)
>  {
> -#ifdef CONFIG_X86_BSWAP
> -	asm("bswap %0" : "=r" (x) : "0" (x));
> -#else
> +#ifdef __i386__
> +# ifdef CONFIG_X86_BSWAP
> +	asm("bswap %0" : "=r" (val) : "0" (val));
> +# else
>  	asm("xchgb %b0,%h0\n\t"	/* swap lower bytes	*/
>  	    "rorl $16,%0\n\t"	/* swap words		*/
>  	    "xchgb %b0,%h0"	/* swap higher bytes	*/
> -	    : "=q" (x)
> -	    : "0" (x));
> +	    : "=q" (val)
> +	    : "0" (val));
> +# endif
> +
> +#else /* __i386__ */
> +	asm("bswapl %0"
> +	    : "=r" (val)
> +	    : "0" (val));
>  #endif
> -	return x;
> +	return val;
>  }
> +#define __arch_swab32 __arch_swab32
>  
> -static inline __attribute_const__ __u64 ___arch__swab64(__u64 val)
> +static inline __attribute_const__ __u64 __arch_swab64(__u64 val)
>  {
> +#ifdef __i386__
>  	union {
>  		struct {
>  			__u32 a;
> @@ -32,50 +39,28 @@ static inline __attribute_const__ __u64 ___arch__swab64(__u64 val)
>  		__u64 u;
>  	} v;
>  	v.u = val;
> -#ifdef CONFIG_X86_BSWAP
> +# ifdef CONFIG_X86_BSWAP
>  	asm("bswapl %0 ; bswapl %1 ; xchgl %0,%1"
>  	    : "=r" (v.s.a), "=r" (v.s.b)
>  	    : "0" (v.s.a), "1" (v.s.b));
> -#else
> +# else
>  	v.s.a = ___arch__swab32(v.s.a);
>  	v.s.b = ___arch__swab32(v.s.b);
>  	asm("xchgl %0,%1"
>  	    : "=r" (v.s.a), "=r" (v.s.b)
>  	    : "0" (v.s.a), "1" (v.s.b));
> -#endif
> +# endif
>  	return v.u;
> -}
>  
>  #else /* __i386__ */
> -
> -static inline __attribute_const__ __u64 ___arch__swab64(__u64 x)
> -{
>  	asm("bswapq %0"
> -	    : "=r" (x)
> -	    : "0" (x));
> -	return x;
> -}
> -
> -static inline __attribute_const__ __u32 ___arch__swab32(__u32 x)
> -{
> -	asm("bswapl %0"
> -	    : "=r" (x)
> -	    : "0" (x));
> -	return x;
> -}
> -
> +	    : "=r" (val)
> +	    : "0" (val));
> +	return val;
>  #endif
> +}
> +#define __arch_swab64 __arch_swab64
>  
> -/* Do not define swab16.  Gcc is smart enough to recognize "C" version and
> -   convert it into rotation or exhange.  */
> -
> -#define __arch__swab64(x) ___arch__swab64(x)
> -#define __arch__swab32(x) ___arch__swab32(x)
> -
> -#define __BYTEORDER_HAS_U64__
> -
> -#endif /* __GNUC__ */
> -
> -#include <linux/byteorder/little_endian.h>
> +#include <linux/byteorder.h>
>  
>  #endif /* _ASM_X86_BYTEORDER_H */

net/tipc/subscr.c: In function 'htohl':
net/tipc/subscr.c:88: error: implicit declaration of function '___constant_swab32'

That's just a basic i386 allmodconfig.  How come you're not seeing this?

Heaven alone knows what tipc thinks it's doing poking into this sort of
thing.  Could you please take a look and teach it some manners?

Thanks.

[PATCH] tipc: Use the appropriate swab helper rather than an internal helper

[Harvey Harrison]

___generally means I am an internal implementation detail, don't use
me.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
Sorry for breaking the build Andrew, my testing has been with all the __constant
versions in-tree removed.

 net/tipc/subscr.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
index 0326d30..f5da05d 100644
--- a/net/tipc/subscr.c
+++ b/net/tipc/subscr.c
@@ -85,7 +85,7 @@ static struct top_srv topsrv = { 0 };
 
 static u32 htohl(u32 in, int swap)
 {
-	return swap ? (u32)___constant_swab32(in) : in;
+	return swap ? (u32)swab32(in) : in;
 }
 
 /**
-- 
1.6.0.rc2.233.g3cb9d

Re: [PATCH] tipc: Use the appropriate swab helper rather than an internal helper

[Andrew Morton]

On Tue, 12 Aug 2008 21:05:47 -0700 Harvey Harrison <harvey.harrison@gmail.com> wrote:

> ___generally means I am an internal implementation detail, don't use
> me.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> ---
> Sorry for breaking the build Andrew, my testing has been with all the __constant
> versions in-tree removed.
> 
>  net/tipc/subscr.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
> index 0326d30..f5da05d 100644
> --- a/net/tipc/subscr.c
> +++ b/net/tipc/subscr.c
> @@ -85,7 +85,7 @@ static struct top_srv topsrv = { 0 };
>  
>  static u32 htohl(u32 in, int swap)
>  {
> -	return swap ? (u32)___constant_swab32(in) : in;
> +	return swap ? (u32)swab32(in) : in;
>  }

yes, that's what I did too, only I dropped the
seems-to-me-to-be-unneeded (u32) cast.

Re: [PATCH] tipc: Use the appropriate swab helper rather than an internal helper

[Harvey Harrison]

On Tue, 2008-08-12 at 21:20 -0700, Andrew Morton wrote:
> On Tue, 12 Aug 2008 21:05:47 -0700 Harvey Harrison <harvey.harrison@gmail.com> wrote:
> 
> > ___generally means I am an internal implementation detail, don't use
> > me.
> > 
> > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > ---
> > Sorry for breaking the build Andrew, my testing has been with all the __constant
> > versions in-tree removed.
> > 
> >  net/tipc/subscr.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
> > index 0326d30..f5da05d 100644
> > --- a/net/tipc/subscr.c
> > +++ b/net/tipc/subscr.c
> > @@ -85,7 +85,7 @@ static struct top_srv topsrv = { 0 };
> >  
> >  static u32 htohl(u32 in, int swap)
> >  {
> > -	return swap ? (u32)___constant_swab32(in) : in;
> > +	return swap ? (u32)swab32(in) : in;
> >  }
> 
> yes, that's what I did too, only I dropped the
> seems-to-me-to-be-unneeded (u32) cast.

I made the mistake of folding this bit into the series that removes all
users of the __constant_ helpers as I forgot it was a build breaker.
That's why I wasn't seeing it here.

Harvey

Re: [PATCH] tipc: Use the appropriate swab helper rather than an internal helper

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Tue, 12 Aug 2008 21:20:17 -0700

> On Tue, 12 Aug 2008 21:05:47 -0700 Harvey Harrison <harvey.harrison@gmail.com> wrote:
> 
> > ___generally means I am an internal implementation detail, don't use
> > me.
> > 
> > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > ---
> > Sorry for breaking the build Andrew, my testing has been with all the __constant
> > versions in-tree removed.
> > 
> >  net/tipc/subscr.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
> > index 0326d30..f5da05d 100644
> > --- a/net/tipc/subscr.c
> > +++ b/net/tipc/subscr.c
> > @@ -85,7 +85,7 @@ static struct top_srv topsrv = { 0 };
> >  
> >  static u32 htohl(u32 in, int swap)
> >  {
> > -	return swap ? (u32)___constant_swab32(in) : in;
> > +	return swap ? (u32)swab32(in) : in;
> >  }
> 
> yes, that's what I did too, only I dropped the
> seems-to-me-to-be-unneeded (u32) cast.

I've applied Andrew's patch.