oops with asus_acpi on P30/P35

oops with asus_acpi on P30/P35

[Christian Aichinger]

[-- Attachment #1.1: Type: text/plain, Size: 915 bytes --]

I've read the other thread about this problem on acpi-devel, and I'm
running into the same thing here.

The oops and ksymoops output is attatched.

I went to debug the problem.

The oops occurs in asus_acpi.c, line 1016:
    hotk->model = END_MODEL;
	if (strncmp(model->string.pointer, "L3D", 3) == 0) // <-- OOPS
		hotk->model = L3D;

Some added debug printk's later it turned out that:
model->type == ACPI_TYPE_INTEGER
model->integer.value == 56

This is why the problem wasn't fixed by your patch, since that
resided in the if (model->type == ACPI_TYPE_STRING) code-path.
I've attatched a patch that works for me, but IMHO it's ugly and
only fixes the sympthoms. Why do we get an integer here in the first
place?

Please mail me if you have more questions, stuff for me to try out,
or eventually figure out what's going on here.

Cheers,
Christian Aichinger

PS: Please CC: me, I'm not subscribed to acpi-devel.

[-- Attachment #1.2: oops.txt --]
[-- Type: text/plain, Size: 1724 bytes --]

Unable to handle kernel NULL pointer dereference at virtual address 00000000
 printing eip:
e1a833ae
*pde = 00000000
Oops: 0000 [#1]
PREEMPT 
Modules linked in: asus_acpi acpi_cpufreq freq_table pcmcia thermal fan button processor ac battery af_packet eth1394 ohci1394 yenta_socket rsrc_nonstatic pcmcia_core 8139too mii snd_intel8x0m snd_intel8x0 snd_ac97_codec i2c_i801 ehci_hcd usbhid uhci_hcd usbcore shpchp pci_hotplug mousedev joydev nls_utf8 ntfs cpuid sbp2 scsi_mod ieee1394 psmouse genrtc evdev
CPU:    0
EIP:    0060:[<e1a833ae>]    Not tainted VLI
EFLAGS: 00010203   (2.6.12-rc6r20050617d) 
EIP is at asus_hotk_get_info+0x1b7/0x7a4 [asus_acpi]
eax: 00000000   ebx: dd05ff38   ecx: 00000002   edx: 00000003
esi: 00000000   edi: e1a9aaf4   ebp: def70840   esp: dd05fefc
ds: 007b   es: 007b   ss: 0068
Process modprobe (pid: 3997, threadinfo=dd05e000 task=df5a0060)
Stack: 00000000 00000002 dff52700 c14dcf34 c0453898 c01cf6d0 c0453898 00000000 
       dd05ff20 00000000 df994100 dff52700 00000000 00005105 dd7b8000 00000010 
       def70840 dfe0b000 e1a9aeb4 dfe0b084 00000000 e1a839cf e1a83a63 dfe0b000 
Call Trace:
 [<c01cf6d0>] idr_get_new_above_int+0x90/0x130
 [<e1a839cf>] asus_hotk_check+0x34/0x35 [asus_acpi]
 [<e1a83a63>] asus_hotk_add+0x93/0x158 [asus_acpi]
 [<c0216436>] acpi_bus_driver_init+0x2c/0x8a
 [<c02164f9>] acpi_driver_attach+0x65/0xab
 [<e1a83b6a>] asus_acpi_init+0x42/0x6e [asus_acpi]
 [<c0137704>] sys_init_module+0x154/0x200
 [<c0103245>] syscall_call+0x7/0xb
Code: e1 e8 b7 7b 69 de 5e 5f a1 90 ce a9 e1 ba 03 00 00 00 bf f4 aa a9 e1 89 d1 c7 40 14 12 00 00 00 8b 45 08 89 c6 89 04 24 49 78 08 <ac> ae 75 08 84 c0 75 f5 31 c0 eb 04 19 c0 0c 01 85 c0 75 11 a1 
 

[-- Attachment #1.3: ksymoops.txt --]
[-- Type: text/plain, Size: 5190 bytes --]

ksymoops 2.4.9 on i686 2.6.12-rc6r20050617d.  Options used
     -V (default)
     -k /tmp/kallsyms (specified)
     -l /proc/modules (default)
     -o /lib/modules/2.6.12-rc6r20050617d/ (default)
     -m /boot/System.map-2.6.12-rc6r20050617d (default)

Warning (read_ksyms): no kernel symbols in ksyms, is /tmp/kallsyms a valid ksyms file?
No modules in ksyms, skipping objects
No ksyms, skipping lsmod
ehci_hcd 0000:00:1d.7: debug port 1
8139too Fast Ethernet driver 0.9.27
Unable to handle kernel NULL pointer dereference at virtual address 00000000
e1a833ae
*pde = 00000000
Oops: 0000 [#1]
CPU:    0
EIP:    0060:[<e1a833ae>]    Not tainted VLI
Using defaults from ksymoops -t elf32-i386 -a i386
EFLAGS: 00010203   (2.6.12-rc6r20050617d) 
eax: 00000000   ebx: dd05ff38   ecx: 00000002   edx: 00000003
esi: 00000000   edi: e1a9aaf4   ebp: def70840   esp: dd05fefc
ds: 007b   es: 007b   ss: 0068
Stack: 00000000 00000002 dff52700 c14dcf34 c0453898 c01cf6d0 c0453898 00000000 
       dd05ff20 00000000 df994100 dff52700 00000000 00005105 dd7b8000 00000010 
       def70840 dfe0b000 e1a9aeb4 dfe0b084 00000000 e1a839cf e1a83a63 dfe0b000 
Call Trace:
 [<c01cf6d0>] idr_get_new_above_int+0x90/0x130
 [<e1a839cf>] asus_hotk_check+0x34/0x35 [asus_acpi]
 [<e1a83a63>] asus_hotk_add+0x93/0x158 [asus_acpi]
 [<c0216436>] acpi_bus_driver_init+0x2c/0x8a
 [<c02164f9>] acpi_driver_attach+0x65/0xab
 [<e1a83b6a>] asus_acpi_init+0x42/0x6e [asus_acpi]
 [<c0137704>] sys_init_module+0x154/0x200
 [<c0103245>] syscall_call+0x7/0xb
Code: e1 e8 b7 7b 69 de 5e 5f a1 90 ce a9 e1 ba 03 00 00 00 bf f4 aa a9 e1 89 d1 c7 40 14 12 00 00 00 8b 45 08 89 c6 89 04 24 49 78 08 <ac> ae 75 08 84 c0 75 f5 31 c0 eb 04 19 c0 0c 01 85 c0 75 11 a1 


>>EIP; e1a833ae <pg0+216123ae/3fb8d400>   <=====

>>ebx; dd05ff38 <pg0+1cbeef38/3fb8d400>
>>edi; e1a9aaf4 <pg0+21629af4/3fb8d400>
>>ebp; def70840 <pg0+1eaff840/3fb8d400>
>>esp; dd05fefc <pg0+1cbeeefc/3fb8d400>

Trace; c01cf6d0 <idr_get_new_above_int+90/130>
Trace; e1a839cf <pg0+216129cf/3fb8d400>
Trace; e1a83a63 <pg0+21612a63/3fb8d400>
Trace; c0216436 <acpi_bus_driver_init+2c/8a>
Trace; c02164f9 <acpi_driver_attach+65/ab>
Trace; e1a83b6a <pg0+21612b6a/3fb8d400>
Trace; c0137704 <sys_init_module+154/200>
Trace; c0103245 <syscall_call+7/b>

This architecture has variable length instructions, decoding before eip
is unreliable, take these instructions with a pinch of salt.

Code;  e1a83383 <pg0+21612383/3fb8d400>
00000000 <_EIP>:
Code;  e1a83383 <pg0+21612383/3fb8d400>
   0:   e1 e8                     loope  ffffffea <_EIP+0xffffffea>
Code;  e1a83385 <pg0+21612385/3fb8d400>
   2:   b7 7b                     mov    $0x7b,%bh
Code;  e1a83387 <pg0+21612387/3fb8d400>
   4:   69 de 5e 5f a1 90         imul   $0x90a15f5e,%esi,%ebx
Code;  e1a8338d <pg0+2161238d/3fb8d400>
   a:   ce                        into   
Code;  e1a8338e <pg0+2161238e/3fb8d400>
   b:   a9 e1 ba 03 00            test   $0x3bae1,%eax
Code;  e1a83393 <pg0+21612393/3fb8d400>
  10:   00 00                     add    %al,(%eax)
Code;  e1a83395 <pg0+21612395/3fb8d400>
  12:   bf f4 aa a9 e1            mov    $0xe1a9aaf4,%edi
Code;  e1a8339a <pg0+2161239a/3fb8d400>
  17:   89 d1                     mov    %edx,%ecx
Code;  e1a8339c <pg0+2161239c/3fb8d400>
  19:   c7 40 14 12 00 00 00      movl   $0x12,0x14(%eax)
Code;  e1a833a3 <pg0+216123a3/3fb8d400>
  20:   8b 45 08                  mov    0x8(%ebp),%eax
Code;  e1a833a6 <pg0+216123a6/3fb8d400>
  23:   89 c6                     mov    %eax,%esi
Code;  e1a833a8 <pg0+216123a8/3fb8d400>
  25:   89 04 24                  mov    %eax,(%esp)
Code;  e1a833ab <pg0+216123ab/3fb8d400>
  28:   49                        dec    %ecx
Code;  e1a833ac <pg0+216123ac/3fb8d400>
  29:   78 08                     js     33 <_EIP+0x33>

This decode from eip onwards should be reliable

Code;  e1a833ae <pg0+216123ae/3fb8d400>
00000000 <_EIP>:
Code;  e1a833ae <pg0+216123ae/3fb8d400>   <=====
   0:   ac                        lods   %ds:(%esi),%al   <=====
Code;  e1a833af <pg0+216123af/3fb8d400>
   1:   ae                        scas   %es:(%edi),%al
Code;  e1a833b0 <pg0+216123b0/3fb8d400>
   2:   75 08                     jne    c <_EIP+0xc>
Code;  e1a833b2 <pg0+216123b2/3fb8d400>
   4:   84 c0                     test   %al,%al
Code;  e1a833b4 <pg0+216123b4/3fb8d400>
   6:   75 f5                     jne    fffffffd <_EIP+0xfffffffd>
Code;  e1a833b6 <pg0+216123b6/3fb8d400>
   8:   31 c0                     xor    %eax,%eax
Code;  e1a833b8 <pg0+216123b8/3fb8d400>
   a:   eb 04                     jmp    10 <_EIP+0x10>
Code;  e1a833ba <pg0+216123ba/3fb8d400>
   c:   19 c0                     sbb    %eax,%eax
Code;  e1a833bc <pg0+216123bc/3fb8d400>
   e:   0c 01                     or     $0x1,%al
Code;  e1a833be <pg0+216123be/3fb8d400>
  10:   85 c0                     test   %eax,%eax
Code;  e1a833c0 <pg0+216123c0/3fb8d400>
  12:   75 11                     jne    25 <_EIP+0x25>
Code;  e1a833c2 <pg0+216123c2/3fb8d400>
  14:   a1                        .byte 0xa1


1 warning issued.  Results may not be reliable.

[-- Attachment #1.4: asus_acpi.c-p35-fix.diff --]
[-- Type: text/plain, Size: 801 bytes --]

diff --git a/drivers/acpi/asus_acpi.c b/drivers/acpi/asus_acpi.c
--- a/drivers/acpi/asus_acpi.c
+++ b/drivers/acpi/asus_acpi.c
@@ -993,6 +993,7 @@ static int __init asus_hotk_get_info(voi
 	/* Samsung P30 has a device with a valid _HID whose INIT does not 
 	 * return anything. Catch this one and any similar here */
 	if (buffer.pointer == NULL) {
+try_p30:
 		if (asus_info && /* Samsung P30 */
 		    strncmp(asus_info->oem_table_id, "ODEM", 4) == 0) {
 			hotk->model = P30;
@@ -1012,6 +1013,9 @@ static int __init asus_hotk_get_info(voi
 		printk(KERN_NOTICE "  %s model detected, ", model->string.pointer);
 	}
 
+	if (model->type != ACPI_TYPE_STRING)
+		goto try_p30;
+
 	hotk->model = END_MODEL;
 	if (strncmp(model->string.pointer, "L3D", 3) == 0)
 		hotk->model = L3D;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: oops with asus_acpi on P30/P35

[Karol Kozimor]

Thus wrote Christian Aichinger:
> The oops occurs in asus_acpi.c, line 1016:
>     hotk->model = END_MODEL;
> 	if (strncmp(model->string.pointer, "L3D", 3) == 0) // <-- OOPS
> 		hotk->model = L3D;
> 
> Some added debug printk's later it turned out that:
> model->type == ACPI_TYPE_INTEGER
> model->integer.value == 56

Thanks, I suspected that but I'm really swamped both with Real Work and
my exams so I'm slow even with catching up with the lists...

> This is why the problem wasn't fixed by your patch, since that
> resided in the if (model->type == ACPI_TYPE_STRING) code-path.
> I've attatched a patch that works for me, but IMHO it's ugly and
> only fixes the sympthoms. Why do we get an integer here in the first
> place?

Bob, is the implicit return code supposed to trigger also when using
acpi_evaluate_object()? FYI, this is what we currently do:

write_acpi_int(hotk->handle, "INIT", 0, &buffer) (drivers/acpi/asus_acpi.c)

which is fine if the INIT method returns a string (the usual), but
apparently not if there is no return statement in the method (the P30
case). The old code assumed the buffer will be null in this case.

Is that a bug in the ACPICA or should the asus_acpi code cover for other
cases of buffer.type?

Best regards,

-- 
Karol 'sziwan' Kozimor
sziwan-DETuoxkZsSqrDJvtcaxF/A@public.gmane.org


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click

Re: oops with asus_acpi on P30/P35

[Carl-Daniel Hailfinger]

Karol Kozimor schrieb:
> Thus wrote Christian Aichinger:
> 
>>The oops occurs in asus_acpi.c, line 1016:
>>    hotk->model = END_MODEL;
>>	if (strncmp(model->string.pointer, "L3D", 3) == 0) // <-- OOPS
>>		hotk->model = L3D;
>>
>>Some added debug printk's later it turned out that:
>>model->type == ACPI_TYPE_INTEGER
>>model->integer.value == 56
> 
> 
> Thanks, I suspected that but I'm really swamped both with Real Work and
> my exams so I'm slow even with catching up with the lists...
> 
> 
>>This is why the problem wasn't fixed by your patch, since that
>>resided in the if (model->type == ACPI_TYPE_STRING) code-path.
>>I've attatched a patch that works for me, but IMHO it's ugly and
>>only fixes the sympthoms. Why do we get an integer here in the first
>>place?
> 
> 
> Bob, is the implicit return code supposed to trigger also when using
> acpi_evaluate_object()? FYI, this is what we currently do:
> 
> write_acpi_int(hotk->handle, "INIT", 0, &buffer) (drivers/acpi/asus_acpi.c)
> 
> which is fine if the INIT method returns a string (the usual), but
> apparently not if there is no return statement in the method (the P30
> case). The old code assumed the buffer will be null in this case.
> 
> Is that a bug in the ACPICA or should the asus_acpi code cover for other
> cases of buffer.type?

Will the fix for this be submitted to stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org? I can't upgrade
to 2.6.12.1 because of this oops.

Regards,
Carl-Daniel
-- 
http://www.hailfinger.org/


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click

Re: oops with asus_acpi on P30/P35

[Karol Kozimor]

Thus wrote Carl-Daniel Hailfinger:
>> Bob, is the implicit return code supposed to trigger also when using
>> acpi_evaluate_object()? FYI, this is what we currently do:
>> 
>> write_acpi_int(hotk->handle, "INIT", 0, &buffer) (drivers/acpi/asus_acpi.c)
>> 
>> which is fine if the INIT method returns a string (the usual), but
>> apparently not if there is no return statement in the method (the P30
>> case). The old code assumed the buffer will be null in this case.
>> 
>> Is that a bug in the ACPICA or should the asus_acpi code cover for other
>> cases of buffer.type?
> 
> Will the fix for this be submitted to stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org? I can't upgrade
> to 2.6.12.1 because of this oops.

This should fix ya for now, but I can't sign it off until I get a comment
from Bob.
Best regards,

-- 
Karol 'sziwan' Kozimor
sziwan-DETuoxkZsSqrDJvtcaxF/A@public.gmane.org


--- linux-2.6.12/drivers/acpi/asus_acpi.c~	2005-06-29 17:37:29.000000000 +0200
+++ linux-2.6.12/drivers/acpi/asus_acpi.c	2005-06-29 17:45:53.000000000 +0200
@@ -990,9 +990,15 @@
 	else if (bsts_result)
 		printk(KERN_NOTICE "  BSTS called, 0x%02x returned\n", bsts_result);
 
+	if (buffer.pointer == NULL)
+		return -EINVAL;
+	model = (union acpi_object *) buffer.pointer;
+
 	/* Samsung P30 has a device with a valid _HID whose INIT does not 
 	 * return anything. Catch this one and any similar here */
-	if (buffer.pointer == NULL) {
+	if (model->type == ACPI_TYPE_STRING) {
+		printk(KERN_NOTICE "  %s model detected, ", model->string.pointer);
+	} else {
 		if (asus_info && /* Samsung P30 */
 		    strncmp(asus_info->oem_table_id, "ODEM", 4) == 0) {
 			hotk->model = P30;
@@ -1007,11 +1013,6 @@
 		return AE_OK;
 	}
 	
-	model = (union acpi_object *) buffer.pointer;
-	if (model->type == ACPI_TYPE_STRING) {
-		printk(KERN_NOTICE "  %s model detected, ", model->string.pointer);
-	}
-
 	hotk->model = END_MODEL;
 	if (strncmp(model->string.pointer, "L3D", 3) == 0)
 		hotk->model = L3D;


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click

Re: oops with asus_acpi on P30/P35

[Hanno Böck]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

Am Mittwoch, 29. Juni 2005 17:50 schrieb Karol Kozimor:
> Thus wrote Carl-Daniel Hailfinger:
> >> Bob, is the implicit return code supposed to trigger also when using
> >> acpi_evaluate_object()? FYI, this is what we currently do:
> >>
> >> write_acpi_int(hotk->handle, "INIT", 0, &buffer)
> >> (drivers/acpi/asus_acpi.c)
> >>
> >> which is fine if the INIT method returns a string (the usual), but
> >> apparently not if there is no return statement in the method (the P30
> >> case). The old code assumed the buffer will be null in this case.
> >>
> >> Is that a bug in the ACPICA or should the asus_acpi code cover for other
> >> cases of buffer.type?
> >
> > Will the fix for this be submitted to stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org? I can't upgrade
> > to 2.6.12.1 because of this oops.
>
> This should fix ya for now, but I can't sign it off until I get a comment
> from Bob.

This is still not fixed within 2.6.13_rc6. As 2.6.13 seems to be on the way 
and this bug is really grave/makes a piece of hardware unusable, can this be 
pushed to the kernel tree as soon as possible?

cu,

Hanno

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: oops with asus_acpi on P30/P35

[Timo Hoenig]

[-- Attachment #1: Type: text/plain, Size: 579 bytes --]

Hi,

On Fri, 2005-08-19 at 17:49 +0200, Hanno Böck wrote:

[...]

> This is still not fixed within 2.6.13_rc6. As 2.6.13 seems to be on the way 
> and this bug is really grave/makes a piece of hardware unusable, can this be 
> pushed to the kernel tree as soon as possible?

This patch does fix the Samsung P30 but this fix will also break support
for other systems (like the ASUS W5A), too.  To get the W5A working
again, it needs something like the attached patch.

I assume that other models will stop working, too.

> cu,
> 
> Hanno

See you,

   Timo

[-- Attachment #2: asus_w5a.patch --]
[-- Type: text/x-patch, Size: 600 bytes --]

--- acpi_asus.c	2005-08-21 16:33:15.000000000 +0100
+++ acpu_asus.c	2005-08-21 16:33:41.000000000 +0100
@@ -1088,6 +1089,9 @@
 	 * return anything. Catch this one and any similar here */
 	if (model->type == ACPI_TYPE_STRING) {
 		printk(KERN_NOTICE "  %s model detected, ", model->string.pointer);
+	} else if (asus_info && /* Asus W5A needs this. Maybe others? */
+			strncmp(asus_info->oem_table_id, "0AAAA000", 8) == 0) {
+			printk(KERN_NOTICE "Found Asus ACPI without model type?\n");
 	} else {
 		if (asus_info && /* Samsung P30 */
 		    strncmp(asus_info->oem_table_id, "ODEM", 4) == 0) {

Re: oops with asus_acpi on P30/P35

[Christian Aichinger]

[-- Attachment #1: Type: text/plain, Size: 3562 bytes --]

On Sun, Aug 21, 2005 at 04:36:17PM +0200, Timo Hoenig wrote:
> Hi,
> 
> On Fri, 2005-08-19 at 17:49 +0200, Hanno Böck wrote:
> 
> [...]
> 
> > This is still not fixed within 2.6.13_rc6. As 2.6.13 seems to be on the way 
> > and this bug is really grave/makes a piece of hardware unusable, can this be 
> > pushed to the kernel tree as soon as possible?
> 
> This patch does fix the Samsung P30 but this fix will also break support
> for other systems (like the ASUS W5A), too.  To get the W5A working
> again, it needs something like the attached patch.
> 
> I assume that other models will stop working, too.

OK, it's getting a little frustrating that this still isn't fixed.
I've now written a patch that tests for ACPI_TYPE_INTEGER, and it
tests for possible return values from P35's INIT.

Since the relevant part hasn't changed between P30 -> P35's DSDT, I
assume that there has been some change in the Linux ACPI code, that
causes integer returns now instead of no returns. This means we
could get rid of the P30 code in the (!buffer.pointer) path. I don't
have a P30 around to test this though :-/

I got the -1/0x58/0x38 values from my DSDT:
INIT there calls WLED, which calls \_SB.PCI0.LPCB.EC0.STC5, last
call with arg 0x58 or 0x38. That function then returns with either
Ones (which I gathered is ~0, i.e. -1) or with Arg0. That's the last
return that happens within the INIT call, so I thought that might be
the result of the call as a whole.

That fits nicely, since 0x58 is really returned on my laptop.

If this fix still breaks other laptops I'd really be puzzled, since
currently the value is dereferenced later when doing the string
comparisons. So the patch at least shouldn't break it more than it
already is ;)

I can resubmit my patch with a proper changelog entry and
Signed-off-by line if it should be merged.

Cheers,
Christian Aichinger

---
c51f431351c648519a9b91de3c5e1d636246d7bc
diff --git a/drivers/acpi/asus_acpi.c b/drivers/acpi/asus_acpi.c
--- a/drivers/acpi/asus_acpi.c
+++ b/drivers/acpi/asus_acpi.c
@@ -1006,6 +1006,24 @@ static int __init asus_hotk_get_info(voi
 	}
 
 	model = (union acpi_object *)buffer.pointer;
+
+	/* INIT on Samsung's P35 returns an integer, possible return
+	 * values are tested below */
+	if (model->type == ACPI_TYPE_INTEGER) {
+		if (model->integer.value == -1 ||
+			model->integer.value == 0x58 ||
+			model->integer.value == 0x38) {
+			hotk->model = P30;
+			printk(KERN_NOTICE
+				       "  Samsung P35 detected, supported\n");
+			goto out_known;
+		} else {
+			printk(KERN_WARNING 
+				"  unknown integer returned by INIT\n");
+			goto out_unknown;
+		}
+	}
+
 	if (model->type == ACPI_TYPE_STRING) {
 		printk(KERN_NOTICE "  %s model detected, ",
 		       model->string.pointer);
@@ -1057,9 +1075,7 @@ static int __init asus_hotk_get_info(voi
 		hotk->model = L5x;
 
 	if (hotk->model == END_MODEL) {
-		printk("unsupported, trying default values, supply the "
-		       "developers with your DSDT\n");
-		hotk->model = M2E;
+		goto out_unknown;
 	} else {
 		printk("supported\n");
 	}
@@ -1088,6 +1104,15 @@ static int __init asus_hotk_get_info(voi
 	acpi_os_free(model);
 
 	return AE_OK;
+
+out_unknown:
+	printk(KERN_WARNING "  unsupported, trying default values, "
+			"supply the developers with your DSDT\n");
+	hotk->model = M2E;
+out_known:
+	hotk->methods = &model_conf[hotk->model];
+	acpi_os_free(model);
+	return AE_OK;
 }
 
 static int __init asus_hotk_check(void)

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: oops with asus_acpi on P30/P35

[Hanno Böck]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]

Am Wednesday, 21. September 2005 11:08 schrieb Christian Aichinger:
> Since the relevant part hasn't changed between P30 -> P35's DSDT, I
> assume that there has been some change in the Linux ACPI code, that
> causes integer returns now instead of no returns. This means we
> could get rid of the P30 code in the (!buffer.pointer) path. I don't
> have a P30 around to test this though :-/

I have a P30 around to test and I'd gladly help, but your patch doesn't apply 
to the current 2.6.14-rc1 and -git5 kernel.
Also note that afaik asus_acpi-cvs and kernel aren't in sync at the moment.

-- 
Hanno Böck		Blog:   http://www.hboeck.de/
GPG: 3DBD3B20		Jabber: jabber-60OJuG18Xr6zQB+pC5nmwQ@public.gmane.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: oops with asus_acpi on P30/P35

[Christian Aichinger]

[-- Attachment #1: Type: text/plain, Size: 2842 bytes --]

On Wed, Sep 21, 2005 at 01:47:10PM +0200, Hanno Böck wrote:
> Am Wednesday, 21. September 2005 11:08 schrieb Christian Aichinger:
> > Since the relevant part hasn't changed between P30 -> P35's DSDT, I
> > assume that there has been some change in the Linux ACPI code, that
> > causes integer returns now instead of no returns. This means we
> > could get rid of the P30 code in the (!buffer.pointer) path. I don't
> > have a P30 around to test this though :-/
> 
> I have a P30 around to test and I'd gladly help, but your patch doesn't apply 
> to the current 2.6.14-rc1 and -git5 kernel.
> Also note that afaik asus_acpi-cvs and kernel aren't in sync at the moment.

I think the conflict should be only a trivial whitespace thing
(according to git-whatchanged). The patch should apply fine to
2.6.14-rc2 (that had it's release mail on LKML on 2005-09-19, but
still isn't on kernel.org) or any recent git checkout.

I've attached a patch that should apply to -rc1. What would be
interesting is if dmesg shows "Samsung P35 detected", or "Samsung
P30 detected". If it indeed shows the former (i.e. the code path my
patch adds), we could probably really get rid of the old cruft.

Cheers,
Christian Aichinger

---
diff --git a/drivers/acpi/asus_acpi.c b/drivers/acpi/asus_acpi.c
--- a/drivers/acpi/asus_acpi.c
+++ b/drivers/acpi/asus_acpi.c
@@ -1006,6 +1006,24 @@ static int __init asus_hotk_get_info(voi
 	}
 
 	model = (union acpi_object *)buffer.pointer;
+
+	/* INIT on Samsung's P35 returns an integer, possible return
+	 * values are tested below */
+	if (model->type == ACPI_TYPE_INTEGER) {
+		if (model->integer.value == -1 ||
+			model->integer.value == 0x58 ||
+			model->integer.value == 0x38) {
+			hotk->model = P30;
+			printk(KERN_NOTICE
+				       "  Samsung P35 detected, supported\n");
+			goto out_known;
+		} else {
+			printk(KERN_WARNING 
+				"  unknown integer returned by INIT\n");
+			goto out_unknown;
+		}
+	}
+
 	if (model->type == ACPI_TYPE_STRING) {
 		printk(KERN_NOTICE "  %s model detected, ",
 		       model->string.pointer);
@@ -1057,9 +1075,7 @@ static int __init asus_hotk_get_info(voi
 		hotk->model = L5x;
 
 	if (hotk->model == END_MODEL) {
-		printk("unsupported, trying default values, supply the "
-		       "developers with your DSDT\n");
-		hotk->model = M2E;
+		goto out_unknown;
 	} else {
 		printk("supported\n");
 	}
@@ -1088,6 +1104,15 @@ static int __init asus_hotk_get_info(voi
 	acpi_os_free(model);
 
 	return AE_OK;
+
+out_unknown:
+	printk(KERN_WARNING "  unsupported, trying default values, "
+			"supply the developers with your DSDT\n");
+	hotk->model = M2E;
+out_known:
+	hotk->methods = &model_conf[hotk->model];
+	acpi_os_free(model);
+	return AE_OK;
 }
 
 static int __init asus_hotk_check(void)

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: oops with asus_acpi on P30/P35

[Timo Hoenig]

Hi,

On Wed, 2005-09-21 at 11:08 +0200, Christian Aichinger wrote:

> OK, it's getting a little frustrating that this still isn't fixed.
> I've now written a patch that tests for ACPI_TYPE_INTEGER, and it
> tests for possible return values from P35's INIT.
> 
> Since the relevant part hasn't changed between P30 -> P35's DSDT, I
> assume that there has been some change in the Linux ACPI code, that
> causes integer returns now instead of no returns. This means we
> could get rid of the P30 code in the (!buffer.pointer) path. I don't
> have a P30 around to test this though :-/
> 
> I got the -1/0x58/0x38 values from my DSDT:
> INIT there calls WLED, which calls \_SB.PCI0.LPCB.EC0.STC5, last
> call with arg 0x58 or 0x38. That function then returns with either
> Ones (which I gathered is ~0, i.e. -1) or with Arg0. That's the last
> return that happens within the INIT call, so I thought that might be
> the result of the call as a whole.
> 
> That fits nicely, since 0x58 is really returned on my laptop.
> 
> If this fix still breaks other laptops I'd really be puzzled, since
> currently the value is dereferenced later when doing the string
> comparisons. So the patch at least shouldn't break it more than it
> already is ;)

I can confirm that your patch works fine for the W5A which previously
needed the mentioned workaround.  But this single case of success should
not be taken as proof.  More testing would be reasonable.

[...]

See you,

   Timo



-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. 
Download it for free - -and be entered to win a 42" plasma tv or your very
own Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php

Re: oops with asus_acpi on P30/P35

[Christian Aichinger]

[-- Attachment #1: Type: text/plain, Size: 2594 bytes --]

On Wed, Sep 21, 2005 at 03:39:53PM +0200, Timo Hoenig wrote:
> On Wed, 2005-09-21 at 11:08 +0200, Christian Aichinger wrote:
> > If this fix still breaks other laptops I'd really be puzzled, since
> > currently the value is dereferenced later when doing the string
> > comparisons. So the patch at least shouldn't break it more than it
> > already is ;)
> 
> I can confirm that your patch works fine for the W5A which previously
> needed the mentioned workaround.  But this single case of success should
> not be taken as proof.  More testing would be reasonable.

The old patch was kind of a "Want to get this thing to work
for me" version. This one is more the "Let's get this thing finally
fixed in mainline" version now.

The patch only changes the behavior if the INIT method returns an
integer instead of a string, or a pointer to an integer or similar.

Now if an integer is returned this breaks the assumption in the
whole rest of the function, namely that model->xxx.pointer can be
dereferenced. This works for ACPI_TYPE_STRING and ACPI_TYPE_BUFFER.
Why the old patch broke you is probably because the INIT method
in your DSDT returns a _BUFFER instead of an _STRING.

In any other case (including ACPI_TYPE_INTEGER) dereferencing
model->pointer (or whatever's stored at that offset) WILL lead to an
oops.

Pointer magic isn't possible in AML AFAIK and it would be sheer luck
(and still a bug) if dereferencing some random integer gotten from
some AML function would actually work.

That's what I ment with the "if this breaks something it was at
least as broken before". If this patch changes the behavior for you,
you got a kernel oops without it.

It would probably be nice to add a check for
(model->type == string || buffer) for defensive-programming's sake.
However that can (and should probably) be done later on.

Cheers,
Christian Aichinger

PS: Would be nice if you could test my assumption that model->type
is really ACPI_TYPE_BUFFER on your laptop, by pasting a dmesg
snippet with that patch applied (should apply to vanilla -rc2):

diff --git a/drivers/acpi/asus_acpi.c b/drivers/acpi/asus_acpi.c
--- a/drivers/acpi/asus_acpi.c
+++ b/drivers/acpi/asus_acpi.c
@@ -1006,6 +1006,7 @@ static int __init asus_hotk_get_info(voi
        }
 
        model = (union acpi_object *)buffer.pointer;
+       printk(KERN_NOTICE "  DEBUG: model->type == %d\n", model->type);
        if (model->type == ACPI_TYPE_STRING) {
                printk(KERN_NOTICE "  %s model detected, ",
                       model->string.pointer);

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: oops with asus_acpi on P30/P35

[Karol Kozimor]

Thus wrote Christian Aichinger:
> The old patch was kind of a "Want to get this thing to work
> for me" version. This one is more the "Let's get this thing finally
> fixed in mainline" version now.

Doesn't the fix for kernel bug #5067 help? The object actually returned by
INIT depends on the DSDT, how can this be a problem is explained in the
comments in the above fix.
Best regards,

-- 
Karol 'sziwan' Kozimor
sziwan-DETuoxkZsSqrDJvtcaxF/A@public.gmane.org


-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. 
Download it for free - -and be entered to win a 42" plasma tv or your very
own Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php

Re: oops with asus_acpi on P30/P35

[Christian Aichinger]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

On Thu, Sep 22, 2005 at 02:13:43PM +0200, Karol Kozimor wrote:
> Thus wrote Christian Aichinger:
> > The old patch was kind of a "Want to get this thing to work
> > for me" version. This one is more the "Let's get this thing finally
> > fixed in mainline" version now.
> 
> Doesn't the fix for kernel bug #5067 help? The object actually returned by
> INIT depends on the DSDT, how can this be a problem is explained in the
> comments in the above fix.
> Best regards,

This is quite simmilar to my first patch actually, in that it
assumes P30 if model->type != ACPI_TYPE_STRING. This fixes the P30
oops, but this breaks ASUS W5A. My suspection (as mentioned
previously) is that on W5A INIT returns an ACPI_TYPE_BUFFER, which
happens to have the same memory layout as ACPI_TYPE_STRING.

So you really have to explicitly check for ACPI_TYPE_INTEGER, as my
patch does it.

As the #5067 patch does it, we can probably throw out the P30 case
in the (!buffer.pointer) path. However I'd prefer to do that cleanup
later. I just want that P30 oops fixed now without breaking other
stuff, and I'm pretty certain my patch accomplishes that.

Cheers,
Christian Aichinger

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: oops with asus_acpi on P30/P35

[Timo Hoenig]

Hi,

On Wed, 2005-09-21 at 17:08 +0200, Christian Aichinger wrote:

[...]

> PS: Would be nice if you could test my assumption that model->type
> is really ACPI_TYPE_BUFFER on your laptop, by pasting a dmesg
> snippet with that patch applied (should apply to vanilla -rc2):

Actually I do not own the M2E, but the one who has one checked the value
for model->type. It is ACPI_TYPE_BUFFER:

        DEBUG: model->type == 3

See you,

   Timo



-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. 
Download it for free - -and be entered to win a 42" plasma tv or your very
own Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php

[PATCH] acpi: Fix oops in asus_acpi.c on Samsung P30/P35 Laptops

[Christian Aichinger]

[-- Attachment #1: Type: text/plain, Size: 2165 bytes --]

Samsung P35's INIT returns an integer (instead of a string or a
plain buffer), which caused an oops when the result was treated as
string in asus_hotk_get_info() (since an invalid pointer got
dereferenced).

This patch explicitly checks for ACPI_TYPE_INTEGER and for the
return values possible on the P30/P35.

Signed-off-by: Christian Aichinger <Greek0-hi6Y0CQ0nG0@public.gmane.org>
---

 drivers/acpi/asus_acpi.c |   31 ++++++++++++++++++++++++++++---
 1 files changed, 28 insertions(+), 3 deletions(-)

c51f431351c648519a9b91de3c5e1d636246d7bc
diff --git a/drivers/acpi/asus_acpi.c b/drivers/acpi/asus_acpi.c
--- a/drivers/acpi/asus_acpi.c
+++ b/drivers/acpi/asus_acpi.c
@@ -1006,6 +1006,24 @@ static int __init asus_hotk_get_info(voi
 	}
 
 	model = (union acpi_object *)buffer.pointer;
+
+	/* INIT on Samsung's P35 returns an integer, possible return
+	 * values are tested below */
+	if (model->type == ACPI_TYPE_INTEGER) {
+		if (model->integer.value == -1 ||
+			model->integer.value == 0x58 ||
+			model->integer.value == 0x38) {
+			hotk->model = P30;
+			printk(KERN_NOTICE
+				       "  Samsung P35 detected, supported\n");
+			goto out_known;
+		} else {
+			printk(KERN_WARNING 
+				"  unknown integer returned by INIT\n");
+			goto out_unknown;
+		}
+	}
+
 	if (model->type == ACPI_TYPE_STRING) {
 		printk(KERN_NOTICE "  %s model detected, ",
 		       model->string.pointer);
@@ -1057,9 +1075,7 @@ static int __init asus_hotk_get_info(voi
 		hotk->model = L5x;
 
 	if (hotk->model == END_MODEL) {
-		printk("unsupported, trying default values, supply the "
-		       "developers with your DSDT\n");
-		hotk->model = M2E;
+		goto out_unknown;
 	} else {
 		printk("supported\n");
 	}
@@ -1088,6 +1104,15 @@ static int __init asus_hotk_get_info(voi
 	acpi_os_free(model);
 
 	return AE_OK;
+
+out_unknown:
+	printk(KERN_WARNING "  unsupported, trying default values, "
+			"supply the developers with your DSDT\n");
+	hotk->model = M2E;
+out_known:
+	hotk->methods = &model_conf[hotk->model];
+	acpi_os_free(model);
+	return AE_OK;
 }
 
 static int __init asus_hotk_check(void)

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

RE: oops with asus_acpi on P30/P35

[Moore, Robert]

Yes, the implicit return code works for EvaluateObject.

In general, code should check for the correct object type before
accessing the object.

There are utility procedures available to execute a method and specify
the expected return type; error otherwise.
Bob


> -----Original Message-----
> From: Karol Kozimor [mailto:sziwan-DETuoxkZsSqrDJvtcaxF/A@public.gmane.org]
> Sent: Wednesday, June 29, 2005 4:11 AM
> To: Moore, Robert
> Cc: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Christian Aichinger
> Subject: Re: [ACPI] oops with asus_acpi on P30/P35
> 
> Thus wrote Christian Aichinger:
> > The oops occurs in asus_acpi.c, line 1016:
> >     hotk->model = END_MODEL;
> > 	if (strncmp(model->string.pointer, "L3D", 3) == 0) // <-- OOPS
> > 		hotk->model = L3D;
> >
> > Some added debug printk's later it turned out that:
> > model->type == ACPI_TYPE_INTEGER
> > model->integer.value == 56
> 
> Thanks, I suspected that but I'm really swamped both with Real Work
and
> my exams so I'm slow even with catching up with the lists...
> 
> > This is why the problem wasn't fixed by your patch, since that
> > resided in the if (model->type == ACPI_TYPE_STRING) code-path.
> > I've attatched a patch that works for me, but IMHO it's ugly and
> > only fixes the sympthoms. Why do we get an integer here in the first
> > place?
> 
> Bob, is the implicit return code supposed to trigger also when using
> acpi_evaluate_object()? FYI, this is what we currently do:
> 
> write_acpi_int(hotk->handle, "INIT", 0, &buffer)
> (drivers/acpi/asus_acpi.c)
> 
> which is fine if the INIT method returns a string (the usual), but
> apparently not if there is no return statement in the method (the P30
> case). The old code assumed the buffer will be null in this case.
> 
> Is that a bug in the ACPICA or should the asus_acpi code cover for
other
> cases of buffer.type?
> 
> Best regards,
> 
> --
> Karol 'sziwan' Kozimor
> sziwan-DETuoxkZsSqrDJvtcaxF/A@public.gmane.org


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_idt77&alloc_id\x16492&op=click

Re: oops with asus_acpi on P30/P35

[Karol Kozimor]

Thus wrote Moore, Robert:
> Yes, the implicit return code works for EvaluateObject.
> 
> In general, code should check for the correct object type before
> accessing the object.
> 
> There are utility procedures available to execute a method and specify
> the expected return type; error otherwise.

I believe the current code allows, in theory, for a situation when a valid
(though useless from the driver's point of view) string is returned in such
a case?
Best regards,

-- 
Karol 'sziwan' Kozimor
sziwan-DETuoxkZsSqrDJvtcaxF/A@public.gmane.org


-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click