RE: 2.6.17-mm1 - possible recursive locking detected

RE: 2.6.17-mm1 - possible recursive locking detected

[Brown, Len]

>It looks like an ACPI problem.

Thanks for the note, and the .config, I reproduced it here.

CONFIG_LOCKDEP complains about this sequence:

...
	<presumed previous acquire/release acpi_gbl_hardware_lock>
...
acpi_ev_gpe_detect()
	spin_lock_irqsave(acpi_gbl_gpe_lock,)

	spin_lock_irqsave(acpi_gbl_hardware_lock,) <stack trace is on
this acquire>
	spin_lock_irqrestore(acpi_gbl_hardware_lock,)

	...

	spin_lock_irqrestore(acpi_gbl_gpe_lock)

It complains about this only the 1st time, even though
this same code sequence runs for every (subsequent) ACPI interrupt.

The intent of the arrangement is that acpi_gbl_hardware_lock is for very
small critical sections around RMW hardware register access.
It can be acquired with or without other locks held, but
nothing else is acquired when it is held.

Nothing jumps out at me as incorrect above, so 
at this point it looks like a CONFIG_LOCKDEP artifact --
but lets ask the experts:-)

-Len


>Setting up standard PCI resources
>=============================================
>[ INFO: possible recursive locking detected ]
>---------------------------------------------
>idle/1 is trying to acquire lock:
> (lock_ptr){....}, at: [<c021cbd2>] acpi_os_acquire_lock+0x8/0xa
>
>but task is already holding lock:
> (lock_ptr){....}, at: [<c021cbd2>] acpi_os_acquire_lock+0x8/0xa
>
>other info that might help us debug this:
>1 lock held by idle/1:
> #0:  (lock_ptr){....}, at: [<c021cbd2>] acpi_os_acquire_lock+0x8/0xa
>
>stack backtrace:
> [<c0103e89>] show_trace+0xd/0x10
> [<c0104483>] dump_stack+0x19/0x1b
> [<c01395fa>] __lock_acquire+0x7d9/0xa50
> [<c0139a98>] lock_acquire+0x71/0x91
> [<c02f0beb>] _spin_lock_irqsave+0x2c/0x3c
> [<c021cbd2>] acpi_os_acquire_lock+0x8/0xa
> [<c0222d95>] acpi_ev_gpe_detect+0x4d/0x10e
> [<c02215c3>] acpi_ev_sci_xrupt_handler+0x15/0x1d
> [<c021c8b1>] acpi_irq+0xe/0x18
> [<c014d36e>] request_irq+0xbe/0x10c
> [<c021cf33>] acpi_os_install_interrupt_handler+0x59/0x87
> [<c02215e7>] acpi_ev_install_sci_handler+0x1c/0x21
> [<c0220d41>] acpi_ev_install_xrupt_handlers+0x9/0x50
> [<c0231772>] acpi_enable_subsystem+0x7d/0x9a
> [<c0416656>] acpi_init+0x3f/0x170
> [<c01003ae>] _stext+0x116/0x26c
> [<c0101005>] kernel_thread_helper+0x5/0xb
>ACPI: Interpreter enabled
>
>Here is a dmesg log 
>http://www.stardust.webpages.pl/files/mm/2.6.17-mm1/mm-dmesg
>Here is a config file
>http://www.stardust.webpages.pl/files/mm/2.6.17-mm1/mm-config
>
>Regards,
>Michal
>
>-- 
>Michal K. K. Piotrowski
>LTG - Linux Testers Group
>(http://www.stardust.webpages.pl/ltg/wiki/)
>

Re: 2.6.17-mm1 - possible recursive locking detected

[Andrew Morton]

On Thu, 22 Jun 2006 00:28:56 -0400
"Brown, Len" <len.brown@intel.com> wrote:

> >It looks like an ACPI problem.
> 
> Thanks for the note, and the .config, I reproduced it here.
> 
> CONFIG_LOCKDEP complains about this sequence:
> 
> ...
> 	<presumed previous acquire/release acpi_gbl_hardware_lock>
> ...
> acpi_ev_gpe_detect()
> 	spin_lock_irqsave(acpi_gbl_gpe_lock,)
> 
> 	spin_lock_irqsave(acpi_gbl_hardware_lock,) <stack trace is on
> this acquire>
> 	spin_lock_irqrestore(acpi_gbl_hardware_lock,)
> 
> 	...
> 
> 	spin_lock_irqrestore(acpi_gbl_gpe_lock)
> 
> It complains about this only the 1st time, even though
> this same code sequence runs for every (subsequent) ACPI interrupt.
> 
> The intent of the arrangement is that acpi_gbl_hardware_lock is for very
> small critical sections around RMW hardware register access.
> It can be acquired with or without other locks held, but
> nothing else is acquired when it is held.
> 
> Nothing jumps out at me as incorrect above, so 
> at this point it looks like a CONFIG_LOCKDEP artifact --
> but lets ask the experts:-)

Yes, lockdep uses the callsite of spin_lock_init() to detect the "type" of
a lock.

But the ACPI obfuscation layers use the same spin_lock_init() site to
initialise two not-the-same locks, so lockdep decides those two locks are
of the same "type" and gets confused.

We had earlier decided to remove that ACPI code which kmallocs a single
spinlock.  When that's done, lockdep will become unconfused.

AFACIT it's all used for just two statically allocated locks anwyay.

[patch] ACPI: reduce code size, clean up, fix validator message

[Ingo Molnar]

* Andrew Morton <akpm@osdl.org> wrote:

> > It complains about this only the 1st time, even though
> > this same code sequence runs for every (subsequent) ACPI interrupt.

that is because the lock validator turns itself off after the first 
complaint.

> Yes, lockdep uses the callsite of spin_lock_init() to detect the 
> "type" of a lock.
> 
> But the ACPI obfuscation layers use the same spin_lock_init() site to 
> initialise two not-the-same locks, so lockdep decides those two locks 
> are of the same "type" and gets confused.
> 
> We had earlier decided to remove that ACPI code which kmallocs a 
> single spinlock.  When that's done, lockdep will become unconfused.
> 
> AFACIT it's all used for just two statically allocated locks anwyay.

Ok, great! Find below the (tested) cleanup that also fixes the validator 
problem.

(if ACPI wants to turn this into platform-independent code it should be 
a build-time and type-correct translation layer that understands things 
like DEFINE_SPINLOCK as well.)

	Ingo

----------------------------
Subject: ACPI: reduce code size, clean up, fix validator message
From: Ingo Molnar <mingo@elte.hu>

this patch:

- reduces ACPI code size by 336 bytes:

   text            data    bss     dec           hex      filename
   21848901        6941178 4515464 33305543      1fc33c7  vmlinux-before
   21848565        6941270 4515464 33305299      1fc32d3  vmlinux-after

- cleans the code up by going from the opaque (void *) acpi_handle
  type to an explicit type-checked spinlock_t and by removing 70
  lines of code of unnecessary layering.

- fixes lock validator message by initializing the two static
  locks build-time instead of runtime.

- speeds up the code a bit by reducing extra runtime layering and 
  improving cache footprint.

build and boot tested.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 drivers/acpi/events/evgpe.c       |   11 ++++---
 drivers/acpi/events/evgpeblk.c    |   20 ++++++-------
 drivers/acpi/events/evxface.c     |    8 ++---
 drivers/acpi/hardware/hwregs.c    |   19 ++++++------
 drivers/acpi/osl.c                |   56 --------------------------------------
 drivers/acpi/utilities/utglobal.c |    3 ++
 drivers/acpi/utilities/utmutex.c  |   13 --------
 include/acpi/acglobal.h           |    4 +-
 include/acpi/acpiosxf.h           |    8 -----
 9 files changed, 35 insertions(+), 107 deletions(-)

Index: linux/drivers/acpi/events/evgpe.c
===================================================================
--- linux.orig/drivers/acpi/events/evgpe.c
+++ linux/drivers/acpi/events/evgpe.c
@@ -396,7 +396,7 @@ u32 acpi_ev_gpe_detect(struct acpi_gpe_x
 
 	/* We need to hold the GPE lock now, hardware lock in the loop */
 
-	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
+	spin_lock_irqsave(&acpi_gbl_gpe_lock, flags);
 
 	/* Examine all GPE blocks attached to this interrupt level */
 
@@ -413,7 +413,7 @@ u32 acpi_ev_gpe_detect(struct acpi_gpe_x
 
 			gpe_register_info = &gpe_block->register_info[i];
 
-			hw_flags = acpi_os_acquire_lock(acpi_gbl_hardware_lock);
+			spin_lock_irqsave(&acpi_gbl_hardware_lock, hw_flags);
 
 			/* Read the Status Register */
 
@@ -423,7 +423,7 @@ u32 acpi_ev_gpe_detect(struct acpi_gpe_x
 						   &gpe_register_info->
 						   status_address);
 			if (ACPI_FAILURE(status)) {
-				acpi_os_release_lock(acpi_gbl_hardware_lock,
+				spin_unlock_irqrestore(&acpi_gbl_hardware_lock,
 						     hw_flags);
 				goto unlock_and_exit;
 			}
@@ -435,7 +435,8 @@ u32 acpi_ev_gpe_detect(struct acpi_gpe_x
 						   &enable_reg,
 						   &gpe_register_info->
 						   enable_address);
-			acpi_os_release_lock(acpi_gbl_hardware_lock, hw_flags);
+			spin_unlock_irqrestore(&acpi_gbl_hardware_lock,
+						hw_flags);
 
 			if (ACPI_FAILURE(status)) {
 				goto unlock_and_exit;
@@ -486,7 +487,7 @@ u32 acpi_ev_gpe_detect(struct acpi_gpe_x
 
       unlock_and_exit:
 
-	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
+	spin_unlock_irqrestore(&acpi_gbl_gpe_lock, flags);
 	return (int_status);
 }
 
Index: linux/drivers/acpi/events/evgpeblk.c
===================================================================
--- linux.orig/drivers/acpi/events/evgpeblk.c
+++ linux/drivers/acpi/events/evgpeblk.c
@@ -140,7 +140,7 @@ acpi_status acpi_ev_walk_gpe_list(acpi_g
 
 	ACPI_FUNCTION_TRACE(ev_walk_gpe_list);
 
-	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
+	spin_lock_irqsave(&acpi_gbl_gpe_lock, flags);
 
 	/* Walk the interrupt level descriptor list */
 
@@ -166,7 +166,7 @@ acpi_status acpi_ev_walk_gpe_list(acpi_g
 	}
 
       unlock_and_exit:
-	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
+	spin_unlock_irqrestore(&acpi_gbl_gpe_lock, flags);
 	return_ACPI_STATUS(status);
 }
 
@@ -513,7 +513,7 @@ static struct acpi_gpe_xrupt_info *acpi_
 
 	/* Install new interrupt descriptor with spin lock */
 
-	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
+	spin_lock_irqsave(&acpi_gbl_gpe_lock, flags);
 	if (acpi_gbl_gpe_xrupt_list_head) {
 		next_gpe_xrupt = acpi_gbl_gpe_xrupt_list_head;
 		while (next_gpe_xrupt->next) {
@@ -525,7 +525,7 @@ static struct acpi_gpe_xrupt_info *acpi_
 	} else {
 		acpi_gbl_gpe_xrupt_list_head = gpe_xrupt;
 	}
-	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
+	spin_unlock_irqrestore(&acpi_gbl_gpe_lock, flags);
 
 	/* Install new interrupt handler if not SCI_INT */
 
@@ -583,7 +583,7 @@ acpi_ev_delete_gpe_xrupt(struct acpi_gpe
 
 	/* Unlink the interrupt block with lock */
 
-	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
+	spin_lock_irqsave(&acpi_gbl_gpe_lock, flags);
 	if (gpe_xrupt->previous) {
 		gpe_xrupt->previous->next = gpe_xrupt->next;
 	}
@@ -591,7 +591,7 @@ acpi_ev_delete_gpe_xrupt(struct acpi_gpe
 	if (gpe_xrupt->next) {
 		gpe_xrupt->next->previous = gpe_xrupt->previous;
 	}
-	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
+	spin_unlock_irqrestore(&acpi_gbl_gpe_lock, flags);
 
 	/* Free the block */
 
@@ -636,7 +636,7 @@ acpi_ev_install_gpe_block(struct acpi_gp
 
 	/* Install the new block at the end of the list with lock */
 
-	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
+	spin_lock_irqsave(&acpi_gbl_gpe_lock, flags);
 	if (gpe_xrupt_block->gpe_block_list_head) {
 		next_gpe_block = gpe_xrupt_block->gpe_block_list_head;
 		while (next_gpe_block->next) {
@@ -650,7 +650,7 @@ acpi_ev_install_gpe_block(struct acpi_gp
 	}
 
 	gpe_block->xrupt_block = gpe_xrupt_block;
-	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
+	spin_unlock_irqrestore(&acpi_gbl_gpe_lock, flags);
 
       unlock_and_exit:
 	status = acpi_ut_release_mutex(ACPI_MTX_EVENTS);
@@ -696,7 +696,7 @@ acpi_status acpi_ev_delete_gpe_block(str
 	} else {
 		/* Remove the block on this interrupt with lock */
 
-		flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
+		spin_lock_irqsave(&acpi_gbl_gpe_lock, flags);
 		if (gpe_block->previous) {
 			gpe_block->previous->next = gpe_block->next;
 		} else {
@@ -707,7 +707,7 @@ acpi_status acpi_ev_delete_gpe_block(str
 		if (gpe_block->next) {
 			gpe_block->next->previous = gpe_block->previous;
 		}
-		acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
+		spin_unlock_irqrestore(&acpi_gbl_gpe_lock, flags);
 	}
 
 	/* Free the gpe_block */
Index: linux/drivers/acpi/events/evxface.c
===================================================================
--- linux.orig/drivers/acpi/events/evxface.c
+++ linux/drivers/acpi/events/evxface.c
@@ -613,7 +613,7 @@ acpi_install_gpe_handler(acpi_handle gpe
 
 	/* Install the handler */
 
-	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
+	spin_lock_irqsave(&acpi_gbl_gpe_lock, flags);
 	gpe_event_info->dispatch.handler = handler;
 
 	/* Setup up dispatch flags to indicate handler (vs. method) */
@@ -621,7 +621,7 @@ acpi_install_gpe_handler(acpi_handle gpe
 	gpe_event_info->flags &= ~(ACPI_GPE_XRUPT_TYPE_MASK | ACPI_GPE_DISPATCH_MASK);	/* Clear bits */
 	gpe_event_info->flags |= (u8) (type | ACPI_GPE_DISPATCH_HANDLER);
 
-	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
+	spin_unlock_irqrestore(&acpi_gbl_gpe_lock, flags);
 
       unlock_and_exit:
 	(void)acpi_ut_release_mutex(ACPI_MTX_EVENTS);
@@ -707,7 +707,7 @@ acpi_remove_gpe_handler(acpi_handle gpe_
 
 	/* Remove the handler */
 
-	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
+	spin_lock_irqsave(&acpi_gbl_gpe_lock, flags);
 	handler = gpe_event_info->dispatch.handler;
 
 	/* Restore Method node (if any), set dispatch flags */
@@ -717,7 +717,7 @@ acpi_remove_gpe_handler(acpi_handle gpe_
 	if (handler->method_node) {
 		gpe_event_info->flags |= ACPI_GPE_DISPATCH_METHOD;
 	}
-	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
+	spin_unlock_irqrestore(&acpi_gbl_gpe_lock, flags);
 
 	/* Now we can free the handler object */
 
Index: linux/drivers/acpi/hardware/hwregs.c
===================================================================
--- linux.orig/drivers/acpi/hardware/hwregs.c
+++ linux/drivers/acpi/hardware/hwregs.c
@@ -67,7 +67,7 @@ ACPI_MODULE_NAME("hwregs")
 acpi_status acpi_hw_clear_acpi_status(u32 flags)
 {
 	acpi_status status;
-	acpi_cpu_flags lock_flags = 0;
+	acpi_cpu_flags lock_flags;
 
 	ACPI_FUNCTION_TRACE(hw_clear_acpi_status);
 
@@ -75,7 +75,7 @@ acpi_status acpi_hw_clear_acpi_status(u3
 			  ACPI_BITMASK_ALL_FIXED_STATUS,
 			  (u16) acpi_gbl_FADT->xpm1a_evt_blk.address));
 
-	lock_flags = acpi_os_acquire_lock(acpi_gbl_hardware_lock);
+	spin_lock_irqsave(&acpi_gbl_hardware_lock, lock_flags);
 
 	status = acpi_hw_register_write(ACPI_MTX_DO_NOT_LOCK,
 					ACPI_REGISTER_PM1_STATUS,
@@ -100,7 +100,8 @@ acpi_status acpi_hw_clear_acpi_status(u3
 	status = acpi_ev_walk_gpe_list(acpi_hw_clear_gpe_block);
 
       unlock_and_exit:
-	acpi_os_release_lock(acpi_gbl_hardware_lock, lock_flags);
+	spin_unlock_irqrestore(&acpi_gbl_hardware_lock, lock_flags);
+
 	return_ACPI_STATUS(status);
 }
 
@@ -339,7 +340,7 @@ acpi_status acpi_set_register(u32 regist
 		return_ACPI_STATUS(AE_BAD_PARAMETER);
 	}
 
-	lock_flags = acpi_os_acquire_lock(acpi_gbl_hardware_lock);
+	spin_lock_irqsave(&acpi_gbl_hardware_lock, lock_flags);
 
 	/* Always do a register read first so we can insert the new bits  */
 
@@ -447,7 +448,7 @@ acpi_status acpi_set_register(u32 regist
 
       unlock_and_exit:
 
-	acpi_os_release_lock(acpi_gbl_hardware_lock, lock_flags);
+	spin_unlock_irqrestore(&acpi_gbl_hardware_lock, lock_flags);
 
 	/* Normalize the value that was read */
 
@@ -488,7 +489,7 @@ acpi_hw_register_read(u8 use_lock, u32 r
 	ACPI_FUNCTION_TRACE(hw_register_read);
 
 	if (ACPI_MTX_LOCK == use_lock) {
-		lock_flags = acpi_os_acquire_lock(acpi_gbl_hardware_lock);
+		spin_lock_irqsave(&acpi_gbl_hardware_lock, lock_flags);
 	}
 
 	switch (register_id) {
@@ -566,7 +567,7 @@ acpi_hw_register_read(u8 use_lock, u32 r
 
       unlock_and_exit:
 	if (ACPI_MTX_LOCK == use_lock) {
-		acpi_os_release_lock(acpi_gbl_hardware_lock, lock_flags);
+		spin_unlock_irqrestore(&acpi_gbl_hardware_lock, lock_flags);
 	}
 
 	if (ACPI_SUCCESS(status)) {
@@ -599,7 +600,7 @@ acpi_status acpi_hw_register_write(u8 us
 	ACPI_FUNCTION_TRACE(hw_register_write);
 
 	if (ACPI_MTX_LOCK == use_lock) {
-		lock_flags = acpi_os_acquire_lock(acpi_gbl_hardware_lock);
+		spin_lock_irqsave(&acpi_gbl_hardware_lock, lock_flags);
 	}
 
 	switch (register_id) {
@@ -689,7 +690,7 @@ acpi_status acpi_hw_register_write(u8 us
 
       unlock_and_exit:
 	if (ACPI_MTX_LOCK == use_lock) {
-		acpi_os_release_lock(acpi_gbl_hardware_lock, lock_flags);
+		spin_unlock_irqrestore(&acpi_gbl_hardware_lock, lock_flags);
 	}
 
 	return_ACPI_STATUS(status);
Index: linux/drivers/acpi/osl.c
===================================================================
--- linux.orig/drivers/acpi/osl.c
+++ linux/drivers/acpi/osl.c
@@ -685,40 +685,6 @@ void acpi_os_wait_events_complete(void *
 
 EXPORT_SYMBOL(acpi_os_wait_events_complete);
 
-/*
- * Allocate the memory for a spinlock and initialize it.
- */
-acpi_status acpi_os_create_lock(acpi_handle * out_handle)
-{
-	spinlock_t *lock_ptr;
-
-	ACPI_FUNCTION_TRACE("os_create_lock");
-
-	lock_ptr = acpi_os_allocate(sizeof(spinlock_t));
-
-	spin_lock_init(lock_ptr);
-
-	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Creating spinlock[%p].\n", lock_ptr));
-
-	*out_handle = lock_ptr;
-
-	return_ACPI_STATUS(AE_OK);
-}
-
-/*
- * Deallocate the memory for a spinlock.
- */
-void acpi_os_delete_lock(acpi_handle handle)
-{
-	ACPI_FUNCTION_TRACE("os_create_lock");
-
-	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Deleting spinlock[%p].\n", handle));
-
-	acpi_os_free(handle);
-
-	return_VOID;
-}
-
 acpi_status
 acpi_os_create_semaphore(u32 max_units, u32 initial_units, acpi_handle * handle)
 {
@@ -1037,28 +1003,6 @@ unsigned int max_cstate = ACPI_PROCESSOR
 
 EXPORT_SYMBOL(max_cstate);
 
-/*
- * Acquire a spinlock.
- *
- * handle is a pointer to the spinlock_t.
- */
-
-acpi_cpu_flags acpi_os_acquire_lock(acpi_handle handle)
-{
-	acpi_cpu_flags flags;
-	spin_lock_irqsave((spinlock_t *) handle, flags);
-	return flags;
-}
-
-/*
- * Release a spinlock. See above.
- */
-
-void acpi_os_release_lock(acpi_handle handle, acpi_cpu_flags flags)
-{
-	spin_unlock_irqrestore((spinlock_t *) handle, flags);
-}
-
 #ifndef ACPI_USE_LOCAL_CACHE
 
 /*******************************************************************************
Index: linux/drivers/acpi/utilities/utglobal.c
===================================================================
--- linux.orig/drivers/acpi/utilities/utglobal.c
+++ linux/drivers/acpi/utilities/utglobal.c
@@ -46,6 +46,9 @@
 #include <acpi/acpi.h>
 #include <acpi/acnamesp.h>
 
+DEFINE_SPINLOCK(acpi_gbl_gpe_lock);
+DEFINE_SPINLOCK(acpi_gbl_hardware_lock);
+
 #define _COMPONENT          ACPI_UTILITIES
 ACPI_MODULE_NAME("utglobal")
 
Index: linux/drivers/acpi/utilities/utmutex.c
===================================================================
--- linux.orig/drivers/acpi/utilities/utmutex.c
+++ linux/drivers/acpi/utilities/utmutex.c
@@ -79,15 +79,6 @@ acpi_status acpi_ut_mutex_initialize(voi
 			return_ACPI_STATUS(status);
 		}
 	}
-
-	/* Create the spinlocks for use at interrupt level */
-
-	status = acpi_os_create_lock(&acpi_gbl_gpe_lock);
-	if (ACPI_FAILURE(status)) {
-		return_ACPI_STATUS(status);
-	}
-
-	status = acpi_os_create_lock(&acpi_gbl_hardware_lock);
 	return_ACPI_STATUS(status);
 }
 
@@ -116,10 +107,6 @@ void acpi_ut_mutex_terminate(void)
 		(void)acpi_ut_delete_mutex(i);
 	}
 
-	/* Delete the spinlocks */
-
-	acpi_os_delete_lock(acpi_gbl_gpe_lock);
-	acpi_os_delete_lock(acpi_gbl_hardware_lock);
 	return_VOID;
 }
 
Index: linux/include/acpi/acglobal.h
===================================================================
--- linux.orig/include/acpi/acglobal.h
+++ linux/include/acpi/acglobal.h
@@ -317,8 +317,8 @@ ACPI_EXTERN struct acpi_gpe_block_info
 
 /* Spinlocks */
 
-ACPI_EXTERN acpi_handle acpi_gbl_gpe_lock;
-ACPI_EXTERN acpi_handle acpi_gbl_hardware_lock;
+extern spinlock_t acpi_gbl_gpe_lock;
+extern spinlock_t acpi_gbl_hardware_lock;
 
 /*****************************************************************************
  *
Index: linux/include/acpi/acpiosxf.h
===================================================================
--- linux.orig/include/acpi/acpiosxf.h
+++ linux/include/acpi/acpiosxf.h
@@ -108,14 +108,6 @@ acpi_status acpi_os_wait_semaphore(acpi_
 
 acpi_status acpi_os_signal_semaphore(acpi_handle handle, u32 units);
 
-acpi_status acpi_os_create_lock(acpi_handle * out_handle);
-
-void acpi_os_delete_lock(acpi_handle handle);
-
-acpi_cpu_flags acpi_os_acquire_lock(acpi_handle handle);
-
-void acpi_os_release_lock(acpi_handle handle, acpi_cpu_flags flags);
-
 /*
  * Memory allocation and mapping
  */

Re: [patch] ACPI: reduce code size, clean up, fix validator message

[Michal Piotrowski]

Hi Ingo,

On 22/06/06, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Andrew Morton <akpm@osdl.org> wrote:
>
> > > It complains about this only the 1st time, even though
> > > this same code sequence runs for every (subsequent) ACPI interrupt.
>
> that is because the lock validator turns itself off after the first
> complaint.
>
> > Yes, lockdep uses the callsite of spin_lock_init() to detect the
> > "type" of a lock.
> >
> > But the ACPI obfuscation layers use the same spin_lock_init() site to
> > initialise two not-the-same locks, so lockdep decides those two locks
> > are of the same "type" and gets confused.
> >
> > We had earlier decided to remove that ACPI code which kmallocs a
> > single spinlock.  When that's done, lockdep will become unconfused.
> >
> > AFACIT it's all used for just two statically allocated locks anwyay.
>
> Ok, great! Find below the (tested) cleanup that also fixes the validator
> problem.

Problem fixed, thanks.

>
> (if ACPI wants to turn this into platform-independent code it should be
> a build-time and type-correct translation layer that understands things
> like DEFINE_SPINLOCK as well.)
>
>         Ingo
>

Regards,
Michal

-- 
Michal K. K. Piotrowski
LTG - Linux Testers Group
(http://www.stardust.webpages.pl/ltg/wiki/)

Re: 2.6.17-mm1 - possible recursive locking detected

[Andrew Morton]

On Thu, 22 Jun 2006 03:51:51 -0400
"Brown, Len" <len.brown@intel.com> wrote:

>  
> >> The key thing here is that our recent changes in
> >> how the locks are _used_ is okay -- and I think it is.
> >
> >Well.  We don't know that.  We just know that this report of unokayness
> >wasn't right.  With Ingo's Linux-only patch we're in a 
> >position to verify
> >that the locking is probably OK.
> 
> If this were really recursive, my machine would have deadlocked
> instead of booting normally like it did, no?

yup.  If it's SMP ;)

RE: 2.6.17-mm1 - possible recursive locking detected

[Brown, Len]

 
>> The key thing here is that our recent changes in
>> how the locks are _used_ is okay -- and I think it is.
>
>Well.  We don't know that.  We just know that this report of unokayness
>wasn't right.  With Ingo's Linux-only patch we're in a 
>position to verify
>that the locking is probably OK.

If this were really recursive, my machine would have deadlocked
instead of booting normally like it did, no?

-Len

Re: 2.6.17-mm1 - possible recursive locking detected

[Andrew Morton]

On Thu, 22 Jun 2006 03:40:36 -0400
"Brown, Len" <len.brown@intel.com> wrote:

> >> Nothing jumps out at me as incorrect above, so 
> >> at this point it looks like a CONFIG_LOCKDEP artifact --
> >> but lets ask the experts:-)
> >
> >Yes, lockdep uses the callsite of spin_lock_init() to detect 
> >the "type" of
> >a lock.
> >
> >But the ACPI obfuscation layers use the same spin_lock_init() site to
> >initialise two not-the-same locks, so lockdep decides those 
> >two locks are of the same "type" and gets confused.
> 
> interesting definition of "type".  I guess it works
> in practice or others would be complaining...

It works out that way, yes.

> >We had earlier decided to remove that ACPI code which kmallocs a single
> >spinlock.  When that's done, lockdep will become unconfused.
> 
> Yes, that change is already on the way.

Is good.

> The key thing here is that our recent changes in
> how the locks are _used_ is okay -- and I think it is.

Well.  We don't know that.  We just know that this report of unokayness
wasn't right.  With Ingo's Linux-only patch we're in a position to verify
that the locking is probably OK.

RE: 2.6.17-mm1 - possible recursive locking detected

[Brown, Len]

>> Nothing jumps out at me as incorrect above, so 
>> at this point it looks like a CONFIG_LOCKDEP artifact --
>> but lets ask the experts:-)
>
>Yes, lockdep uses the callsite of spin_lock_init() to detect 
>the "type" of
>a lock.
>
>But the ACPI obfuscation layers use the same spin_lock_init() site to
>initialise two not-the-same locks, so lockdep decides those 
>two locks are of the same "type" and gets confused.

interesting definition of "type".  I guess it works
in practice or others would be complaining...

>We had earlier decided to remove that ACPI code which kmallocs a single
>spinlock.  When that's done, lockdep will become unconfused.

Yes, that change is already on the way.
The key thing here is that our recent changes in
how the locks are _used_ is okay -- and I think it is.

thanks,
-Len