asus_acpi still broken on Samsung P30/P35

PATCH In review Brown, Len 7 participants 10 messages Dec 21, 2005 – Feb 20, 2006 ACPI

lore

Share & tools

lore

Brown, Len

author

2005-12-21 19:06 UTC #0 lore raw

Karol,
Do you have an update of your asus driver in the pipeline that addresses this?

thanks,
-Len

Quoted 83 lines

-----Original Message-----
From: Linus Torvalds [mailto:[email protected]]
Sent: Wednesday, December 21, 2005 1:37 PM
To: Hanno BÃ¶ck
Cc: Andrew Morton; Brown, Len; [email protected]; [email protected]; Karol Kozimor; Christian Aichinger
Subject: Re: asus_acpi still broken on Samsung P30/P35
Quoted 2 lines · Hanno BÃ¶ck
This is not "some minor issue", this completely breaks the
usage of current
Quoted 1 line
vanilla-kernels on certain Hardware. Can please, please,
please anyone in the
Quoted 1 line
position to do this take care that this patch get's accepted
before 2.6.15?
Quoted 5 lines
The patch is available inside mm-sources or here:
http://www.int21.de/samsung/p30-2.6.14.diff
If I should send it to anyone else or if there's anything I
can do to help
Quoted 1 line
fixing this, I'm glad to help.
Last I saw this patch, I wrote this reply (the patch above is still broken). Nobody ever came back to me on it.
Linus
---
Date: Tue, 13 Dec 2005 21:15:56 -0800 (PST)
From: Linus Torvalds <[email protected]>
To: Carl-Daniel Hailfinger <[email protected]>
cc: Greg KH <[email protected]>,
Linux Kernel Mailing List <[email protected]>, [email protected], acpi-devel <[email protected]>
Subject: Re: [PATCH] Fix oops in asus_acpi.c on Samsung P30/P35 Laptops
Quoted 3 lines · Carl-Daniel Hailfinger
The patch has been tested and verified, is shipped in the SUSE 10.0 kernel and does not cause any regressions.
I'd be _much_ happier if
- the patch wasn't totally whitespace-damaged (your mailer seems to not only remove spaces at the end of lines, it _also_
adds them to
the beginning when there was another space there, as far as
I can tell)
Being right "on average" thanks to having two different
bugs does not a
good mailer make.
- you were to separate out the oops-fixing code from the code that adds
handling for that (strange?) model type logic.
It seems that the _oops_ is because the later paths just
assume that
it's a ACPI_TYPE_STRING and will dereference
"model->string.pointer"
regardless of whether that is true or not. And you add a test for ACPI_TYPE_INTEGER, however, you do _not_ fix the oops for any other type, so the exact _same_ bug is still waiting to happen if
there is
some other strange ACPI table entry some day.
So I think the proper fix is to _first_ just do something like
	if (model->type != ACPI_TYPE_STRING)
		goto unknown;
which should fix the oops (no?), and then handling ACPI_TYPE_INTEGER above
that as one case would be a separate patch.
Linus

Karol Kozimor

2005-12-22 10:53 UTC #1 lore raw

Thus wrote Brown, Len:

Quoted 3 lines

Karol,
Do you have an update of your asus driver in the pipeline that addresses this?

I still believe the only _right_ *workaround* is
http://bugme.osdl.org/attachment.cgi?id=6006&action=view

I'll take a shot at rediffing it against recent kernels in a couple of hours (unless someone beats me to it).

acpi=strict will work until a suitable patch is merged.

Note: it's still a workaround, to properly fix this we need to make ACPI interpreter behave predictably, as written in
http://bugme.osdl.org/show_bug.cgi?id=5067#c6 -- I believe I still haven't heard from Robert Moore on the feasibility of such a solution.

Please also see http://bugme.osdl.org/show_bug.cgi?id=5067 and
http://bugzilla.kernel.org/show_bug.cgi?id=5092 for more info.

Best regards,

Signature · 9 lines

Karol 'sziwan' Kozimor
sziwan-DETuoxkZsSqrDJvtcaxF/[email protected]


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Karol Kozimor

2005-12-22 17:42 UTC #2 lore raw

PATCH 1/1 Work around asus_acpi driver oopses on Samsung P30s and the like due to the ACPI implicit return

In reply to Brown, Len

Thus wrote Brown, Len:

Quoted 3 lines

Karol,
Do you have an update of your asus driver in the pipeline that addresses this?

Here it goes. Rediffed, also plugs a leak my previous patch introduced. I believe it addresses Linus' comments. It's still not a proper fix (see below), but I believe it's better than none.
Best regards,

Signature · 61 lines

Karol 'sziwan' Kozimor
[email protected]


Work around asus_acpi driver oopses on Samsung P30s and the like due to the
ACPI implicit return.

The code used to rely on a certain method to return a NULL buffer, which
is now hardly possible with the implicit return code on by default. This
sort of fixes bugs #5067 and #5092 for now.

Note: this patch makes the driver unusable on said machines (and on said
machines only) iff acpi=strict is specified, but it seems noone really uses
that.

Signed-off-by: Karol Kozimor <[email protected]>

--- a/drivers/acpi/asus_acpi.c	2005-12-22 18:08:56.000000000 +0100
+++ b/drivers/acpi/asus_acpi.c	2005-12-22 18:16:19.000000000 +0100
@@ -987,9 +987,21 @@ static int __init asus_hotk_get_info(voi
 		printk(KERN_NOTICE "  BSTS called, 0x%02x returned\n",
 		       bsts_result);
 
-	/* 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) {
+	/* This is unlikely with implicit return */
+	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. It used to be possible to catch this exception,
+	 * but the implicit return code will now happily confuse the 
+	 * driver. We assume that every ACPI_TYPE_STRING is a valid model
+	 * identifier but it's still possible to get completely bogus data.
+	 */
+	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;
@@ -1002,13 +1014,10 @@ static int __init asus_hotk_get_info(voi
 			       "the developers with your DSDT\n");
 		}
 		hotk->methods = &model_conf[hotk->model];
-		return AE_OK;
-	}
+		
+		acpi_os_free(model);
 
-	model = (union acpi_object *)buffer.pointer;
-	if (model->type == ACPI_TYPE_STRING) {
-		printk(KERN_NOTICE "  %s model detected, ",
-		       model->string.pointer);
+		return AE_OK;
 	}
 
 	hotk->model = END_MODEL;

Christian Aichinger

2005-12-23 11:33 UTC #3 lore raw

Re: [PATCH] Work around asus_acpi driver oopses on Samsung P30s and the like due to the ACPI implicit return

Quoted 9 lines · Karol Kozimor

Thus wrote Brown, Len:
Quoted 3 lines
Karol,
Do you have an update of your asus driver in the pipeline that addresses this?
Here it goes. Rediffed, also plugs a leak my previous patch introduced. I believe it addresses Linus' comments. It's still not a proper fix (see below), but I believe it's better than none.
Best regards,

This will break other hardware as the P30/P35 as well, since there are some buggy DSDT's out there that return an ACPI_TYPE_BUFFER. That's the whole reason why I was testing exactly for ACPI_TYPE_INTEGER in my patch.

My first version was pretty simmilar to yours, until I was told on acpi-devel that this breaks someone elses hardware (causing it to be considered as P30/P35, while it isn't). I can dig up the mails if you want.

Cheers,
Christian

Karol Kozimor

2005-12-23 12:19 UTC #4 lore raw

Re: [PATCH] Work around asus_acpi driver oopses on Samsung P30s and the like due to the ACPI implicit return

Thus wrote Christian Aichinger:

Quoted 6 lines

Quoted 4 lines
Here it goes. Rediffed, also plugs a leak my previous patch introduced. I believe it addresses Linus' comments. It's still not a proper fix (see below), but I believe it's better than none.
Best regards,
This will break other hardware as the P30/P35 as well, since there are some buggy DSDT's out there that return an ACPI_TYPE_BUFFER.

I've never seen one, I'd like to look at those if you've got them. Are you sure it's the actual machine code that returns buffers, and not an implicit return of the interpreter?

Quoted 2 lines

That's the whole reason why I was testing exactly for ACPI_TYPE_INTEGER in my patch.

I don't really seem to follow: my logic checks for ACPI_TYPE_STRING, and if not found, looks at DSDT signatures. If something different than a string is returned by INIT _and_ the machine isn't a P30/P35 (DSDT sigs) then the defaults are set as in every other case a machine is not recognized by the driver.

Quoted 4 lines

My first version was pretty simmilar to yours, until I was told on acpi-devel that this breaks someone elses hardware (causing it to be considered as P30/P35, while it isn't). I can dig up the mails if you want.

The original driver behaviour was: ( if INIT returns NULL && DSDT sigs match ) machine is a P30. My patch changes that to ( if INIT returns !ACPI_TYPE_STRING && DSDT sigs match ) machine is a P30. I'd like to see those reports please.
Best regards,

Signature · 2 lines

Karol 'sziwan' Kozimor
[email protected]

Hanno Böck

2006-01-16 11:03 UTC #5 lore raw

Re: [PATCH] Work around asus_acpi driver oopses on Samsung P30s and the like due to the ACPI implicit return

Any news on this?

Signature · 2 lines

Hanno Böck		Blog:   http://www.hboeck.de/
GPG: 3DBD3B20		Jabber: [email protected]

Karol Kozimor

2006-01-17 01:06 UTC #6 lore raw

Re: [PATCH] Work around asus_acpi driver oopses on Samsung P30s and the like due to the ACPI implicit return

Thus wrote Hanno Böck:

Quoted 1 line

Any news on this?

The fix is in acpi-test already, and a rework of this code is in the acpi4asus CVS and will be sent to Len as soon as I rediff the patches against the git tree.

As for -stable, my understanding is that Chris wants to wait for the fix to boil in the mainline for a while.

Best regards,

Signature · 2 lines

Karol 'sziwan' Kozimor
[email protected]

Alex Riesen

2006-02-19 12:53 UTC #7 lore raw

Re: [PATCH] Work around asus_acpi driver oopses on Samsung P30s and the like due to the ACPI implicit return

In reply to Karol Kozimor

FWIW, I need the patch below to stop ACPI freezing at boot on Asus S1300N. There is a BIOS update from Asus, but no mention of any fixes in ACPI, so as I have no means to backup the BIOS in case something goes wrong I didn't do the update.

I found out (by putting printks in the initialization code) that a call to INI (whatever it is) of VGA_ (whatever this is) immediately freezes the notebook and the fan goes on shortly afterwards.

drivers/acpi/asus_acpi.c +2 -2

index f4c8775..d415b30 100644
@@ -352,8 +352,8 @@ static struct model_data model_conf[END_
 	 .lcd_status = "\\BKLT",
 	 .brightness_set = "SPLV",
 	 .brightness_get = "GPLV",
-	 .display_set = "SDSP",
-	 .display_get = "\\ADVG"}
+	 /* .display_set = "SDSP",
+	 .display_get = "\\ADVG" */}
 };
 
 /* procdir we use */

drivers/acpi/namespace/nsinit.c +6 -1

index 9f929e4..79fa2ec 100644
@@ -384,7 +384,12 @@ acpi_ns_init_one_device(acpi_handle obj_
 	pinfo.parameters = NULL;
 	pinfo.parameter_type = ACPI_PARAM_ARGS;
 
-	status = acpi_ut_execute_STA(pinfo.node, &flags);
+	/* workaround Asus S1300N freeze at INI */
+	if ( memcmp(pinfo.node->name.ascii, "VGA_",4)==0 ) {
+	    printk(KERN_ERR "acpi: VGA_ ignored\n");
+	    status = AE_NOT_FOUND;
+	} else
+	    status = acpi_ut_execute_STA(pinfo.node, &flags);
 	if (ACPI_FAILURE(status)) {
 		/* Ignore error and move on to next device */

Andrew Morton

2006-02-20 05:20 UTC #8 lore raw

Re: [PATCH] Work around asus_acpi driver oopses on Samsung P30s and the like due to the ACPI implicit return

Quoted 5 lines · Alex Riesen

FWIW, I need the patch below to stop ACPI freezing at boot on Asus S1300N. There is a BIOS update from Asus, but no mention of any fixes in ACPI, so as I have no means to backup the BIOS in case something goes wrong I didn't do the update.

I think it'd be worth trying the update anyway please. Normally those updating programs are pretty careful to not give you a dead box.

Quoted 4 lines

I found out (by putting printks in the initialization code) that a call to INI (whatever it is) of VGA_ (whatever this is) immediately freezes the notebook and the fan goes on shortly afterwards.

Is this a recent problem, or did earlier 2.6.x kernels also fail?

Quoted 33 lines

drivers/acpi/asus_acpi.c +2 -2

index f4c8775..d415b30 100644
@@ -352,8 +352,8 @@ static struct model_data model_conf[END_
 	 .lcd_status = "\\BKLT",
 	 .brightness_set = "SPLV",
 	 .brightness_get = "GPLV",
-	 .display_set = "SDSP",
-	 .display_get = "\\ADVG"}
+	 /* .display_set = "SDSP",
+	 .display_get = "\\ADVG" */}
 };
 
 /* procdir we use */

drivers/acpi/namespace/nsinit.c +6 -1

index 9f929e4..79fa2ec 100644
@@ -384,7 +384,12 @@ acpi_ns_init_one_device(acpi_handle obj_
 	pinfo.parameters = NULL;
 	pinfo.parameter_type = ACPI_PARAM_ARGS;
 
-	status = acpi_ut_execute_STA(pinfo.node, &flags);
+	/* workaround Asus S1300N freeze at INI */
+	if ( memcmp(pinfo.node->name.ascii, "VGA_",4)==0 ) {
+	    printk(KERN_ERR "acpi: VGA_ ignored\n");
+	    status = AE_NOT_FOUND;
+	} else
+	    status = acpi_ut_execute_STA(pinfo.node, &flags);
 	if (ACPI_FAILURE(status)) {
 		/* Ignore error and move on to next device */

Alex Riesen

2006-02-20 09:45 UTC #9 lore raw

Re: [PATCH] Work around asus_acpi driver oopses on Samsung P30s and the like due to the ACPI implicit return

Quoted 9 lines · Andrew Morton

Quoted 5 lines · Alex Riesen
FWIW, I need the patch below to stop ACPI freezing at boot on Asus S1300N. There is a BIOS update from Asus, but no mention of any fixes in ACPI, so as I have no means to backup the BIOS in case something goes wrong I didn't do the update.
I think it'd be worth trying the update anyway please. Normally those updating programs are pretty careful to not give you a dead box.

You think? ... Still, it'll have to wait till the next weekend. I really cannot afford a "dead box" midweek.

Quoted 6 lines

Quoted 4 lines
I found out (by putting printks in the initialization code) that a call to INI (whatever it is) of VGA_ (whatever this is) immediately freezes the notebook and the fan goes on shortly afterwards.
Is this a recent problem, or did earlier 2.6.x kernels also fail?

Yes. I think I got the notebook around 2.6.9, and it already had the problem. I don't remember what I did back than, probably run it with ACPI disabled, or maybe I made that workaround immediately (there were some sound problems without ACPI, I recall).