RE: asus_acpi still broken on Samsung P30/P35

RE: asus_acpi still broken on Samsung P30/P35

[Brown, Len]

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

thanks,
-Len
 

>-----Original Message-----
>From: Linus Torvalds [mailto:torvalds@osdl.org] 
>Sent: Wednesday, December 21, 2005 1:37 PM
>To: Hanno Böck
>Cc: Andrew Morton; Brown, Len; 
>acpi-devel@lists.sourceforge.net; 
>linux-kernel@vger.kernel.org; Karol Kozimor; Christian Aichinger
>Subject: Re: asus_acpi still broken on Samsung P30/P35
>
>
>
>On Wed, 21 Dec 2005, Hanno Böck wrote:
>> 
>> This is not "some minor issue", this completely breaks the 
>usage of current 
>> vanilla-kernels on certain Hardware. Can please, please, 
>please anyone in the 
>> position to do this take care that this patch get's accepted 
>before 2.6.15?
>> 
>> 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 
>> 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 <torvalds@osdl.org>
>To: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2005@gmx.net>
>cc: Greg KH <greg@kroah.com>, 
>    Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, 
>    stable@kernel.org, acpi-devel <acpi-devel@lists.sourceforge.net>
>Subject: Re: [PATCH] Fix oops in asus_acpi.c on Samsung P30/P35 Laptops
>
>On Wed, 14 Dec 2005, Carl-Daniel Hailfinger wrote:
>> 
>> 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
>

Re: asus_acpi still broken on Samsung P30/P35

[Karol Kozimor]

Thus wrote Brown, Len:
> 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,

-- 
Karol 'sziwan' Kozimor
sziwan@hell.org.pl

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

[Karol Kozimor]

Thus wrote Brown, Len:
> 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,

-- 
Karol 'sziwan' Kozimor
sziwan@hell.org.pl


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 <sziwan@hell.org.pl>

--- 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;

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

[Christian Aichinger]

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

On Thu, Dec 22, 2005 at 06:42:26PM +0100, Karol Kozimor wrote:
> Thus wrote Brown, Len:
> > 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

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

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

[Karol Kozimor]

Thus wrote Christian Aichinger:
> > 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?

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

> 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,

-- 
Karol 'sziwan' Kozimor
sziwan@hell.org.pl

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

[Hanno Böck]

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

Any news on this?

-- 
Hanno Böck		Blog:   http://www.hboeck.de/
GPG: 3DBD3B20		Jabber: jabber@hboeck.de

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

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

[Karol Kozimor]

Thus wrote Hanno Böck:
> 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,

-- 
Karol 'sziwan' Kozimor
sziwan@hell.org.pl

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

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


diff --git a/drivers/acpi/asus_acpi.c b/drivers/acpi/asus_acpi.c
index f4c8775..d415b30 100644
--- a/drivers/acpi/asus_acpi.c
+++ b/drivers/acpi/asus_acpi.c
@@ -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 */
diff --git a/drivers/acpi/namespace/nsinit.c b/drivers/acpi/namespace/nsinit.c
index 9f929e4..79fa2ec 100644
--- a/drivers/acpi/namespace/nsinit.c
+++ b/drivers/acpi/namespace/nsinit.c
@@ -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 */
 

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

[Andrew Morton]

Alex Riesen <fork0@users.sourceforge.net> wrote:
>
> 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.

> 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?


> diff --git a/drivers/acpi/asus_acpi.c b/drivers/acpi/asus_acpi.c
> index f4c8775..d415b30 100644
> --- a/drivers/acpi/asus_acpi.c
> +++ b/drivers/acpi/asus_acpi.c
> @@ -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 */
> diff --git a/drivers/acpi/namespace/nsinit.c b/drivers/acpi/namespace/nsinit.c
> index 9f929e4..79fa2ec 100644
> --- a/drivers/acpi/namespace/nsinit.c
> +++ b/drivers/acpi/namespace/nsinit.c
> @@ -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 */
>  

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

[Alex Riesen]

On 2/20/06, Andrew Morton <akpm@osdl.org> wrote:
> Alex Riesen <fork0@users.sourceforge.net> wrote:
> >
> > 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.

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