Patchwork Quieten arch/powerpc in a allmodconfig build.

login
register
mail settings
Submitter Michael Ellerman
Date April 8, 2009, 6:48 a.m.
Message ID <1239173283.10104.38.camel@localhost>
Download mbox | patch
Permalink /patch/25708/
State Changes Requested
Headers show

Comments

Michael Ellerman - April 8, 2009, 6:48 a.m.
On Wed, 2009-04-08 at 15:51 +1000, Tony Breeds wrote:
> On Wed, Apr 08, 2009 at 03:08:55PM +1000, Michael Ellerman wrote:
> 
> > The getter routines in here could really multiplex their return values
> > with a negative error code, which I generally prefer, but this works I
> > guess.
> 
> I was hoping someone would notice and suggest it.  tag you're it!

I meant we /could/ change them, but we could also leave them, it's a bit
of a coin-flip which is better. Nathan might have an opinion?

Something like this:



cheers
Geert Uytterhoeven - April 8, 2009, 7:42 a.m.
On Wed, 8 Apr 2009, Michael Ellerman wrote:
> On Wed, 2009-04-08 at 15:51 +1000, Tony Breeds wrote:
> > On Wed, Apr 08, 2009 at 03:08:55PM +1000, Michael Ellerman wrote:
> > 
> > > The getter routines in here could really multiplex their return values
> > > with a negative error code, which I generally prefer, but this works I
> > > guess.
> > 
> > I was hoping someone would notice and suggest it.  tag you're it!
> 
> I meant we /could/ change them, but we could also leave them, it's a bit
> of a coin-flip which is better. Nathan might have an opinion?
> 
> Something like this:
> 
> diff --git a/arch/powerpc/kernel/cacheinfo.c b/arch/powerpc/kernel/cacheinfo.c
> index bb37b1d..9f3a155 100644
> --- a/arch/powerpc/kernel/cacheinfo.c
> +++ b/arch/powerpc/kernel/cacheinfo.c

> -static int cache_size_kb(const struct cache *cache, unsigned int *ret)
> +static int cache_size_kb(const struct cache *cache)
>  {
>         unsigned int size;
          ^^^^^^^^^^^^
`int size', or Roel will come to get you ;-)

>  
> -       if (cache_size(cache, &size))
> -               return -ENODEV;
> +       size = cache_size(cache);
> +       if (size < 0)
              ^^^^^^^^
> +               return size;

I can't check the others, due to lacking context...

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010
Nathan Lynch - April 8, 2009, 6:47 p.m.
Michael Ellerman <michael@ellerman.id.au> wrote:

> On Wed, 2009-04-08 at 15:51 +1000, Tony Breeds wrote:
> > On Wed, Apr 08, 2009 at 03:08:55PM +1000, Michael Ellerman wrote:
> > 
> > > The getter routines in here could really multiplex their return values
> > > with a negative error code, which I generally prefer, but this works I
> > > guess.
> > 
> > I was hoping someone would notice and suggest it.  tag you're it!
> 
> I meant we /could/ change them, but we could also leave them, it's a bit
> of a coin-flip which is better. Nathan might have an opinion?

I think I had some reason for doing it this way, but I'm drawing a
blank right now.

In the meantime, can someone post the warnings that gcc emits for
cacheinfo.c, as well as the gcc version?  I can't reproduce them with
4.3.2 on Fedora.
Tony Breeds - April 9, 2009, 12:01 a.m.
On Wed, Apr 08, 2009 at 01:47:36PM -0500, Nathan Lynch wrote:

> I think I had some reason for doing it this way, but I'm drawing a
> blank right now.
> 
> In the meantime, can someone post the warnings that gcc emits for
> cacheinfo.c, as well as the gcc version?  I can't reproduce them with
> 4.3.2 on Fedora.

---
[tony@thor ~]$ egrep cacheinfo tmp/build.log 
/scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function 'associativity_show':
/scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:562: warning: 'associativity' may be used uninitialized in this function
/scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function 'size_show':
/scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:513: warning: 'size_kb' may be used uninitialized in this function
tony@Sprygo:~/scratch$ /opt/crosstool/gcc-4.4.0-20090216-nolibc/powerpc-linux/bin/powerpc-linux-gcc -v
Using built-in specs.
Target: powerpc-linux
Configured with: /home/tony/buildall/src/gcc/configure --target=powerpc-linux --enable-targets=all --prefix=/opt/crosstool/gcc-4.4.0-20090216-nolibc/powerpc-linux --program-prefix=powerpc-linux- --disable-bootstrap --with-mpfr=/usr --enable-languages=c --without-headers --enable-sjlj-exceptions --with-system-libunwind --disable-nls --disable-threads --disable-shared --disable-libmudflap --disable-libssp --disable-libgomp --disable-decimal-float --enable-checking=release
Thread model: single
gcc version 4.4.0 20090216 (experimental) (GCC) 
tony@Sprygo:~/scratch$ 
---

That's the compiler in rawhide

Yours Tony
Nathan Lynch - April 10, 2009, 4:21 a.m.
Tony Breeds <tony@bakeyournoodle.com> wrote:

> On Wed, Apr 08, 2009 at 01:47:36PM -0500, Nathan Lynch wrote:
> 
> > I think I had some reason for doing it this way, but I'm drawing a
> > blank right now.
> > 
> > In the meantime, can someone post the warnings that gcc emits for
> > cacheinfo.c, as well as the gcc version?  I can't reproduce them with
> > 4.3.2 on Fedora.
> 
> ---
> [tony@thor ~]$ egrep cacheinfo tmp/build.log 
> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function 'associativity_show':
> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:562: warning: 'associativity' may be used uninitialized in this function
> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function 'size_show':
> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:513: warning: 'size_kb' may be used uninitialized in this function

Thanks.

So I think I've convinced myself that the warnings are incorrect and
that uninitialized use is not possible.

But I find it odd that gcc gives warnings for these sites but not others
in the file that use the same idiom (e.g. line_size_show,
nr_sets_show).  I'd guess that inlining is implicated somehow.  Would I
be justified in worrying that this version of gcc is generating
incorrect code?

If not, then I'm fine with the uninitialized_var() changes, but do
please include the warnings and the compiler version in the changelog.
Segher Boessenkool - April 10, 2009, 5:19 p.m.
>> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function  
>> 'associativity_show':
>> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:562:  
>> warning: 'associativity' may be used uninitialized in this function
>> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c: In function  
>> 'size_show':
>> /scratch/tony/working/arch/powerpc/kernel/cacheinfo.c:513:  
>> warning: 'size_kb' may be used uninitialized in this function
>
> Thanks.
>
> So I think I've convinced myself that the warnings are incorrect and
> that uninitialized use is not possible.

Strictly speaking the warnings aren't incorrect: the variables
"may be used uninitialized", they just never are.  I agree this
isn't very helpful.

> But I find it odd that gcc gives warnings for these sites but not  
> others
> in the file that use the same idiom (e.g. line_size_show,
> nr_sets_show).

Yeah.

> I'd guess that inlining is implicated somehow.  Would I
> be justified in worrying that this version of gcc is generating
> incorrect code?

Not really.  Sub-optimal code perhaps, but not incorrect.

> If not, then I'm fine with the uninitialized_var() changes, but do
> please include the warnings and the compiler version in the changelog.

If this happens with a non-ancient GCC version, can we have a bugreport
please?


Segher

Patch

diff --git a/arch/powerpc/kernel/cacheinfo.c b/arch/powerpc/kernel/cacheinfo.c
index bb37b1d..9f3a155 100644
--- a/arch/powerpc/kernel/cacheinfo.c
+++ b/arch/powerpc/kernel/cacheinfo.c
@@ -191,7 +191,7 @@  static void cache_cpu_set(struct cache *cache, int cpu)
        }
 }
 
-static int cache_size(const struct cache *cache, unsigned int *ret)
+static int cache_size(const struct cache *cache)
 {
        const char *propname;
        const u32 *cache_size;
@@ -202,19 +202,18 @@  static int cache_size(const struct cache *cache, unsigned 
        if (!cache_size)
                return -ENODEV;
 
-       *ret = *cache_size;
-       return 0;
+       return cache_size;
 }
 
-static int cache_size_kb(const struct cache *cache, unsigned int *ret)
+static int cache_size_kb(const struct cache *cache)
 {
        unsigned int size;
 
-       if (cache_size(cache, &size))
-               return -ENODEV;
+       size = cache_size(cache);
+       if (size < 0)
+               return size;
 
-       *ret = size / 1024;
-       return 0;
+       return size / 1024;
 }
 
 /* not cache_line_size() because that's a macro in include/linux/cache.h */
@@ -515,8 +514,9 @@  static ssize_t size_show(struct kobject *k, struct kobj_attr
 
        cache = index_kobj_to_cache(k);
 
-       if (cache_size_kb(cache, &size_kb))
-               return -ENODEV;
+       size_kb = cache_size_kb(cache);
+       if (size_kb < 0)
+               return size_kb;
 
        return sprintf(buf, "%uK\n", size_kb);
 }