2006-09-13 06:46:23

by Jarek Poplawski

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

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;


2006-09-13 16:35:58

by Dave Jones

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

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

2006-09-14 05:32:59

by Jarek Poplawski

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

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.

2006-09-14 06:35:49

by Dave Jones

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

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

2006-09-14 07:32:43

by Jarek Poplawski

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

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.

2006-09-14 11:55:36

by Andi Kleen

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

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

2006-09-14 16:08:25

by Dave Jones

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

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

2006-09-15 08:07:35

by Jarek Poplawski

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

On Thu, Sep 14, 2006 at 06:17:54PM -0700, Andrew Morton wrote:
> On Wed, 13 Sep 2006 08:50:10 +0200
> Jarek Poplawski <[email protected]> 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.

2006-09-15 08:23:20

by Andrew Morton

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

On Fri, 15 Sep 2006 10:11:23 +0200
Jarek Poplawski <[email protected]> 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 <[email protected]> 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...

2006-09-15 08:28:43

by Andi Kleen

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

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

2006-09-15 09:04:28

by Jarek Poplawski

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

On Fri, Sep 15, 2006 at 01:23:02AM -0700, Andrew Morton wrote:
> On Fri, 15 Sep 2006 10:11:23 +0200
> Jarek Poplawski <[email protected]> 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.

2006-09-15 15:24:14

by Dave Jones

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

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

2006-09-15 19:34:19

by Andrew Morton

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

On Fri, 15 Sep 2006 11:23:49 -0400
Dave Jones <[email protected]> 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 <[email protected]>

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 */

2006-09-16 06:19:56

by Andi Kleen

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

On Fri, Sep 15, 2006 at 12:33:40PM -0700, Andrew Morton wrote:
> On Fri, 15 Sep 2006 11:23:49 -0400
> Dave Jones <[email protected]> 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 <[email protected]>
>
> 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