Re: Linux 2.6.27.27

[Krzysztof Oledzki]

On Wed, 22 Jul 2009, Krzysztof Oledzki wrote:

> On Wed, 22 Jul 2009, Krzysztof Oledzki wrote:
>
> > On Wed, 22 Jul 2009, Krzysztof Oledzki wrote:
> >
> > > On Tue, 21 Jul 2009, Linus Torvalds wrote:
> > >
> > > > On Tue, 21 Jul 2009, Linus Torvalds wrote:
> > > > > Anyway, I bet we can work around the compiler bug by just changing the
> > > > > type of "i" from "unsigned char" to be a plain "int".
> > > >
> > > > IOW, like this.
> > > >
> > > > 		Linus
> > > >
> > > > ---
> > > > drivers/video/fbmon.c |    4 ++--
> > > > 1 files changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
> > > > index 5c1a2c0..af4a15c 100644
> > > > --- a/drivers/video/fbmon.c
> > > > +++ b/drivers/video/fbmon.c
> > > > @@ -256,8 +256,8 @@ static void fix_edid(unsigned char *edid, int fix)
> > > >
> > > > static int edid_checksum(unsigned char *edid)
> > > > {
> > > > -	unsigned char i, csum = 0, all_null = 0;
> > > > -	int err = 0, fix = check_edid(edid);
> > > > +	unsigned csum = 0, all_null = 0;
> > > > +	int i, err = 0, fix = check_edid(edid);
> > > >
> > > > 	if (fix)
> > > > 		fix_edid(edid, fix);
> > >
> > > Wow! You guys rock! ;)
> > >
> > > Indeed, this simple change is enough to make my kernel bootable. However, 
> > > there is still something wrong. My console is now 80x30 instead of 128x48:
> > >
> > > -Console: switching to colour frame buffer device 128x48
> > > +Console: switching to colour frame buffer device 80x30
> > >
> > > So, it looks like the loop may be still miscompiled.
> > >
> > > The kernel is here:
> > > http://noc.axelspringer.pl/no-strict-overflow-vs-wrapv/vmlinux-fno-strict-overflow-fixed.bz2
> >
> > OK, by adding a simple debug printk I'm now sure that the problem is indeed 
> > in the loop:
> >
> > --- linux-2.6.27.27-a/drivers/video/fbmon.c	2009-07-20 05:45:22.000000000 
> > +0200
> > +++ linux-2.6.27.27-b/drivers/video/fbmon.c	2009-07-22 09:45:34.000000000 
> > +0200
> > @@ -272,6 +272,8 @@
> > 		err = 1;
> > 	}
> >
> > +	printk("edid_checksum debug: csum=%u, all_null=%u, err=%d\n", csum, 
> > all_null, err);
> > +
> > 	return err;
> > }
> >
> >
> > Here is a diff between a good and a bad kernel:
> >
> > -edid_checksum debug: csum=0, all_null=255, err=1
> > -edid_checksum debug: csum=0, all_null=255, err=1
> > -Console: switching to colour frame buffer device 128x48
> > +edid_checksum debug: csum=6400, all_null=255, err=0
> > +Console: switching to colour frame buffer device 80x30
> >
> > In the good one the function is called twice and it returns err=1 (==OK). 
> > In the bad kernel it returns 0 because csum!=0x00 (==6400).
>
> More fun.
>
> Actually, the problem is indepenent from -fno-strict-overflow and also exists 
> with -fwrapv. It is probably caused by "unsigned char i" to "int i" 
> translation and the variables reordering.
>
> With "unsigned char i" before "unsigned char csum = 0":
> edid_checksum loop: edid[0]=0, csum=0
> edid_checksum loop: edid[1]=255, csum=0
> edid_checksum loop: edid[2]=255, csum=255
> -edid_checksum loop: edid[3]=255, csum=254
> -edid_checksum loop: edid[4]=255, csum=253
> -edid_checksum loop: edid[5]=255, csum=252
> -edid_checksum loop: edid[6]=255, csum=251
> -edid_checksum loop: edid[7]=0, csum=250
> -edid_checksum loop: edid[8]=6, csum=250
> -edid_checksum loop: edid[9]=207, csum=0
> (...)
> -edid_checksum debug: csum=0, all_null=255, err=1 -> OK
>
> With "int i" after "unsigned char csum = 0":
> edid_checksum loop: edid[0]=0, csum=0
> edid_checksum loop: edid[1]=255, csum=0
> edid_checksum loop: edid[2]=255, csum=255
> +edid_checksum loop: edid[3]=255, csum=510
> +edid_checksum loop: edid[4]=255, csum=765
> +edid_checksum loop: edid[5]=255, csum=1020
> +edid_checksum loop: edid[6]=255, csum=1275
> +edid_checksum loop: edid[7]=0, csum=1530
> +edid_checksum loop: edid[8]=6, csum=1530
> +edid_checksum loop: edid[9]=207, csum=1536
> (...)
> +edid_checksum debug: csum=6400, all_null=255, err=0 -> ERROR
>
> Funny. ;)

OK. I'm just blind. The oryginal patch is wrong. Instead of:

-       unsigned char i, csum = 0, all_null = 0;
-       int err = 0, fix = check_edid(edid);
+       unsigned csum = 0, all_null = 0;
+       int i, err = 0, fix = check_edid(edid);


It should be:

-       unsigned char i, csum = 0, all_null = 0;
-       int err = 0, fix = check_edid(edid);
+       unsigned char csum = 0, all_null = 0;
+       int i, err = 0, fix = check_edid(edid);

The patch should not change "unsigned char (...) csum" to "unsigned (...) csum".

Best regards,

			Krzysztof Olędzki