[BUG] mtd redboot (also gcc 4.1 warning fix)

[BUG] mtd redboot (also gcc 4.1 warning fix)

[Daniel Walker]

unsigned long may not always be 32 bits, right ? This patch fixes the 
warning, but not the bug .

Fixes the following warning,

drivers/mtd/redboot.c: In function 'parse_redboot_partitions':
drivers/mtd/redboot.c:103: warning: passing argument 1 of '__swab32s' from incompatible pointer type
drivers/mtd/redboot.c:104: warning: passing argument 1 of '__swab32s' from incompatible pointer type
drivers/mtd/redboot.c:105: warning: passing argument 1 of '__swab32s' from incompatible pointer type
drivers/mtd/redboot.c:106: warning: passing argument 1 of '__swab32s' from incompatible pointer type
drivers/mtd/redboot.c:107: warning: passing argument 1 of '__swab32s' from incompatible pointer type
drivers/mtd/redboot.c:108: warning: passing argument 1 of '__swab32s' from incompatible pointer type
drivers/mtd/redboot.c:109: warning: passing argument 1 of '__swab32s' from incompatible pointer type

Signed-Off-By: Daniel Walker <dwalker@mvista.com>

Index: linux-2.6.16/drivers/mtd/redboot.c
===================================================================
--- linux-2.6.16.orig/drivers/mtd/redboot.c
+++ linux-2.6.16/drivers/mtd/redboot.c
@@ -100,13 +100,13 @@ static int parse_redboot_partitions(stru
 					/* The unsigned long fields were written with the
 					 * wrong byte sex, name and pad have no byte sex.
 					 */
-					swab32s(&buf[j].flash_base);
-					swab32s(&buf[j].mem_base);
-					swab32s(&buf[j].size);
-					swab32s(&buf[j].entry_point);
-					swab32s(&buf[j].data_length);
-					swab32s(&buf[j].desc_cksum);
-					swab32s(&buf[j].file_cksum);
+					swab32s((unsigned int *)&buf[j].flash_base);
+					swab32s((unsigned int *)&buf[j].mem_base);
+					swab32s((unsigned int *)&buf[j].size);
+					swab32s((unsigned int *)&buf[j].entry_point);
+					swab32s((unsigned int *)&buf[j].data_length);
+					swab32s((unsigned int *)&buf[j].desc_cksum);
+					swab32s((unsigned int *)&buf[j].file_cksum);
 				}
 			}
 			break;

Re: [BUG] mtd redboot (also gcc 4.1 warning fix)

[Matheus Izvekov]

On 5/9/06, Daniel Walker <dwalker@mvista.com> wrote:
> unsigned long may not always be 32 bits, right ? This patch fixes the
Incorrect, its defined as 32bits for every standard C compiler

Re: [BUG] mtd redboot (also gcc 4.1 warning fix)

[Daniel Walker]

On Wed, 2006-05-10 at 00:14 -0300, Matheus Izvekov wrote:
> On 5/9/06, Daniel Walker <dwalker@mvista.com> wrote:
> > unsigned long may not always be 32 bits, right ? This patch fixes the
> Incorrect, its defined as 32bits for every standard C compiler

My 64bit Athlon says it's 64bits (with gcc 3.4) .. Maybe the kernel uses
some compiler options to reduce the size .

Daniel 

Re: [BUG] mtd redboot (also gcc 4.1 warning fix)

[Olof Johansson]

On Wed, May 10, 2006 at 12:14:45AM -0300, Matheus Izvekov wrote:
> On 5/9/06, Daniel Walker <dwalker@mvista.com> wrote:
> >unsigned long may not always be 32 bits, right ? This patch fixes the
> Incorrect, its defined as 32bits for every standard C compiler

Wrong. The only environment I'm aware of that has only P64 is Win64.

Still, that's a bad patch, since it removes the warning without fixing
the bug. It's a valid warning, the underlying problem should be fixed
instead. It's better to keep the warning around until that's been done.


-Olof

Re: [BUG] mtd redboot (also gcc 4.1 warning fix)

[Daniel Walker]

On Tue, 2006-05-09 at 22:29 -0500, Olof Johansson wrote:
> On Wed, May 10, 2006 at 12:14:45AM -0300, Matheus Izvekov wrote:
> > On 5/9/06, Daniel Walker <dwalker@mvista.com> wrote:
> > >unsigned long may not always be 32 bits, right ? This patch fixes the
> > Incorrect, its defined as 32bits for every standard C compiler
> 
> Wrong. The only environment I'm aware of that has only P64 is Win64.
> 
> Still, that's a bad patch, since it removes the warning without fixing
> the bug. It's a valid warning, the underlying problem should be fixed
> instead. It's better to keep the warning around until that's been done.

I never claimed to fix it ..

Daniel

Re: [BUG] mtd redboot (also gcc 4.1 warning fix)

[Matheus Izvekov]

On 5/10/06, Olof Johansson <olof@lixom.net> wrote:
> On Wed, May 10, 2006 at 12:14:45AM -0300, Matheus Izvekov wrote:
> > On 5/9/06, Daniel Walker <dwalker@mvista.com> wrote:
> > >unsigned long may not always be 32 bits, right ? This patch fixes the
> > Incorrect, its defined as 32bits for every standard C compiler
>
> Wrong. The only environment I'm aware of that has only P64 is Win64.
>

Yeah sorry, now im aware of it, sorry for the noise

Re: [BUG] mtd redboot (also gcc 4.1 warning fix)

[Adrian Bunk]

On Tue, May 09, 2006 at 07:56:04PM -0700, Daniel Walker wrote:

> unsigned long may not always be 32 bits, right ? This patch fixes the 
> warning, but not the bug .
>...

IOW, all your patch does is to hide a bug?

Not exactly an improvement...

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [BUG] mtd redboot (also gcc 4.1 warning fix)

[Steven Rostedt]

On Wed, 10 May 2006, Adrian Bunk wrote:

> On Tue, May 09, 2006 at 07:56:04PM -0700, Daniel Walker wrote:
>
> > unsigned long may not always be 32 bits, right ? This patch fixes the
> > warning, but not the bug .
> >...
>
> IOW, all your patch does is to hide a bug?
>
> Not exactly an improvement...
>

Perfect example of what Andrew Morten stated in his keynote at LinuxTag.
He mentioned patches that fixed warnings but not the problem that they
warn about.  He stated that these are very dangerous, because, not only is
the problem not fixed, but now no one knows of the problem.

Daniel,

We appreciate the clean up of the kernel.  Just try to focus on the
obvious stuff, and maybe only flag those that you're not sure is still a
problem.  Email the author of the code, or kick his butt to get him to
move and fix it.

Unintialized variables and bad compares may not be as trivial as they
appear.  Look closely at them to see if there isn't really a problem that
lies underneath.

Thanks,

-- Steve

Re: [BUG] mtd redboot (also gcc 4.1 warning fix)

[Steven Rostedt]

On Wed, 10 May 2006, Matheus Izvekov wrote:

> On 5/9/06, Daniel Walker <dwalker@mvista.com> wrote:
> > unsigned long may not always be 32 bits, right ? This patch fixes the
> Incorrect, its defined as 32bits for every standard C compiler

Nope, I believe that long is always suppose to be big enough to hold a
pointer.  So if you have 64 bit pointers, long needs to be 64 bits.  That
way it is always safe to do:

void *func(void *p) {
	unsigned long l = (unsigned long)p;

	/* do stuff with l */

	p = (void*)l;
	return p;
}

-- Steve

Re: [BUG] mtd redboot (also gcc 4.1 warning fix)

[Daniel Walker]

On Wed, 2006-05-10 at 07:37 +0200, Adrian Bunk wrote:
> On Tue, May 09, 2006 at 07:56:04PM -0700, Daniel Walker wrote:
> 
> > unsigned long may not always be 32 bits, right ? This patch fixes the 
> > warning, but not the bug .
> >...
> 
> IOW, all your patch does is to hide a bug?
> 
> Not exactly an improvement...

This is a _bug report_ , not a fix .

Daniel

Re: [BUG] mtd redboot (also gcc 4.1 warning fix)

[Daniel Walker]

On Wed, 2006-05-10 at 05:00 -0400, Steven Rostedt wrote:

> Perfect example of what Andrew Morten stated in his keynote at LinuxTag.
> He mentioned patches that fixed warnings but not the problem that they
> warn about.  He stated that these are very dangerous, because, not only is
> the problem not fixed, but now no one knows of the problem.

This is the third time I've had to remind people that this email is a
bug report .. It's not a fix for anything , I even said as much in my
initial email.  (Also notice Andrew isn't CC'd on this one..)

Daniel