[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