[PATCH] powerpc/align: Use #ifdef __BIG_ENDIAN__ #else for REG_BYTE

[PATCH] powerpc/align: Use #ifdef __BIG_ENDIAN__ #else for REG_BYTE

[Michael Ellerman]

From: Daniel Axtens <dja@axtens.net>

Sparse complains that it doesn't know what REG_BYTE is:

  arch/powerpc/kernel/align.c:313:29: error: undefined identifier 'REG_BYTE'

REG_BYTE is defined differently based on whether we're compiling for
LE, BE32 or BE64. Sparse apparently doesn't provide __BIG_ENDIAN__ or
__LITTLE_ENDIAN__, which means we get no definition.

Rather than check for __BIG_ENDIAN__ and then separately for
__LITTLE_ENDIAN__, just switch the #ifdef to check for __BIG_ENDIAN__
and then #else we define the little endian version. Technically that's
dicey because PDP_ENDIAN is also a possibility, but we already do it in
a lot of places so one more hardly matters.

Signed-off-by: Daniel Axtens <dja@axtens.net>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/kernel/align.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
index 8e7cb8e2b21a..d7ad66bc5bdf 100644
--- a/arch/powerpc/kernel/align.c
+++ b/arch/powerpc/kernel/align.c
@@ -228,9 +228,7 @@ static int emulate_dcbz(struct pt_regs *regs, unsigned char __user *addr)
 #else
 #define REG_BYTE(rp, i)		*((u8 *)(rp) + (i))
 #endif
-#endif
-
-#ifdef __LITTLE_ENDIAN__
+#else
 #define REG_BYTE(rp, i)		(*(((u8 *)((rp) + ((i)>>2)) + ((i)&3))))
 #endif
 
-- 
2.5.0

Re: [PATCH] powerpc/align: Use #ifdef __BIG_ENDIAN__ #else for REG_BYTE

[Arnd Bergmann]

On Thursday, June 16, 2016 10:33:41 PM CEST Michael Ellerman wrote:
> From: Daniel Axtens <dja@axtens.net>
> 
> Sparse complains that it doesn't know what REG_BYTE is:
> 
>   arch/powerpc/kernel/align.c:313:29: error: undefined identifier 'REG_BYTE'
> 
> REG_BYTE is defined differently based on whether we're compiling for
> LE, BE32 or BE64. Sparse apparently doesn't provide __BIG_ENDIAN__ or
> __LITTLE_ENDIAN__, which means we get no definition.
> 
> Rather than check for __BIG_ENDIAN__ and then separately for
> __LITTLE_ENDIAN__, just switch the #ifdef to check for __BIG_ENDIAN__
> and then #else we define the little endian version. Technically that's
> dicey because PDP_ENDIAN is also a possibility, but we already do it in
> a lot of places so one more hardly matters.
> 
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> 

That makes the code less robust for the cases that accidentally we
have neither or both of __LITTLE_ENDIAN__/__BIG_ENDIAN__ set during
an actual compilation.

It would be better to fix the sparse compilation so the same endianess
is set that you get when calling gcc.

	Arnd

Re: [PATCH] powerpc/align: Use #ifdef __BIG_ENDIAN__ #else for REG_BYTE

[Daniel Axtens]

[-- Attachment #1: Type: text/plain, Size: 334 bytes --]

> It would be better to fix the sparse compilation so the same endianess
> is set that you get when calling gcc.

I will definitely work on a patch to sparse! I'd still like this or
something like it to go in though, so we can keep working on reducing
the sparse warning count while the sparse patch is in the works.

Regards,
Daniel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 859 bytes --]

Re: [PATCH] powerpc/align: Use #ifdef __BIG_ENDIAN__ #else for REG_BYTE

[Michael Ellerman]

On Thu, 2016-06-16 at 15:05 +0200, Arnd Bergmann wrote:
> On Thursday, June 16, 2016 10:33:41 PM CEST Michael Ellerman wrote:
> > From: Daniel Axtens <dja@axtens.net>
> > 
> > Sparse complains that it doesn't know what REG_BYTE is:
> > 
> >   arch/powerpc/kernel/align.c:313:29: error: undefined identifier 'REG_BYTE'
> > 
> > REG_BYTE is defined differently based on whether we're compiling for
> > LE, BE32 or BE64. Sparse apparently doesn't provide __BIG_ENDIAN__ or
> > __LITTLE_ENDIAN__, which means we get no definition.
> > 
> > Rather than check for __BIG_ENDIAN__ and then separately for
> > __LITTLE_ENDIAN__, just switch the #ifdef to check for __BIG_ENDIAN__
> > and then #else we define the little endian version. Technically that's
> > dicey because PDP_ENDIAN is also a possibility, but we already do it in
> > a lot of places so one more hardly matters.
> > 
> > Signed-off-by: Daniel Axtens <dja@axtens.net>
> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> 
> That makes the code less robust for the cases that accidentally we
> have neither or both of __LITTLE_ENDIAN__/__BIG_ENDIAN__ set during
> an actual compilation.
 
True, but I already count ~30 locations where we do the #if/else check. So any
compiler that defines both or neither has zero chance of building the kernel
already, and is clearly broken IMHO.

> It would be better to fix the sparse compilation so the same endianess
> is set that you get when calling gcc.

Agree, but I don't want us to get road-blocked on that.

cheers

Re: [PATCH] powerpc/align: Use #ifdef __BIG_ENDIAN__ #else for REG_BYTE

[Arnd Bergmann]

On Friday, June 17, 2016 1:35:35 PM CEST Daniel Axtens wrote:
> > It would be better to fix the sparse compilation so the same endianess
> > is set that you get when calling gcc.
> 
> I will definitely work on a patch to sparse! I'd still like this or
> something like it to go in though, so we can keep working on reducing
> the sparse warning count while the sparse patch is in the works.

I think you just need to fix the Makefile so it sets the right
arguments when calling sparse.

Something like the (untested) patch below, similar to how we
already handle the word size and how some other architectures
handle setting __BIG_ENDIAN__.

	Arnd

diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 709a22a3e824..8617c71c3bdb 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -181,6 +181,11 @@ KBUILD_CFLAGS	+= -pipe -Iarch/$(ARCH) $(CFLAGS-y)
 CPP		= $(CC) -E $(KBUILD_CFLAGS)
 
 CHECKFLAGS	+= -m$(CONFIG_WORD_SIZE) -D__powerpc__ -D__powerpc$(CONFIG_WORD_SIZE)__
+ifdef CONFIG_CPU_BIG_ENDIAN
+CHECKFLAGS	+= -D__BIG_ENDIAN__
+else
+CHECKFLAGS	+= -D__LITTLE_ENDIAN__
+endif
 
 KBUILD_LDFLAGS_MODULE += arch/powerpc/lib/crtsavres.o
 

Re: powerpc/align: Use #ifdef __BIG_ENDIAN__ #else for REG_BYTE

[Michael Ellerman]

On Thu, 2016-16-06 at 12:33:41 UTC, Michael Ellerman wrote:
> From: Daniel Axtens <dja@axtens.net>
> 
> Sparse complains that it doesn't know what REG_BYTE is:
> 
>   arch/powerpc/kernel/align.c:313:29: error: undefined identifier 'REG_BYTE'
> 
> REG_BYTE is defined differently based on whether we're compiling for
> LE, BE32 or BE64. Sparse apparently doesn't provide __BIG_ENDIAN__ or
> __LITTLE_ENDIAN__, which means we get no definition.
> 
> Rather than check for __BIG_ENDIAN__ and then separately for
> __LITTLE_ENDIAN__, just switch the #ifdef to check for __BIG_ENDIAN__
> and then #else we define the little endian version. Technically that's
> dicey because PDP_ENDIAN is also a possibility, but we already do it in
> a lot of places so one more hardly matters.
> 
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Applied to powerpc next.

https://git.kernel.org/powerpc/c/a9650e9bc53239c30c39f77d9d

cheers

Re: [PATCH] powerpc/align: Use #ifdef __BIG_ENDIAN__ #else for REG_BYTE

[Michael Ellerman]

On Fri, 2016-06-17 at 12:46 +0200, Arnd Bergmann wrote:
> On Friday, June 17, 2016 1:35:35 PM CEST Daniel Axtens wrote:
> > > It would be better to fix the sparse compilation so the same endianess
> > > is set that you get when calling gcc.
> > 
> > I will definitely work on a patch to sparse! I'd still like this or
> > something like it to go in though, so we can keep working on reducing
> > the sparse warning count while the sparse patch is in the works.
> 
> I think you just need to fix the Makefile so it sets the right
> arguments when calling sparse.
> 
> Something like the (untested) patch below, similar to how we
> already handle the word size and how some other architectures
> handle setting __BIG_ENDIAN__.

Yep that's clearly better. I didn't know we had separate CHECKER_FLAGS.

Daniel can you test that?

Arnd we'll add Suggested-by: you, or send a SOB if you like?

cheers

Re: [PATCH] powerpc/align: Use #ifdef __BIG_ENDIAN__ #else for REG_BYTE

[Daniel Axtens]

Hi Arnd,

> Something like the (untested) patch below, similar to how we
> already handle the word size and how some other architectures
> handle setting __BIG_ENDIAN__.

I tested this by reverting Michael's patch and applying yours.

Not only does it successfully fix the errors that patch fixes, it
manages to clean up the following four errors as well:

+/scratch/dja/linux/arch/powerpc/lib/sstep.c:371:32: error: cast from unknown type
+/scratch/dja/linux/arch/powerpc/lib/sstep.c:371:59: error: using member 'word' in incomplete struct <unnamed>
+/scratch/dja/linux/arch/powerpc/lib/sstep.c:411:32: error: cast from unknown type
+/scratch/dja/linux/arch/powerpc/lib/sstep.c:411:59: error: using member 'word' in incomplete struct <unnamed>

So:
Tested-by: Daniel Axtens <dja@axtens.net>

(I think the patch also needs your sign-off.)

Regards,
Daniel

Re: [PATCH] powerpc/align: Use #ifdef __BIG_ENDIAN__ #else for REG_BYTE

[Arnd Bergmann]

On Tuesday, June 21, 2016 10:51:00 AM CEST Michael Ellerman wrote:
> On Fri, 2016-06-17 at 12:46 +0200, Arnd Bergmann wrote:
> > On Friday, June 17, 2016 1:35:35 PM CEST Daniel Axtens wrote:
> > > > It would be better to fix the sparse compilation so the same endianess
> > > > is set that you get when calling gcc.
> > > 
> > > I will definitely work on a patch to sparse! I'd still like this or
> > > something like it to go in though, so we can keep working on reducing
> > > the sparse warning count while the sparse patch is in the works.
> > 
> > I think you just need to fix the Makefile so it sets the right
> > arguments when calling sparse.
> > 
> > Something like the (untested) patch below, similar to how we
> > already handle the word size and how some other architectures
> > handle setting __BIG_ENDIAN__.
> 
> Yep that's clearly better. I didn't know we had separate CHECKER_FLAGS.
> 
> Daniel can you test that?
> 
> Arnd we'll add Suggested-by: you, or send a SOB if you like?
> 

Please use 'Suggested-by', the main work for this patch was in
analysing the problem and writing the changelog, and Daniel did that.

	Arnd