[PATCH 0/3] acpi: acpi: sem out, mutex/completion in

[PATCH 0/3] acpi: acpi: sem out, mutex/completion in

[Daniel Walker]

This patch set is not merge ready. I'm just sending out an early release to get
at least some reviews on the approach taken. There is some breakage in timeouts
for the mutex changes. Any lock timeouts are ignored since there isn't a
similar mutex function to replace "down_timeout".

I have been able to boot with these changes, and it seems to be stable
so far. However, I do get lockdep warnings which, I think, are actual
locking problems in ACPI.

Daniel

[PATCH 1/3] acpi: add real mutex function calls

[Daniel Walker]

Instead of re-using semaphores for the mutex operation, I've
added usage of the kernel mutex for the os mutex implementation.

Cc: len.brown@intel.com
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Daniel Walker <dwalker@mvista.com>
---
 drivers/acpi/osl.c      |   86 +++++++++++++++++++++++++++++++++++++++++++++++
 include/acpi/acpiosxf.h |   11 +-----
 2 files changed, 88 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 235a138..8546f59 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -871,6 +871,92 @@ acpi_status acpi_os_signal_semaphore(acpi_handle handle, u32 units)
 	return AE_OK;
 }
 
+acpi_status
+acpi_os_create_mutex(acpi_mutex *handle)
+{
+	struct mutex *mutex = NULL;
+
+	mutex = acpi_os_allocate(sizeof(struct mutex));
+	if (!mutex)
+		return AE_NO_MEMORY;
+	memset(mutex, 0, sizeof(struct mutex));
+
+	mutex_init(mutex);
+
+	*handle = (acpi_handle *) mutex;
+
+	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Creating mutex[%p].\n",
+			  *handle));
+
+	return AE_OK;
+}
+
+acpi_status acpi_os_delete_mutex(acpi_mutex handle)
+{
+	struct mutex *mutex = (struct mutex *)handle;
+
+	if (!mutex)
+		return AE_BAD_PARAMETER;
+
+	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Deleting mutex[%p].\n", handle));
+
+	BUG_ON(mutex_is_locked(mutex));
+	kfree(mutex);
+	mutex = NULL;
+
+	return AE_OK;
+}
+
+acpi_status acpi_os_acquire_mutex(acpi_mutex handle, u16 timeout)
+{
+	acpi_status status = AE_OK;
+	struct mutex *mutex = (struct mutex *)handle;
+	long jiffies;
+	int ret = 0;
+
+	if (!mutex)
+		return AE_BAD_PARAMETER;
+
+	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Waiting for mutex[%p|%d]\n",
+			  handle, timeout));
+
+	if (timeout == ACPI_WAIT_FOREVER)
+		jiffies = MAX_SCHEDULE_TIMEOUT;
+	else
+		jiffies = msecs_to_jiffies(timeout);
+
+	ret = mutex_lock_interruptible_nested(mutex);
+	if (ret == -EINTR)
+		status = AE_TIME;
+
+	if (ACPI_FAILURE(status)) {
+		ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
+				  "Failed to acquire mutex[%p|%d], %s",
+				  handle, timeout,
+				  acpi_format_exception(status)));
+	} else {
+		ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
+				  "Acquired mutex[%p|%d]", handle,
+				  timeout));
+	}
+
+	return status;
+}
+
+acpi_status acpi_os_release_mutex(acpi_mutex handle)
+{
+	struct mutex *mutex = (struct mutex *)handle;
+
+	if (!mutex)
+		return AE_BAD_PARAMETER;
+
+	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Signaling mutex[%p]\n", handle));
+
+	mutex_unlock(mutex);
+
+	return AE_OK;
+}
+
 #ifdef ACPI_FUTURE_USAGE
 u32 acpi_os_get_line(char *buffer)
 {
diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
index 3f93a6b..9032ec3 100644
--- a/include/acpi/acpiosxf.h
+++ b/include/acpi/acpiosxf.h
@@ -125,18 +125,11 @@ acpi_status acpi_os_signal_semaphore(acpi_semaphore handle, u32 units);
  */
 acpi_status acpi_os_create_mutex(acpi_mutex * out_handle);
 
-void acpi_os_delete_mutex(acpi_mutex handle);
+acpi_status acpi_os_delete_mutex(acpi_mutex handle);
 
 acpi_status acpi_os_acquire_mutex(acpi_mutex handle, u16 timeout);
 
-void acpi_os_release_mutex(acpi_mutex handle);
-
-/* Temporary macros for Mutex* interfaces, map to existing semaphore xfaces */
-
-#define acpi_os_create_mutex(out_handle)    acpi_os_create_semaphore (1, 1, out_handle)
-#define acpi_os_delete_mutex(handle)        (void) acpi_os_delete_semaphore (handle)
-#define acpi_os_acquire_mutex(handle,time)  acpi_os_wait_semaphore (handle, 1, time)
-#define acpi_os_release_mutex(handle)       (void) acpi_os_signal_semaphore (handle, 1)
+acpi_status acpi_os_release_mutex(acpi_mutex handle);
 
 /*
  * Memory allocation and mapping
-- 
1.5.5.1.32.gba7d2

[PATCH 2/3] acpi: semaphore removal

[Daniel Walker]

The semaphore is used pretty extensively in acpi, but the actual
usage model is really more like completions. The ASL functions
getting implemented here are signals which follow a "wait for",
signaled, or rest format.

This implements the ACPI signaling methods with the Linux
completion API, instead of using semaphores.

completion_done() taken from Dave Chinner.

Cc: len.brown@intel.com
Cc: linux-acpi@vger.kernel.org
Cc: Dave Chinner <david@fromorbit.com>
Signed-off-by: Daniel Walker <dwalker@mvista.com>
---
 drivers/acpi/events/evmisc.c      |   12 ++++----
 drivers/acpi/executer/excreate.c  |   10 +++---
 drivers/acpi/executer/exdump.c    |    2 +-
 drivers/acpi/executer/exsystem.c  |   22 +++++++-------
 drivers/acpi/namespace/nsaccess.c |    4 +-
 drivers/acpi/osl.c                |   60 ++++++++++++++++++------------------
 drivers/acpi/utilities/utdelete.c |   12 ++++----
 drivers/acpi/utilities/utglobal.c |    2 +-
 include/acpi/acglobal.h           |    4 +-
 include/acpi/acinterp.h           |    2 +-
 include/acpi/acobject.h           |    2 +-
 include/acpi/acpiosxf.h           |   10 +++---
 include/acpi/actypes.h            |    2 +-
 include/linux/completion.h        |   19 +++++++++++
 14 files changed, 91 insertions(+), 72 deletions(-)

diff --git a/drivers/acpi/events/evmisc.c b/drivers/acpi/events/evmisc.c
index 1d5670b..b0bd8f7 100644
--- a/drivers/acpi/events/evmisc.c
+++ b/drivers/acpi/events/evmisc.c
@@ -283,7 +283,7 @@ static void ACPI_SYSTEM_XFACE acpi_ev_notify_dispatch(void *context)
  *              release interrupt occurs. Attempt to acquire the global lock,
  *              if successful, signal the thread waiting for the lock.
  *
- * NOTE: Assumes that the semaphore can be signaled from interrupt level. If
+ * NOTE: Assumes that the completion can be signaled from interrupt level. If
  * this is not possible for some reason, a separate thread will have to be
  * scheduled to do this.
  *
@@ -305,13 +305,13 @@ static u32 acpi_ev_global_lock_handler(void *context)
 		/* Got the lock, now wake all threads waiting for it */
 
 		acpi_gbl_global_lock_acquired = TRUE;
-		/* Send a unit to the semaphore */
+		/* Send a unit to the completion */
 
 		if (ACPI_FAILURE
-		    (acpi_os_signal_semaphore
-		     (acpi_gbl_global_lock_semaphore, 1))) {
+		    (acpi_os_signal_complete
+		     (acpi_gbl_global_lock_completion, 1))) {
 			ACPI_ERROR((AE_INFO,
-				    "Could not signal Global Lock semaphore"));
+				    "Could not signal Global Lock completion"));
 		}
 	}
 
@@ -494,7 +494,7 @@ acpi_status acpi_ev_acquire_global_lock(u16 timeout)
 	 * Wait for handshake with the global lock interrupt handler.
 	 * This interface releases the interpreter if we must wait.
 	 */
-	status = acpi_ex_system_wait_semaphore(acpi_gbl_global_lock_semaphore,
+	status = acpi_ex_system_wait_semaphore(acpi_gbl_global_lock_completion,
 					       ACPI_WAIT_FOREVER);
 
 	return_ACPI_STATUS(status);
diff --git a/drivers/acpi/executer/excreate.c b/drivers/acpi/executer/excreate.c
index ad09696..625be14 100644
--- a/drivers/acpi/executer/excreate.c
+++ b/drivers/acpi/executer/excreate.c
@@ -180,11 +180,11 @@ acpi_status acpi_ex_create_event(struct acpi_walk_state *walk_state)
 	}
 
 	/*
-	 * Create the actual OS semaphore, with zero initial units -- meaning
+	 * Create the actual OS completion, with zero initial units -- meaning
 	 * that the event is created in an unsignalled state
 	 */
-	status = acpi_os_create_semaphore(ACPI_NO_UNIT_LIMIT, 0,
-					  &obj_desc->event.os_semaphore);
+	status = acpi_os_create_completion(ACPI_NO_UNIT_LIMIT, 0,
+					  &obj_desc->event.os_completion);
 	if (ACPI_FAILURE(status)) {
 		goto cleanup;
 	}
@@ -198,7 +198,7 @@ acpi_status acpi_ex_create_event(struct acpi_walk_state *walk_state)
       cleanup:
 	/*
 	 * Remove local reference to the object (on error, will cause deletion
-	 * of both object and semaphore if present.)
+	 * of both object and completion if present.)
 	 */
 	acpi_ut_remove_reference(obj_desc);
 	return_ACPI_STATUS(status);
@@ -254,7 +254,7 @@ acpi_status acpi_ex_create_mutex(struct acpi_walk_state *walk_state)
       cleanup:
 	/*
 	 * Remove local reference to the object (on error, will cause deletion
-	 * of both object and semaphore if present.)
+	 * of both object and completion if present.)
 	 */
 	acpi_ut_remove_reference(obj_desc);
 	return_ACPI_STATUS(status);
diff --git a/drivers/acpi/executer/exdump.c b/drivers/acpi/executer/exdump.c
index 2be2e2b..9182ae4 100644
--- a/drivers/acpi/executer/exdump.c
+++ b/drivers/acpi/executer/exdump.c
@@ -117,7 +117,7 @@ static struct acpi_exdump_info acpi_ex_dump_device[4] = {
 
 static struct acpi_exdump_info acpi_ex_dump_event[2] = {
 	{ACPI_EXD_INIT, ACPI_EXD_TABLE_SIZE(acpi_ex_dump_event), NULL},
-	{ACPI_EXD_POINTER, ACPI_EXD_OFFSET(event.os_semaphore), "OsSemaphore"}
+	{ACPI_EXD_POINTER, ACPI_EXD_OFFSET(event.os_completion), "OsSemaphore"}
 };
 
 static struct acpi_exdump_info acpi_ex_dump_method[8] = {
diff --git a/drivers/acpi/executer/exsystem.c b/drivers/acpi/executer/exsystem.c
index 68990f1..f35508a 100644
--- a/drivers/acpi/executer/exsystem.c
+++ b/drivers/acpi/executer/exsystem.c
@@ -62,13 +62,13 @@ ACPI_MODULE_NAME("exsystem")
  *              interpreter is released before waiting.
  *
  ******************************************************************************/
-acpi_status acpi_ex_system_wait_semaphore(acpi_semaphore semaphore, u16 timeout)
+acpi_status acpi_ex_system_wait_semaphore(acpi_completion comp, u16 timeout)
 {
 	acpi_status status;
 
-	ACPI_FUNCTION_TRACE(ex_system_wait_semaphore);
+	ACPI_FUNCTION_TRACE(ex_system_wait_for_completion);
 
-	status = acpi_os_wait_semaphore(semaphore, 1, ACPI_DO_NOT_WAIT);
+	status = acpi_os_wait_for_completion(comp, 1, ACPI_DO_NOT_WAIT);
 	if (ACPI_SUCCESS(status)) {
 		return_ACPI_STATUS(status);
 	}
@@ -79,7 +79,7 @@ acpi_status acpi_ex_system_wait_semaphore(acpi_semaphore semaphore, u16 timeout)
 
 		acpi_ex_relinquish_interpreter();
 
-		status = acpi_os_wait_semaphore(semaphore, 1, timeout);
+		status = acpi_os_wait_for_completion(comp, 1, timeout);
 
 		ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
 				  "*** Thread awake after blocking, %s\n",
@@ -229,7 +229,7 @@ acpi_status acpi_ex_system_signal_event(union acpi_operand_object * obj_desc)
 
 	if (obj_desc) {
 		status =
-		    acpi_os_signal_semaphore(obj_desc->event.os_semaphore, 1);
+		    acpi_os_signal_complete(obj_desc->event.os_completion, 1);
 	}
 
 	return_ACPI_STATUS(status);
@@ -260,7 +260,7 @@ acpi_ex_system_wait_event(union acpi_operand_object *time_desc,
 
 	if (obj_desc) {
 		status =
-		    acpi_ex_system_wait_semaphore(obj_desc->event.os_semaphore,
+		    acpi_ex_system_wait_semaphore(obj_desc->event.os_completion,
 						  (u16) time_desc->integer.
 						  value);
 	}
@@ -283,19 +283,19 @@ acpi_ex_system_wait_event(union acpi_operand_object *time_desc,
 acpi_status acpi_ex_system_reset_event(union acpi_operand_object *obj_desc)
 {
 	acpi_status status = AE_OK;
-	acpi_semaphore temp_semaphore;
+	acpi_completion temp_completion;
 
 	ACPI_FUNCTION_ENTRY();
 
 	/*
-	 * We are going to simply delete the existing semaphore and
+	 * We are going to simply delete the existing completion and
 	 * create a new one!
 	 */
 	status =
-	    acpi_os_create_semaphore(ACPI_NO_UNIT_LIMIT, 0, &temp_semaphore);
+	    acpi_os_create_completion(ACPI_NO_UNIT_LIMIT, 0, &temp_completion);
 	if (ACPI_SUCCESS(status)) {
-		(void)acpi_os_delete_semaphore(obj_desc->event.os_semaphore);
-		obj_desc->event.os_semaphore = temp_semaphore;
+		(void)acpi_os_delete_completion(obj_desc->event.os_completion);
+		obj_desc->event.os_completion = temp_completion;
 	}
 
 	return (status);
diff --git a/drivers/acpi/namespace/nsaccess.c b/drivers/acpi/namespace/nsaccess.c
index c39a7f6..fc54f45 100644
--- a/drivers/acpi/namespace/nsaccess.c
+++ b/drivers/acpi/namespace/nsaccess.c
@@ -213,8 +213,8 @@ acpi_status acpi_ns_root_initialize(void)
 					/* Create additional counting semaphore for global lock */
 
 					status =
-					    acpi_os_create_semaphore(1, 0,
-								     &acpi_gbl_global_lock_semaphore);
+					    acpi_os_create_completion(1, 0,
+								     &acpi_gbl_global_lock_completion);
 					if (ACPI_FAILURE(status)) {
 						acpi_ut_remove_reference
 						    (obj_desc);
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 8546f59..3cb9b89 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -43,7 +43,7 @@
 #include <linux/ioport.h>
 #include <linux/list.h>
 #include <linux/jiffies.h>
-#include <linux/semaphore.h>
+#include <linux/completion.h>
 
 #include <asm/io.h>
 #include <asm/uaccess.h>
@@ -766,44 +766,44 @@ void acpi_os_delete_lock(acpi_spinlock handle)
 }
 
 acpi_status
-acpi_os_create_semaphore(u32 max_units, u32 initial_units, acpi_handle * handle)
+acpi_os_create_completion(u32 max_units, u32 initial_units, acpi_handle * handle)
 {
-	struct semaphore *sem = NULL;
+	struct completion *comp = NULL;
 
-	sem = acpi_os_allocate(sizeof(struct semaphore));
-	if (!sem)
+	comp = acpi_os_allocate(sizeof(struct completion));
+	if (!comp)
 		return AE_NO_MEMORY;
-	memset(sem, 0, sizeof(struct semaphore));
+	memset(comp, 0, sizeof(struct completion));
 
-	sema_init(sem, initial_units);
+	init_completion(comp);
 
-	*handle = (acpi_handle *) sem;
+	*handle = (acpi_handle *) comp;
 
-	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Creating semaphore[%p|%d].\n",
+	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Creating completion[%p|%d].\n",
 			  *handle, initial_units));
 
 	return AE_OK;
 }
 
 /*
- * TODO: A better way to delete semaphores?  Linux doesn't have a
- * 'delete_semaphore()' function -- may result in an invalid
+ * TODO: A better way to delete completions?  Linux doesn't have a
+ * 'delete_completions()' function -- may result in an invalid
  * pointer dereference for non-synchronized consumers.	Should
  * we at least check for blocked threads and signal/cancel them?
  */
 
-acpi_status acpi_os_delete_semaphore(acpi_handle handle)
+acpi_status acpi_os_delete_completion(acpi_handle handle)
 {
-	struct semaphore *sem = (struct semaphore *)handle;
+	struct completion *comp = (struct completion *)handle;
 
-	if (!sem)
+	if (!comp)
 		return AE_BAD_PARAMETER;
 
-	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Deleting semaphore[%p].\n", handle));
+	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Deleting completion[%p].\n", handle));
 
-	BUG_ON(!list_empty(&sem->wait_list));
-	kfree(sem);
-	sem = NULL;
+	BUG_ON(!completion_done(comp));
+	kfree(comp);
+	comp = NULL;
 
 	return AE_OK;
 }
@@ -811,20 +811,20 @@ acpi_status acpi_os_delete_semaphore(acpi_handle handle)
 /*
  * TODO: Support for units > 1?
  */
-acpi_status acpi_os_wait_semaphore(acpi_handle handle, u32 units, u16 timeout)
+acpi_status acpi_os_wait_for_completion(acpi_handle handle, u32 units, u16 timeout)
 {
 	acpi_status status = AE_OK;
-	struct semaphore *sem = (struct semaphore *)handle;
+	struct completion *comp = (struct completion *)handle;
 	long jiffies;
 	int ret = 0;
 
-	if (!sem || (units < 1))
+	if (!comp || (units < 1))
 		return AE_BAD_PARAMETER;
 
 	if (units > 1)
 		return AE_SUPPORT;
 
-	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Waiting for semaphore[%p|%d|%d]\n",
+	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Waiting for completion[%p|%d|%d]\n",
 			  handle, units, timeout));
 
 	if (timeout == ACPI_WAIT_FOREVER)
@@ -832,18 +832,18 @@ acpi_status acpi_os_wait_semaphore(acpi_handle handle, u32 units, u16 timeout)
 	else
 		jiffies = msecs_to_jiffies(timeout);
 	
-	ret = down_timeout(sem, jiffies);
+	ret = wait_for_completion_timeout(comp, jiffies);
 	if (ret)
 		status = AE_TIME;
 
 	if (ACPI_FAILURE(status)) {
 		ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
-				  "Failed to acquire semaphore[%p|%d|%d], %s",
+				  "Failed to acquire completion[%p|%d|%d], %s",
 				  handle, units, timeout,
 				  acpi_format_exception(status)));
 	} else {
 		ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
-				  "Acquired semaphore[%p|%d|%d]", handle,
+				  "Acquired completion[%p|%d|%d]", handle,
 				  units, timeout));
 	}
 
@@ -853,20 +853,20 @@ acpi_status acpi_os_wait_semaphore(acpi_handle handle, u32 units, u16 timeout)
 /*
  * TODO: Support for units > 1?
  */
-acpi_status acpi_os_signal_semaphore(acpi_handle handle, u32 units)
+acpi_status acpi_os_signal_complete(acpi_handle handle, u32 units)
 {
-	struct semaphore *sem = (struct semaphore *)handle;
+	struct completion *comp = (struct completion *)handle;
 
-	if (!sem || (units < 1))
+	if (!comp || (units < 1))
 		return AE_BAD_PARAMETER;
 
 	if (units > 1)
 		return AE_SUPPORT;
 
-	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Signaling semaphore[%p|%d]\n", handle,
+	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Signaling completion[%p|%d]\n", handle,
 			  units));
 
-	up(sem);
+	complete(comp);
 
 	return AE_OK;
 }
diff --git a/drivers/acpi/utilities/utdelete.c b/drivers/acpi/utilities/utdelete.c
index c5c791a..95d557e 100644
--- a/drivers/acpi/utilities/utdelete.c
+++ b/drivers/acpi/utilities/utdelete.c
@@ -163,9 +163,9 @@ static void acpi_ut_delete_internal_obj(union acpi_operand_object *object)
 			/* Global Lock has extra semaphore */
 
 			(void)
-			    acpi_os_delete_semaphore
-			    (acpi_gbl_global_lock_semaphore);
-			acpi_gbl_global_lock_semaphore = NULL;
+			    acpi_os_delete_completion
+			    (acpi_gbl_global_lock_completion);
+			acpi_gbl_global_lock_completion = NULL;
 
 			acpi_os_delete_mutex(object->mutex.os_mutex);
 			acpi_gbl_global_lock_mutex = NULL;
@@ -179,10 +179,10 @@ static void acpi_ut_delete_internal_obj(union acpi_operand_object *object)
 
 		ACPI_DEBUG_PRINT((ACPI_DB_ALLOCATIONS,
 				  "***** Event %p, OS Semaphore %p\n",
-				  object, object->event.os_semaphore));
+				  object, object->event.os_completion));
 
-		(void)acpi_os_delete_semaphore(object->event.os_semaphore);
-		object->event.os_semaphore = NULL;
+		(void)acpi_os_delete_completion(object->event.os_completion);
+		object->event.os_completion = NULL;
 		break;
 
 	case ACPI_TYPE_METHOD:
diff --git a/drivers/acpi/utilities/utglobal.c b/drivers/acpi/utilities/utglobal.c
index a6e71b8..a0ef412 100644
--- a/drivers/acpi/utilities/utglobal.c
+++ b/drivers/acpi/utilities/utglobal.c
@@ -727,7 +727,7 @@ void acpi_ut_init_globals(void)
 
 	/* Global Lock support */
 
-	acpi_gbl_global_lock_semaphore = NULL;
+	acpi_gbl_global_lock_completion = NULL;
 	acpi_gbl_global_lock_mutex = NULL;
 	acpi_gbl_global_lock_acquired = FALSE;
 	acpi_gbl_global_lock_handle = 0;
diff --git a/include/acpi/acglobal.h b/include/acpi/acglobal.h
index 15dda46..50f520f 100644
--- a/include/acpi/acglobal.h
+++ b/include/acpi/acglobal.h
@@ -171,10 +171,10 @@ ACPI_EXTERN struct acpi_mutex_info acpi_gbl_mutex_info[ACPI_NUM_MUTEX];
 
 /*
  * Global lock mutex is an actual AML mutex object
- * Global lock semaphore works in conjunction with the HW global lock
+ * System Global lock works in conjunction with the HW global lock
  */
 ACPI_EXTERN union acpi_operand_object *acpi_gbl_global_lock_mutex;
-ACPI_EXTERN acpi_semaphore acpi_gbl_global_lock_semaphore;
+ACPI_EXTERN acpi_completion acpi_gbl_global_lock_completion;
 ACPI_EXTERN u16 acpi_gbl_global_lock_handle;
 ACPI_EXTERN u8 acpi_gbl_global_lock_acquired;
 ACPI_EXTERN u8 acpi_gbl_global_lock_present;
diff --git a/include/acpi/acinterp.h b/include/acpi/acinterp.h
index e8db7a3..daaa478 100644
--- a/include/acpi/acinterp.h
+++ b/include/acpi/acinterp.h
@@ -291,7 +291,7 @@ acpi_ex_system_wait_event(union acpi_operand_object *time,
 acpi_status acpi_ex_system_reset_event(union acpi_operand_object *obj_desc);
 
 acpi_status
-acpi_ex_system_wait_semaphore(acpi_semaphore semaphore, u16 timeout);
+acpi_ex_system_wait_semaphore(acpi_completion semaphore, u16 timeout);
 
 acpi_status acpi_ex_system_wait_mutex(acpi_mutex mutex, u16 timeout);
 
diff --git a/include/acpi/acobject.h b/include/acpi/acobject.h
index e9657da..f63117d 100644
--- a/include/acpi/acobject.h
+++ b/include/acpi/acobject.h
@@ -149,7 +149,7 @@ struct acpi_object_package {
  *****************************************************************************/
 
 struct acpi_object_event {
-	ACPI_OBJECT_COMMON_HEADER acpi_semaphore os_semaphore;	/* Actual OS synchronization object */
+	ACPI_OBJECT_COMMON_HEADER acpi_completion os_completion;	/* Actual OS synchronization object */
 };
 
 struct acpi_object_mutex {
diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
index 9032ec3..140e078 100644
--- a/include/acpi/acpiosxf.h
+++ b/include/acpi/acpiosxf.h
@@ -110,15 +110,15 @@ void acpi_os_release_lock(acpi_spinlock handle, acpi_cpu_flags flags);
  * Semaphore primitives
  */
 acpi_status
-acpi_os_create_semaphore(u32 max_units,
-			 u32 initial_units, acpi_semaphore * out_handle);
+acpi_os_create_completion(u32 max_units,
+			 u32 initial_units, acpi_completion * out_handle);
 
-acpi_status acpi_os_delete_semaphore(acpi_semaphore handle);
+acpi_status acpi_os_delete_completion(acpi_completion handle);
 
 acpi_status
-acpi_os_wait_semaphore(acpi_semaphore handle, u32 units, u16 timeout);
+acpi_os_wait_for_completion(acpi_completion handle, u32 units, u16 timeout);
 
-acpi_status acpi_os_signal_semaphore(acpi_semaphore handle, u32 units);
+acpi_status acpi_os_signal_complete(acpi_completion handle, u32 units);
 
 /*
  * Mutex primitives
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index 4ea4f40..f580f12 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -316,7 +316,7 @@ struct uint32_struct {
 /* Synchronization objects */
 
 #define acpi_mutex                      void *
-#define acpi_semaphore                  void *
+#define acpi_completion                 void *
 
 /*
  * Acpi integer width. In ACPI version 1, integers are 32 bits.  In ACPI
diff --git a/include/linux/completion.h b/include/linux/completion.h
index d2961b6..00b1288 100644
--- a/include/linux/completion.h
+++ b/include/linux/completion.h
@@ -36,6 +36,25 @@ struct completion {
 # define DECLARE_COMPLETION_ONSTACK(work) DECLARE_COMPLETION(work)
 #endif
 
+/**
+ *	completion_done - Test to see if a completion has any waiters
+ *	@x:	completion structure
+ *
+ *	Returns: 0 if there are waiters (wait_for_completion() in progress)
+ *		 1 if there are no waiters.
+ *
+ */
+static inline bool completion_done(struct completion *x)
+{
+	int ret = 1;
+
+	spin_lock_irq(&x->wait.lock);
+	if (!x->done)
+		ret = 0;
+	spin_unlock_irq(&x->wait.lock);
+	return ret;
+}
+
 static inline void init_completion(struct completion *x)
 {
 	x->done = 0;
-- 
1.5.5.1.32.gba7d2

[PATCH 3/3] Add lockdep integration for the ACPI mutex usage.

[Daniel Walker]

Cc: len.brown@intel.com
Cc: linux-acpi@vger.kernel.org
Signed-off-by: Daniel Walker <dwalker@mvista.com>
---
 drivers/acpi/dispatcher/dsmethod.c |    3 ++-
 drivers/acpi/executer/excreate.c   |    7 ++++---
 drivers/acpi/executer/exmutex.c    |    5 +++--
 drivers/acpi/executer/exsystem.c   |    6 ++++--
 drivers/acpi/namespace/nsaccess.c  |    4 +++-
 drivers/acpi/osl.c                 |   11 +++++++----
 drivers/acpi/utilities/utmutex.c   |    7 ++++---
 include/acpi/acinterp.h            |    5 ++++-
 include/acpi/acpiosxf.h            |    8 ++++++--
 9 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/drivers/acpi/dispatcher/dsmethod.c b/drivers/acpi/dispatcher/dsmethod.c
index 4613b9c..cbc6e60 100644
--- a/drivers/acpi/dispatcher/dsmethod.c
+++ b/drivers/acpi/dispatcher/dsmethod.c
@@ -130,6 +130,7 @@ acpi_ds_method_error(acpi_status status, struct acpi_walk_state *walk_state)
 static acpi_status
 acpi_ds_create_method_mutex(union acpi_operand_object *method_desc)
 {
+	static struct lock_class_key method_mutex;
 	union acpi_operand_object *mutex_desc;
 	acpi_status status;
 
@@ -144,7 +145,7 @@ acpi_ds_create_method_mutex(union acpi_operand_object *method_desc)
 
 	/* Create the actual OS Mutex */
 
-	status = acpi_os_create_mutex(&mutex_desc->mutex.os_mutex);
+	status = acpi_os_create_mutex(&mutex_desc->mutex.os_mutex, &method_mutex);
 	if (ACPI_FAILURE(status)) {
 		return_ACPI_STATUS(status);
 	}
diff --git a/drivers/acpi/executer/excreate.c b/drivers/acpi/executer/excreate.c
index 625be14..c3657e6 100644
--- a/drivers/acpi/executer/excreate.c
+++ b/drivers/acpi/executer/excreate.c
@@ -222,6 +222,7 @@ acpi_status acpi_ex_create_mutex(struct acpi_walk_state *walk_state)
 {
 	acpi_status status = AE_OK;
 	union acpi_operand_object *obj_desc;
+	static struct lock_class_key interpreter_class;
 
 	ACPI_FUNCTION_TRACE_PTR(ex_create_mutex, ACPI_WALK_OPERANDS);
 
@@ -233,9 +234,9 @@ acpi_status acpi_ex_create_mutex(struct acpi_walk_state *walk_state)
 		goto cleanup;
 	}
 
-	/* Create the actual OS Mutex */
-
-	status = acpi_os_create_mutex(&obj_desc->mutex.os_mutex);
+	status = 
+	acpi_os_create_mutex(&obj_desc->mutex.os_mutex,
+			    &interpreter_class);
 	if (ACPI_FAILURE(status)) {
 		goto cleanup;
 	}
diff --git a/drivers/acpi/executer/exmutex.c b/drivers/acpi/executer/exmutex.c
index a8bf3d7..6472a24 100644
--- a/drivers/acpi/executer/exmutex.c
+++ b/drivers/acpi/executer/exmutex.c
@@ -176,8 +176,9 @@ acpi_ex_acquire_mutex_object(u16 timeout,
 	if (obj_desc == acpi_gbl_global_lock_mutex) {
 		status = acpi_ev_acquire_global_lock(timeout);
 	} else {
-		status = acpi_ex_system_wait_mutex(obj_desc->mutex.os_mutex,
-						   timeout);
+		status =
+		acpi_ex_system_wait_mutex(obj_desc->mutex.os_mutex,
+						 timeout);
 	}
 
 	if (ACPI_FAILURE(status)) {
diff --git a/drivers/acpi/executer/exsystem.c b/drivers/acpi/executer/exsystem.c
index f35508a..5088e3a 100644
--- a/drivers/acpi/executer/exsystem.c
+++ b/drivers/acpi/executer/exsystem.c
@@ -108,13 +108,15 @@ acpi_status acpi_ex_system_wait_semaphore(acpi_completion comp, u16 timeout)
  *
  ******************************************************************************/
 
-acpi_status acpi_ex_system_wait_mutex(acpi_mutex mutex, u16 timeout)
+acpi_status
+acpi_ex_system_wait_mutex_nested(acpi_mutex mutex, u16 timeout,
+				 unsigned int subclass)
 {
 	acpi_status status;
 
 	ACPI_FUNCTION_TRACE(ex_system_wait_mutex);
 
-	status = acpi_os_acquire_mutex(mutex, ACPI_DO_NOT_WAIT);
+	status = acpi_os_acquire_mutex_nested(mutex, ACPI_DO_NOT_WAIT, subclass);
 	if (ACPI_SUCCESS(status)) {
 		return_ACPI_STATUS(status);
 	}
diff --git a/drivers/acpi/namespace/nsaccess.c b/drivers/acpi/namespace/nsaccess.c
index fc54f45..36579c5 100644
--- a/drivers/acpi/namespace/nsaccess.c
+++ b/drivers/acpi/namespace/nsaccess.c
@@ -64,6 +64,7 @@ ACPI_MODULE_NAME("nsaccess")
  ******************************************************************************/
 acpi_status acpi_ns_root_initialize(void)
 {
+	static struct lock_class_key ns_root_mutex;
 	acpi_status status;
 	const struct acpi_predefined_names *init_val = NULL;
 	struct acpi_namespace_node *new_node;
@@ -199,7 +200,8 @@ acpi_status acpi_ns_root_initialize(void)
 
 				status =
 				    acpi_os_create_mutex(&obj_desc->mutex.
-							 os_mutex);
+							 os_mutex,
+							 &ns_root_mutex);
 				if (ACPI_FAILURE(status)) {
 					acpi_ut_remove_reference(obj_desc);
 					goto unlock_and_exit;
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 3cb9b89..7c6abd6 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -872,7 +872,7 @@ acpi_status acpi_os_signal_complete(acpi_handle handle, u32 units)
 }
 
 acpi_status
-acpi_os_create_mutex(acpi_mutex *handle)
+acpi_os_create_mutex(acpi_mutex * handle, struct lock_class_key *key)
 {
 	struct mutex *mutex = NULL;
 
@@ -882,6 +882,7 @@ acpi_os_create_mutex(acpi_mutex *handle)
 	memset(mutex, 0, sizeof(struct mutex));
 
 	mutex_init(mutex);
+	lockdep_set_class(mutex, key);
 
 	*handle = (acpi_handle *) mutex;
 
@@ -907,7 +908,9 @@ acpi_status acpi_os_delete_mutex(acpi_mutex handle)
 	return AE_OK;
 }
 
-acpi_status acpi_os_acquire_mutex(acpi_mutex handle, u16 timeout)
+acpi_status
+acpi_os_acquire_mutex_nested(acpi_mutex handle, u16 timeout,
+			     unsigned int subclass)
 {
 	acpi_status status = AE_OK;
 	struct mutex *mutex = (struct mutex *)handle;
@@ -924,8 +927,8 @@ acpi_status acpi_os_acquire_mutex(acpi_mutex handle, u16 timeout)
 		jiffies = MAX_SCHEDULE_TIMEOUT;
 	else
 		jiffies = msecs_to_jiffies(timeout);
-
-	ret = mutex_lock_interruptible_nested(mutex);
+	
+	ret = mutex_lock_interruptible_nested(mutex, subclass);
 	if (ret == -EINTR)
 		status = AE_TIME;
 
diff --git a/drivers/acpi/utilities/utmutex.c b/drivers/acpi/utilities/utmutex.c
index 7331dde..b4c6f33 100644
--- a/drivers/acpi/utilities/utmutex.c
+++ b/drivers/acpi/utilities/utmutex.c
@@ -134,6 +134,7 @@ void acpi_ut_mutex_terminate(void)
 
 static acpi_status acpi_ut_create_mutex(acpi_mutex_handle mutex_id)
 {
+	static struct lock_class_key utility_mutex;
 	acpi_status status = AE_OK;
 
 	ACPI_FUNCTION_TRACE_U32(ut_create_mutex, mutex_id);
@@ -144,7 +145,7 @@ static acpi_status acpi_ut_create_mutex(acpi_mutex_handle mutex_id)
 
 	if (!acpi_gbl_mutex_info[mutex_id].mutex) {
 		status =
-		    acpi_os_create_mutex(&acpi_gbl_mutex_info[mutex_id].mutex);
+		    acpi_os_create_mutex(&acpi_gbl_mutex_info[mutex_id].mutex, &utility_mutex);
 		acpi_gbl_mutex_info[mutex_id].thread_id =
 		    ACPI_MUTEX_NOT_ACQUIRED;
 		acpi_gbl_mutex_info[mutex_id].use_count = 0;
@@ -247,8 +248,8 @@ acpi_status acpi_ut_acquire_mutex(acpi_mutex_handle mutex_id)
 			  (unsigned long)this_thread_id,
 			  acpi_ut_get_mutex_name(mutex_id)));
 
-	status = acpi_os_acquire_mutex(acpi_gbl_mutex_info[mutex_id].mutex,
-				       ACPI_WAIT_FOREVER);
+	status = acpi_os_acquire_mutex_nested(acpi_gbl_mutex_info[mutex_id].mutex,
+				       ACPI_WAIT_FOREVER, mutex_id);
 	if (ACPI_SUCCESS(status)) {
 		ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
 				  "Thread %lX acquired Mutex [%s]\n",
diff --git a/include/acpi/acinterp.h b/include/acpi/acinterp.h
index daaa478..a3cca24 100644
--- a/include/acpi/acinterp.h
+++ b/include/acpi/acinterp.h
@@ -293,7 +293,10 @@ acpi_status acpi_ex_system_reset_event(union acpi_operand_object *obj_desc);
 acpi_status
 acpi_ex_system_wait_semaphore(acpi_completion semaphore, u16 timeout);
 
-acpi_status acpi_ex_system_wait_mutex(acpi_mutex mutex, u16 timeout);
+acpi_status acpi_ex_system_wait_mutex_nested(acpi_mutex mutex, u16 timeout,
+					     unsigned int subclass);
+#define acpi_ex_system_wait_mutex(mutex, timeout) \
+	acpi_ex_system_wait_mutex_nested(mutex, timeout, 0)
 
 /*
  * exoparg1 - ACPI AML execution, 1 operand
diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
index 140e078..dd34079 100644
--- a/include/acpi/acpiosxf.h
+++ b/include/acpi/acpiosxf.h
@@ -123,11 +123,15 @@ acpi_status acpi_os_signal_complete(acpi_completion handle, u32 units);
 /*
  * Mutex primitives
  */
-acpi_status acpi_os_create_mutex(acpi_mutex * out_handle);
+acpi_status
+acpi_os_create_mutex(acpi_mutex * handle, struct lock_class_key *key);
 
 acpi_status acpi_os_delete_mutex(acpi_mutex handle);
 
-acpi_status acpi_os_acquire_mutex(acpi_mutex handle, u16 timeout);
+acpi_status
+acpi_os_acquire_mutex_nested(acpi_mutex handle, u16 timeout, unsigned int subclass);
+#define acpi_os_acquire_mutex(handle, timeout) \
+	acpi_os_acquire_mutex_nested(handle, timeout, 0)
 
 acpi_status acpi_os_release_mutex(acpi_mutex handle);
 
-- 
1.5.5.1.32.gba7d2

Re: [PATCH 2/3] acpi: semaphore removal

[Dave Chinner]

On Sat, Jul 19, 2008 at 11:16:53AM -0700, Daniel Walker wrote:
> The semaphore is used pretty extensively in acpi, but the actual
> usage model is really more like completions. The ASL functions
> getting implemented here are signals which follow a "wait for",
> signaled, or rest format.
> 
> This implements the ACPI signaling methods with the Linux
> completion API, instead of using semaphores.
> 
> completion_done() taken from Dave Chinner.

The patch series that contained completion_done() is (AFAIK)
in the -mm tree at the moment. You'd probably do best to keep
it as a separate patch in your series so we don't end up with
random conflicts all over the place....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/3] acpi: semaphore removal

[Daniel Walker]

On Sun, 2008-07-20 at 18:15 +1000, Dave Chinner wrote:
> On Sat, Jul 19, 2008 at 11:16:53AM -0700, Daniel Walker wrote:
> > The semaphore is used pretty extensively in acpi, but the actual
> > usage model is really more like completions. The ASL functions
> > getting implemented here are signals which follow a "wait for",
> > signaled, or rest format.
> > 
> > This implements the ACPI signaling methods with the Linux
> > completion API, instead of using semaphores.
> > 
> > completion_done() taken from Dave Chinner.
> 
> The patch series that contained completion_done() is (AFAIK)
> in the -mm tree at the moment. You'd probably do best to keep
> it as a separate patch in your series so we don't end up with
> random conflicts all over the place....

Yeah,  I will eventually pull it out .. I'm not too worried about it,
since these patches are a ways from even going into -mm ..

Daniel

Re: [PATCH 1/3] acpi: add real mutex function calls

[Zhao Yakui]

On Sat, 2008-07-19 at 11:16 -0700, Daniel Walker wrote:
> Instead of re-using semaphores for the mutex operation, I've
> added usage of the kernel mutex for the os mutex implementation.
> 
What is the advantage that the kernel mutex is used for the ACPI mutex
implementation instead of using semaphore?
And it seems that too much ACPICA source code is touched.

Thanks
   Yakui

> Cc: len.brown@intel.com
> Cc: linux-acpi@vger.kernel.org
> Signed-off-by: Daniel Walker <dwalker@mvista.com>
> ---
>  drivers/acpi/osl.c      |   86 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/acpi/acpiosxf.h |   11 +-----
>  2 files changed, 88 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 235a138..8546f59 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -871,6 +871,92 @@ acpi_status acpi_os_signal_semaphore(acpi_handle handle, u32 units)
>  	return AE_OK;
>  }
>  
> +acpi_status
> +acpi_os_create_mutex(acpi_mutex *handle)
> +{
> +	struct mutex *mutex = NULL;
> +
> +	mutex = acpi_os_allocate(sizeof(struct mutex));
> +	if (!mutex)
> +		return AE_NO_MEMORY;
> +	memset(mutex, 0, sizeof(struct mutex));
> +
> +	mutex_init(mutex);
> +
> +	*handle = (acpi_handle *) mutex;
> +
> +	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Creating mutex[%p].\n",
> +			  *handle));
> +
> +	return AE_OK;
> +}
> +
> +acpi_status acpi_os_delete_mutex(acpi_mutex handle)
> +{
> +	struct mutex *mutex = (struct mutex *)handle;
> +
> +	if (!mutex)
> +		return AE_BAD_PARAMETER;
> +
> +	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Deleting mutex[%p].\n", handle));
> +
> +	BUG_ON(mutex_is_locked(mutex));
> +	kfree(mutex);
> +	mutex = NULL;
> +
> +	return AE_OK;
> +}
> +
> +acpi_status acpi_os_acquire_mutex(acpi_mutex handle, u16 timeout)
> +{
> +	acpi_status status = AE_OK;
> +	struct mutex *mutex = (struct mutex *)handle;
> +	long jiffies;
> +	int ret = 0;
> +
> +	if (!mutex)
> +		return AE_BAD_PARAMETER;
> +
> +	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Waiting for mutex[%p|%d]\n",
> +			  handle, timeout));
> +
> +	if (timeout == ACPI_WAIT_FOREVER)
> +		jiffies = MAX_SCHEDULE_TIMEOUT;
> +	else
> +		jiffies = msecs_to_jiffies(timeout);
> +
> +	ret = mutex_lock_interruptible_nested(mutex);
> +	if (ret == -EINTR)
> +		status = AE_TIME;
> +
> +	if (ACPI_FAILURE(status)) {
> +		ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
> +				  "Failed to acquire mutex[%p|%d], %s",
> +				  handle, timeout,
> +				  acpi_format_exception(status)));
> +	} else {
> +		ACPI_DEBUG_PRINT((ACPI_DB_MUTEX,
> +				  "Acquired mutex[%p|%d]", handle,
> +				  timeout));
> +	}
> +
> +	return status;
> +}
> +
> +acpi_status acpi_os_release_mutex(acpi_mutex handle)
> +{
> +	struct mutex *mutex = (struct mutex *)handle;
> +
> +	if (!mutex)
> +		return AE_BAD_PARAMETER;
> +
> +	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Signaling mutex[%p]\n", handle));
> +
> +	mutex_unlock(mutex);
> +
> +	return AE_OK;
> +}
> +
>  #ifdef ACPI_FUTURE_USAGE
>  u32 acpi_os_get_line(char *buffer)
>  {
> diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
> index 3f93a6b..9032ec3 100644
> --- a/include/acpi/acpiosxf.h
> +++ b/include/acpi/acpiosxf.h
> @@ -125,18 +125,11 @@ acpi_status acpi_os_signal_semaphore(acpi_semaphore handle, u32 units);
>   */
>  acpi_status acpi_os_create_mutex(acpi_mutex * out_handle);
>  
> -void acpi_os_delete_mutex(acpi_mutex handle);
> +acpi_status acpi_os_delete_mutex(acpi_mutex handle);
>  
>  acpi_status acpi_os_acquire_mutex(acpi_mutex handle, u16 timeout);
>  
> -void acpi_os_release_mutex(acpi_mutex handle);
> -
> -/* Temporary macros for Mutex* interfaces, map to existing semaphore xfaces */
> -
> -#define acpi_os_create_mutex(out_handle)    acpi_os_create_semaphore (1, 1, out_handle)
> -#define acpi_os_delete_mutex(handle)        (void) acpi_os_delete_semaphore (handle)
> -#define acpi_os_acquire_mutex(handle,time)  acpi_os_wait_semaphore (handle, 1, time)
> -#define acpi_os_release_mutex(handle)       (void) acpi_os_signal_semaphore (handle, 1)
> +acpi_status acpi_os_release_mutex(acpi_mutex handle);
>  
>  /*
>   * Memory allocation and mapping

Re: [PATCH 1/3] acpi: add real mutex function calls

[Peter Zijlstra]

On Mon, 2008-07-21 at 09:51 +0800, Zhao Yakui wrote:
> On Sat, 2008-07-19 at 11:16 -0700, Daniel Walker wrote:
> > Instead of re-using semaphores for the mutex operation, I've
> > added usage of the kernel mutex for the os mutex implementation.
> > 
> What is the advantage that the kernel mutex is used for the ACPI mutex
> implementation instead of using semaphore?
> And it seems that too much ACPICA source code is touched.

You get help from lockdep, and also our goal is to fully eradicate
semaphore usage.

Re: [PATCH 1/3] acpi: add real mutex function calls

[Andi Kleen]

Peter Zijlstra wrote:
> On Mon, 2008-07-21 at 09:51 +0800, Zhao Yakui wrote:
>> On Sat, 2008-07-19 at 11:16 -0700, Daniel Walker wrote:
>>> Instead of re-using semaphores for the mutex operation, I've
>>> added usage of the kernel mutex for the os mutex implementation.
>>>
>> What is the advantage that the kernel mutex is used for the ACPI mutex
>> implementation instead of using semaphore?
>> And it seems that too much ACPICA source code is touched.
> 
> You get help from lockdep, and also our goal is to fully eradicate
> semaphore usage.

Issue is that ACPICA is shared with other OS source code and to replace 
a major interface like this would mean replacing it for everyone. It 
might end up with ACPICA just reimplementing a semaphore like wrapper if 
semaphores really go away, but I don't really see that coming anyways.

-Andi

Re: [PATCH 1/3] acpi: add real mutex function calls

[Peter Zijlstra]

On Mon, 2008-07-21 at 11:17 +0200, Andi Kleen wrote:
> Peter Zijlstra wrote:
> > On Mon, 2008-07-21 at 09:51 +0800, Zhao Yakui wrote:
> >> On Sat, 2008-07-19 at 11:16 -0700, Daniel Walker wrote:
> >>> Instead of re-using semaphores for the mutex operation, I've
> >>> added usage of the kernel mutex for the os mutex implementation.
> >>>
> >> What is the advantage that the kernel mutex is used for the ACPI mutex
> >> implementation instead of using semaphore?
> >> And it seems that too much ACPICA source code is touched.
> > 
> > You get help from lockdep, and also our goal is to fully eradicate
> > semaphore usage.
> 
> Issue is that ACPICA is shared with other OS source code and to replace 
> a major interface like this would mean replacing it for everyone. It 
> might end up with ACPICA just reimplementing a semaphore like wrapper if 
> semaphores really go away, but I don't really see that coming anyways.

Andi, you know better than that.

Re: [PATCH 1/3] acpi: add real mutex function calls

[Daniel Walker]

On Mon, 2008-07-21 at 09:51 +0800, Zhao Yakui wrote:
> On Sat, 2008-07-19 at 11:16 -0700, Daniel Walker wrote:
> > Instead of re-using semaphores for the mutex operation, I've
> > added usage of the kernel mutex for the os mutex implementation.
> > 
> What is the advantage that the kernel mutex is used for the ACPI mutex
> implementation instead of using semaphore?
> And it seems that too much ACPICA source code is touched.

In general you would want to use a mutex whenever your using mutex-like
semantics. If I see a mutex used in code then I have a pretty good idea
the locking is sane.. As Peter mentioned , you also get lockdep which
helps to find lots of different locking problems.

You can also look at Documentation/mutex-design.txt which has a fairly
big list of benefits over the semaphore. (tho when that was written the
semaphore implementation was different.)

Daniel

Re: [PATCH 1/3] acpi: add real mutex function calls

[Daniel Walker]

On Mon, 2008-07-21 at 11:17 +0200, Andi Kleen wrote:
> Peter Zijlstra wrote:
> > On Mon, 2008-07-21 at 09:51 +0800, Zhao Yakui wrote:
> >> On Sat, 2008-07-19 at 11:16 -0700, Daniel Walker wrote:
> >>> Instead of re-using semaphores for the mutex operation, I've
> >>> added usage of the kernel mutex for the os mutex implementation.
> >>>
> >> What is the advantage that the kernel mutex is used for the ACPI mutex
> >> implementation instead of using semaphore?
> >> And it seems that too much ACPICA source code is touched.
> > 
> > You get help from lockdep, and also our goal is to fully eradicate
> > semaphore usage.
> 
> Issue is that ACPICA is shared with other OS source code and to replace 
> a major interface like this would mean replacing it for everyone. It 
> might end up with ACPICA just reimplementing a semaphore like wrapper if 
> semaphores really go away, but I don't really see that coming anyways.

For other OS's the "completion" interface would just be connected to the
semaphore .. I could even remove my rename, but still connect ACPI
"semaphores" to completions . Of course you can always attach the mutex
interface to a semaphore, that's how it currently is in Linux ACPI.

Daniel

Re: [PATCH 1/3] acpi: add real mutex function calls

[Andi Kleen]

Peter Zijlstra wrote:
> On Mon, 2008-07-21 at 11:17 +0200, Andi Kleen wrote:
>> Peter Zijlstra wrote:
>>> On Mon, 2008-07-21 at 09:51 +0800, Zhao Yakui wrote:
>>>> On Sat, 2008-07-19 at 11:16 -0700, Daniel Walker wrote:
>>>>> Instead of re-using semaphores for the mutex operation, I've
>>>>> added usage of the kernel mutex for the os mutex implementation.
>>>>>
>>>> What is the advantage that the kernel mutex is used for the ACPI mutex
>>>> implementation instead of using semaphore?
>>>> And it seems that too much ACPICA source code is touched.
>>> You get help from lockdep, and also our goal is to fully eradicate
>>> semaphore usage.
>> Issue is that ACPICA is shared with other OS source code and to replace 
>> a major interface like this would mean replacing it for everyone. It 
>> might end up with ACPICA just reimplementing a semaphore like wrapper if 
>> semaphores really go away, but I don't really see that coming anyways.
> 
> Andi, you know better than that.

Know better than what?

My understanding was that there are a few areas
in the kernel who really use true semaphore semantics and I don't see it 
as particularly useful to force them to use something else that doesn't 
fit them as well.  And there are areas like ACPICA where semaphores are 
an useful abstraction because of other consideration (in ACPICA's case 
due to portability).

Especially now that semaphores are not duplicated per architecture 
anymore so actually keeping them around is not that costly. Yes they are 
incompatible to lockdep, but lockdep support is only one consideration 
among others and not the master of all code.

Especially adding own semaphore wrappers to some code like ACPICA
when there are already perfectly usable generic semaphores wouldn't
strike me as an improvement and that ugliness of that would IMHO far 
outweight the benefit of lockdep support.

-Andi

Re: [PATCH 1/3] acpi: add real mutex function calls

[Andi Kleen]

Daniel Walker wrote:
> On Mon, 2008-07-21 at 09:51 +0800, Zhao Yakui wrote:
>> On Sat, 2008-07-19 at 11:16 -0700, Daniel Walker wrote:
>>> Instead of re-using semaphores for the mutex operation, I've
>>> added usage of the kernel mutex for the os mutex implementation.
>>>
>> What is the advantage that the kernel mutex is used for the ACPI mutex
>> implementation instead of using semaphore?
>> And it seems that too much ACPICA source code is touched.
> 
> In general you would want to use a mutex whenever your using mutex-like
> semantics. If I see a mutex used in code then I have a pretty good idea
> the locking is sane..

With a mutex the locking can be totally broken and with a semaphore the 
locking can be completely fine. They are both just tools which can be 
used correctly and also incorrectly.

The main advantage of mutexes is that they can be slightly more 
efficient (doesn't matter for the ACPI case, the ACPI interpreter is not 
performance critical) and that they are easier to check with lockdep. 
But there are also other considerations like in ACPICA's case portability.

My feeling is that adding own semaphore wrappers to ACPICA wouldn't
be an improvement. Defering to Bob Moore for his opinion, since
he maintains ACPICA.

-Andi

Re: [PATCH 1/3] acpi: add real mutex function calls

[Peter Zijlstra]

On Mon, 2008-07-21 at 21:15 +0200, Andi Kleen wrote:
> Peter Zijlstra wrote:
> > On Mon, 2008-07-21 at 11:17 +0200, Andi Kleen wrote:
> >> Peter Zijlstra wrote:
> >>> On Mon, 2008-07-21 at 09:51 +0800, Zhao Yakui wrote:
> >>>> On Sat, 2008-07-19 at 11:16 -0700, Daniel Walker wrote:
> >>>>> Instead of re-using semaphores for the mutex operation, I've
> >>>>> added usage of the kernel mutex for the os mutex implementation.
> >>>>>
> >>>> What is the advantage that the kernel mutex is used for the ACPI mutex
> >>>> implementation instead of using semaphore?
> >>>> And it seems that too much ACPICA source code is touched.
> >>> You get help from lockdep, and also our goal is to fully eradicate
> >>> semaphore usage.
> >> Issue is that ACPICA is shared with other OS source code and to replace 
> >> a major interface like this would mean replacing it for everyone. It 
> >> might end up with ACPICA just reimplementing a semaphore like wrapper if 
> >> semaphores really go away, but I don't really see that coming anyways.
> > 
> > Andi, you know better than that.
> 
> Know better than what?

Know better than to say we need ugly code in Linux because
$SOME_OTHER_OS.

> My understanding was that there are a few areas
> in the kernel who really use true semaphore semantics and I don't see it 
> as particularly useful to force them to use something else that doesn't 
> fit them as well.  And there are areas like ACPICA where semaphores are 
> an useful abstraction because of other consideration (in ACPICA's case 
> due to portability).

Does ACPICA use counting semaphores? If so, you could have used real
arguments against his patches, instead of this other-os bull.

Also, what is the justification for using counting semaphores? Are we
counting hardware slots or is it just generic ACPI braindamage?

Clearly this all wasn't extremely clear from the code - otherwise Daniel
wouldn't even have done these patches.

> Especially now that semaphores are not duplicated per architecture 
> anymore so actually keeping them around is not that costly.

Having them around might give people the idea its a good idea to use
them. Not having them around is a good way to discourage that.

Re: [PATCH 1/3] acpi: add real mutex function calls

[Daniel Walker]

On Mon, 2008-07-21 at 21:20 +0200, Andi Kleen wrote:
> Daniel Walker wrote:
> > On Mon, 2008-07-21 at 09:51 +0800, Zhao Yakui wrote:
> >> On Sat, 2008-07-19 at 11:16 -0700, Daniel Walker wrote:
> >>> Instead of re-using semaphores for the mutex operation, I've
> >>> added usage of the kernel mutex for the os mutex implementation.
> >>>
> >> What is the advantage that the kernel mutex is used for the ACPI mutex
> >> implementation instead of using semaphore?
> >> And it seems that too much ACPICA source code is touched.
> > 
> > In general you would want to use a mutex whenever your using mutex-like
> > semantics. If I see a mutex used in code then I have a pretty good idea
> > the locking is sane..
> 
> With a mutex the locking can be totally broken and with a semaphore the 
> locking can be completely fine. They are both just tools which can be 
> used correctly and also incorrectly.

True ..

> The main advantage of mutexes is that they can be slightly more 
> efficient (doesn't matter for the ACPI case, the ACPI interpreter is not 
> performance critical) and that they are easier to check with lockdep. 
> But there are also other considerations like in ACPICA's case portability.

The ACPI interpreter actually specifically implements a mutex in the
AML. The interpreter wants a mutex, and not a semaphore. I'm not sure
why your bringing up portability issues ..

> My feeling is that adding own semaphore wrappers to ACPICA wouldn't
> be an improvement. Defering to Bob Moore for his opinion, since
> he maintains ACPICA.

I'm not trying to mod the locking that ACPI internally does (which I
think is mutex compatible) .. I'm just changing what the linux wrappers
are calling .. The additional lockdep calls are linux specific but they
aren't changing how the locking is already being done.

Daniel

Re: [PATCH 1/3] acpi: add real mutex function calls

[Matthew Wilcox]

On Mon, Jul 21, 2008 at 09:33:55PM +0200, Peter Zijlstra wrote:
> Does ACPICA use counting semaphores? If so, you could have used real
> arguments against his patches, instead of this other-os bull.

Yep.  In theory, it allows for a more powerful implementation than
Linux's semaphores (allowing you to do the equivalent of init_sem(5);
down(1); down(3); down(2); and have the third down() block until there's
two units available).  In practice, no ACPI code is asking for more than
one unit at a time.

> Also, what is the justification for using counting semaphores? Are we
> counting hardware slots or is it just generic ACPI braindamage?

It's the interface that AML is allowed to use, iirc.  So short of
revising the spec ... we have no idea how hardware are using it.

> Clearly this all wasn't extremely clear from the code - otherwise Daniel
> wouldn't even have done these patches.

Some people get religion about a topic and push pointless patches
anyway.

> > Especially now that semaphores are not duplicated per architecture 
> > anymore so actually keeping them around is not that costly.
> 
> Having them around might give people the idea its a good idea to use
> them. Not having them around is a good way to discourage that.

We'll end up making completions be more of a mess than semaphores ever
were or pushing semaphore implementations into every user that really
wanted a counting semaphore implementation.  People need to drop this
crusade, it's causing more harm than good.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 1/3] acpi: add real mutex function calls

[Andi Kleen]

Peter Zijlstra wrote:
> On Mon, 2008-07-21 at 21:15 +0200, Andi Kleen wrote:
>> Peter Zijlstra wrote:
>>> On Mon, 2008-07-21 at 11:17 +0200, Andi Kleen wrote:
>>>> Peter Zijlstra wrote:
>>>>> On Mon, 2008-07-21 at 09:51 +0800, Zhao Yakui wrote:
>>>>>> On Sat, 2008-07-19 at 11:16 -0700, Daniel Walker wrote:
>>>>>>> Instead of re-using semaphores for the mutex operation, I've
>>>>>>> added usage of the kernel mutex for the os mutex implementation.
>>>>>>>
>>>>>> What is the advantage that the kernel mutex is used for the ACPI mutex
>>>>>> implementation instead of using semaphore?
>>>>>> And it seems that too much ACPICA source code is touched.
>>>>> You get help from lockdep, and also our goal is to fully eradicate
>>>>> semaphore usage.
>>>> Issue is that ACPICA is shared with other OS source code and to replace 
>>>> a major interface like this would mean replacing it for everyone. It 
>>>> might end up with ACPICA just reimplementing a semaphore like wrapper if 
>>>> semaphores really go away, but I don't really see that coming anyways.
>>> Andi, you know better than that.
>> Know better than what?
> 
> Know better than to say we need ugly code in Linux because
> $SOME_OTHER_OS.

What ugly code do you refer to?

Ok ACPICA has its dark corners (I won't deny that), but at least willy's 
new semaphore code cannot really be described as "ugly".

>> My understanding was that there are a few areas
>> in the kernel who really use true semaphore semantics and I don't see it 
>> as particularly useful to force them to use something else that doesn't 
>> fit them as well.  And there are areas like ACPICA where semaphores are 
>> an useful abstraction because of other consideration (in ACPICA's case 
>> due to portability).
> 
> Does ACPICA use counting semaphores? If so, you could have used real
> arguments against his patches, instead of this other-os bull.

It is how ACPICA is maintained. You can yell at that as much as you want
and call it "bull", but that won't change it.

> Also, what is the justification for using counting semaphores? Are we
> counting hardware slots or is it just generic ACPI braindamage?

ACPICA is portable code and that needs a reasonably wide spread locking
abstraction. Happens to be semaphores for various reasons.

Now if mutexes were possible (AFAIK that's not possible in the general
case) then that would work too. But using "completions" as the low
level API doesn't make sense to me because it'll just lead to higher
level locking code being reinvented and also because it's unlikely
anything else has those with these semantics.

I think what would make sense is to identify the locks in ACPICA
which only have mutex semantics and convert them to a new osl mutex 
abstraction. Then at least some of the locks could get lockdep
checking.

On the other hand to be honest I'm not looking forward to handle
the usual 90+% false positives that you get out of lockdep ...

> Clearly this all wasn't extremely clear from the code - otherwise Daniel
> wouldn't even have done these patches.
> 
>> Especially now that semaphores are not duplicated per architecture 
>> anymore so actually keeping them around is not that costly.
> 
> Having them around might give people the idea its a good idea to use
> them. Not having them around is a good way to discourage that.

Code reviewers will take care of that.

-Andi

Re: [PATCH 1/3] acpi: add real mutex function calls

[Daniel Walker]

On Mon, 2008-07-21 at 13:55 -0600, Matthew Wilcox wrote:
> > > Especially now that semaphores are not duplicated per
> architecture 
> > > anymore so actually keeping them around is not that costly.
> > 
> > Having them around might give people the idea its a good idea to use
> > them. Not having them around is a good way to discourage that.
> 
> We'll end up making completions be more of a mess than semaphores ever
> were or pushing semaphore implementations into every user that really
> wanted a counting semaphore implementation.  People need to drop this
> crusade, it's causing more harm than good.

I'm not trying to push that .. I would like it if completions stayed
roughly the same, and the code using semaphores was changed to use
mutexes, instead of making completions look like semaphores ..

Daniel

Re: [PATCH 1/3] acpi: add real mutex function calls

[Daniel Walker]

On Mon, 2008-07-21 at 22:00 +0200, Andi Kleen wrote:

> Now if mutexes were possible (AFAIK that's not possible in the general
> case) then that would work too. But using "completions" as the low
> level API doesn't make sense to me because it'll just lead to higher
> level locking code being reinvented and also because it's unlikely
> anything else has those with these semantics.

The completions should only be used by the interpreter to implement the
ASL style signals which fit completions really well (that was my goal
anyway)..

> I think what would make sense is to identify the locks in ACPICA
> which only have mutex semantics and convert them to a new osl mutex 
> abstraction. Then at least some of the locks could get lockdep
> checking.

To an extent that's already done .. There is a osl mutex abstraction
which just uses semaphores. I switched that over to use a mutex ..

Those areas that already used the osl mutex abstraction should now be
using the actual linux mutex.

> On the other hand to be honest I'm not looking forward to handle
> the usual 90+% false positives that you get out of lockdep ...
> 
> > Clearly this all wasn't extremely clear from the code - otherwise Daniel
> > wouldn't even have done these patches.
> > 
> >> Especially now that semaphores are not duplicated per architecture 
> >> anymore so actually keeping them around is not that costly.
> > 
> > Having them around might give people the idea its a good idea to use
> > them. Not having them around is a good way to discourage that.

I agree with Peter here.. I've seen a lot of bad locking, and I think we
really need explicit checks over just code review.

Daniel

RE: [PATCH 1/3] acpi: add real mutex function calls

[Moore, Robert]

We've already added ACPI_MUTEX and ACPI_SPINLOCK to ACPICA (split off
from ACPI_SEMAPHORE), essentially at the request of Linux. I really
would rather not add yet another type, for what looks like little gain.

As far as portability, this is always an important issue for ACPICA. I
am still getting complaints from other OSVs about the MUTEX and SPINLOCK
types that were added.

Bob



>-----Original Message-----
>From: Daniel Walker [mailto:dwalker@mvista.com]
>Sent: Monday, July 21, 2008 12:40 PM
>To: Andi Kleen
>Cc: Zhao, Yakui; linux-acpi@vger.kernel.org; Ingo Molnar; Peter
Zijlstra;
>Brown, Len; Moore, Robert
>Subject: Re: [PATCH 1/3] acpi: add real mutex function calls
>
>On Mon, 2008-07-21 at 21:20 +0200, Andi Kleen wrote:
>> Daniel Walker wrote:
>> > On Mon, 2008-07-21 at 09:51 +0800, Zhao Yakui wrote:
>> >> On Sat, 2008-07-19 at 11:16 -0700, Daniel Walker wrote:
>> >>> Instead of re-using semaphores for the mutex operation, I've
>> >>> added usage of the kernel mutex for the os mutex implementation.
>> >>>
>> >> What is the advantage that the kernel mutex is used for the ACPI
mutex
>> >> implementation instead of using semaphore?
>> >> And it seems that too much ACPICA source code is touched.
>> >
>> > In general you would want to use a mutex whenever your using
mutex-like
>> > semantics. If I see a mutex used in code then I have a pretty good
idea
>> > the locking is sane..
>>
>> With a mutex the locking can be totally broken and with a semaphore
the
>> locking can be completely fine. They are both just tools which can be
>> used correctly and also incorrectly.
>
>True ..
>
>> The main advantage of mutexes is that they can be slightly more
>> efficient (doesn't matter for the ACPI case, the ACPI interpreter is
not
>> performance critical) and that they are easier to check with lockdep.
>> But there are also other considerations like in ACPICA's case
>portability.
>
>The ACPI interpreter actually specifically implements a mutex in the
>AML. The interpreter wants a mutex, and not a semaphore. I'm not sure
>why your bringing up portability issues ..
>
>> My feeling is that adding own semaphore wrappers to ACPICA wouldn't
>> be an improvement. Defering to Bob Moore for his opinion, since
>> he maintains ACPICA.
>
>I'm not trying to mod the locking that ACPI internally does (which I
>think is mutex compatible) .. I'm just changing what the linux wrappers
>are calling .. The additional lockdep calls are linux specific but they
>aren't changing how the locking is already being done.
>
>Daniel

RE: [PATCH 1/3] acpi: add real mutex function calls

[Daniel Walker]

(Added back the CC list.)

I don't think the problem is in the machine specific ACPI info .. As far
as I can tell the problem is related to how the interpreter lock is
interleaved with the AML mutexes . For example, a series of AML locks
can be A->B->C and at any point the interpreter lock can be released,
and reacquired. So you have a possible lock sequence of I->A->B->C ,
then in another path you have A->I->B->C. That creates a circular lock
scenario and with multiple threads in those different paths you could
end up with a deadlock (or a lockdep warning).

I was also reading the ACPI spec on how the ASL compiler does checking
on mutex usage similar to what lockdep is doing. I would hope there are
no lockdep warning coming from that code.

Daniel