All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] mips: diverse Makefile updates
@ 2010-05-30 14:19 Sam Ravnborg
  2010-05-30 14:26 ` [PATCH 1/6] mips: introduce arch/mips/Kbuild Sam Ravnborg
                   ` (11 more replies)
  0 siblings, 12 replies; 24+ messages in thread
From: Sam Ravnborg @ 2010-05-30 14:19 UTC (permalink / raw)
  To: linux-mips, Ralf Baechle, Wu Zhangjin; +Cc: sam

This patchset does the following:
- introduce arch/mips/Kbuild
- use -Werror on all core-y files of the mips kernel
- introduce a distributed way to specify platform definitions
- refactor a few Makefiles
- clean up cleaning 

Ralf asked in private mail if I could try to implement
a working varient of a suggestion I made some time ago.
The idea was to move platform specific definitions to
dedicated platfrom files.

This is implmented in the third patch.

The idea is to move the platform definitions from arch/mips/Makefile
to arch/mips/<platform>/Platfrom

The content of this file is used in arch/mips/Makefile
and arch/mips/Kbuild.

On top of this is a few patches that refactor the
boot and boot/compressed Makefiles so they are more
kbuild conformant.
This beautify the output when we build a kernel.

Wu Zhangjin have pointed out a few bugs in the first
variants of the patches that hit the mailing list - thanks!


Patches will follow.

Note: I tried to test a little with bigsur_defconfig
but get_user() is buggy. Or at least my gcc thinks that
first argument may be used uninitialized.
I think mips needs to fix the 64 bit variant of get_user().
I took a quick look but ran away.

	Sam


Sam Ravnborg (6):
      mips: introduce arch/mips/Kbuild
      mips: add -Werror to arch/mips/Kbuild
      mips: introduce support for Platform definitions
      mips: refactor arch/mips/boot/Makefile
      mips: refactor arch/mips/boot/compressed/Makefile
      mips: clean up arch/mips/Makefile

 arch/mips/Kbuild                   |   15 +++++++++
 arch/mips/Kbuild.platforms         |    6 ++++
 arch/mips/Makefile                 |   57 +++++++++---------------------------
 arch/mips/ar7/Platform             |    7 ++++
 arch/mips/boot/Makefile            |   49 ++++++++++++++----------------
 arch/mips/boot/compressed/Makefile |   54 ++++++++++++++++++----------------
 arch/mips/kernel/Makefile          |    2 -
 arch/mips/math-emu/Makefile        |    1 -
 arch/mips/mm/Makefile              |    2 -
 9 files changed, 94 insertions(+), 99 deletions(-)

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/6] mips: introduce arch/mips/Kbuild
  2010-05-30 14:19 [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
@ 2010-05-30 14:26 ` Sam Ravnborg
  2010-05-30 14:26 ` [PATCH 2/6] mips: add -Werror to arch/mips/Kbuild Sam Ravnborg
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 24+ messages in thread
From: Sam Ravnborg @ 2010-05-30 14:26 UTC (permalink / raw)
  To: linux-mips, Ralf Baechle, Wu Zhangjin



^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 2/6] mips: add -Werror to arch/mips/Kbuild
  2010-05-30 14:19 [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
  2010-05-30 14:26 ` [PATCH 1/6] mips: introduce arch/mips/Kbuild Sam Ravnborg
@ 2010-05-30 14:26 ` Sam Ravnborg
  2010-05-30 14:27 ` [PATCH 3/6] mips: introduce support for Platform definitions Sam Ravnborg
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 24+ messages in thread
From: Sam Ravnborg @ 2010-05-30 14:26 UTC (permalink / raw)
  To: linux-mips, Ralf Baechle, Wu Zhangjin



^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 3/6] mips: introduce support for Platform definitions
  2010-05-30 14:19 [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
  2010-05-30 14:26 ` [PATCH 1/6] mips: introduce arch/mips/Kbuild Sam Ravnborg
  2010-05-30 14:26 ` [PATCH 2/6] mips: add -Werror to arch/mips/Kbuild Sam Ravnborg
@ 2010-05-30 14:27 ` Sam Ravnborg
  2010-05-30 14:27 ` [PATCH 4/6] mips: refactor arch/mips/boot/Makefile Sam Ravnborg
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 24+ messages in thread
From: Sam Ravnborg @ 2010-05-30 14:27 UTC (permalink / raw)
  To: linux-mips, Ralf Baechle, Wu Zhangjin



^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 4/6] mips: refactor arch/mips/boot/Makefile
  2010-05-30 14:19 [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
                   ` (2 preceding siblings ...)
  2010-05-30 14:27 ` [PATCH 3/6] mips: introduce support for Platform definitions Sam Ravnborg
@ 2010-05-30 14:27 ` Sam Ravnborg
  2010-05-30 14:28 ` [PATCH 5/6] mips: refactor arch/mips/boot/compressed/Makefile Sam Ravnborg
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 24+ messages in thread
From: Sam Ravnborg @ 2010-05-30 14:27 UTC (permalink / raw)
  To: linux-mips, Ralf Baechle, Wu Zhangjin



^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 5/6] mips: refactor arch/mips/boot/compressed/Makefile
  2010-05-30 14:19 [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
                   ` (3 preceding siblings ...)
  2010-05-30 14:27 ` [PATCH 4/6] mips: refactor arch/mips/boot/Makefile Sam Ravnborg
@ 2010-05-30 14:28 ` Sam Ravnborg
  2010-05-30 14:28 ` [PATCH 6/6] mips: clean up arch/mips/Makefile Sam Ravnborg
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 24+ messages in thread
From: Sam Ravnborg @ 2010-05-30 14:28 UTC (permalink / raw)
  To: linux-mips, Ralf Baechle, Wu Zhangjin



^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 6/6] mips: clean up arch/mips/Makefile
  2010-05-30 14:19 [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
                   ` (4 preceding siblings ...)
  2010-05-30 14:28 ` [PATCH 5/6] mips: refactor arch/mips/boot/compressed/Makefile Sam Ravnborg
@ 2010-05-30 14:28 ` Sam Ravnborg
  2010-05-30 15:39 ` [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 24+ messages in thread
From: Sam Ravnborg @ 2010-05-30 14:28 UTC (permalink / raw)
  To: linux-mips, Ralf Baechle, Wu Zhangjin



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] mips: diverse Makefile updates
  2010-05-30 14:19 [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
                   ` (5 preceding siblings ...)
  2010-05-30 14:28 ` [PATCH 6/6] mips: clean up arch/mips/Makefile Sam Ravnborg
@ 2010-05-30 15:39 ` Sam Ravnborg
  2010-05-30 23:19   ` Ralf Baechle
  2010-05-30 18:03 ` [ Sam Ravnborg
                   ` (4 subsequent siblings)
  11 siblings, 1 reply; 24+ messages in thread
From: Sam Ravnborg @ 2010-05-30 15:39 UTC (permalink / raw)
  To: linux-mips, Ralf Baechle, Wu Zhangjin

> 
> Note: I tried to test a little with bigsur_defconfig
> but get_user() is buggy. Or at least my gcc thinks that
> first argument may be used uninitialized.
> I think mips needs to fix the 64 bit variant of get_user().
> I took a quick look but ran away.

My gcc:
mips-linux-gcc (GCC) 4.1.2
Copyright (C) 2006 Free Software Foundation, Inc.

I downloaded it from:
ftp://ftp.linux-mips.org/pub/linux/mips/people/macro/RPMS/i386/

	Sam

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [
  2010-05-30 14:19 [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
                   ` (6 preceding siblings ...)
  2010-05-30 15:39 ` [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
@ 2010-05-30 18:03 ` Sam Ravnborg
  2010-05-30 18:06 ` [PATCH] mips: fix uninitialized warning when using get_user() Sam Ravnborg
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 24+ messages in thread
From: Sam Ravnborg @ 2010-05-30 18:03 UTC (permalink / raw)
  To: linux-mips, Ralf Baechle, Wu Zhangjin



^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH] mips: fix uninitialized warning when using get_user()
  2010-05-30 14:19 [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
                   ` (7 preceding siblings ...)
  2010-05-30 18:03 ` [ Sam Ravnborg
@ 2010-05-30 18:06 ` Sam Ravnborg
  2010-05-31  8:45 ` [PATCH 0/6] mips: diverse Makefile updates Wu Zhangjin
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 24+ messages in thread
From: Sam Ravnborg @ 2010-05-30 18:06 UTC (permalink / raw)
  To: linux-mips, Ralf Baechle, Wu Zhangjin



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] mips: diverse Makefile updates
  2010-05-30 15:39 ` [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
@ 2010-05-30 23:19   ` Ralf Baechle
  2010-05-31 10:29     ` Ralf Baechle
  0 siblings, 1 reply; 24+ messages in thread
From: Ralf Baechle @ 2010-05-30 23:19 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: linux-mips, Wu Zhangjin

On Sun, May 30, 2010 at 05:39:39PM +0200, Sam Ravnborg wrote:

> > Note: I tried to test a little with bigsur_defconfig
> > but get_user() is buggy. Or at least my gcc thinks that
> > first argument may be used uninitialized.
> > I think mips needs to fix the 64 bit variant of get_user().
> > I took a quick look but ran away.
> 
> My gcc:
> mips-linux-gcc (GCC) 4.1.2
> Copyright (C) 2006 Free Software Foundation, Inc.
> 
> I downloaded it from:
> ftp://ftp.linux-mips.org/pub/linux/mips/people/macro/RPMS/i386/

Uh, get_user.  The living envy the dead.  This may be the best macro soup
in the whole kernel.  The code is so horrible because it's a functions that
is used very frequently in the kernel.  Breathe at it, you get an extra
instruction and that expanded a few thousand times acrosst he kernel.  And
quite often I had to pick the ugly solution because the bogus warnings as
you are experiencing are haunting the implementation forever.

Since a few days I'm using binutils 2.20 and gcc 4.5.0; all defconfigs
are building ok.  I'm uploading the rpms (see list below signature) I
built for my own use to

  ftp://ftp.linux-mips.org/pub/linux/mips/crossdev/
  http://ftp.linux-mips.org/pub/linux/mips/crossdev/

These were built on F12 and come with no userland libraries so are
perfect for your kernel-only use.

  Ralf

2daa8559f5287ae59bbcff5f84b71262  binutils-mips64el-linux-2.20-1.i686.rpm
dbd4531741d7154c74085bacca3667b1  binutils-mips64el-linux-2.20-1.x86_64.rpm
8c5d9d19c3f9b50d100490fe0e3b8c0e  binutils-mips64-linux-2.20-1.i686.rpm
5821dd4385a8b5329e2b75fe546ef627  binutils-mips64-linux-2.20-1.x86_64.rpm
0290f7a4be5903655144f2751e78b0ff  binutils-mipsel-linux-2.20-1.i686.rpm
8dfc605c430a9c0a14f3e62fc836bec1  binutils-mipsel-linux-2.20-1.x86_64.rpm
dfb88224bef5249ba6b2a6215409ea0b  binutils-mips-linux-2.20-1.i686.rpm
8575e9dbcee89d09cdaf44d0649f639f  binutils-mips-linux-2.20-1.x86_64.rpm
8b393e603db6217549d840c85b5fb2cb  cross-binutils-2.20-1.src.rpm
bd7a376b6aa693c0279d6c9cba178fb5  cross-gcc-4.5.0-1.src.rpm
2aa1e9710eb9df0cd475b56711ea6ac1  gcc-mips64el-linux-4.5.0-1.i686.rpm
ca364bfda7b855e86cd19335210700d6  gcc-mips64el-linux-4.5.0-1.x86_64.rpm
2ce8bc4ec8004acb1f0d089b5921287e  gcc-mips64-linux-4.5.0-1.i686.rpm
50c7115ff7c6793d008a4f1f8ecfb6eb  gcc-mips64-linux-4.5.0-1.x86_64.rpm
d20dd7b25fbfd54a3ac795608f20e680  gcc-mipsel-linux-4.5.0-1.i686.rpm
4a3172cdb4b78d2541c39304969377cb  gcc-mipsel-linux-4.5.0-1.x86_64.rpm
2fe8b5f2fefc76491e9b5013791dc5f9  gcc-mips-linux-4.5.0-1.i686.rpm
af54f71683d9c47ce21bbc1162f23d88  gcc-mips-linux-4.5.0-1.x86_64.rpm

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] mips: diverse Makefile updates
  2010-05-30 14:19 [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
                   ` (8 preceding siblings ...)
  2010-05-30 18:06 ` [PATCH] mips: fix uninitialized warning when using get_user() Sam Ravnborg
@ 2010-05-31  8:45 ` Wu Zhangjin
  2010-05-31  9:10   ` Sam Ravnborg
  2010-05-31 14:56 ` Ralf Baechle
  2010-05-31 15:33 ` Manuel Lauss
  11 siblings, 1 reply; 24+ messages in thread
From: Wu Zhangjin @ 2010-05-31  8:45 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: linux-mips, Ralf Baechle

Hi, Sam & Ralf

Perhaps we also need to fix the following stuff:

...
  LD      vmlinux
  SYSMAP  System.map
  SYSMAP  .tmp_System.map
mips64el-unknown-linux-gnu-objcopy -O elf32-tradlittlemips  --remove-section=.reginfo vmlinux vmlinux.32
  AS      arch/mips/boot/compressed/head.o
  CC      arch/mips/boot/compressed/decompress.o
  CC      arch/mips/boot/compressed/dbg.o
...

The related Makefile is arch/mips/Makefile:

> 721 #
> 722 # Some machines like the Indy need 32-bit ELF binaries for booting purposes.
> 723 # Other need ECOFF, so we build a 32-bit ELF binary for them which we then
> 724 # convert to ECOFF using elf2ecoff.
> 725 #
> 726 vmlinux.32: vmlinux
> 727         $(OBJCOPY) -O $(32bit-bfd) $(OBJCOPYFLAGS) $< $@
> 728 
> 729 #
> 730 # The 64-bit ELF tools are pretty broken so at this time we generate 64-bit
> 731 # ELF files from 32-bit files by conversion.
> 732 #
> 733 vmlinux.64: vmlinux
> 734         $(OBJCOPY) -O $(64bit-bfd) $(OBJCOPYFLAGS) $< $@

Best Regards,
	Wu Zhangjin

On Sun, 2010-05-30 at 16:19 +0200, Sam Ravnborg wrote:
> This patchset does the following:
> - introduce arch/mips/Kbuild
> - use -Werror on all core-y files of the mips kernel
> - introduce a distributed way to specify platform definitions
> - refactor a few Makefiles
> - clean up cleaning 
> 
> Ralf asked in private mail if I could try to implement
> a working varient of a suggestion I made some time ago.
> The idea was to move platform specific definitions to
> dedicated platfrom files.
> 
> This is implmented in the third patch.
> 
> The idea is to move the platform definitions from arch/mips/Makefile
> to arch/mips/<platform>/Platfrom
> 
> The content of this file is used in arch/mips/Makefile
> and arch/mips/Kbuild.
> 
> On top of this is a few patches that refactor the
> boot and boot/compressed Makefiles so they are more
> kbuild conformant.
> This beautify the output when we build a kernel.
> 
> Wu Zhangjin have pointed out a few bugs in the first
> variants of the patches that hit the mailing list - thanks!
> 
> 
> Patches will follow.
> 
> Note: I tried to test a little with bigsur_defconfig
> but get_user() is buggy. Or at least my gcc thinks that
> first argument may be used uninitialized.
> I think mips needs to fix the 64 bit variant of get_user().
> I took a quick look but ran away.
> 
> 	Sam
> 
> 
> Sam Ravnborg (6):
>       mips: introduce arch/mips/Kbuild
>       mips: add -Werror to arch/mips/Kbuild
>       mips: introduce support for Platform definitions
>       mips: refactor arch/mips/boot/Makefile
>       mips: refactor arch/mips/boot/compressed/Makefile
>       mips: clean up arch/mips/Makefile
> 
>  arch/mips/Kbuild                   |   15 +++++++++
>  arch/mips/Kbuild.platforms         |    6 ++++
>  arch/mips/Makefile                 |   57 +++++++++---------------------------
>  arch/mips/ar7/Platform             |    7 ++++
>  arch/mips/boot/Makefile            |   49 ++++++++++++++----------------
>  arch/mips/boot/compressed/Makefile |   54 ++++++++++++++++++----------------
>  arch/mips/kernel/Makefile          |    2 -
>  arch/mips/math-emu/Makefile        |    1 -
>  arch/mips/mm/Makefile              |    2 -
>  9 files changed, 94 insertions(+), 99 deletions(-)

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] mips: diverse Makefile updates
  2010-05-31  8:45 ` [PATCH 0/6] mips: diverse Makefile updates Wu Zhangjin
@ 2010-05-31  9:10   ` Sam Ravnborg
  0 siblings, 0 replies; 24+ messages in thread
From: Sam Ravnborg @ 2010-05-31  9:10 UTC (permalink / raw)
  To: Wu Zhangjin; +Cc: linux-mips, Ralf Baechle

On Mon, May 31, 2010 at 04:45:31PM +0800, Wu Zhangjin wrote:
> Hi, Sam & Ralf
> 
> Perhaps we also need to fix the following stuff:
> 
> ...
>   LD      vmlinux
>   SYSMAP  System.map
>   SYSMAP  .tmp_System.map
> mips64el-unknown-linux-gnu-objcopy -O elf32-tradlittlemips  --remove-section=.reginfo vmlinux vmlinux.32
>   AS      arch/mips/boot/compressed/head.o
>   CC      arch/mips/boot/compressed/decompress.o
>   CC      arch/mips/boot/compressed/dbg.o
> ...
> 
> The related Makefile is arch/mips/Makefile:
> 
> > 721 #
> > 722 # Some machines like the Indy need 32-bit ELF binaries for booting purposes.
> > 723 # Other need ECOFF, so we build a 32-bit ELF binary for them which we then
> > 724 # convert to ECOFF using elf2ecoff.
> > 725 #
> > 726 vmlinux.32: vmlinux
> > 727         $(OBJCOPY) -O $(32bit-bfd) $(OBJCOPYFLAGS) $< $@
> > 728 
> > 729 #
> > 730 # The 64-bit ELF tools are pretty broken so at this time we generate 64-bit
> > 731 # ELF files from 32-bit files by conversion.
> > 732 #
> > 733 vmlinux.64: vmlinux
> > 734         $(OBJCOPY) -O $(64bit-bfd) $(OBJCOPYFLAGS) $< $@


I have looked at it but I was confused.
vmlinux.64 seems to be used by two SGI machines only.
I wonder if this is really required - but I did not look to much.

vmlinux.32 is even more strange....
When building a 64 bit kernel vmlinux.32 is used as input to
boot/Makefile. boot.Makefile uses this file for all of:
vmlinux.bin, vmlinux.srec, vmlinux.ecoff.

But boot/compressed produces an own variant of vmlinux.32 and there
it is _only_ used as input for vmlinuz.ecoff. vmlinuz.bin + vmlinuz.srec
uses the unmodified vmlinux as input?!?

So it all looked messy and I do not have the background knowledge to
clean it up.

My thinking was to do something like this:
1) move creation of vmlinux.64 to boot/Makefile (or even better to drop it)
2) let vmlinux.32 be an intermediate step used only
   for vmlinux.ecoff and thus move it to boot/Makefile
3) adjust Makefile so thay stop producing targets in the top-level directory
   Today it is inconsistent. boot/Makefile does it in one way
   boot/compressed/Makefile does it in another way.
   Always end a build with "Kernel '...' is ready so user see the path.

Step 1) is simple.
Step 2) is questionable. Is it the correct approach?
Step 3) is a visible change but more aligned with other archs.


What do you think?

I can obviously just beautify the output by using
some more kbuild like stuff.
But I prefer to understand what is going on - at least to some extent.

PS. I have not looked at the lasat specific stuff. Is it relevant to update it?

	Sam

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] mips: diverse Makefile updates
  2010-05-30 23:19   ` Ralf Baechle
@ 2010-05-31 10:29     ` Ralf Baechle
  2010-05-31 10:55       ` Sam Ravnborg
  0 siblings, 1 reply; 24+ messages in thread
From: Ralf Baechle @ 2010-05-31 10:29 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: linux-mips, Wu Zhangjin

On Mon, May 31, 2010 at 12:19:54AM +0100, Ralf Baechle wrote:

> > > Note: I tried to test a little with bigsur_defconfig
> > > but get_user() is buggy. Or at least my gcc thinks that
> > > first argument may be used uninitialized.
> > > I think mips needs to fix the 64 bit variant of get_user().
> > > I took a quick look but ran away.
> > 
> > My gcc:
> > mips-linux-gcc (GCC) 4.1.2
> > Copyright (C) 2006 Free Software Foundation, Inc.

I played with it for a bit.  The warning is present in all gcc 4.1.0 to
4.1.2 and it is bogus.  When I first looked into this years ago I just
gave up on gcc 4.1 as a newer version was already available.

The variable returned by get_user is undefined in case of an error, so
what get_user() is doing is entirely legitimate.  This is different from
copy_from_user() which in case of an error will clear the remainder of
the destination area which couldn't not be copied from userspace.

A test compile of a bigsur_defconfig without and with your patch shows
a size increase of only 80 bytes with gcc 4.5.0.  So I'm considering
your patch for inclusion but I want to play with alternatives for a bit.
Outlining may the right thing with modern compilers.

  Ralf

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] mips: diverse Makefile updates
  2010-05-31 10:29     ` Ralf Baechle
@ 2010-05-31 10:55       ` Sam Ravnborg
  2010-06-01 10:28         ` Ralf Baechle
  0 siblings, 1 reply; 24+ messages in thread
From: Sam Ravnborg @ 2010-05-31 10:55 UTC (permalink / raw)
  To: Ralf Baechle; +Cc: linux-mips, Wu Zhangjin

On Mon, May 31, 2010 at 11:29:54AM +0100, Ralf Baechle wrote:
> On Mon, May 31, 2010 at 12:19:54AM +0100, Ralf Baechle wrote:
> 
> > > > Note: I tried to test a little with bigsur_defconfig
> > > > but get_user() is buggy. Or at least my gcc thinks that
> > > > first argument may be used uninitialized.
> > > > I think mips needs to fix the 64 bit variant of get_user().
> > > > I took a quick look but ran away.
> > > 
> > > My gcc:
> > > mips-linux-gcc (GCC) 4.1.2
> > > Copyright (C) 2006 Free Software Foundation, Inc.
> 
> I played with it for a bit.  The warning is present in all gcc 4.1.0 to
> 4.1.2 and it is bogus.  When I first looked into this years ago I just
> gave up on gcc 4.1 as a newer version was already available.
> 
> The variable returned by get_user is undefined in case of an error, so
> what get_user() is doing is entirely legitimate.  This is different from
> copy_from_user() which in case of an error will clear the remainder of
> the destination area which couldn't not be copied from userspace.

What I looked at:

1)	u32 word;
2)	if (unlikely(get_user(word, header)))
3)		word = 0;
4)	if (word == magic.cmp)
5)		return PAGE_SIZE;

If gcc does not see an assignment to word in line 2) then
it complains about the use line 4).

If we look at the implementation of get_user it is more or less the
following:
({
	int err = -EFAULT;
	if (access_ok(VERIFY_READ, header))
		switch (sizeof(word)) {
		case 4:
			word = *(u32 *)header;
			err = 0;
			break;
		default:
			__get_user_unknown();
			break;
		}
	err;
})

Simplified a lot - I know. But it shows my point.
(And simplifying the code also helped me understand the macros).

gcc needs to be smart enough to deduce that we always return != 0
in the cases where word is not assigned - in which case line 3)
will do the assignment.

So gcc is indeed wrong when is sas "uninitialized" but considering
the complexity of these macros I think it is excused.

The x86 version has the following assignment

    (val) = (__typeof__(*(addr))) __gu_tmp; 

unconditionally - so they avoid the " = 0" thing.
sparc has explicit "= 0" assignments.

So refactoring the macros may do the trick too.
But I do not think it is worth the effort.

	Sam

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] mips: diverse Makefile updates
  2010-05-30 14:19 [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
                   ` (9 preceding siblings ...)
  2010-05-31  8:45 ` [PATCH 0/6] mips: diverse Makefile updates Wu Zhangjin
@ 2010-05-31 14:56 ` Ralf Baechle
  2010-05-31 15:33 ` Manuel Lauss
  11 siblings, 0 replies; 24+ messages in thread
From: Ralf Baechle @ 2010-05-31 14:56 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: linux-mips, Wu Zhangjin

On Sun, May 30, 2010 at 04:19:39PM +0200, Sam Ravnborg wrote:

> This patchset does the following:
> - introduce arch/mips/Kbuild
> - use -Werror on all core-y files of the mips kernel
> - introduce a distributed way to specify platform definitions
> - refactor a few Makefiles
> - clean up cleaning 
> 
> Ralf asked in private mail if I could try to implement
> a working varient of a suggestion I made some time ago.
> The idea was to move platform specific definitions to
> dedicated platfrom files.
> 
> This is implmented in the third patch.
> 
> The idea is to move the platform definitions from arch/mips/Makefile
> to arch/mips/<platform>/Platfrom
> 
> The content of this file is used in arch/mips/Makefile
> and arch/mips/Kbuild.
> 
> On top of this is a few patches that refactor the
> boot and boot/compressed Makefiles so they are more
> kbuild conformant.
> This beautify the output when we build a kernel.

This will need some extra work on the remaining platforms so is 2.6.36
material so I queued the entire series for 2.6.36.  The queue tree is
at

   git://ftp.linux-mips.org/pub/scm/linux-queue.git

And of course a big thanks for your work!

  Ralf

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] mips: diverse Makefile updates
  2010-05-30 14:19 [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
                   ` (10 preceding siblings ...)
  2010-05-31 14:56 ` Ralf Baechle
@ 2010-05-31 15:33 ` Manuel Lauss
  2010-05-31 18:03     ` Sam Ravnborg
  11 siblings, 1 reply; 24+ messages in thread
From: Manuel Lauss @ 2010-05-31 15:33 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: linux-mips, Ralf Baechle, Wu Zhangjin

Hi Sam, Ralf,

On Sun, May 30, 2010 at 4:19 PM, Sam Ravnborg <sam@ravnborg.org> wrote:
> This patchset does the following:
> - introduce arch/mips/Kbuild
> - use -Werror on all core-y files of the mips kernel
> - introduce a distributed way to specify platform definitions
> - refactor a few Makefiles
> - clean up cleaning


I took the patches from mips-queue and did my usual build:

$ make ARCH=mips CROSS_COMPILE=mipsel-softfloat-linux-gnu-
O=/home/mano/db120 /kernel/kbuild-db1200 menuconfig
/mnt/data/_home/mano/db1200/kernel/linux-2.6.git/arch/mips/Makefile:212:
arch/mips/Kbuild.platforms: No such file or directory
make[1]: *** No rule to make target `arch/mips/Kbuild.platforms'.  Stop.
make: *** [sub-make] Error 2

Any ideas?


> Ralf asked in private mail if I could try to implement
> a working varient of a suggestion I made some time ago.
> The idea was to move platform specific definitions to
> dedicated platfrom files.
>
> This is implmented in the third patch.

I'll implement this for Alchemy shortly.


Thanks!
      Manuel Lauss

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH] mips: fix build with O=...
@ 2010-05-31 18:03     ` Sam Ravnborg
  0 siblings, 0 replies; 24+ messages in thread
From: Sam Ravnborg @ 2010-05-31 18:03 UTC (permalink / raw)
  To: Manuel Lauss, Ralf Baechle; +Cc: linux-mips, Ralf Baechle, Wu Zhangjin



^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH] mips: fix build with O=...
@ 2010-05-31 18:03     ` Sam Ravnborg
  0 siblings, 0 replies; 24+ messages in thread
From: Sam Ravnborg @ 2010-05-31 18:03 UTC (permalink / raw)
  To: Manuel Lauss, Ralf Baechle; +Cc: linux-mips, Wu Zhangjin



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mips: fix build with O=...
  2010-05-31 18:03     ` Sam Ravnborg
  (?)
@ 2010-05-31 18:19     ` Manuel Lauss
  2010-05-31 19:00       ` Sam Ravnborg
  -1 siblings, 1 reply; 24+ messages in thread
From: Manuel Lauss @ 2010-05-31 18:19 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: Ralf Baechle, linux-mips, Wu Zhangjin

Hi Sam,

Thanks a lot.

> Hi Manuel.
> Thanks for testing and reporting so quick.
> The following fixed it for me - I tried only the ar7 platfrom.
> And do not hesitate to ask if you hit troubles converting your
> platform.

I just moved stuff from main Makefile to Platform, so far it seems
to work for 2


> Note: On top of my tree since git did not see the
> git tree posted by Ralf as a git tree?!?

git://git.linux-mips.org/pub/scm/linux-queue.git works

Manuel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mips: fix build with O=...
  2010-05-31 18:19     ` Manuel Lauss
@ 2010-05-31 19:00       ` Sam Ravnborg
  2010-05-31 22:36         ` Ralf Baechle
  0 siblings, 1 reply; 24+ messages in thread
From: Sam Ravnborg @ 2010-05-31 19:00 UTC (permalink / raw)
  To: Manuel Lauss; +Cc: Ralf Baechle, linux-mips, Wu Zhangjin

> 
> 
> > Note: On top of my tree since git did not see the
> > git tree posted by Ralf as a git tree?!?
> 
> git://git.linux-mips.org/pub/scm/linux-queue.git works
color me stupid :-(
I used git pull xxx instead of git clone xxx.

	Sam

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mips: fix build with O=...
  2010-05-31 19:00       ` Sam Ravnborg
@ 2010-05-31 22:36         ` Ralf Baechle
  0 siblings, 0 replies; 24+ messages in thread
From: Ralf Baechle @ 2010-05-31 22:36 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: Manuel Lauss, linux-mips, Wu Zhangjin

On Mon, May 31, 2010 at 09:00:11PM +0200, Sam Ravnborg wrote:

> > > Note: On top of my tree since git did not see the
> > > git tree posted by Ralf as a git tree?!?
> > 
> > git://git.linux-mips.org/pub/scm/linux-queue.git works
> color me stupid :-(
> I used git pull xxx instead of git clone xxx.

Be careful - that tree is frequently rebased.  This is also why I keep
it a separate tree - a branch that is rebased has the potencial to
confuse less experienced git users.

  Ralf

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mips: fix build with O=...
  2010-05-31 18:03     ` Sam Ravnborg
  (?)
  (?)
@ 2010-05-31 22:46     ` Ralf Baechle
  -1 siblings, 0 replies; 24+ messages in thread
From: Ralf Baechle @ 2010-05-31 22:46 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: Manuel Lauss, linux-mips, Wu Zhangjin

On Mon, May 31, 2010 at 08:03:21PM +0200, Sam Ravnborg wrote:

> >From b96ea542b6786a44ab1a70d53a2796bd0d60b521 Mon Sep 17 00:00:00 2001
> From: Sam Ravnborg <sam@ravnborg.org>
> Date: Mon, 31 May 2010 19:58:18 +0200
> Subject: [PATCH] mips: fix build with O=...
> 
> The newly added platform support introduced
> a regression so build with O=... failed.
> 
> Fix this by prefixing Makefile include paths with $(srctree).

Thanks, I've folded this patch into the original patch.

  Ralf

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 0/6] mips: diverse Makefile updates
  2010-05-31 10:55       ` Sam Ravnborg
@ 2010-06-01 10:28         ` Ralf Baechle
  0 siblings, 0 replies; 24+ messages in thread
From: Ralf Baechle @ 2010-06-01 10:28 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: linux-mips, Wu Zhangjin

On Mon, May 31, 2010 at 12:55:50PM +0200, Sam Ravnborg wrote:

> > I played with it for a bit.  The warning is present in all gcc 4.1.0 to
> > 4.1.2 and it is bogus.  When I first looked into this years ago I just
> > gave up on gcc 4.1 as a newer version was already available.
> > 
> > The variable returned by get_user is undefined in case of an error, so
> > what get_user() is doing is entirely legitimate.  This is different from
> > copy_from_user() which in case of an error will clear the remainder of
> > the destination area which couldn't not be copied from userspace.
> 
> What I looked at:
> 
> 1)	u32 word;
> 2)	if (unlikely(get_user(word, header)))
> 3)		word = 0;
> 4)	if (word == magic.cmp)
> 5)		return PAGE_SIZE;
> 
> If gcc does not see an assignment to word in line 2) then
> it complains about the use line 4).
> 
> If we look at the implementation of get_user it is more or less the
> following:
> ({
> 	int err = -EFAULT;
> 	if (access_ok(VERIFY_READ, header))
> 		switch (sizeof(word)) {
> 		case 4:
> 			word = *(u32 *)header;
> 			err = 0;
> 			break;
> 		default:
> 			__get_user_unknown();
> 			break;
> 		}
> 	err;
> })
> 
> Simplified a lot - I know. But it shows my point.
> (And simplifying the code also helped me understand the macros).
> 
> gcc needs to be smart enough to deduce that we always return != 0
> in the cases where word is not assigned - in which case line 3)
> will do the assignment.
> 
> So gcc is indeed wrong when is sas "uninitialized" but considering
> the complexity of these macros I think it is excused.
> 
> The x86 version has the following assignment
> 
>     (val) = (__typeof__(*(addr))) __gu_tmp; 
> 
> unconditionally - so they avoid the " = 0" thing.
> sparc has explicit "= 0" assignments.
> 
> So refactoring the macros may do the trick too.
> But I do not think it is worth the effort.

One reason for the being written as they are is also that this allows a
compiler to generate some useful warnings such as:

	get_user(var, ptr);
	printk("var is %d\n", var);

Where var indeed can be used uninitialzed.  The return value of get_user()
being ignored is really a separate problem which should be attacked as
well.  __must_check would be the obvious way of doing this but it can only
be used as a function attribute.  Will have to experiment to see if it's
possible to use it within get_user / put_user in something like:

static inline int __must_check __gu_must_check(int __gu_err)
{
	return __gu_err
}

#define __get_user_check(x, ptr, size)                                  \
({                                                                      \
        int __gu_err = -EFAULT;                                         \
        const __typeof__(*(ptr)) __user * __gu_ptr = (ptr);             \
                                                                        \
        might_fault();                                                  \
        if (likely(access_ok(VERIFY_READ,  __gu_ptr, size)))            \
                __get_user_common((x), size, __gu_ptr);                 \
                                                                        \
        __gu_must_check(__gu_err);					\
})

  Ralf

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2010-06-01 10:28 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-05-30 14:19 [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
2010-05-30 14:26 ` [PATCH 1/6] mips: introduce arch/mips/Kbuild Sam Ravnborg
2010-05-30 14:26 ` [PATCH 2/6] mips: add -Werror to arch/mips/Kbuild Sam Ravnborg
2010-05-30 14:27 ` [PATCH 3/6] mips: introduce support for Platform definitions Sam Ravnborg
2010-05-30 14:27 ` [PATCH 4/6] mips: refactor arch/mips/boot/Makefile Sam Ravnborg
2010-05-30 14:28 ` [PATCH 5/6] mips: refactor arch/mips/boot/compressed/Makefile Sam Ravnborg
2010-05-30 14:28 ` [PATCH 6/6] mips: clean up arch/mips/Makefile Sam Ravnborg
2010-05-30 15:39 ` [PATCH 0/6] mips: diverse Makefile updates Sam Ravnborg
2010-05-30 23:19   ` Ralf Baechle
2010-05-31 10:29     ` Ralf Baechle
2010-05-31 10:55       ` Sam Ravnborg
2010-06-01 10:28         ` Ralf Baechle
2010-05-30 18:03 ` [ Sam Ravnborg
2010-05-30 18:06 ` [PATCH] mips: fix uninitialized warning when using get_user() Sam Ravnborg
2010-05-31  8:45 ` [PATCH 0/6] mips: diverse Makefile updates Wu Zhangjin
2010-05-31  9:10   ` Sam Ravnborg
2010-05-31 14:56 ` Ralf Baechle
2010-05-31 15:33 ` Manuel Lauss
2010-05-31 18:03   ` [PATCH] mips: fix build with O= Sam Ravnborg
2010-05-31 18:03     ` Sam Ravnborg
2010-05-31 18:19     ` Manuel Lauss
2010-05-31 19:00       ` Sam Ravnborg
2010-05-31 22:36         ` Ralf Baechle
2010-05-31 22:46     ` Ralf Baechle

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.