[patch] acpi compile fix

[patch] acpi compile fix

[Andrew Morton]

ACPI is performing a spin_lock() on a `void *'.  That's OK when spin_lock is
implemented via an inline function.  But when it is implemented via macros
(eg, with spinlock debugging enabled) we get:

drivers/acpi/osl.c:739: warning: dereferencing `void *' pointer
drivers/acpi/osl.c:739: request for member `owner' in something not a structure or union

So cast it to the right type.


diff -puN drivers/acpi/osl.c~acpi-spinlock-casts drivers/acpi/osl.c
--- 25/drivers/acpi/osl.c~acpi-spinlock-casts	Thu Apr  3 13:00:54 2003
+++ 25-akpm/drivers/acpi/osl.c	Thu Apr  3 13:01:25 2003
@@ -736,7 +736,7 @@ acpi_os_acquire_lock (
 	if (flags & ACPI_NOT_ISR)
 		ACPI_DISABLE_IRQS();
 
-	spin_lock(handle);
+	spin_lock((spinlock_t *)handle);
 
 	return_VOID;
 }
@@ -755,7 +755,7 @@ acpi_os_release_lock (
 	ACPI_DEBUG_PRINT ((ACPI_DB_MUTEX, "Releasing spinlock[%p] from %s level\n", handle,
 		((flags & ACPI_NOT_ISR) ? "non-interrupt" : "interrupt")));
 
-	spin_unlock(handle);
+	spin_unlock((spinlock_t *)handle);
 
 	if (flags & ACPI_NOT_ISR)
 		ACPI_ENABLE_IRQS();

_

Re: [patch] acpi compile fix

[J Sloan]

Andrew Morton wrote:

>ACPI is performing a spin_lock() on a `void *'.  That's OK when spin_lock is
>implemented via an inline function.  But when it is implemented via macros
>(eg, with spinlock debugging enabled) we get:
>
>drivers/acpi/osl.c:739: warning: dereferencing `void *' pointer
>drivers/acpi/osl.c:739: request for member `owner' in something not a structure or union
>
>So cast it to the right type.
>

Yep 2.5.66 is happy with my config now
and is running nicely - thanks for the fix!

Joe

Re: [patch] acpi compile fix

[Dave Jones]

On Thu, Apr 03, 2003 at 01:05:05PM -0800, Andrew Morton wrote:

 > diff -puN drivers/acpi/osl.c~acpi-spinlock-casts drivers/acpi/osl.c
 > --- 25/drivers/acpi/osl.c~acpi-spinlock-casts	Thu Apr  3 13:00:54 2003
 > +++ 25-akpm/drivers/acpi/osl.c	Thu Apr  3 13:01:25 2003
 > @@ -736,7 +736,7 @@ acpi_os_acquire_lock (
 >  	if (flags & ACPI_NOT_ISR)
 >  		ACPI_DISABLE_IRQS();
 >  
 > -	spin_lock(handle);
 > +	spin_lock((spinlock_t *)handle);

Is there a reason these functions can't just have their arguments
changed to take a spinlock_t* instead of an acpi_handle ?
That cast looks really fugly IMO.

		Dave

Re: [patch] acpi compile fix

[Andrew Morton]

Dave Jones <davej@codemonkey.org.uk> wrote:
>
> On Thu, Apr 03, 2003 at 01:05:05PM -0800, Andrew Morton wrote:
> 
>  > diff -puN drivers/acpi/osl.c~acpi-spinlock-casts drivers/acpi/osl.c
>  > --- 25/drivers/acpi/osl.c~acpi-spinlock-casts	Thu Apr  3 13:00:54 2003
>  > +++ 25-akpm/drivers/acpi/osl.c	Thu Apr  3 13:01:25 2003
>  > @@ -736,7 +736,7 @@ acpi_os_acquire_lock (
>  >  	if (flags & ACPI_NOT_ISR)
>  >  		ACPI_DISABLE_IRQS();
>  >  
>  > -	spin_lock(handle);
>  > +	spin_lock((spinlock_t *)handle);
> 
> Is there a reason these functions can't just have their arguments
> changed to take a spinlock_t* instead of an acpi_handle ?
> That cast looks really fugly IMO.
> 

I think acpi_handle_t is "an opaque type specific to the OS on which the APCI
code happens to be running".

It is presently `void *', implicitly pointing at a spinlock_t.

If the above guesses (I'd prefer not to look) are correct then

	struct acpi_handle_t {
		spinlock_t lock;
	};

would make a ton more sense.

Re: [patch] acpi compile fix

[Dave Jones]

On Fri, Apr 04, 2003 at 11:57:56AM -0800, Andrew Morton wrote:

 > I think acpi_handle_t is "an opaque type specific to the OS on which the APCI
 > code happens to be running".

I don't see how putting a spinlock_t cast in the code is any more
portable between OS's than spinlock_t as a function parameter.

 > If the above guesses (I'd prefer not to look) are correct then
 > 	struct acpi_handle_t {
 > 		spinlock_t lock;
 > 	};
 > 
 > would make a ton more sense.

That would solve the portability argument in my eyes if that is
indeed the case here. It's still ugly, but it at least kills
the problem in a slightly more tasteful way.

		Dave

RE: [patch] acpi compile fix

[Grover, Andrew]

> From: Dave Jones [mailto:davej@codemonkey.org.uk] 
> I don't see how putting a spinlock_t cast in the code is any 
> more portable between OS's than spinlock_t as a function parameter.

The code that calls osl.c does not know about spinlock_t. Either the
function's definition and declaration don't match, or the other code
needs to know what a spinlock_t is, doesn't it?

>  > If the above guesses (I'd prefer not to look) are correct then
>  > 	struct acpi_handle_t {
>  > 		spinlock_t lock;
>  > 	};
>  > 
>  > would make a ton more sense.
> 
> That would solve the portability argument in my eyes if that 
> is indeed the case here. It's still ugly, but it at least 
> kills the problem in a slightly more tasteful way.

I don't see the cast as being particularly onerous. It's just a cookie.
osl.c knows what it actually points to, the rest doesn't.

Regards -- Andy

[PATCH] ACPI compile fix

[Andy Grover]

Hi Linus,

This fixes the ACPI_DEBUG compile issue that turned up.

(acpi-devel people, I'll be updating sf.net patches with this asap)

Regards -- Andy

diff -Naur -X bin/dontdiff acpi-old/drivers/acpi/include/aclocal.h linux/drivers/acpi/include/aclocal.h
--- acpi-old/drivers/acpi/include/aclocal.h	Fri Jul 26 11:27:34 2002
+++ linux/drivers/acpi/include/aclocal.h	Fri Jul 26 11:20:05 2002
@@ -1,7 +1,7 @@
 /******************************************************************************
  *
  * Name: aclocal.h - Internal data types used across the ACPI subsystem
- *       $Revision: 172 $
+ *       $Revision: 173 $
  *
  *****************************************************************************/
 
@@ -567,7 +567,7 @@
  */
 typedef struct acpi_opcode_info
 {
-#ifdef ACPI_DISASSEMBLER
+#if defined(ACPI_DISASSEMBLER) || defined(ACPI_DEBUG)
 	NATIVE_CHAR             *name;          /* Opcode name (disassembler/debug only) */
 #endif
 	u32                     parse_args;     /* Grammar/Parse time arguments */
diff -Naur -X bin/dontdiff acpi-old/drivers/acpi/include/acmacros.h linux/drivers/acpi/include/acmacros.h
--- acpi-old/drivers/acpi/include/acmacros.h	Fri Jul 26 11:27:34 2002
+++ linux/drivers/acpi/include/acmacros.h	Fri Jul 26 11:20:05 2002
@@ -1,7 +1,7 @@
 /******************************************************************************
  *
  * Name: acmacros.h - C macros for the entire subsystem.
- *       $Revision: 125 $
+ *       $Revision: 126 $
  *
  *****************************************************************************/
 
@@ -287,11 +287,15 @@
 /*
  * Macros for the master AML opcode table
  */
-#ifdef ACPI_DISASSEMBLER
+#if defined(ACPI_DISASSEMBLER) || defined (ACPI_DEBUG)
 #define ACPI_OP(name,Pargs,Iargs,obj_type,class,type,flags)    {name,Pargs,Iargs,flags,obj_type,class,type}
-#define ACPI_DISASM_ONLY_MEMBERS(a)      a;
 #else
 #define ACPI_OP(name,Pargs,Iargs,obj_type,class,type,flags)    {Pargs,Iargs,flags,obj_type,class,type}
+#endif
+
+#ifdef ACPI_DISASSEMBLER
+#define ACPI_DISASM_ONLY_MEMBERS(a)      a;
+#else
 #define ACPI_DISASM_ONLY_MEMBERS(a)
 #endif
 
diff -Naur -X bin/dontdiff acpi-old/drivers/acpi/parser/psutils.c linux/drivers/acpi/parser/psutils.c
--- acpi-old/drivers/acpi/parser/psutils.c	Fri Jul 26 11:27:34 2002
+++ linux/drivers/acpi/parser/psutils.c	Fri Jul 26 11:20:05 2002
@@ -1,7 +1,7 @@
 /******************************************************************************
  *
  * Module Name: psutils - Parser miscellaneous utilities (Parser only)
- *              $Revision: 53 $
+ *              $Revision: 54 $
  *
  *****************************************************************************/
 
@@ -88,7 +88,7 @@
 	op->common.data_type = ACPI_DESC_TYPE_PARSER;
 	op->common.aml_opcode = opcode;
 
-	ACPI_DEBUG_ONLY_MEMBERS (ACPI_STRNCPY (op->common.aml_op_name,
+	ACPI_DISASM_ONLY_MEMBERS (ACPI_STRNCPY (op->common.aml_op_name,
 			(acpi_ps_get_opcode_info (opcode))->name, sizeof (op->common.aml_op_name)));
 }
 



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