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


On Wed, 22 Jul 2009, Krzysztof Oledzki wrote:
> >
> > 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.
> >

Yes. I took a look at the -fixed binary you sent out. edid_checksum() is
now compiled to this (I added some notes on the side):

ffffffff803b37ed: <edid_checksum>:
53 push %rbx
48 89 fb mov %rdi,%rbx %ebx = edid

[... Calls to check_edid and fix_edid ... ]

31 c9 xor %ecx,%ecx csum = 0
31 f6 xor %esi,%esi all_null = 0
31 d2 xor %edx,%edx i = 0
L: 0f b6 04 1a movzbl (%rdx,%rbx,1),%eax %eax = *(i + edid)
48 ff c2 inc %rdx i++
01 c1 add %eax,%ecx csum += %eax
09 c6 or %eax,%esi all_null |= %eax
48 81 fa 80 00 00 00 cmp $0x80,%rdx
75 ec jne L if i != 80 goto L
85 c9 test %ecx,%ecx
0f 94 c0 sete %al %al == (csum == 0)
85 f6 test %esi,%esi
5b pop %rbx
0f 95 c2 setne %dl %dl == (all_null == 0)
21 d0 and %edx,%eax
0f b6 c0 movzbl %al,%eax %eax == (%al && %dl)
c3 retq return %eax

The problem is that csum is stored in %ecx (a 32-bit register) at all
times and is never truncated to a byte. In other words, the compiler is
treating csum like it's an 'int', not an 'unsigned char'.

> 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).

That makes sense - since csum is being treated like an 'int', it never
wraps, so it just ends up holding the total sum of all the bytes, which
apparently is 6400. Notice that 6400 % 256 == 0, so if it *had* wrapped,
it would have ended up being 0, as expected.

One "fix" might be to just make 'csum' an 'int' (since that's what the
compiler seems to think anyway :p) and do the wrapping by hand (patch
below, if you want to try this).

However, I wouldn't be surprised if other kernel functions are also being
miscompiled. It seems to me that any function that does arithmetic on
'unsigned char's and depends on the wrapping behaviour could potentially
be broken...

Best regards,

Troy

diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
index 5c1a2c0..6802b4c 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 char all_null = 0;
+ int i, csum = 0, err = 0, fix = check_edid(edid);

if (fix)
fix_edid(edid, fix);
@@ -267,7 +267,7 @@ static int edid_checksum(unsigned char *edid)
all_null |= edid[i];
}

- if (csum == 0x00 && all_null) {
+ if ((csum & 0xff) == 0x00 && all_null) {
/* checksum passed, everything's good */
err = 1;
}

\
 
 \ /
  Last update: 2009-07-22 13:03    [W:0.162 / U:2.256 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site