[PATCH] mpparse.c:231: warning: comparison is always false

[PATCH] mpparse.c:231: warning: comparison is always false

[Jarek Poplawski]

Hello,

Probably after 2.6.18-rc6-git1 there is this cc warning: 
"arch/i386/kernel/mpparse.c:231: warning: comparison is
always false due to limited range of data type".
Maybe this patch will be helpful.

Jarek P.


diff -Nurp linux-2.6.18-rc6-git4-/arch/i386/kernel/mpparse.c linux-2.6.18-rc6-git4/arch/i386/kernel/mpparse.c
--- linux-2.6.18-rc6-git4-/arch/i386/kernel/mpparse.c	2006-09-13 00:01:00.000000000 +0200
+++ linux-2.6.18-rc6-git4/arch/i386/kernel/mpparse.c	2006-09-13 00:01:00.000000000 +0200
@@ -228,12 +228,14 @@ static void __init MP_bus_info (struct m
 
 	mpc_oem_bus_info(m, str, translation_table[mpc_record]);
 
+#if 0xFF >= MAX_MP_BUSSES
 	if (m->mpc_busid >= MAX_MP_BUSSES) {
 		printk(KERN_WARNING "MP table busid value (%d) for bustype %s "
 			" is too large, max. supported is %d\n",
 			m->mpc_busid, str, MAX_MP_BUSSES - 1);
 		return;
 	}
+#endif
 
 	if (strncmp(str, BUSTYPE_ISA, sizeof(BUSTYPE_ISA)-1) == 0) {
 		mp_bus_id_to_type[m->mpc_busid] = MP_BUS_ISA;

Re: [PATCH] mpparse.c:231: warning: comparison is always false

[Dave Jones]

On Wed, Sep 13, 2006 at 08:50:10AM +0200, Jarek Poplawski wrote:
 
 > Probably after 2.6.18-rc6-git1 there is this cc warning: 
 > "arch/i386/kernel/mpparse.c:231: warning: comparison is
 > always false due to limited range of data type".
 > Maybe this patch will be helpful.
 > 
 > diff -Nurp linux-2.6.18-rc6-git4-/arch/i386/kernel/mpparse.c linux-2.6.18-rc6-git4/arch/i386/kernel/mpparse.c
 > --- linux-2.6.18-rc6-git4-/arch/i386/kernel/mpparse.c	2006-09-13 00:01:00.000000000 +0200
 > +++ linux-2.6.18-rc6-git4/arch/i386/kernel/mpparse.c	2006-09-13 00:01:00.000000000 +0200
 > @@ -228,12 +228,14 @@ static void __init MP_bus_info (struct m
 >  
 >  	mpc_oem_bus_info(m, str, translation_table[mpc_record]);
 >  
 > +#if 0xFF >= MAX_MP_BUSSES
 >  	if (m->mpc_busid >= MAX_MP_BUSSES) {
 >  		printk(KERN_WARNING "MP table busid value (%d) for bustype %s "
 >  			" is too large, max. supported is %d\n",
 >  			m->mpc_busid, str, MAX_MP_BUSSES - 1);
 >  		return;
 >  	}
 > +#endif

mpc_busid is a uchar. I don't see how this can ever be > 0xff, yet
mach-summit and mach-generic have MAX_MP_BUSSES set to 260.

I don't see how this can possibly work.

	Dave

Re: [PATCH] mpparse.c:231: warning: comparison is always false

[Jarek Poplawski]

On Wed, Sep 13, 2006 at 12:42:51PM -0400, Dave Jones wrote:
> On Wed, Sep 13, 2006 at 08:50:10AM +0200, Jarek Poplawski wrote:
...  
>  > +#if 0xFF >= MAX_MP_BUSSES
>  >  	if (m->mpc_busid >= MAX_MP_BUSSES) {
>  >  		printk(KERN_WARNING "MP table busid value (%d) for bustype %s "
>  >  			" is too large, max. supported is %d\n",
>  >  			m->mpc_busid, str, MAX_MP_BUSSES - 1);
>  >  		return;
>  >  	}
>  > +#endif
> 
> mpc_busid is a uchar. I don't see how this can ever be > 0xff, yet
> mach-summit and mach-generic have MAX_MP_BUSSES set to 260.
> 
> I don't see how this can possibly work.
> 
> 	Dave
> 

0xFF >= 260 is false so the block is not compiled and
the warning is gone (+ several bytes of useless code).

Jarek P.

Re: [PATCH] mpparse.c:231: warning: comparison is always false

[Dave Jones]

On Thu, Sep 14, 2006 at 07:36:47AM +0200, Jarek Poplawski wrote:
 > On Wed, Sep 13, 2006 at 12:42:51PM -0400, Dave Jones wrote:
 > > On Wed, Sep 13, 2006 at 08:50:10AM +0200, Jarek Poplawski wrote:
 > ...  
 > >  > +#if 0xFF >= MAX_MP_BUSSES
 > >  >  	if (m->mpc_busid >= MAX_MP_BUSSES) {
 > >  >  		printk(KERN_WARNING "MP table busid value (%d) for bustype %s "
 > >  >  			" is too large, max. supported is %d\n",
 > >  >  			m->mpc_busid, str, MAX_MP_BUSSES - 1);
 > >  >  		return;
 > >  >  	}
 > >  > +#endif
 > > 
 > > mpc_busid is a uchar. I don't see how this can ever be > 0xff, yet
 > > mach-summit and mach-generic have MAX_MP_BUSSES set to 260.
 > > 
 > > I don't see how this can possibly work.
 > 
 > 0xFF >= 260 is false so the block is not compiled and
 > the warning is gone (+ several bytes of useless code).

Yes, I understand your patch. What I don't understand is why summit/generic
have this set so high in the first place.

	Dave

Re: [PATCH] mpparse.c:231: warning: comparison is always false

[Jarek Poplawski]

On Thu, Sep 14, 2006 at 02:35:42AM -0400, Dave Jones wrote:
...  
> Yes, I understand your patch. What I don't understand is why summit/generic
> have this set so high in the first place.

Sorry for misunderstanding your understanding...
I simply tried to understand another question (how this warning
could be tolerated for all those gits?).

Jarek P.  

Re: [PATCH] mpparse.c:231: warning: comparison is always false

[Andi Kleen]

On Wednesday 13 September 2006 18:42, Dave Jones wrote:
> On Wed, Sep 13, 2006 at 08:50:10AM +0200, Jarek Poplawski wrote:
>  > Probably after 2.6.18-rc6-git1 there is this cc warning:
>  > "arch/i386/kernel/mpparse.c:231: warning: comparison is
>  > always false due to limited range of data type".
>  > Maybe this patch will be helpful.
>  >

It's already fixed.

-Andi

Re: [PATCH] mpparse.c:231: warning: comparison is always false

[Dave Jones]

On Thu, Sep 14, 2006 at 12:12:50PM +0200, Andi Kleen wrote:
 > On Wednesday 13 September 2006 18:42, Dave Jones wrote:
 > > On Wed, Sep 13, 2006 at 08:50:10AM +0200, Jarek Poplawski wrote:
 > >  > Probably after 2.6.18-rc6-git1 there is this cc warning:
 > >  > "arch/i386/kernel/mpparse.c:231: warning: comparison is
 > >  > always false due to limited range of data type".
 > >  > Maybe this patch will be helpful.
 > >  >
 > 
 > It's already fixed.
 > 
 > -Andi

Still looks busted to me in 2.6.18rc7

	Dave 

Re: [PATCH] mpparse.c:231: warning: comparison is always false

[Andi Kleen]

On Thursday 14 September 2006 18:08, Dave Jones wrote:
> On Thu, Sep 14, 2006 at 12:12:50PM +0200, Andi Kleen wrote:
>  > On Wednesday 13 September 2006 18:42, Dave Jones wrote:
>  > > On Wed, Sep 13, 2006 at 08:50:10AM +0200, Jarek Poplawski wrote:
>  > >  > Probably after 2.6.18-rc6-git1 there is this cc warning:
>  > >  > "arch/i386/kernel/mpparse.c:231: warning: comparison is
>  > >  > always false due to limited range of data type".
>  > >  > Maybe this patch will be helpful.
>  >
>  > It's already fixed.
>  >
>  > -Andi
>
> Still looks busted to me in 2.6.18rc7

Fixed in the queued for 2.6.19 tree. I didn't consider it important enough
to move up.

-Andi

Re: [PATCH] mpparse.c:231: warning: comparison is always false

[Jarek Poplawski]

On Thu, Sep 14, 2006 at 06:17:54PM -0700, Andrew Morton wrote:
> On Wed, 13 Sep 2006 08:50:10 +0200
> Jarek Poplawski <jarkao2@o2.pl> wrote:
> 
> > Hello,
> > 
> > Probably after 2.6.18-rc6-git1 there is this cc warning: 
> > "arch/i386/kernel/mpparse.c:231: warning: comparison is
> > always false due to limited range of data type".
> > Maybe this patch will be helpful.
> > 
> 
> Thanks.   Andi has already queued a similar patch.
> 
> Andi, you might as well scoot that upstream, otherwise we'll get lots of
> emails about it.
...
> > +#if 0xFF >= MAX_MP_BUSSES
> >  	if (m->mpc_busid >= MAX_MP_BUSSES) {

As a matter of fact today I think my patch is wrong.

I don't know how Andi has fixed it, but after rethinking
the question of Dave Jones I see it's fixing the result
instead of the source of a problem (char or not char).

So it's more serious problem just for really serious guys
like you.

Thanks for response,

Jarek P. 

Re: [PATCH] mpparse.c:231: warning: comparison is always false

[Andrew Morton]

On Fri, 15 Sep 2006 10:11:23 +0200
Jarek Poplawski <jarkao2@o2.pl> wrote:

> On Thu, Sep 14, 2006 at 06:17:54PM -0700, Andrew Morton wrote:
> > On Wed, 13 Sep 2006 08:50:10 +0200
> > Jarek Poplawski <jarkao2@o2.pl> wrote:
> > 
> > > Hello,
> > > 
> > > Probably after 2.6.18-rc6-git1 there is this cc warning: 
> > > "arch/i386/kernel/mpparse.c:231: warning: comparison is
> > > always false due to limited range of data type".
> > > Maybe this patch will be helpful.
> > > 
> > 
> > Thanks.   Andi has already queued a similar patch.
> > 
> > Andi, you might as well scoot that upstream, otherwise we'll get lots of
> > emails about it.
> ...
> > > +#if 0xFF >= MAX_MP_BUSSES
> > >  	if (m->mpc_busid >= MAX_MP_BUSSES) {
> 
> As a matter of fact today I think my patch is wrong.

No, I think it's OK.  Well, you had an off-by-one...

> I don't know how Andi has fixed it,

Same thing.  (He has `#if MAX_MP_BUSSES < 256').

> but after rethinking
> the question of Dave Jones I see it's fixing the result
> instead of the source of a problem (char or not char).

The mpc_busid field is set to eight-bits by BIOS; there's nothing we can do
about that...

Re: [PATCH] mpparse.c:231: warning: comparison is always false

[Jarek Poplawski]

On Fri, Sep 15, 2006 at 01:23:02AM -0700, Andrew Morton wrote:
> On Fri, 15 Sep 2006 10:11:23 +0200
> Jarek Poplawski <jarkao2@o2.pl> wrote:
> > As a matter of fact today I think my patch is wrong.
... 
> No, I think it's OK.  Well, you had an off-by-one...

just like the source:

 > +#if 0xFF >= MAX_MP_BUSSES
 >  	if (m->mpc_busid >= MAX_MP_BUSSES) {

...
> > but after rethinking
> > the question of Dave Jones I see it's fixing the result
> > instead of the source of a problem (char or not char).
> 
> The mpc_busid field is set to eight-bits by BIOS; there's nothing we can do
> about that...
 
So IMHO maybe: if we can know this only by BIOS it should be
eight-bits - if there is another way to get this: shouln't
you add second constant? Now it's unlogical for me (and it
induces this strange #ifs in the code instead of headers).

Jarek P.  

Re: [PATCH] mpparse.c:231: warning: comparison is always false

[Dave Jones]

On Fri, Sep 15, 2006 at 01:23:02AM -0700, Andrew Morton wrote:

 > > > Thanks.   Andi has already queued a similar patch.
 > > > 
 > > > Andi, you might as well scoot that upstream, otherwise we'll get lots of
 > > > emails about it.
 > > ...
 > > > > +#if 0xFF >= MAX_MP_BUSSES
 > > > >  	if (m->mpc_busid >= MAX_MP_BUSSES) {
 > > I don't know how Andi has fixed it,
 > Same thing.  (He has `#if MAX_MP_BUSSES < 256').

How can this be the right the right thing to do ?
It should *never* be >=256. mach-summit/mach-generic need fixing
to be 255, not this ridiculous band-aid.  Where did 260 come from anyway?
 
	Dave 

Re: [PATCH] mpparse.c:231: warning: comparison is always false

[Andrew Morton]

On Fri, 15 Sep 2006 11:23:49 -0400
Dave Jones <davej@redhat.com> wrote:

> On Fri, Sep 15, 2006 at 01:23:02AM -0700, Andrew Morton wrote:
> 
>  > > > Thanks.   Andi has already queued a similar patch.
>  > > > 
>  > > > Andi, you might as well scoot that upstream, otherwise we'll get lots of
>  > > > emails about it.
>  > > ...
>  > > > > +#if 0xFF >= MAX_MP_BUSSES
>  > > > >  	if (m->mpc_busid >= MAX_MP_BUSSES) {
>  > > I don't know how Andi has fixed it,
>  > Same thing.  (He has `#if MAX_MP_BUSSES < 256').
> 
> How can this be the right the right thing to do ?
> It should *never* be >=256. mach-summit/mach-generic need fixing
> to be 255, not this ridiculous band-aid.  Where did 260 come from anyway?
>  

commit f0bacaf5cec4e677a00b5ab06d95664d03a30f7a
Author: akpm <akpm>
Date:   Mon Apr 12 20:06:32 2004 +0000

    [PATCH] summmit: increase MAX_MP_BUSSES
    
    From: James Cleverdon <jamesclv@us.ibm.com>
    
    Bump up MAX_MP_BUSSES for summit/generic subarch to cope with big IBM x440
    systems.
    
    BKrev: 407af6c8l8rvwRmEU-JHTS98MurIZA

diff --git a/include/asm-i386/mach-generic/mach_mpspec.h b/include/asm-i386/mach-generic/mach_mpspec.h
index ef10cd2..fbb6a40 100644
--- a/include/asm-i386/mach-generic/mach_mpspec.h
+++ b/include/asm-i386/mach-generic/mach_mpspec.h
@@ -8,6 +8,8 @@ #define MAX_APICS 256
 
 #define MAX_IRQ_SOURCES 256
 
-#define MAX_MP_BUSSES 32
+/* Summit or generic (i.e. installer) kernels need lots of bus entries. */
+/* Maximum 256 PCI busses, plus 1 ISA bus in each of 4 cabinets. */
+#define MAX_MP_BUSSES 260
 
 #endif /* __ASM_MACH_MPSPEC_H */
diff --git a/include/asm-i386/mach-summit/mach_mpspec.h b/include/asm-i386/mach-summit/mach_mpspec.h
index ef10cd2..bc8f717 100644
--- a/include/asm-i386/mach-summit/mach_mpspec.h
+++ b/include/asm-i386/mach-summit/mach_mpspec.h
@@ -8,6 +8,7 @@ #define MAX_APICS 256
 
 #define MAX_IRQ_SOURCES 256
 
-#define MAX_MP_BUSSES 32
+/* Maximum 256 PCI busses, plus 1 ISA bus in each of 4 cabinets. */
+#define MAX_MP_BUSSES 260
 
 #endif /* __ASM_MACH_MPSPEC_H */

Re: [PATCH] mpparse.c:231: warning: comparison is always false

[Andi Kleen]

On Fri, Sep 15, 2006 at 12:33:40PM -0700, Andrew Morton wrote:
> On Fri, 15 Sep 2006 11:23:49 -0400
> Dave Jones <davej@redhat.com> wrote:
> 
> > On Fri, Sep 15, 2006 at 01:23:02AM -0700, Andrew Morton wrote:
> > 
> >  > > > Thanks.   Andi has already queued a similar patch.
> >  > > > 
> >  > > > Andi, you might as well scoot that upstream, otherwise we'll get lots of
> >  > > > emails about it.
> >  > > ...
> >  > > > > +#if 0xFF >= MAX_MP_BUSSES
> >  > > > >  	if (m->mpc_busid >= MAX_MP_BUSSES) {
> >  > > I don't know how Andi has fixed it,
> >  > Same thing.  (He has `#if MAX_MP_BUSSES < 256').
> > 
> > How can this be the right the right thing to do ?
> > It should *never* be >=256. mach-summit/mach-generic need fixing
> > to be 255, not this ridiculous band-aid.  Where did 260 come from anyway?
> >  
> 
> commit f0bacaf5cec4e677a00b5ab06d95664d03a30f7a
> Author: akpm <akpm>
> Date:   Mon Apr 12 20:06:32 2004 +0000
> 
>     [PATCH] summmit: increase MAX_MP_BUSSES
>     
>     From: James Cleverdon <jamesclv@us.ibm.com>
>     
>     Bump up MAX_MP_BUSSES for summit/generic subarch to cope with big IBM x440
>     systems.

The 260 is because ACPI can create larger busses and the summit
boxes only run with ACPI anyways.

-Andi