[PATCH] resolve ACPI oops on boot

[PATCH] resolve ACPI oops on boot

[Dominik Brodowski]

An u8 was casted into an u32, then all 32 bits were set to zero, this
causing another variable - in my case, processor flags - to be corrupted. 

Dominik

--- linux/drivers/acpi-original/ec.c	Fri Jul 12 22:43:11 2002
+++ linux/drivers/acpi/ec.c	Fri Jul 12 23:28:14 2002
@@ -134,7 +134,7 @@
 acpi_ec_read (
 	struct acpi_ec		*ec,
 	u8			address,
-	u8			*data)
+	u32			*data)
 {
 	acpi_status		status = AE_OK;
 	int			result = 0;
@@ -167,7 +167,7 @@
 		goto end;
 
 
-	acpi_hw_low_level_read(8, (u32*) data, &ec->data_addr, 0);
+	acpi_hw_low_level_read(8, data, &ec->data_addr, 0);
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Read [%02x] from address [%02x]\n",
 		*data, address));
@@ -237,7 +237,7 @@
 static int
 acpi_ec_query (
 	struct acpi_ec		*ec,
-	u8			*data)
+	u32			*data)
 {
 	int			result = 0;
 	acpi_status		status = AE_OK;
@@ -269,7 +269,7 @@
 	if (result)
 		goto end;
 	
-	acpi_hw_low_level_read(8, (u32*) data, &ec->data_addr, 0);
+	acpi_hw_low_level_read(8, data, &ec->data_addr, 0);
 	if (!*data)
 		result = -ENODATA;
 
@@ -328,7 +328,7 @@
 {
 	acpi_status		status = AE_OK;
 	struct acpi_ec		*ec = (struct acpi_ec *) data;
-	u8			value = 0;
+	u32			value = 0;
 	unsigned long		flags = 0;
 	struct acpi_ec_query_data *query_data = NULL;
 
@@ -336,7 +336,7 @@
 		return;
 
 	spin_lock_irqsave(&ec->lock, flags);
-	acpi_hw_low_level_read(8, (u32*) &value, &ec->command_addr, 0);
+	acpi_hw_low_level_read(8, &value, &ec->command_addr, 0);
 	spin_unlock_irqrestore(&ec->lock, flags);
 
 	/* TBD: Implement asynch events!
@@ -398,6 +398,7 @@
 {
 	int			result = 0;
 	struct acpi_ec		*ec = NULL;
+	u32          		tmp = 0;
 
 	ACPI_FUNCTION_TRACE("acpi_ec_space_handler");
 
@@ -408,7 +409,8 @@
 
 	switch (function) {
 	case ACPI_READ:
-		result = acpi_ec_read(ec, (u8) address, (u8*) value);
+		result = acpi_ec_read(ec, (u8) address, &tmp);
+		*value = (acpi_integer) tmp;
 		break;
 	case ACPI_WRITE:
 		result = acpi_ec_write(ec, (u8) address, (u8) *value);

Re: [PATCH] resolve ACPI oops on boot

[Bill Davidsen]

On Thu, 18 Jul 2002, Dominik Brodowski wrote:

> An u8 was casted into an u32, then all 32 bits were set to zero, this
> causing another variable - in my case, processor flags - to be corrupted. 
> 
> Dominik

But that's not what's happening here, the pointer is being cast, if the
object of the pointer is not u32, then casting the pointer doesn't fix the
real problem. If the "data" pointer points to a u8, then no casting will
make it work right when you save 32 bits into an 8 bit space. If this
changes the problem it's because of alignment, perhaps.

You give neither the kernel version nor the architecture, so I can't be
sure what's happening or what the compiler might do. I don't find that
code in the kernel I have on this machine (2.4.19-pre7-jam6) but that
diesn't mean much. The routine in hardware/hwregs.c on my kernel would
seem to pass the width correctly, but clearly this is a very different
version.

I think the cast is just to avoid the compiler whining about types, the
version I have is actually type "(acpi_generic_address*)" not (u32*), I
would think the compiler would still complain, but maybe only with
-pedantic or whatever.

In any case only the number of bits requested should be written, the
problem may have been avoided rather than fixed.

 
> --- linux/drivers/acpi-original/ec.c	Fri Jul 12 22:43:11 2002
> +++ linux/drivers/acpi/ec.c	Fri Jul 12 23:28:14 2002
> @@ -134,7 +134,7 @@
>  acpi_ec_read (
>  	struct acpi_ec		*ec,
>  	u8			address,
> -	u8			*data)
> +	u32			*data)
>  {
>  	acpi_status		status = AE_OK;
>  	int			result = 0;
> @@ -167,7 +167,7 @@
>  		goto end;
>  
>  
> -	acpi_hw_low_level_read(8, (u32*) data, &ec->data_addr, 0);
> +	acpi_hw_low_level_read(8, data, &ec->data_addr, 0);
>  
>  	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Read [%02x] from address [%02x]\n",
>  		*data, address));


-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] resolve ACPI oops on boot

[Dominik Brodowski]

On Tue, Jul 23, 2002 at 05:20:57PM -0400, Bill Davidsen wrote:
> On Thu, 18 Jul 2002, Dominik Brodowski wrote:
> 
> > An u8 was casted into an u32, then all 32 bits were set to zero, this
> > causing another variable - in my case, processor flags - to be corrupted. 
> > 
> > Dominik
> 
> But that's not what's happening here, the pointer is being cast, if the
> object of the pointer is not u32, then casting the pointer doesn't fix the
> real problem. If the "data" pointer points to a u8, then no casting will
> make it work right when you save 32 bits into an 8 bit space. If this
> changes the problem it's because of alignment, perhaps.
There is the argument "size" to acpi_hw_low_level_read which makes sure that
only the right data is being read. Problem is that a sort of
"segmentation fault" occurs when such a cast allows writing in different
memory than allocated for the value.
 
> You give neither the kernel version nor the architecture
kernel version 2.5.2[5,6] or 2.4.-kernels with acpi-patch 20020709, 
architecture: all architectures that implement Embedded Controllers.

> I think the cast is just to avoid the compiler whining about types, the
> version I have is actually type "(acpi_generic_address*)" not (u32*), I
> would think the compiler would still complain, but maybe only with
> -pedantic or whatever.
The casts were probably introduced for that reason. Per se, they are not
critical - but if there is any assumption later on that the data structure
is indeed of the large size, there is a problem.

Dominik

Re: [PATCH] resolve ACPI oops on boot

[Bill Davidsen]

On Wed, 24 Jul 2002, Dominik Brodowski wrote:

> The casts were probably introduced for that reason. Per se, they are not
> critical - but if there is any assumption later on that the data structure
> is indeed of the large size, there is a problem.

Perhaps the pointer chould be (void *) everywhere, and then cast to the
right size where and when it is used. Clearly casting the pointer to a
larger size will result in allignment issues on some machines.

-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: [PATCH] resolve ACPI oops on boot

[Matthew Harrell]

> You give neither the kernel version nor the architecture, so I can't be
> sure what's happening or what the compiler might do. I don't find that
> code in the kernel I have on this machine (2.4.19-pre7-jam6) but that
> diesn't mean much. The routine in hardware/hwregs.c on my kernel would
> seem to pass the width correctly, but clearly this is a very different
> version.

Actually, I found this patch accidently since it didn't list the kernel
version but it exactly fixed my ACPI oops I was getting on boots of the
2.5.2[6-7] kernels.  I posted the relevent oops about a week ago under the
subject "2.5.2[6-7] ACPI oops" but I can send it to you if you're interested

-- 
  Matthew Harrell                          Any sufficiently advanced technology
  Bit Twiddlers, Inc.                       is indistinguishable from a rigged
  mharrell@bittwiddlers.com                 demo.