RE: [buggy index field handling] [patch]

RE: [buggy index field handling] [patch]

[Moore, Robert]

Is this the final patch?  If so, we will integrate it into the ACPI CA
core
Bob


-----Original Message-----
From: acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
[mailto:acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of Ducrot
Bruno
Sent: Monday, September 22, 2003 10:19 AM
To: Grover, Andrew
Cc: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Robert Vollmert; Mads Paulin
Subject: [ACPI] [buggy index field handling] [patch]

Hi Andy,

There is a funny bug in acpi/executer/exfldio.c::acpi_ex_field_datum_io

When the field come from an indexed one, we read all the time from the
address obj_desc->index_field.value
Unfortunately, this is wrong, since the correct address is
obviously &obj_desc->index_field.value + field_datum_byte_offset





--- linux-2.6.0-test5/drivers/acpi/executer/exfldio.c	2003/09/22
17:09:20	1.1
+++ linux-2.6.0-test5/drivers/acpi/executer/exfldio.c	2003/09/22
17:10:11
@@ -425,7 +425,7 @@
 		/* Write the index value to the index_register (itself a
region_field) */
 
 		status = acpi_ex_insert_into_field
(obj_desc->index_field.index_obj,
-				 &obj_desc->index_field.value,
+				 &obj_desc->index_field.value +
field_datum_byte_offset,
 				 sizeof (obj_desc->index_field.value));
 		if (ACPI_FAILURE (status)) {
 			return_ACPI_STATUS (status);



Cheers,

-- 
Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
Acpi-devel mailing list
Acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/acpi-devel


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [buggy index field handling] [patch]

[Ducrot Bruno]

On Tue, Sep 23, 2003 at 10:27:52AM -0700, Moore, Robert wrote:
> Is this the final patch?  If so, we will integrate it into the ACPI CA
> core
> Bob
> 
> 
> -----Original Message-----
> From: acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> [mailto:acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of Ducrot
> Bruno
> Sent: Monday, September 22, 2003 10:19 AM
> To: Grover, Andrew
> Cc: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Robert Vollmert; Mads Paulin
> Subject: [ACPI] [buggy index field handling] [patch]
> 
> Hi Andy,
> 
> There is a funny bug in acpi/executer/exfldio.c::acpi_ex_field_datum_io
> 
> When the field come from an indexed one, we read all the time from the
> address obj_desc->index_field.value
> Unfortunately, this is wrong, since the correct address is
> obviously &obj_desc->index_field.value + field_datum_byte_offset

I would like much you integrate Robert Vollmert version: it does overflow
checking.

Patch is from Robert Vollmert, but rediffed against 2.6.0-test5:


--- linux-2.6.0-test5/drivers/acpi/executer/exfldio.c	2003/09/22 17:09:20	1.1
+++ linux-2.6.0-test5/drivers/acpi/executer/exfldio.c	2003/09/23 18:05:37
@@ -312,6 +312,7 @@
 {
 	acpi_status                     status;
 	acpi_integer                    local_value;
+	acpi_integer                    index_field_value;
 
 
 	ACPI_FUNCTION_TRACE_U32 ("ex_field_datum_io", field_datum_byte_offset);
@@ -415,18 +416,22 @@
 	case ACPI_TYPE_LOCAL_INDEX_FIELD:
 
 
+		/* Compute index value to access the current datum */
+		index_field_value = obj_desc->index_field.value
+			+ field_datum_byte_offset;
+
 		/* Ensure that the index_value is not beyond the capacity of the register */
 
 		if (acpi_ex_register_overflow (obj_desc->index_field.index_obj,
-				  (acpi_integer) obj_desc->index_field.value)) {
+				  index_field_value)) {
 			return_ACPI_STATUS (AE_AML_REGISTER_LIMIT);
 		}
 
 		/* Write the index value to the index_register (itself a region_field) */
 
 		status = acpi_ex_insert_into_field (obj_desc->index_field.index_obj,
-				 &obj_desc->index_field.value,
-				 sizeof (obj_desc->index_field.value));
+				 &index_field_value,
+				 sizeof (index_field_value));
 		if (ACPI_FAILURE (status)) {
 			return_ACPI_STATUS (status);
 		}


-- 
Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

RE: [buggy index field handling] [patch]

[Nate Lawson]

No, that is not the right patch.  The better one is at the bottom of my
email, by Robert Vollmert.

-Nate

On Tue, 23 Sep 2003, Moore, Robert wrote:
> Is this the final patch?  If so, we will integrate it into the ACPI CA
> core
> Bob
>
> -----Original Message-----
> From: acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> [mailto:acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of Ducrot
> Bruno
> Sent: Monday, September 22, 2003 10:19 AM
> To: Grover, Andrew
> Cc: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Robert Vollmert; Mads Paulin
> Subject: [ACPI] [buggy index field handling] [patch]
>
> Hi Andy,
>
> There is a funny bug in acpi/executer/exfldio.c::acpi_ex_field_datum_io
> When the field come from an indexed one, we read all the time from the
> address obj_desc->index_field.value
> Unfortunately, this is wrong, since the correct address is
> obviously &obj_desc->index_field.value + field_datum_byte_offset
>
> --- linux-2.6.0-test5/drivers/acpi/executer/exfldio.c	2003/09/22
> 17:09:20	1.1
> +++ linux-2.6.0-test5/drivers/acpi/executer/exfldio.c	2003/09/22
> 17:10:11
> @@ -425,7 +425,7 @@
>  		/* Write the index value to the index_register (itself a
> region_field) */
>
>  		status = acpi_ex_insert_into_field
> (obj_desc->index_field.index_obj,
> -				 &obj_desc->index_field.value,
> +				 &obj_desc->index_field.value +
> field_datum_byte_offset,
>  				 sizeof (obj_desc->index_field.value));
>  		if (ACPI_FAILURE (status)) {
>  			return_ACPI_STATUS (status);

Here's a patch.

--- exfldio.c.orig	2003-09-22 18:22:09.000000000 +0200
+++ exfldio.c	2003-09-22 19:16:11.000000000 +0200
@@ -346,6 +346,7 @@
 {
 	acpi_status                     status;
 	acpi_integer                    local_value;
+	acpi_integer                    index_field_value;


 	ACPI_FUNCTION_TRACE_U32 ("ex_field_datum_io", field_datum_byte_offset);
@@ -448,18 +449,22 @@

 	case ACPI_TYPE_LOCAL_INDEX_FIELD:

+		/* Compute index value to access the current datum */
+
+		index_field_value = (acpi_integer) obj_desc->index_field.value
+			+ field_datum_byte_offset;

 		/* Ensure that the index_value is not beyond the capacity of the register */

 		if (acpi_ex_register_overflow (obj_desc->index_field.index_obj,
-				  (acpi_integer) obj_desc->index_field.value)) {
+					       index_field_value)) {
 			return_ACPI_STATUS (AE_AML_REGISTER_LIMIT);
 		}

 		/* Write the index value to the index_register (itself a region_field) */

 		status = acpi_ex_insert_into_field (obj_desc->index_field.index_obj,
-				 &obj_desc->index_field.value,
+				 &index_field_value,
 				 sizeof (obj_desc->index_field.value));
 		if (ACPI_FAILURE (status)) {
 			return_ACPI_STATUS (status);




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

RE: [buggy index field handling] [patch]

[Moore, Robert]

Please send me some ASL code that exhibits the problem so I can analyze
and verify.

Thanks,
Bob


-----Original Message-----
From: acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
[mailto:acpi-devel-admin-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org] On Behalf Of Ducrot
Bruno
Sent: Monday, September 22, 2003 10:36 AM
To: Grover, Andrew
Cc: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Robert Vollmert; Mads Paulin
Subject: Re: [ACPI] [buggy index field handling] [patch]

On Mon, Sep 22, 2003 at 07:18:47PM +0200, Ducrot Bruno wrote:
> Hi Andy,
> 
> There is a funny bug in
acpi/executer/exfldio.c::acpi_ex_field_datum_io
> 
> When the field come from an indexed one, we read all the time from the
> address obj_desc->index_field.value
> Unfortunately, this is wrong, since the correct address is
> obviously &obj_desc->index_field.value + field_datum_byte_offset

Ok, that one should be OK, now.  My apologize for the first one.


--- linux-2.6.0-test5/drivers/acpi/executer/exfldio.c	2003/09/22
17:09:20	1.1
+++ linux-2.6.0-test5/drivers/acpi/executer/exfldio.c	2003/09/22
17:31:55
@@ -413,6 +413,8 @@
 
 
 	case ACPI_TYPE_LOCAL_INDEX_FIELD:
+		{
+		u32 old_addr = 0;
 
 
 		/* Ensure that the index_value is not beyond the
capacity of the register */
@@ -424,9 +426,12 @@
 
 		/* Write the index value to the index_register (itself a
region_field) */
 
+		old_addr = obj_desc->index_field.value;
+		obj_desc->index_field.value += field_datum_byte_offset;
 		status = acpi_ex_insert_into_field
(obj_desc->index_field.index_obj,
-				 &obj_desc->index_field.value,
+				 &obj_desc->index_field.value +
field_datum_byte_offset,
 				 sizeof (obj_desc->index_field.value));
+		obj_desc->index_field.value = old_address;
 		if (ACPI_FAILURE (status)) {
 			return_ACPI_STATUS (status);
 		}
@@ -443,6 +448,7 @@
 			status = acpi_ex_insert_into_field
(obj_desc->index_field.data_obj,
 					  value,
obj_desc->common_field.access_byte_width);
 		}
+		}
 		break;
 
 

-- 
Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
Acpi-devel mailing list
Acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/acpi-devel


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

[buggy index field handling] [patch]

[Ducrot Bruno]

Hi Andy,

There is a funny bug in acpi/executer/exfldio.c::acpi_ex_field_datum_io

When the field come from an indexed one, we read all the time from the
address obj_desc->index_field.value
Unfortunately, this is wrong, since the correct address is
obviously &obj_desc->index_field.value + field_datum_byte_offset





--- linux-2.6.0-test5/drivers/acpi/executer/exfldio.c	2003/09/22 17:09:20	1.1
+++ linux-2.6.0-test5/drivers/acpi/executer/exfldio.c	2003/09/22 17:10:11
@@ -425,7 +425,7 @@
 		/* Write the index value to the index_register (itself a region_field) */
 
 		status = acpi_ex_insert_into_field (obj_desc->index_field.index_obj,
-				 &obj_desc->index_field.value,
+				 &obj_desc->index_field.value + field_datum_byte_offset,
 				 sizeof (obj_desc->index_field.value));
 		if (ACPI_FAILURE (status)) {
 			return_ACPI_STATUS (status);



Cheers,

-- 
Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [buggy index field handling] [patch]

[Ducrot Bruno]

On Mon, Sep 22, 2003 at 07:18:47PM +0200, Ducrot Bruno wrote:
> Hi Andy,
> 
> There is a funny bug in acpi/executer/exfldio.c::acpi_ex_field_datum_io
> 
> When the field come from an indexed one, we read all the time from the
> address obj_desc->index_field.value
> Unfortunately, this is wrong, since the correct address is
> obviously &obj_desc->index_field.value + field_datum_byte_offset
> 


Oh my,  that patch is wrong indeed.  Will give you an updated one.

> 
> 
> 
> 
> --- linux-2.6.0-test5/drivers/acpi/executer/exfldio.c	2003/09/22 17:09:20	1.1
> +++ linux-2.6.0-test5/drivers/acpi/executer/exfldio.c	2003/09/22 17:10:11
> @@ -425,7 +425,7 @@
>  		/* Write the index value to the index_register (itself a region_field) */
>  
>  		status = acpi_ex_insert_into_field (obj_desc->index_field.index_obj,
> -				 &obj_desc->index_field.value,
> +				 &obj_desc->index_field.value + field_datum_byte_offset,
>  				 sizeof (obj_desc->index_field.value));
>  		if (ACPI_FAILURE (status)) {
>  			return_ACPI_STATUS (status);
> 
> 
> 
> Cheers,
> 
> -- 
> Ducrot Bruno
> 
> --  Which is worse:  ignorance or apathy?
> --  Don't know.  Don't care.

-- 
Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [buggy index field handling] [patch]

[Ducrot Bruno]

On Mon, Sep 22, 2003 at 07:18:47PM +0200, Ducrot Bruno wrote:
> Hi Andy,
> 
> There is a funny bug in acpi/executer/exfldio.c::acpi_ex_field_datum_io
> 
> When the field come from an indexed one, we read all the time from the
> address obj_desc->index_field.value
> Unfortunately, this is wrong, since the correct address is
> obviously &obj_desc->index_field.value + field_datum_byte_offset

Ok, that one should be OK, now.  My apologize for the first one.


--- linux-2.6.0-test5/drivers/acpi/executer/exfldio.c	2003/09/22 17:09:20	1.1
+++ linux-2.6.0-test5/drivers/acpi/executer/exfldio.c	2003/09/22 17:31:55
@@ -413,6 +413,8 @@
 
 
 	case ACPI_TYPE_LOCAL_INDEX_FIELD:
+		{
+		u32 old_addr = 0;
 
 
 		/* Ensure that the index_value is not beyond the capacity of the register */
@@ -424,9 +426,12 @@
 
 		/* Write the index value to the index_register (itself a region_field) */
 
+		old_addr = obj_desc->index_field.value;
+		obj_desc->index_field.value += field_datum_byte_offset;
 		status = acpi_ex_insert_into_field (obj_desc->index_field.index_obj,
-				 &obj_desc->index_field.value,
+				 &obj_desc->index_field.value + field_datum_byte_offset,
 				 sizeof (obj_desc->index_field.value));
+		obj_desc->index_field.value = old_address;
 		if (ACPI_FAILURE (status)) {
 			return_ACPI_STATUS (status);
 		}
@@ -443,6 +448,7 @@
 			status = acpi_ex_insert_into_field (obj_desc->index_field.data_obj,
 					  value, obj_desc->common_field.access_byte_width);
 		}
+		}
 		break;
 
 

-- 
Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: [buggy index field handling] [patch]

[Ducrot Bruno]

On Mon, Sep 22, 2003 at 07:36:15PM +0200, Ducrot Bruno wrote:
> On Mon, Sep 22, 2003 at 07:18:47PM +0200, Ducrot Bruno wrote:
> > Hi Andy,
> > 
> > There is a funny bug in acpi/executer/exfldio.c::acpi_ex_field_datum_io
> > 
> > When the field come from an indexed one, we read all the time from the
> > address obj_desc->index_field.value
> > Unfortunately, this is wrong, since the correct address is
> > obviously &obj_desc->index_field.value + field_datum_byte_offset
> 
> Ok, that one should be OK, now.  My apologize for the first one.
> 


Definitely, I need to go to sleep..  Take the one from Robert Vollmert
and that should be OK.

> 
> --- linux-2.6.0-test5/drivers/acpi/executer/exfldio.c	2003/09/22 17:09:20	1.1
> +++ linux-2.6.0-test5/drivers/acpi/executer/exfldio.c	2003/09/22 17:31:55
> @@ -413,6 +413,8 @@
>  
>  
>  	case ACPI_TYPE_LOCAL_INDEX_FIELD:
> +		{
> +		u32 old_addr = 0;
>  
>  
>  		/* Ensure that the index_value is not beyond the capacity of the register */
> @@ -424,9 +426,12 @@
>  
>  		/* Write the index value to the index_register (itself a region_field) */
>  
> +		old_addr = obj_desc->index_field.value;
> +		obj_desc->index_field.value += field_datum_byte_offset;
>  		status = acpi_ex_insert_into_field (obj_desc->index_field.index_obj,
> -				 &obj_desc->index_field.value,
> +				 &obj_desc->index_field.value + field_datum_byte_offset,
                                                              ^^^^^^^^^^^^^^^^^^^^^^^^^
							      WEIRD!
>  				 sizeof (obj_desc->index_field.value));
> +		obj_desc->index_field.value = old_address;
>  		if (ACPI_FAILURE (status)) {
>  			return_ACPI_STATUS (status);
>  		}
> @@ -443,6 +448,7 @@
>  			status = acpi_ex_insert_into_field (obj_desc->index_field.data_obj,
>  					  value, obj_desc->common_field.access_byte_width);
>  		}
> +		}
>  		break;
>  
>  

-- 
Ducrot Bruno

--  Which is worse:  ignorance or apathy?
--  Don't know.  Don't care.


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf