Re: powerpc: introduce asm/swab.h

Re: powerpc: introduce asm/swab.h

[Benjamin Herrenschmidt]

On Wed, 2009-01-07 at 04:00 +0000, Linux Kernel Mailing List wrote:
> Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=156ca2bbf6503a02d7d6829886ce381d572de66e
> Commit:     156ca2bbf6503a02d7d6829886ce381d572de66e
> Parent:     8cdd3a9261e8efe36aeb6c708edb76d7e2b5d13f
> Author:     Harvey Harrison <harvey.harrison@gmail.com>
> AuthorDate: Tue Jan 6 14:56:23 2009 -0800
> Committer:  Linus Torvalds <torvalds@linux-foundation.org>
> CommitDate: Tue Jan 6 18:10:27 2009 -0800
> 
>     powerpc: introduce asm/swab.h
>     
>     Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Was this tested ? I see no arch maintainer signed-off here, it appears
to at least break ppc32, and contains hunks that paulus says were
explicitely nacked (removing of our ld_* macros) etc...

Linus, please revert.

Cheers,
Ben.

Re: powerpc: introduce asm/swab.h

[Harvey Harrison]

On Wed, 2009-01-07 at 15:42 +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2009-01-07 at 04:00 +0000, Linux Kernel Mailing List wrote:
> > Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=156ca2bbf6503a02d7d6829886ce381d572de66e
> > Commit:     156ca2bbf6503a02d7d6829886ce381d572de66e
> > Parent:     8cdd3a9261e8efe36aeb6c708edb76d7e2b5d13f
> > Author:     Harvey Harrison <harvey.harrison@gmail.com>
> > AuthorDate: Tue Jan 6 14:56:23 2009 -0800
> > Committer:  Linus Torvalds <torvalds@linux-foundation.org>
> > CommitDate: Tue Jan 6 18:10:27 2009 -0800
> > 
> >     powerpc: introduce asm/swab.h
> >     
> >     Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> >     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> Was this tested ? I see no arch maintainer signed-off here, it appears
> to at least break ppc32, and contains hunks that paulus says were
> explicitely nacked (removing of our ld_* macros) etc...
> 
> Linus, please revert.

Please look a bit closer, it's a pure movement of code out of byteorder.h into swab.h,
no changes whatsoever.

Cheers,

Harvey

Re: powerpc: introduce asm/swab.h

[Nicolas Pitre]

On Tue, 6 Jan 2009, Harvey Harrison wrote:

> On Wed, 2009-01-07 at 15:42 +1100, Benjamin Herrenschmidt wrote:
> > On Wed, 2009-01-07 at 04:00 +0000, Linux Kernel Mailing List wrote:
> > > Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=156ca2bbf6503a02d7d6829886ce381d572de66e
> > > Commit:     156ca2bbf6503a02d7d6829886ce381d572de66e
> > > Parent:     8cdd3a9261e8efe36aeb6c708edb76d7e2b5d13f
> > > Author:     Harvey Harrison <harvey.harrison@gmail.com>
> > > AuthorDate: Tue Jan 6 14:56:23 2009 -0800
> > > Committer:  Linus Torvalds <torvalds@linux-foundation.org>
> > > CommitDate: Tue Jan 6 18:10:27 2009 -0800
> > > 
> > >     powerpc: introduce asm/swab.h
> > >     
> > >     Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > >     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > 
> > Was this tested ? I see no arch maintainer signed-off here, it appears
> > to at least break ppc32, and contains hunks that paulus says were
> > explicitely nacked (removing of our ld_* macros) etc...
> > 
> > Linus, please revert.
> 
> Please look a bit closer, it's a pure movement of code out of byteorder.h into swab.h,
> no changes whatsoever.

Well, this series breaks ARM as well:

In file included from include/linux/byteorder/little_endian.h:12,
                 from arch/arm/include/asm/byteorder.h:23,
                 from include/linux/kernel.h:20,
                 from include/linux/sched.h:52,
                 from arch/arm/kernel/asm-offsets.c:13:
include/linux/swab.h: In function '__fswab64':
include/linux/swab.h:71: error: implicit declaration of function '___swab32'


Nicolas

Re: powerpc: introduce asm/swab.h

[Linus Torvalds]

On Tue, 6 Jan 2009, Nicolas Pitre wrote:
> 
> Well, this series breaks ARM as well:

Ahh. I think it's the __SWAB_64_THRU_32__ case that is broken. 

Does this fix things? Totally untested. Of course.

		Linus
---
 include/linux/swab.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/swab.h b/include/linux/swab.h
index 9a2d33e..be5284d 100644
--- a/include/linux/swab.h
+++ b/include/linux/swab.h
@@ -68,7 +68,7 @@ static inline __attribute_const__ __u64 __fswab64(__u64 val)
 #elif defined(__SWAB_64_THRU_32__)
 	__u32 h = val >> 32;
 	__u32 l = val & ((1ULL << 32) - 1);
-	return (((__u64)___swab32(l)) << 32) | ((__u64)(___swab32(h)));
+	return (((__u64)__fswab32(l)) << 32) | ((__u64)(__fswab32(h)));
 #else
 	return ___constant_swab64(val);
 #endif

Re: powerpc: introduce asm/swab.h

[Nicolas Pitre]

On Tue, 6 Jan 2009, Linus Torvalds wrote:

> 
> 
> On Tue, 6 Jan 2009, Nicolas Pitre wrote:
> > 
> > Well, this series breaks ARM as well:
> 
> Ahh. I think it's the __SWAB_64_THRU_32__ case that is broken. 
> 
> Does this fix things? Totally untested. Of course.

Yep.


> 
> 		Linus
> ---
>  include/linux/swab.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/swab.h b/include/linux/swab.h
> index 9a2d33e..be5284d 100644
> --- a/include/linux/swab.h
> +++ b/include/linux/swab.h
> @@ -68,7 +68,7 @@ static inline __attribute_const__ __u64 __fswab64(__u64 val)
>  #elif defined(__SWAB_64_THRU_32__)
>  	__u32 h = val >> 32;
>  	__u32 l = val & ((1ULL << 32) - 1);
> -	return (((__u64)___swab32(l)) << 32) | ((__u64)(___swab32(h)));
> +	return (((__u64)__fswab32(l)) << 32) | ((__u64)(__fswab32(h)));
>  #else
>  	return ___constant_swab64(val);
>  #endif
> 

Re: powerpc: introduce asm/swab.h

[Linus Torvalds]

On Wed, 7 Jan 2009, Benjamin Herrenschmidt wrote:
> 
> Was this tested ? I see no arch maintainer signed-off here, it appears
> to at least break ppc32, and contains hunks that paulus says were
> explicitely nacked (removing of our ld_* macros) etc...

It did no such thing. It just moved the file.

Use

	git show -M -B 156ca2bbf6503a02d7d6829886ce381d572de66e

to see how <asm/swab.h> is just the old <asm/byteorder.h> renamed, and 
with some stuff moved around. Eg it contains:

	-#define __BIG_ENDIAN
	-#include <linux/byteorder.h>

because now the <asm/byteorder.h> header file on powerpc will just do

	+#include <asm/swab.h>
	+#include <linux/byteorder/big_endian.h>

instead of doing that __BIG_ENDIAN define and conditionals that didn't 
work with user space.

So it really should be a no-op. And no, I'm not reverting it, because 
I guarantee that reverting it won't result in a working build - the broken 
<linux/byteorder.h> file no longer exists (and will not be re-instated).

So I'd suggest testing it, and if it really doesn't work, trying to figure 
out why. Because a revert won't be helping.

			Linus

Re: powerpc: introduce asm/swab.h

[Nicolas Pitre]

On Tue, 6 Jan 2009, Linus Torvalds wrote:

> 
> 
> On Wed, 7 Jan 2009, Benjamin Herrenschmidt wrote:
> > 
> > Was this tested ? I see no arch maintainer signed-off here, it appears
> > to at least break ppc32, and contains hunks that paulus says were
> > explicitely nacked (removing of our ld_* macros) etc...
> 
> It did no such thing. It just moved the file.
> 
> Use
> 
> 	git show -M -B 156ca2bbf6503a02d7d6829886ce381d572de66e
> 
> to see how <asm/swab.h> is just the old <asm/byteorder.h> renamed, and 
> with some stuff moved around.

I suspect that all archs that define __SWAB_64_THRU_32__ are broken atm.

According to "git grep ___swab32", there is no more definition for 
___swab32 anywhere.


Nicolas

Re: powerpc: introduce asm/swab.h

[Benjamin Herrenschmidt]

On Wed, 2009-01-07 at 15:42 +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2009-01-07 at 04:00 +0000, Linux Kernel Mailing List wrote:
> > Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=156ca2bbf6503a02d7d6829886ce381d572de66e
> > Commit:     156ca2bbf6503a02d7d6829886ce381d572de66e
> > Parent:     8cdd3a9261e8efe36aeb6c708edb76d7e2b5d13f
> > Author:     Harvey Harrison <harvey.harrison@gmail.com>
> > AuthorDate: Tue Jan 6 14:56:23 2009 -0800
> > Committer:  Linus Torvalds <torvalds@linux-foundation.org>
> > CommitDate: Tue Jan 6 18:10:27 2009 -0800
> > 
> >     powerpc: introduce asm/swab.h
> >     
> >     Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> >     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> Was this tested ? I see no arch maintainer signed-off here, it appears
> to at least break ppc32, and contains hunks that paulus says were
> explicitely nacked (removing of our ld_* macros) etc...
> 
> Linus, please revert.

More details:

include/linux/swab.h: In function ‘__fswab64’:
include/linux/swab.h:71: error: implicit declaration of function ‘___swab32’
make[1]: *** [arch/powerpc/kernel/asm-offsets.s] Error 1
make: *** [prepare0] Error 2

I don't see any place where ___swab32() is defined (with 3 underscores).

Even if fixing that to use __swab32() instead, then it fails because
this is defined after it's used. I worked around it using fswab in
there instead.

This tentative patches fixes that too, but I haven't followed the whole
discussion closely, so I can't tell whether it's fully in the original
intend. In any case, it makes ppc32 build again (and probably others).

byteorder: Fix a couple of bugs in the new include/linux/swab.h

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

diff --git a/include/linux/swab.h b/include/linux/swab.h
index 9a2d33e..351c456 100644
--- a/include/linux/swab.h
+++ b/include/linux/swab.h
@@ -40,7 +40,7 @@
 /*
  * Implement the following as inlines, but define the interface using
  * macros to allow constant folding when possible:
- * ___swab16, ___swab32, ___swab64, ___swahw32, ___swahb32
+ * __swab16, __swab32, __swab64, __swahw32, __swahb32
  */
 
 static inline __attribute_const__ __u16 __fswab16(__u16 val)
@@ -68,7 +68,7 @@ static inline __attribute_const__ __u64 __fswab64(__u64 val)
 #elif defined(__SWAB_64_THRU_32__)
 	__u32 h = val >> 32;
 	__u32 l = val & ((1ULL << 32) - 1);
-	return (((__u64)___swab32(l)) << 32) | ((__u64)(___swab32(h)));
+	return (((__u64)__fswab32(l)) << 32) | ((__u64)(__fswab32(h)));
 #else
 	return ___constant_swab64(val);
 #endif

Re: powerpc: introduce asm/swab.h

[Linus Torvalds]

On Wed, 7 Jan 2009, Benjamin Herrenschmidt wrote:
> 
> Even if fixing that to use __swab32() instead, then it fails because
> this is defined after it's used. I worked around it using fswab in
> there instead.

That's the same patch I just sent out (but you changed comments too), so I 
obviously agree. 

x86 didn't see this (even in 32-bit mode) because it doesn't use that odd 
__SWAB_64_THRU_32__ case, but does the 64-bit swab with native code.

Can you also verify that it works for you (not just compiles), just so 
that I can commit it?

			Linus

Re: powerpc: introduce asm/swab.h

[Nicolas Pitre]

On Tue, 6 Jan 2009, Linus Torvalds wrote:

> 
> 
> On Wed, 7 Jan 2009, Benjamin Herrenschmidt wrote:
> > 
> > Even if fixing that to use __swab32() instead, then it fails because
> > this is defined after it's used. I worked around it using fswab in
> > there instead.
> 
> That's the same patch I just sent out (but you changed comments too), so I 
> obviously agree. 
> 
> x86 didn't see this (even in 32-bit mode) because it doesn't use that odd 
> __SWAB_64_THRU_32__ case, but does the 64-bit swab with native code.
> 
> Can you also verify that it works for you (not just compiles), just so 
> that I can commit it?

Tested OK on ARM (using LE mode with networking, etc.)


Nicolas

Re: powerpc: introduce asm/swab.h

[Nicolas Pitre]

On Wed, 7 Jan 2009, Nicolas Pitre wrote:

> On Tue, 6 Jan 2009, Linus Torvalds wrote:
> 
> > 
> > 
> > On Wed, 7 Jan 2009, Benjamin Herrenschmidt wrote:
> > > 
> > > Even if fixing that to use __swab32() instead, then it fails because
> > > this is defined after it's used. I worked around it using fswab in
> > > there instead.
> > 
> > That's the same patch I just sent out (but you changed comments too), so I 
> > obviously agree. 
> > 
> > x86 didn't see this (even in 32-bit mode) because it doesn't use that odd 
> > __SWAB_64_THRU_32__ case, but does the 64-bit swab with native code.
> > 
> > Can you also verify that it works for you (not just compiles), just so 
> > that I can commit it?
> 
> Tested OK on ARM (using LE mode with networking, etc.)

Well, given that I have no way to test any code path including 
be64_to_cpu() and its opposite, my testing is only valid for 
compilation issues.


Nicolas

Re: powerpc: introduce asm/swab.h

[Linus Torvalds]

On Wed, 7 Jan 2009, Nicolas Pitre wrote:
> 
> Well, given that I have no way to test any code path including 
> be64_to_cpu() and its opposite, my testing is only valid for 
> compilation issues.

Thinking about it, there's probably not a whole lot of users. If any.

So if it compiles, I guess it's good ;)

		Linus

Re: powerpc: introduce asm/swab.h

[Linus Torvalds]

On Wed, 7 Jan 2009, Nicolas Pitre wrote:

> On Tue, 6 Jan 2009, Linus Torvalds wrote:
> > On Wed, 7 Jan 2009, Benjamin Herrenschmidt wrote:
> > 
> > Can you also verify that it works for you (not just compiles), just so 
> > that I can commit it?
> 
> Tested OK on ARM (using LE mode with networking, etc.)

Ok, I committed it as a quick-fix. I'm not sure that is necessarily the 
final one, but at least it is better than not compiling.

For example, it's kind of silly to use two __fswab32()'s with other 
oddness if that one just falls back on __constant_swab32: maybe we'd want 
to make sure that we'd use ___constant_swab64() in that case, and only do 
the whole __SWAB_64_THRU_32__ if we really have a __arch_swab32() 
function.

Of course, I do hope that anybody who #defines __SWAB_64_THRU_32__ already 
has that __arch_swab32() thing, so it's likely fine.

I also wonder whether gcc generates better code with a union than with 
that 64-bit math...

			Linus

Re: powerpc: introduce asm/swab.h

[Harvey Harrison]

On Tue, 2009-01-06 at 21:37 -0800, Linus Torvalds wrote:
> 
> On Wed, 7 Jan 2009, Nicolas Pitre wrote:
> 
> > On Tue, 6 Jan 2009, Linus Torvalds wrote:
> > > On Wed, 7 Jan 2009, Benjamin Herrenschmidt wrote:
> > > 
> > > Can you also verify that it works for you (not just compiles), just so 
> > > that I can commit it?
> > 
> > Tested OK on ARM (using LE mode with networking, etc.)
> 
> Ok, I committed it as a quick-fix. I'm not sure that is necessarily the 
> final one, but at least it is better than not compiling.

Yes, your fix should be all that is needed to fix it.  My apologies for not
rerunning all the compile tests (only did x86 but did a compile test with
x86 faking itself as a big endian arch)

> 
> For example, it's kind of silly to use two __fswab32()'s with other 
> oddness if that one just falls back on __constant_swab32: maybe we'd want 
> to make sure that we'd use ___constant_swab64() in that case, and only do 
> the whole __SWAB_64_THRU_32__ if we really have a __arch_swab32() 
> function.
> 
> Of course, I do hope that anybody who #defines __SWAB_64_THRU_32__ already 
> has that __arch_swab32() thing, so it's likely fine.

Hmm, this is a direct holdover from the old swab.h/swabb.h pieces.  I
came this close to eliminating that case too, but didn't have disassembly
to justify it.  You're right I think that changing that to:

#elif defined(__SWAB_64_THRU_32__) && defined(__arch_swab32)

makes a lot of sense, but I'd like to here from some arch guys.

The following arches define __SWAB_64_THRU_32__ and have __arch_swab32
arm, blackfin, avr32, cris, m68knommu, m68k, mips, sh, xtensa

And the following define __SWAB_64_THRU_32__ and don't have __arch_swab32
h8300
s390 (conditional on _ifndef __s390x__)
sparc (32-bit only)
frv
m32r
mn10300

Cheers,

Harvey

Re: powerpc: introduce asm/swab.h

[Benjamin Herrenschmidt]

> Ok, I committed it as a quick-fix. I'm not sure that is necessarily the 
> final one, but at least it is better than not compiling.
> 
> For example, it's kind of silly to use two __fswab32()'s with other 
> oddness if that one just falls back on __constant_swab32: maybe we'd want 
> to make sure that we'd use ___constant_swab64() in that case, and only do 
> the whole __SWAB_64_THRU_32__ if we really have a __arch_swab32() 
> function.
> 
> Of course, I do hope that anybody who #defines __SWAB_64_THRU_32__ already 
> has that __arch_swab32() thing, so it's likely fine.
> 
> I also wonder whether gcc generates better code with a union than with 
> that 64-bit math...

Allright, it boots here on a powerbook, though IDE seems to be
busticated (it gets lost interrupts trying to enable DMA, though it does
fallback properly to PIO), but I think that's unrelated. I'll have a
closer look tomorrow.

Cheers,
Ben.

Re: powerpc: introduce asm/swab.h

[Steven Rostedt]

On Tue, Jan 06, 2009 at 09:11:42PM -0800, Linus Torvalds wrote:
> 
> 
> On Wed, 7 Jan 2009, Benjamin Herrenschmidt wrote:
> > 
> > Even if fixing that to use __swab32() instead, then it fails because
> > this is defined after it's used. I worked around it using fswab in
> > there instead.
> 
> That's the same patch I just sent out (but you changed comments too), so I 
> obviously agree. 
> 
> x86 didn't see this (even in 32-bit mode) because it doesn't use that odd 
> __SWAB_64_THRU_32__ case, but does the 64-bit swab with native code.
> 
> Can you also verify that it works for you (not just compiles), just so 
> that I can commit it?

I'm the one that originally told Ben about this breaking the PPC32 build.
His patch compiles on my PPC32 box. I just rebooted with the new kernel
and it boots.

Tested-by: Steven Rostedt <srostedt@redhat.com>

-- Steve

Re: powerpc: introduce asm/swab.h

[Benjamin Herrenschmidt]

On Tue, 2009-01-06 at 21:11 -0800, Linus Torvalds wrote:
> That's the same patch I just sent out (but you changed comments too), so I 
> obviously agree. 
> 
> x86 didn't see this (even in 32-bit mode) because it doesn't use that odd 
> __SWAB_64_THRU_32__ case, but does the 64-bit swab with native code.
> 
> Can you also verify that it works for you (not just compiles), just so 
> that I can commit it?

I'll try asap, just had to go home, and I have less build horsepower
here ;-)

Cheers,
Ben.

Re: powerpc: introduce asm/swab.h

[Geert Uytterhoeven]

On Wed, 7 Jan 2009, Benjamin Herrenschmidt wrote:
> On Wed, 2009-01-07 at 04:00 +0000, Linux Kernel Mailing List wrote:
> > Gitweb:     http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=156ca2bbf6503a02d7d6829886ce381d572de66e
> > Commit:     156ca2bbf6503a02d7d6829886ce381d572de66e
> > Parent:     8cdd3a9261e8efe36aeb6c708edb76d7e2b5d13f
> > Author:     Harvey Harrison <harvey.harrison@gmail.com>
> > AuthorDate: Tue Jan 6 14:56:23 2009 -0800
> > Committer:  Linus Torvalds <torvalds@linux-foundation.org>
> > CommitDate: Tue Jan 6 18:10:27 2009 -0800
> > 
> >     powerpc: introduce asm/swab.h
> >     
> >     Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> >     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> Was this tested ? I see no arch maintainer signed-off here, it appears
> to at least break ppc32, and contains hunks that paulus says were
> explicitely nacked (removing of our ld_* macros) etc...

And m68k (sun3, why not the others/):
http://kisskb.ellerman.id.au/kisskb/buildresult/66040/

Why hasn't this been in linux-next?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds