lkml.org 
[lkml]   [2009]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: Linux 2.6.27.27


    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
    \
     
     \ /
      Last update: 2009-07-22 12:49    [W:0.032 / U:120.576 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site