[PATCH 0/3] acpi: semaphore removal -v2

[PATCH 0/3] acpi: semaphore removal -v2

[Daniel Walker]

I did a lot of cleanup .. I added the mutex_lock_timeout function
which I didn't have in the last set of patches.

I also trimmed down the semaphore to completions patch. So ACPI keeps the
name "semaphore" internally, but it can only be initialized with "0"
initial units. It means that ACPI developers can't randomly use the
semaphore as a mutex (which I haven't seen anyway).

Daniel

[PATCH 1/5] add mutex_lock_timeout()

[Daniel Walker]

Adds mutex_lock_timeout() and mutex_lock_timeout_nested() used inside ACPI.

Cc: linux-acpi@vger.kernel.org
Signed-off-by: Daniel Walker <dwalker@mvista.com>
---
 include/asm-generic/mutex-dec.h  |   23 ++++++++++++
 include/asm-generic/mutex-null.h |    3 ++
 include/asm-generic/mutex-xchg.h |   23 ++++++++++++
 include/asm-x86/mutex_32.h       |   21 +++++++++++
 include/asm-x86/mutex_64.h       |   21 +++++++++++
 include/linux/mutex.h            |    8 ++++
 kernel/mutex.c                   |   71 +++++++++++++++++++++++++++++++++-----
 7 files changed, 161 insertions(+), 9 deletions(-)

diff --git a/include/asm-generic/mutex-dec.h b/include/asm-generic/mutex-dec.h
index ed108be..eddc8c4 100644
--- a/include/asm-generic/mutex-dec.h
+++ b/include/asm-generic/mutex-dec.h
@@ -109,4 +109,27 @@ __mutex_fastpath_trylock(atomic_t *count, int (*fail_fn)(atomic_t *))
 #endif
 }
 
+/**
+ *  __mutex_fastpath_lock_timeout - try to take the lock by moving the count
+ *                                  from 1 to a 0 value
+ *  @count: pointer of type atomic_t
+ *  @fail_fn: function to call if the original value was not 1
+ *  	      This function will need to accept two arguments.
+ *  @timeout: Timeout value as second argument to fail_fn.
+ *
+ * Change the count from 1 to a value lower than 1, and call <fail_fn> if
+ * it wasn't 1 originally. This function returns 0 if the fastpath succeeds,
+ * or anything the slow path function returns.
+ */
+static inline int
+__mutex_fastpath_lock_timeout(atomic_t *count, long timeout,
+					int (*fail_fn)(atomic_t *, long))
+{
+	if (unlikely(atomic_dec_return(count) < 0))
+		return fail_fn(count, timeout);
+	else {
+		smp_mb();
+		return 0;
+	}
+}
 #endif
diff --git a/include/asm-generic/mutex-null.h b/include/asm-generic/mutex-null.h
index e1bbbc7..192a756 100644
--- a/include/asm-generic/mutex-null.h
+++ b/include/asm-generic/mutex-null.h
@@ -14,6 +14,9 @@
 #define __mutex_fastpath_lock_retval(count, fail_fn)	fail_fn(count)
 #define __mutex_fastpath_unlock(count, fail_fn)		fail_fn(count)
 #define __mutex_fastpath_trylock(count, fail_fn)	fail_fn(count)
+
+#define __mutex_fastpath_timeout(count, timeout, fail_fn) \
+	fail_fn(count, timeout)
 #define __mutex_slowpath_needs_to_unlock()		1
 
 #endif
diff --git a/include/asm-generic/mutex-xchg.h b/include/asm-generic/mutex-xchg.h
index 7b9cd2c..34ccdd3 100644
--- a/include/asm-generic/mutex-xchg.h
+++ b/include/asm-generic/mutex-xchg.h
@@ -115,4 +115,27 @@ __mutex_fastpath_trylock(atomic_t *count, int (*fail_fn)(atomic_t *))
 	return prev;
 }
 
+/**
+ *  __mutex_fastpath_lock_timeout - try to take the lock by moving the count
+ *                                  from 1 to a 0 value
+ *  @count: pointer of type atomic_t
+ *  @fail_fn: function to call if the original value was not 1
+ *	      This function will need to accept two arguments.
+ *  @timeout: Timeout value as second argument to fail_fn.
+ *
+ * Change the count from 1 to a value lower than 1, and call <fail_fn> if it
+ * wasn't 1 originally. This function returns 0 if the fastpath succeeds,
+ * or anything the slow path function returns
+ */
+static inline int
+__mutex_fastpath_lock_timeout(atomic_t *count, timeout,
+			      int (*fail_fn)(atomic_t *, long))
+{
+	if (unlikely(atomic_xchg(count, 0) != 1))
+		return fail_fn(count, timeout);
+	else {
+		smp_mb();
+		return 0;
+	}
+}
 #endif
diff --git a/include/asm-x86/mutex_32.h b/include/asm-x86/mutex_32.h
index 73e928e..7d4696d 100644
--- a/include/asm-x86/mutex_32.h
+++ b/include/asm-x86/mutex_32.h
@@ -122,4 +122,25 @@ static inline int __mutex_fastpath_trylock(atomic_t *count,
 #endif
 }
 
+/**
+ *  __mutex_fastpath_lock_timeout - try to take the lock by moving the count
+ *                                  from 1 to a 0 value
+ *  @count: pointer of type atomic_t
+ *  @fail_fn: function to call if the original value was not 1
+ *	      This function will need to accept two arguments.
+ *  @timeout: Timeout value as second argument to fail_fn.
+ *
+ * Change the count from 1 to a value lower than 1, and call <fail_fn> if it
+ * wasn't 1 originally. This function returns 0 if the fastpath succeeds,
+ * or anything the slow path function returns
+ */
+static inline int
+__mutex_fastpath_lock_timeout(atomic_t *count, long timeout,
+			      int (*fail_fn)(atomic_t *, long))
+{
+	if (unlikely(atomic_dec_return(count) < 0))
+		return fail_fn(count, timeout);
+	else
+		return 0;
+}
 #endif
diff --git a/include/asm-x86/mutex_64.h b/include/asm-x86/mutex_64.h
index f3fae9b..3e63b61 100644
--- a/include/asm-x86/mutex_64.h
+++ b/include/asm-x86/mutex_64.h
@@ -97,4 +97,25 @@ static inline int __mutex_fastpath_trylock(atomic_t *count,
 		return 0;
 }
 
+/**
+ *  __mutex_fastpath_lock_timeout - try to take the lock by moving the count
+ *                                  from 1 to a 0 value
+ *  @count: pointer of type atomic_t
+ *  @fail_fn: function to call if the original value was not 1
+ *	      This function will need to accept two arguments.
+ *  @timeout: Timeout value as second argument to fail_fn.
+ *
+ * Change the count from 1 to a value lower than 1, and call <fail_fn> if
+ * it wasn't 1 originally. This function returns 0 if the fastpath succeeds,
+ * or anything the slow path function returns
+ */
+static inline int
+__mutex_fastpath_lock_timeout(atomic_t *count, long timeout,
+			      int (*fail_fn)(atomic_t *, long))
+{
+	if (unlikely(atomic_dec_return(count) < 0))
+		return fail_fn(count, timeout);
+	else
+		return 0;
+}
 #endif
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index bc6da10..bb84cd4 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -127,18 +127,26 @@ extern int __must_check mutex_lock_interruptible_nested(struct mutex *lock,
 					unsigned int subclass);
 extern int __must_check mutex_lock_killable_nested(struct mutex *lock,
 					unsigned int subclass);
+extern int __must_check
+mutex_lock_timeout_nested(struct mutex *lock, long jiffies,
+			  unsigned int subclass);
 
 #define mutex_lock(lock) mutex_lock_nested(lock, 0)
 #define mutex_lock_interruptible(lock) mutex_lock_interruptible_nested(lock, 0)
 #define mutex_lock_killable(lock) mutex_lock_killable_nested(lock, 0)
+#define mutex_lock_timeout(lock, jiffies) \
+	mutex_lock_timeout_nested(lock, jiffies, 0)
 #else
 extern void mutex_lock(struct mutex *lock);
 extern int __must_check mutex_lock_interruptible(struct mutex *lock);
 extern int __must_check mutex_lock_killable(struct mutex *lock);
+extern int __must_check mutex_lock_timeout(struct mutex *lock, long jiffies);
 
 # define mutex_lock_nested(lock, subclass) mutex_lock(lock)
 # define mutex_lock_interruptible_nested(lock, subclass) mutex_lock_interruptible(lock)
 # define mutex_lock_killable_nested(lock, subclass) mutex_lock_killable(lock)
+# define mutex_lock_timeout_nested(lock, jiffies, subclass) \
+	mutex_lock_timeout(lock, jiffies)
 #endif
 
 /*
diff --git a/kernel/mutex.c b/kernel/mutex.c
index 12c779d..cf06f62 100644
--- a/kernel/mutex.c
+++ b/kernel/mutex.c
@@ -124,8 +124,8 @@ EXPORT_SYMBOL(mutex_unlock);
  * Lock a mutex (possibly interruptible), slowpath:
  */
 static inline int __sched
-__mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
-	       	unsigned long ip)
+__mutex_lock_common(struct mutex *lock, long state, long timeout,
+		    unsigned int subclass, unsigned long ip)
 {
 	struct task_struct *task = current;
 	struct mutex_waiter waiter;
@@ -179,7 +179,22 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass,
 
 		/* didnt get the lock, go to sleep: */
 		spin_unlock_mutex(&lock->wait_lock, flags);
-		schedule();
+		if (timeout == MAX_SCHEDULE_TIMEOUT)
+			schedule();
+		else {
+			timeout = schedule_timeout(timeout);
+
+			if (timeout == 0) {
+				spin_lock_mutex(&lock->wait_lock, flags);
+				mutex_remove_waiter(lock, &waiter,
+						    task_thread_info(task));
+				mutex_release(&lock->dep_map, 1, ip);
+				spin_unlock_mutex(&lock->wait_lock, flags);
+
+				debug_mutex_free_waiter(&waiter);
+				return -ETIME;
+			}
+		}
 		spin_lock_mutex(&lock->wait_lock, flags);
 	}
 
@@ -205,7 +220,8 @@ void __sched
 mutex_lock_nested(struct mutex *lock, unsigned int subclass)
 {
 	might_sleep();
-	__mutex_lock_common(lock, TASK_UNINTERRUPTIBLE, subclass, _RET_IP_);
+	__mutex_lock_common(lock, TASK_UNINTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT,
+			    subclass, _RET_IP_);
 }
 
 EXPORT_SYMBOL_GPL(mutex_lock_nested);
@@ -214,7 +230,8 @@ int __sched
 mutex_lock_killable_nested(struct mutex *lock, unsigned int subclass)
 {
 	might_sleep();
-	return __mutex_lock_common(lock, TASK_KILLABLE, subclass, _RET_IP_);
+	return __mutex_lock_common(lock, TASK_KILLABLE, MAX_SCHEDULE_TIMEOUT,
+				   subclass, _RET_IP_);
 }
 EXPORT_SYMBOL_GPL(mutex_lock_killable_nested);
 
@@ -222,10 +239,23 @@ int __sched
 mutex_lock_interruptible_nested(struct mutex *lock, unsigned int subclass)
 {
 	might_sleep();
-	return __mutex_lock_common(lock, TASK_INTERRUPTIBLE, subclass, _RET_IP_);
+	return __mutex_lock_common(lock, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT,
+				   subclass, _RET_IP_);
 }
 
 EXPORT_SYMBOL_GPL(mutex_lock_interruptible_nested);
+
+
+int __sched
+mutex_lock_timeout_nested(struct mutex *lock, long timeout,
+			  unsigned int subclass)
+{
+	might_sleep();
+	return __mutex_lock_common(lock, TASK_UNINTERRUPTIBLE, timeout,
+				   subclass, _RET_IP_);
+}
+EXPORT_SYMBOL_GPL(mutex_lock_timeout_nested);
+
 #endif
 
 /*
@@ -285,6 +315,9 @@ __mutex_lock_killable_slowpath(atomic_t *lock_count);
 static noinline int __sched
 __mutex_lock_interruptible_slowpath(atomic_t *lock_count);
 
+static noinline int __sched
+__mutex_lock_timeout_slowpath(atomic_t *lock_count, long timeout);
+
 /***
  * mutex_lock_interruptible - acquire the mutex, interruptable
  * @lock: the mutex to be acquired
@@ -313,12 +346,21 @@ int __sched mutex_lock_killable(struct mutex *lock)
 }
 EXPORT_SYMBOL(mutex_lock_killable);
 
+int __sched mutex_lock_timeout(struct mutex *lock, long timeout)
+{
+	might_sleep();
+	return __mutex_fastpath_lock_timeout
+			(&lock->count, timeout, __mutex_lock_timeout_slowpath);
+}
+EXPORT_SYMBOL(mutex_lock_timeout);
+
 static noinline void __sched
 __mutex_lock_slowpath(atomic_t *lock_count)
 {
 	struct mutex *lock = container_of(lock_count, struct mutex, count);
 
-	__mutex_lock_common(lock, TASK_UNINTERRUPTIBLE, 0, _RET_IP_);
+	__mutex_lock_common(lock, TASK_UNINTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT,
+			    _RET_IP_);
 }
 
 static noinline int __sched
@@ -326,7 +368,8 @@ __mutex_lock_killable_slowpath(atomic_t *lock_count)
 {
 	struct mutex *lock = container_of(lock_count, struct mutex, count);
 
-	return __mutex_lock_common(lock, TASK_KILLABLE, 0, _RET_IP_);
+	return __mutex_lock_common(lock, TASK_KILLABLE, MAX_SCHEDULE_TIMEOUT,
+				   _RET_IP_);
 }
 
 static noinline int __sched
@@ -334,7 +377,17 @@ __mutex_lock_interruptible_slowpath(atomic_t *lock_count)
 {
 	struct mutex *lock = container_of(lock_count, struct mutex, count);
 
-	return __mutex_lock_common(lock, TASK_INTERRUPTIBLE, 0, _RET_IP_);
+	return __mutex_lock_common(lock, TASK_INTERRUPTIBLE,
+				   MAX_SCHEDULE_TIMEOUT, _RET_IP_);
+}
+
+static noinline int __sched
+__mutex_lock_timeout_slowpath(atomic_t *lock_count, long timeout)
+{
+	struct mutex *lock = container_of(lock_count, struct mutex, count);
+
+	return __mutex_lock_common(lock, TASK_INTERRUPTIBLE, timeout,
+				   _RET_IP_);
 }
 #endif
 
-- 
1.5.5.1.32.gba7d2

[PATCH 2/5] acpi: add real mutex function calls

[Daniel Walker]

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

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..c0184f4 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_timeout(mutex, jiffies);
+	if (ret == -ETIME)
+		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 3/5] acpi: add lockdep magic

[Daniel Walker]

Add lockdep integration for the ACPI mutex usage.

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

diff --git a/drivers/acpi/dispatcher/dsmethod.c b/drivers/acpi/dispatcher/dsmethod.c
index 4613b9c..a3cf3d4 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;
 
@@ -149,6 +150,9 @@ acpi_ds_create_method_mutex(union acpi_operand_object *method_desc)
 		return_ACPI_STATUS(status);
 	}
 
+	lockdep_set_class((struct mutex *)&mutex_desc->mutex.os_mutex,
+			  &method_mutex);
+
 	mutex_desc->mutex.sync_level = method_desc->method.sync_level;
 	method_desc->method.mutex = mutex_desc;
 	return_ACPI_STATUS(AE_OK);
diff --git a/drivers/acpi/executer/excreate.c b/drivers/acpi/executer/excreate.c
index ad09696..4397015 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);
 
@@ -235,11 +236,15 @@ acpi_status acpi_ex_create_mutex(struct acpi_walk_state *walk_state)
 
 	/* 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);
 	if (ACPI_FAILURE(status)) {
 		goto cleanup;
 	}
 
+	lockdep_set_class((struct mutex *)obj_desc->mutex.os_mutex,
+			  &interpreter_class);
+
 	/* Init object and attach to NS node */
 
 	obj_desc->mutex.sync_level =
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 68990f1..1a4aa5e 100644
--- a/drivers/acpi/executer/exsystem.c
+++ b/drivers/acpi/executer/exsystem.c
@@ -108,7 +108,8 @@ acpi_status acpi_ex_system_wait_semaphore(acpi_semaphore semaphore, u16 timeout)
  *
  ******************************************************************************/
 
-acpi_status acpi_ex_system_wait_mutex(acpi_mutex mutex, u16 timeout)
+acpi_status
+acpi_ex_system_wait_mutex(acpi_mutex mutex, u16 timeout)
 {
 	acpi_status status;
 
diff --git a/drivers/acpi/namespace/nsaccess.c b/drivers/acpi/namespace/nsaccess.c
index c39a7f6..293c0ba 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;
@@ -205,6 +206,10 @@ acpi_status acpi_ns_root_initialize(void)
 					goto unlock_and_exit;
 				}
 
+				lockdep_set_class((struct mutex *)
+					obj_desc->mutex.os_mutex,
+					&ns_root_mutex);
+
 				/* Special case for ACPI Global Lock */
 
 				if (ACPI_STRCMP(init_val->name, "_GL_") == 0) {
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index c0184f4..7340baf 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -907,7 +907,8 @@ 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(acpi_mutex handle, u16 timeout)
 {
 	acpi_status status = AE_OK;
 	struct mutex *mutex = (struct mutex *)handle;
diff --git a/drivers/acpi/utilities/utmutex.c b/drivers/acpi/utilities/utmutex.c
index 7331dde..3f7f16c 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_class_keys[ACPI_MAX_MUTEX];
 	acpi_status status = AE_OK;
 
 	ACPI_FUNCTION_TRACE_U32(ut_create_mutex, mutex_id);
@@ -145,6 +146,11 @@ 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);
+
+		lockdep_set_class(
+			(struct mutex *)acpi_gbl_mutex_info[mutex_id].mutex,
+			&utility_class_keys[mutex_id]);
+
 		acpi_gbl_mutex_info[mutex_id].thread_id =
 		    ACPI_MUTEX_NOT_ACQUIRED;
 		acpi_gbl_mutex_info[mutex_id].use_count = 0;
diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
index 9032ec3..0f4247b 100644
--- a/include/acpi/acpiosxf.h
+++ b/include/acpi/acpiosxf.h
@@ -123,11 +123,13 @@ acpi_status acpi_os_signal_semaphore(acpi_semaphore handle, u32 units);
 /*
  * Mutex primitives
  */
-acpi_status acpi_os_create_mutex(acpi_mutex * out_handle);
+acpi_status
+acpi_os_create_mutex(acpi_mutex * handle);
 
 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(acpi_mutex handle, u16 timeout);
 
 acpi_status acpi_os_release_mutex(acpi_mutex handle);
 
-- 
1.5.5.1.32.gba7d2

[PATCH 4/5] acpi: remove interpreter lock

[Daniel Walker]

Lockdep has identified this lock as a source of potential deadlock in
acpi. After reviewing the lock it's not clear what it's protecting. I
invite anyone who might know what the point of the lock is to describe it.

In the mean time I removed the lock completely which, so far, hasn't had
any ill effects on my test machines.

Cc: linux-acpi@vger.kernel.org
Signed-off-by: Daniel Walker <dwalker@mvista.com>
---
 drivers/acpi/dispatcher/dsmethod.c |    5 --
 drivers/acpi/events/evregion.c     |   30 ---------
 drivers/acpi/events/evxface.c      |    5 --
 drivers/acpi/executer/exsystem.c   |   21 ------
 drivers/acpi/executer/exutils.c    |  127 ------------------------------------
 drivers/acpi/namespace/nseval.c    |    6 --
 drivers/acpi/namespace/nsinit.c    |    6 --
 drivers/acpi/namespace/nsxfeval.c  |    5 +-
 8 files changed, 1 insertions(+), 204 deletions(-)

diff --git a/drivers/acpi/dispatcher/dsmethod.c b/drivers/acpi/dispatcher/dsmethod.c
index a3cf3d4..0b8af17 100644
--- a/drivers/acpi/dispatcher/dsmethod.c
+++ b/drivers/acpi/dispatcher/dsmethod.c
@@ -86,10 +86,6 @@ acpi_ds_method_error(acpi_status status, struct acpi_walk_state *walk_state)
 
 	if (acpi_gbl_exception_handler) {
 
-		/* Exit the interpreter, allow handler to execute methods */
-
-		acpi_ex_exit_interpreter();
-
 		/*
 		 * Handler can map the exception code to anything it wants, including
 		 * AE_OK, in which case the executing method will not be aborted.
@@ -101,7 +97,6 @@ acpi_ds_method_error(acpi_status status, struct acpi_walk_state *walk_state)
 						    walk_state->opcode,
 						    walk_state->aml_offset,
 						    NULL);
-		acpi_ex_enter_interpreter();
 	}
 #ifdef ACPI_DISASSEMBLER
 	if (ACPI_FAILURE(status)) {
diff --git a/drivers/acpi/events/evregion.c b/drivers/acpi/events/evregion.c
index 236fbd1..4eb4c30 100644
--- a/drivers/acpi/events/evregion.c
+++ b/drivers/acpi/events/evregion.c
@@ -338,21 +338,10 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 			return_ACPI_STATUS(AE_NOT_EXIST);
 		}
 
-		/*
-		 * We must exit the interpreter because the region
-		 * setup will potentially execute control methods
-		 * (e.g., _REG method for this region)
-		 */
-		acpi_ex_exit_interpreter();
-
 		status = region_setup(region_obj, ACPI_REGION_ACTIVATE,
 				      handler_desc->address_space.context,
 				      &region_context);
 
-		/* Re-enter the interpreter */
-
-		acpi_ex_enter_interpreter();
-
 		/* Check for failure of the Region Setup */
 
 		if (ACPI_FAILURE(status)) {
@@ -397,16 +386,6 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 			  acpi_ut_get_region_name(region_obj->region.
 						  space_id)));
 
-	if (!(handler_desc->address_space.handler_flags &
-	      ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
-		/*
-		 * For handlers other than the default (supplied) handlers, we must
-		 * exit the interpreter because the handler *might* block -- we don't
-		 * know what it will do, so we can't hold the lock on the intepreter.
-		 */
-		acpi_ex_exit_interpreter();
-	}
-
 	/* Call the handler */
 
 	status = handler(function, address, bit_width, value,
@@ -419,15 +398,6 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
 							space_id)));
 	}
 
-	if (!(handler_desc->address_space.handler_flags &
-	      ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
-		/*
-		 * We just returned from a non-default handler, we must re-enter the
-		 * interpreter
-		 */
-		acpi_ex_enter_interpreter();
-	}
-
 	return_ACPI_STATUS(status);
 }
 
diff --git a/drivers/acpi/events/evxface.c b/drivers/acpi/events/evxface.c
index 94a6efe..4e4bd7a 100644
--- a/drivers/acpi/events/evxface.c
+++ b/drivers/acpi/events/evxface.c
@@ -773,10 +773,6 @@ acpi_status acpi_acquire_global_lock(u16 timeout, u32 * handle)
 		return (AE_BAD_PARAMETER);
 	}
 
-	/* Must lock interpreter to prevent race conditions */
-
-	acpi_ex_enter_interpreter();
-
 	status = acpi_ex_acquire_mutex_object(timeout,
 					      acpi_gbl_global_lock_mutex,
 					      acpi_os_get_thread_id());
@@ -788,7 +784,6 @@ acpi_status acpi_acquire_global_lock(u16 timeout, u32 * handle)
 		*handle = acpi_gbl_global_lock_handle;
 	}
 
-	acpi_ex_exit_interpreter();
 	return (status);
 }
 
diff --git a/drivers/acpi/executer/exsystem.c b/drivers/acpi/executer/exsystem.c
index 1a4aa5e..6c82143 100644
--- a/drivers/acpi/executer/exsystem.c
+++ b/drivers/acpi/executer/exsystem.c
@@ -75,19 +75,12 @@ acpi_status acpi_ex_system_wait_semaphore(acpi_semaphore semaphore, u16 timeout)
 
 	if (status == AE_TIME) {
 
-		/* We must wait, so unlock the interpreter */
-
-		acpi_ex_relinquish_interpreter();
-
 		status = acpi_os_wait_semaphore(semaphore, 1, timeout);
 
 		ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
 				  "*** Thread awake after blocking, %s\n",
 				  acpi_format_exception(status)));
 
-		/* Reacquire the interpreter */
-
-		acpi_ex_reacquire_interpreter();
 	}
 
 	return_ACPI_STATUS(status);
@@ -122,19 +115,12 @@ acpi_ex_system_wait_mutex(acpi_mutex mutex, u16 timeout)
 
 	if (status == AE_TIME) {
 
-		/* We must wait, so unlock the interpreter */
-
-		acpi_ex_relinquish_interpreter();
-
 		status = acpi_os_acquire_mutex(mutex, timeout);
 
 		ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
 				  "*** Thread awake after blocking, %s\n",
 				  acpi_format_exception(status)));
 
-		/* Reacquire the interpreter */
-
-		acpi_ex_reacquire_interpreter();
 	}
 
 	return_ACPI_STATUS(status);
@@ -197,15 +183,8 @@ acpi_status acpi_ex_system_do_suspend(acpi_integer how_long)
 {
 	ACPI_FUNCTION_ENTRY();
 
-	/* Since this thread will sleep, we must release the interpreter */
-
-	acpi_ex_relinquish_interpreter();
-
 	acpi_os_sleep(how_long);
 
-	/* And now we must get the interpreter again */
-
-	acpi_ex_reacquire_interpreter();
 	return (AE_OK);
 }
 
diff --git a/drivers/acpi/executer/exutils.c b/drivers/acpi/executer/exutils.c
index 86c0388..815defd 100644
--- a/drivers/acpi/executer/exutils.c
+++ b/drivers/acpi/executer/exutils.c
@@ -71,133 +71,6 @@ static u32 acpi_ex_digits_needed(acpi_integer value, u32 base);
 #ifndef ACPI_NO_METHOD_EXECUTION
 /*******************************************************************************
  *
- * FUNCTION:    acpi_ex_enter_interpreter
- *
- * PARAMETERS:  None
- *
- * RETURN:      None
- *
- * DESCRIPTION: Enter the interpreter execution region. Failure to enter
- *              the interpreter region is a fatal system error. Used in
- *              conjunction with exit_interpreter.
- *
- ******************************************************************************/
-
-void acpi_ex_enter_interpreter(void)
-{
-	acpi_status status;
-
-	ACPI_FUNCTION_TRACE(ex_enter_interpreter);
-
-	status = acpi_ut_acquire_mutex(ACPI_MTX_INTERPRETER);
-	if (ACPI_FAILURE(status)) {
-		ACPI_ERROR((AE_INFO,
-			    "Could not acquire AML Interpreter mutex"));
-	}
-
-	return_VOID;
-}
-
-/*******************************************************************************
- *
- * FUNCTION:    acpi_ex_reacquire_interpreter
- *
- * PARAMETERS:  None
- *
- * RETURN:      None
- *
- * DESCRIPTION: Reacquire the interpreter execution region from within the
- *              interpreter code. Failure to enter the interpreter region is a
- *              fatal system error. Used in  conjuction with
- *              relinquish_interpreter
- *
- ******************************************************************************/
-
-void acpi_ex_reacquire_interpreter(void)
-{
-	ACPI_FUNCTION_TRACE(ex_reacquire_interpreter);
-
-	/*
-	 * If the global serialized flag is set, do not release the interpreter,
-	 * since it was not actually released by acpi_ex_relinquish_interpreter.
-	 * This forces the interpreter to be single threaded.
-	 */
-	if (!acpi_gbl_all_methods_serialized) {
-		acpi_ex_enter_interpreter();
-	}
-
-	return_VOID;
-}
-
-/*******************************************************************************
- *
- * FUNCTION:    acpi_ex_exit_interpreter
- *
- * PARAMETERS:  None
- *
- * RETURN:      None
- *
- * DESCRIPTION: Exit the interpreter execution region. This is the top level
- *              routine used to exit the interpreter when all processing has
- *              been completed.
- *
- ******************************************************************************/
-
-void acpi_ex_exit_interpreter(void)
-{
-	acpi_status status;
-
-	ACPI_FUNCTION_TRACE(ex_exit_interpreter);
-
-	status = acpi_ut_release_mutex(ACPI_MTX_INTERPRETER);
-	if (ACPI_FAILURE(status)) {
-		ACPI_ERROR((AE_INFO,
-			    "Could not release AML Interpreter mutex"));
-	}
-
-	return_VOID;
-}
-
-/*******************************************************************************
- *
- * FUNCTION:    acpi_ex_relinquish_interpreter
- *
- * PARAMETERS:  None
- *
- * RETURN:      None
- *
- * DESCRIPTION: Exit the interpreter execution region, from within the
- *              interpreter - before attempting an operation that will possibly
- *              block the running thread.
- *
- * Cases where the interpreter is unlocked internally
- *      1) Method to be blocked on a Sleep() AML opcode
- *      2) Method to be blocked on an Acquire() AML opcode
- *      3) Method to be blocked on a Wait() AML opcode
- *      4) Method to be blocked to acquire the global lock
- *      5) Method to be blocked waiting to execute a serialized control method
- *          that is currently executing
- *      6) About to invoke a user-installed opregion handler
- *
- ******************************************************************************/
-
-void acpi_ex_relinquish_interpreter(void)
-{
-	ACPI_FUNCTION_TRACE(ex_relinquish_interpreter);
-
-	/*
-	 * If the global serialized flag is set, do not release the interpreter.
-	 * This forces the interpreter to be single threaded.
-	 */
-	if (!acpi_gbl_all_methods_serialized) {
-		acpi_ex_exit_interpreter();
-	}
-
-	return_VOID;
-}
-
-/*******************************************************************************
- *
  * FUNCTION:    acpi_ex_truncate_for32bit_table
  *
  * PARAMETERS:  obj_desc        - Object to be truncated
diff --git a/drivers/acpi/namespace/nseval.c b/drivers/acpi/namespace/nseval.c
index d369164..2a98cc7 100644
--- a/drivers/acpi/namespace/nseval.c
+++ b/drivers/acpi/namespace/nseval.c
@@ -72,8 +72,6 @@ ACPI_MODULE_NAME("nseval")
  * DESCRIPTION: Execute a control method or return the current value of an
  *              ACPI namespace object.
  *
- * MUTEX:       Locks interpreter
- *
  ******************************************************************************/
 acpi_status acpi_ns_evaluate(struct acpi_evaluate_info * info)
 {
@@ -189,9 +187,7 @@ acpi_status acpi_ns_evaluate(struct acpi_evaluate_info * info)
 		 * Execute the method via the interpreter. The interpreter is locked
 		 * here before calling into the AML parser
 		 */
-		acpi_ex_enter_interpreter();
 		status = acpi_ps_execute_method(info);
-		acpi_ex_exit_interpreter();
 	} else {
 		/*
 		 * 2) Object is not a method, return its current value
@@ -213,13 +209,11 @@ acpi_status acpi_ns_evaluate(struct acpi_evaluate_info * info)
 		 * resolution, we must lock it because we could access an opregion.
 		 * The opregion access code assumes that the interpreter is locked.
 		 */
-		acpi_ex_enter_interpreter();
 
 		/* Function has a strange interface */
 
 		status =
 		    acpi_ex_resolve_node_to_value(&info->resolved_node, NULL);
-		acpi_ex_exit_interpreter();
 
 		/*
 		 * If acpi_ex_resolve_node_to_value() succeeded, the return value was placed
diff --git a/drivers/acpi/namespace/nsinit.c b/drivers/acpi/namespace/nsinit.c
index e4c5751..e6c93d4 100644
--- a/drivers/acpi/namespace/nsinit.c
+++ b/drivers/acpi/namespace/nsinit.c
@@ -270,11 +270,6 @@ acpi_ns_init_one_object(acpi_handle obj_handle,
 	}
 
 	/*
-	 * Must lock the interpreter before executing AML code
-	 */
-	acpi_ex_enter_interpreter();
-
-	/*
 	 * Each of these types can contain executable AML code within the
 	 * declaration.
 	 */
@@ -333,7 +328,6 @@ acpi_ns_init_one_object(acpi_handle obj_handle,
 	 * We ignore errors from above, and always return OK, since we don't want
 	 * to abort the walk on any single error.
 	 */
-	acpi_ex_exit_interpreter();
 	return (AE_OK);
 }
 
diff --git a/drivers/acpi/namespace/nsxfeval.c b/drivers/acpi/namespace/nsxfeval.c
index 38be586..64c63f0 100644
--- a/drivers/acpi/namespace/nsxfeval.c
+++ b/drivers/acpi/namespace/nsxfeval.c
@@ -322,15 +322,12 @@ acpi_evaluate_object(acpi_handle handle,
 
 	if (info->return_object) {
 		/*
-		 * Delete the internal return object. NOTE: Interpreter must be
-		 * locked to avoid race condition.
+		 * Delete the internal return object.
 		 */
-		acpi_ex_enter_interpreter();
 
 		/* Remove one reference on the return object (should delete it) */
 
 		acpi_ut_remove_reference(info->return_object);
-		acpi_ex_exit_interpreter();
 	}
 
       cleanup:
-- 
1.5.5.1.32.gba7d2

[PATCH 5/5] acpi: semaphore removal

[Daniel Walker]

The semaphore usage in ACPI is more like completions. The ASL
functions getting implemented here are signals which follow a
"wait for", signaled, or reset format.

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

completion_done() taken from Dave Chinner.

Cc: linux-acpi@vger.kernel.org
Cc: Dave Chinner <david@fromorbit.com>
Signed-off-by: Daniel Walker <dwalker@mvista.com>
---
 drivers/acpi/osl.c         |   54 ++++++++++++++++++++++---------------------
 include/linux/completion.h |   19 +++++++++++++++
 2 files changed, 47 insertions(+), 26 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 7340baf..2e4ae01 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>
@@ -768,42 +768,44 @@ void acpi_os_delete_lock(acpi_spinlock handle)
 acpi_status
 acpi_os_create_semaphore(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)
+	BUG_ON(initial_units);
+
+	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)
 {
-	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;
 }
@@ -814,17 +816,17 @@ acpi_status acpi_os_delete_semaphore(acpi_handle handle)
 acpi_status acpi_os_wait_semaphore(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 +834,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));
 	}
 
@@ -855,18 +857,18 @@ acpi_status acpi_os_wait_semaphore(acpi_handle handle, u32 units, u16 timeout)
  */
 acpi_status acpi_os_signal_semaphore(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/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

Re: [PATCH 5/5] acpi: semaphore removal

[Matthew Wilcox]

On Thu, Aug 07, 2008 at 07:59:43AM -0700, Daniel Walker wrote:
> The semaphore usage in ACPI is more like completions. The ASL
> functions getting implemented here are signals which follow a
> "wait for", signaled, or reset format.
> 
> This implements the ACPI signaling methods with the Linux
> completion API, instead of using semaphores.
> 
> completion_done() taken from Dave Chinner.

completion_done is an abomination.  Stop this.

-- 
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 5/5] acpi: semaphore removal

[Daniel Walker]

On Thu, 2008-08-07 at 18:34 -0600, Matthew Wilcox wrote:
> On Thu, Aug 07, 2008 at 07:59:43AM -0700, Daniel Walker wrote:
> > The semaphore usage in ACPI is more like completions. The ASL
> > functions getting implemented here are signals which follow a
> > "wait for", signaled, or reset format.
> > 
> > This implements the ACPI signaling methods with the Linux
> > completion API, instead of using semaphores.
> > 
> > completion_done() taken from Dave Chinner.
> 
> completion_done is an abomination.  Stop this.
> 

In my ACPI changes it's just used for a BUG_ON to trigger if there is
something waiting on the completion before the memory gets free'd ..

You don't like how it's used, or you don't like the code itself?

Daniel

Re: [PATCH 5/5] acpi: semaphore removal

[Dave Chinner]

On Thu, Aug 07, 2008 at 06:34:08PM -0600, Matthew Wilcox wrote:
> On Thu, Aug 07, 2008 at 07:59:43AM -0700, Daniel Walker wrote:
> > The semaphore usage in ACPI is more like completions. The ASL
> > functions getting implemented here are signals which follow a
> > "wait for", signaled, or reset format.
> > 
> > This implements the ACPI signaling methods with the Linux
> > completion API, instead of using semaphores.
> > 
> > completion_done() taken from Dave Chinner.
> 
> completion_done is an abomination.  Stop this.

Say what?

You've had several opportunities to comment on that *you suggested*
[1]. You did not respond to any of them when it was being discussed
6 weeks ago  and subsequent resends [2] and now you're saying what
you suggested is an abortion?

What's the problem? The implementation, the interface, or
something else?

[1] http://oss.sgi.com/archives/xfs/2008-06/msg00380.html

[2] http://oss.sgi.com/archives/xfs/2008-06/msg00382.html
    http://oss.sgi.com/archives/xfs/2008-06/msg00400.html
    http://oss.sgi.com/archives/xfs/2008-07/msg00173.html

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 5/5] acpi: semaphore removal

[Matthew Wilcox]

On Fri, Aug 08, 2008 at 11:28:49AM +1000, Dave Chinner wrote:
> > completion_done is an abomination.  Stop this.
> 
> Say what?
> 
> You've had several opportunities to comment on that *you suggested*
> [1]. You did not respond to any of them when it was being discussed
> 6 weeks ago  and subsequent resends [2] and now you're saying what
> you suggested is an abortion?

Sorry, I had it confused with these:

http://oss.sgi.com/archives/xfs/2008-06/msg00372.html

-- 
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 5/5] acpi: semaphore removal

[Daniel Walker]

On Thu, 2008-08-07 at 20:53 -0600, Matthew Wilcox wrote:
> On Fri, Aug 08, 2008 at 11:28:49AM +1000, Dave Chinner wrote:
> > > completion_done is an abomination.  Stop this.
> > 
> > Say what?
> > 
> > You've had several opportunities to comment on that *you suggested*
> > [1]. You did not respond to any of them when it was being discussed
> > 6 weeks ago  and subsequent resends [2] and now you're saying what
> > you suggested is an abortion?
> 
> Sorry, I had it confused with these:
> 
> http://oss.sgi.com/archives/xfs/2008-06/msg00372.html


I was just saying that completions should be used for resource counting.
Like the usage in drivers/usb/usb-skeleton.c,

sema_init(&dev->limit_sem, WRITES_IN_FLIGHT);

This writes in flight usage happens 5-10 times in Linux, and completion
can fit that usage model .. It's not really locking, it's just counting.

Daniel

Re: [PATCH 5/5] acpi: semaphore removal

[Andi Kleen]

Daniel Walker wrote:
> The semaphore usage in ACPI is more like completions. The ASL
> functions getting implemented here are signals which follow a
> "wait for", signaled, or reset format.
> 
> This implements the ACPI signaling methods with the Linux
> completion API, instead of using semaphores.
> 
> completion_done() taken from Dave Chinner.

I agree with Matthew's earlier criticism. It doesn't make sense to
reinvent semaphores using completions when we really need semaphores
here for several cases (e.g. the AML locks by themselves)

So I'm not going to apply this patch.

What I'm open for is to convert specific non AML instances
of the ACPICA semaphores to mutexes. Such changes would need
to be coordinated with Bob of course though because it would affect
his codebase. In fact that has been already done for some.

-Andi

Re: [PATCH 5/5] acpi: semaphore removal

[Daniel Walker]

On Fri, 2008-08-08 at 04:44 +0200, Andi Kleen wrote:
> Daniel Walker wrote:
> > The semaphore usage in ACPI is more like completions. The ASL
> > functions getting implemented here are signals which follow a
> > "wait for", signaled, or reset format.
> > 
> > This implements the ACPI signaling methods with the Linux
> > completion API, instead of using semaphores.
> > 
> > completion_done() taken from Dave Chinner.
> 
> I agree with Matthew's earlier criticism. It doesn't make sense to
> reinvent semaphores using completions when we really need semaphores
> here for several cases (e.g. the AML locks by themselves)

Not following you .. The AML locks are strict mutexes (I was happily
surprised to find out how strict).. The AML signals are just strict
completions .. I didn't have to mod the completion API to make this
work, the one API addition could be removed without any ill effects
(except less debug checking).

It's really a clean replacement .. ACPI has it's own mutex API which now
just calls the Linux mutex, and that change leaves AML signals as the
only semaphore user which are completions ..

Everything should be fine if Bob just uses two types of locking either
he uses a mutex, or he uses a semaphore with an initial unit of 0..
Which appears to be what he's been doing for a while anyway..

> So I'm not going to apply this patch.

Ok .. I think the interpreter locking needs to get straightened out
before it's even considered to be applied ..

> What I'm open for is to convert specific non AML instances
> of the ACPICA semaphores to mutexes. Such changes would need
> to be coordinated with Bob of course though because it would affect
> his codebase. In fact that has been already done for some.

You don't want lockdep checking the AML mutexes?

Daniel

RE: [PATCH 4/5] acpi: remove interpreter lock

[Moore, Robert]

I believe the point was to allow the handler to execute methods, even
from other threads and to not block the interpreter for an unknown
amount of time.


>-----Original Message-----
>From: Daniel Walker [mailto:dwalker@mvista.com]
>Sent: Thursday, August 07, 2008 8:00 AM
>To: linux-acpi@vger.kernel.org
>Cc: Moore, Robert; Andi Kleen; Matthew Wilcox; Peter Zijlstra; Zhao,
Yakui;
>Dave Chinner; Ingo Molnar
>Subject: [PATCH 4/5] acpi: remove interpreter lock
>
>Lockdep has identified this lock as a source of potential deadlock in
>acpi. After reviewing the lock it's not clear what it's protecting. I
>invite anyone who might know what the point of the lock is to describe
it.
>
>In the mean time I removed the lock completely which, so far, hasn't
had
>any ill effects on my test machines.
>
>Cc: linux-acpi@vger.kernel.org
>Signed-off-by: Daniel Walker <dwalker@mvista.com>
>---
> drivers/acpi/dispatcher/dsmethod.c |    5 --
> drivers/acpi/events/evregion.c     |   30 ---------
> drivers/acpi/events/evxface.c      |    5 --
> drivers/acpi/executer/exsystem.c   |   21 ------
> drivers/acpi/executer/exutils.c    |  127
--------------------------------
>----
> drivers/acpi/namespace/nseval.c    |    6 --
> drivers/acpi/namespace/nsinit.c    |    6 --
> drivers/acpi/namespace/nsxfeval.c  |    5 +-
> 8 files changed, 1 insertions(+), 204 deletions(-)
>
>diff --git a/drivers/acpi/dispatcher/dsmethod.c
>b/drivers/acpi/dispatcher/dsmethod.c
>index a3cf3d4..0b8af17 100644
>--- a/drivers/acpi/dispatcher/dsmethod.c
>+++ b/drivers/acpi/dispatcher/dsmethod.c
>@@ -86,10 +86,6 @@ acpi_ds_method_error(acpi_status status, struct
>acpi_walk_state *walk_state)
>
> 	if (acpi_gbl_exception_handler) {
>
>-		/* Exit the interpreter, allow handler to execute
methods */
>-
>-		acpi_ex_exit_interpreter();
>-
> 		/*
> 		 * Handler can map the exception code to anything it
wants,
>including
> 		 * AE_OK, in which case the executing method will not be
>aborted.
>@@ -101,7 +97,6 @@ acpi_ds_method_error(acpi_status status, struct
>acpi_walk_state *walk_state)
> 						    walk_state->opcode,
>
walk_state->aml_offset,
> 						    NULL);
>-		acpi_ex_enter_interpreter();
> 	}
> #ifdef ACPI_DISASSEMBLER
> 	if (ACPI_FAILURE(status)) {
>diff --git a/drivers/acpi/events/evregion.c
>b/drivers/acpi/events/evregion.c
>index 236fbd1..4eb4c30 100644
>--- a/drivers/acpi/events/evregion.c
>+++ b/drivers/acpi/events/evregion.c
>@@ -338,21 +338,10 @@ acpi_ev_address_space_dispatch(union
>acpi_operand_object *region_obj,
> 			return_ACPI_STATUS(AE_NOT_EXIST);
> 		}
>
>-		/*
>-		 * We must exit the interpreter because the region
>-		 * setup will potentially execute control methods
>-		 * (e.g., _REG method for this region)
>-		 */
>-		acpi_ex_exit_interpreter();
>-
> 		status = region_setup(region_obj, ACPI_REGION_ACTIVATE,
>
handler_desc->address_space.context,
> 				      &region_context);
>
>-		/* Re-enter the interpreter */
>-
>-		acpi_ex_enter_interpreter();
>-
> 		/* Check for failure of the Region Setup */
>
> 		if (ACPI_FAILURE(status)) {
>@@ -397,16 +386,6 @@ acpi_ev_address_space_dispatch(union
>acpi_operand_object *region_obj,
> 			  acpi_ut_get_region_name(region_obj->region.
> 						  space_id)));
>
>-	if (!(handler_desc->address_space.handler_flags &
>-	      ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
>-		/*
>-		 * For handlers other than the default (supplied)
handlers, we
>must
>-		 * exit the interpreter because the handler *might*
block -- we
>don't
>-		 * know what it will do, so we can't hold the lock on
the
>intepreter.
>-		 */
>-		acpi_ex_exit_interpreter();
>-	}
>-
> 	/* Call the handler */
>
> 	status = handler(function, address, bit_width, value,
>@@ -419,15 +398,6 @@ acpi_ev_address_space_dispatch(union
>acpi_operand_object *region_obj,
> 							space_id)));
> 	}
>
>-	if (!(handler_desc->address_space.handler_flags &
>-	      ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
>-		/*
>-		 * We just returned from a non-default handler, we must
re-
>enter the
>-		 * interpreter
>-		 */
>-		acpi_ex_enter_interpreter();
>-	}
>-
> 	return_ACPI_STATUS(status);
> }
>
>diff --git a/drivers/acpi/events/evxface.c
b/drivers/acpi/events/evxface.c
>index 94a6efe..4e4bd7a 100644
>--- a/drivers/acpi/events/evxface.c
>+++ b/drivers/acpi/events/evxface.c
>@@ -773,10 +773,6 @@ acpi_status acpi_acquire_global_lock(u16 timeout,
u32
>* handle)
> 		return (AE_BAD_PARAMETER);
> 	}
>
>-	/* Must lock interpreter to prevent race conditions */
>-
>-	acpi_ex_enter_interpreter();
>-
> 	status = acpi_ex_acquire_mutex_object(timeout,
>
acpi_gbl_global_lock_mutex,
> 					      acpi_os_get_thread_id());
>@@ -788,7 +784,6 @@ acpi_status acpi_acquire_global_lock(u16 timeout,
u32 *
>handle)
> 		*handle = acpi_gbl_global_lock_handle;
> 	}
>
>-	acpi_ex_exit_interpreter();
> 	return (status);
> }
>
>diff --git a/drivers/acpi/executer/exsystem.c
>b/drivers/acpi/executer/exsystem.c
>index 1a4aa5e..6c82143 100644
>--- a/drivers/acpi/executer/exsystem.c
>+++ b/drivers/acpi/executer/exsystem.c
>@@ -75,19 +75,12 @@ acpi_status
>acpi_ex_system_wait_semaphore(acpi_semaphore semaphore, u16 timeout)
>
> 	if (status == AE_TIME) {
>
>-		/* We must wait, so unlock the interpreter */
>-
>-		acpi_ex_relinquish_interpreter();
>-
> 		status = acpi_os_wait_semaphore(semaphore, 1, timeout);
>
> 		ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
> 				  "*** Thread awake after blocking,
%s\n",
> 				  acpi_format_exception(status)));
>
>-		/* Reacquire the interpreter */
>-
>-		acpi_ex_reacquire_interpreter();
> 	}
>
> 	return_ACPI_STATUS(status);
>@@ -122,19 +115,12 @@ acpi_ex_system_wait_mutex(acpi_mutex mutex, u16
>timeout)
>
> 	if (status == AE_TIME) {
>
>-		/* We must wait, so unlock the interpreter */
>-
>-		acpi_ex_relinquish_interpreter();
>-
> 		status = acpi_os_acquire_mutex(mutex, timeout);
>
> 		ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
> 				  "*** Thread awake after blocking,
%s\n",
> 				  acpi_format_exception(status)));
>
>-		/* Reacquire the interpreter */
>-
>-		acpi_ex_reacquire_interpreter();
> 	}
>
> 	return_ACPI_STATUS(status);
>@@ -197,15 +183,8 @@ acpi_status acpi_ex_system_do_suspend(acpi_integer
>how_long)
> {
> 	ACPI_FUNCTION_ENTRY();
>
>-	/* Since this thread will sleep, we must release the interpreter
*/
>-
>-	acpi_ex_relinquish_interpreter();
>-
> 	acpi_os_sleep(how_long);
>
>-	/* And now we must get the interpreter again */
>-
>-	acpi_ex_reacquire_interpreter();
> 	return (AE_OK);
> }
>
>diff --git a/drivers/acpi/executer/exutils.c
>b/drivers/acpi/executer/exutils.c
>index 86c0388..815defd 100644
>--- a/drivers/acpi/executer/exutils.c
>+++ b/drivers/acpi/executer/exutils.c
>@@ -71,133 +71,6 @@ static u32 acpi_ex_digits_needed(acpi_integer
value,
>u32 base);
> #ifndef ACPI_NO_METHOD_EXECUTION
>
>/**********************************************************************
****
>*****
>  *
>- * FUNCTION:    acpi_ex_enter_interpreter
>- *
>- * PARAMETERS:  None
>- *
>- * RETURN:      None
>- *
>- * DESCRIPTION: Enter the interpreter execution region. Failure to
enter
>- *              the interpreter region is a fatal system error. Used
in
>- *              conjunction with exit_interpreter.
>- *
>-
>***********************************************************************
****
>***/
>-
>-void acpi_ex_enter_interpreter(void)
>-{
>-	acpi_status status;
>-
>-	ACPI_FUNCTION_TRACE(ex_enter_interpreter);
>-
>-	status = acpi_ut_acquire_mutex(ACPI_MTX_INTERPRETER);
>-	if (ACPI_FAILURE(status)) {
>-		ACPI_ERROR((AE_INFO,
>-			    "Could not acquire AML Interpreter mutex"));
>-	}
>-
>-	return_VOID;
>-}
>-
>-
>/**********************************************************************
****
>*****
>- *
>- * FUNCTION:    acpi_ex_reacquire_interpreter
>- *
>- * PARAMETERS:  None
>- *
>- * RETURN:      None
>- *
>- * DESCRIPTION: Reacquire the interpreter execution region from within
the
>- *              interpreter code. Failure to enter the interpreter
region
>is a
>- *              fatal system error. Used in  conjuction with
>- *              relinquish_interpreter
>- *
>-
>***********************************************************************
****
>***/
>-
>-void acpi_ex_reacquire_interpreter(void)
>-{
>-	ACPI_FUNCTION_TRACE(ex_reacquire_interpreter);
>-
>-	/*
>-	 * If the global serialized flag is set, do not release the
>interpreter,
>-	 * since it was not actually released by
>acpi_ex_relinquish_interpreter.
>-	 * This forces the interpreter to be single threaded.
>-	 */
>-	if (!acpi_gbl_all_methods_serialized) {
>-		acpi_ex_enter_interpreter();
>-	}
>-
>-	return_VOID;
>-}
>-
>-
>/**********************************************************************
****
>*****
>- *
>- * FUNCTION:    acpi_ex_exit_interpreter
>- *
>- * PARAMETERS:  None
>- *
>- * RETURN:      None
>- *
>- * DESCRIPTION: Exit the interpreter execution region. This is the top
>level
>- *              routine used to exit the interpreter when all
processing
>has
>- *              been completed.
>- *
>-
>***********************************************************************
****
>***/
>-
>-void acpi_ex_exit_interpreter(void)
>-{
>-	acpi_status status;
>-
>-	ACPI_FUNCTION_TRACE(ex_exit_interpreter);
>-
>-	status = acpi_ut_release_mutex(ACPI_MTX_INTERPRETER);
>-	if (ACPI_FAILURE(status)) {
>-		ACPI_ERROR((AE_INFO,
>-			    "Could not release AML Interpreter mutex"));
>-	}
>-
>-	return_VOID;
>-}
>-
>-
>/**********************************************************************
****
>*****
>- *
>- * FUNCTION:    acpi_ex_relinquish_interpreter
>- *
>- * PARAMETERS:  None
>- *
>- * RETURN:      None
>- *
>- * DESCRIPTION: Exit the interpreter execution region, from within the
>- *              interpreter - before attempting an operation that will
>possibly
>- *              block the running thread.
>- *
>- * Cases where the interpreter is unlocked internally
>- *      1) Method to be blocked on a Sleep() AML opcode
>- *      2) Method to be blocked on an Acquire() AML opcode
>- *      3) Method to be blocked on a Wait() AML opcode
>- *      4) Method to be blocked to acquire the global lock
>- *      5) Method to be blocked waiting to execute a serialized
control
>method
>- *          that is currently executing
>- *      6) About to invoke a user-installed opregion handler
>- *
>-
>***********************************************************************
****
>***/
>-
>-void acpi_ex_relinquish_interpreter(void)
>-{
>-	ACPI_FUNCTION_TRACE(ex_relinquish_interpreter);
>-
>-	/*
>-	 * If the global serialized flag is set, do not release the
>interpreter.
>-	 * This forces the interpreter to be single threaded.
>-	 */
>-	if (!acpi_gbl_all_methods_serialized) {
>-		acpi_ex_exit_interpreter();
>-	}
>-
>-	return_VOID;
>-}
>-
>-
>/**********************************************************************
****
>*****
>- *
>  * FUNCTION:    acpi_ex_truncate_for32bit_table
>  *
>  * PARAMETERS:  obj_desc        - Object to be truncated
>diff --git a/drivers/acpi/namespace/nseval.c
>b/drivers/acpi/namespace/nseval.c
>index d369164..2a98cc7 100644
>--- a/drivers/acpi/namespace/nseval.c
>+++ b/drivers/acpi/namespace/nseval.c
>@@ -72,8 +72,6 @@ ACPI_MODULE_NAME("nseval")
>  * DESCRIPTION: Execute a control method or return the current value
of an
>  *              ACPI namespace object.
>  *
>- * MUTEX:       Locks interpreter
>- *
>
>***********************************************************************
****
>***/
> acpi_status acpi_ns_evaluate(struct acpi_evaluate_info * info)
> {
>@@ -189,9 +187,7 @@ acpi_status acpi_ns_evaluate(struct
acpi_evaluate_info
>* info)
> 		 * Execute the method via the interpreter. The
interpreter is
>locked
> 		 * here before calling into the AML parser
> 		 */
>-		acpi_ex_enter_interpreter();
> 		status = acpi_ps_execute_method(info);
>-		acpi_ex_exit_interpreter();
> 	} else {
> 		/*
> 		 * 2) Object is not a method, return its current value
>@@ -213,13 +209,11 @@ acpi_status acpi_ns_evaluate(struct
>acpi_evaluate_info * info)
> 		 * resolution, we must lock it because we could access
an
>opregion.
> 		 * The opregion access code assumes that the interpreter
is
>locked.
> 		 */
>-		acpi_ex_enter_interpreter();
>
> 		/* Function has a strange interface */
>
> 		status =
> 		    acpi_ex_resolve_node_to_value(&info->resolved_node,
NULL);
>-		acpi_ex_exit_interpreter();
>
> 		/*
> 		 * If acpi_ex_resolve_node_to_value() succeeded, the
return
>value was placed
>diff --git a/drivers/acpi/namespace/nsinit.c
>b/drivers/acpi/namespace/nsinit.c
>index e4c5751..e6c93d4 100644
>--- a/drivers/acpi/namespace/nsinit.c
>+++ b/drivers/acpi/namespace/nsinit.c
>@@ -270,11 +270,6 @@ acpi_ns_init_one_object(acpi_handle obj_handle,
> 	}
>
> 	/*
>-	 * Must lock the interpreter before executing AML code
>-	 */
>-	acpi_ex_enter_interpreter();
>-
>-	/*
> 	 * Each of these types can contain executable AML code within
the
> 	 * declaration.
> 	 */
>@@ -333,7 +328,6 @@ acpi_ns_init_one_object(acpi_handle obj_handle,
> 	 * We ignore errors from above, and always return OK, since we
don't
>want
> 	 * to abort the walk on any single error.
> 	 */
>-	acpi_ex_exit_interpreter();
> 	return (AE_OK);
> }
>
>diff --git a/drivers/acpi/namespace/nsxfeval.c
>b/drivers/acpi/namespace/nsxfeval.c
>index 38be586..64c63f0 100644
>--- a/drivers/acpi/namespace/nsxfeval.c
>+++ b/drivers/acpi/namespace/nsxfeval.c
>@@ -322,15 +322,12 @@ acpi_evaluate_object(acpi_handle handle,
>
> 	if (info->return_object) {
> 		/*
>-		 * Delete the internal return object. NOTE: Interpreter
must be
>-		 * locked to avoid race condition.
>+		 * Delete the internal return object.
> 		 */
>-		acpi_ex_enter_interpreter();
>
> 		/* Remove one reference on the return object (should
delete it)
>*/
>
> 		acpi_ut_remove_reference(info->return_object);
>-		acpi_ex_exit_interpreter();
> 	}
>
>       cleanup:
>--
>1.5.5.1.32.gba7d2
>

RE: [PATCH 4/5] acpi: remove interpreter lock

[Daniel Walker]

On Thu, 2008-08-07 at 13:32 -0700, Moore, Robert wrote:
> I believe the point was to allow the handler to execute methods, even
> from other threads and to not block the interpreter for an unknown
> amount of time.
> 

Could you elaborate on this more, I'm not following you.. What do you
mean by "handler"? The name "interpreter lock" to me sort of indicates
that it's protecting the interpreter. So I'm guessing the point was to
stop multiple threads from being inside the interpreter at once.

If that's the case it doesn't make sense to release the mutex while your
inside the interpreter. That means another thread could then enter and
the protection doesn't hold.

It looked like most if the interpreter functions allocate their own
memory, then use that memory without sharing it with other instances of
the interpreter. So it's all contained into the one thread. 

You also have the mutexes embedded in the AML , which seem to indicate
the AML is thread safe (might be a big assumption tho).

Daniel

RE: [PATCH 4/5] acpi: remove interpreter lock

[Moore, Robert]

Yes, we lock the interpreter for major parts of control method execution
in order to implement the multithreading model of the ACPI
specification.

Here are a couple of sections from the ACPICA programmer reference that
explain the ACPICA multithreading, control method execution, reentrancy,
etc.


--------------------

2.2.5	Multitasking and Reentrancy
All components of the ACPI subsystem are intended to be fully reentrant
and support multiple threads of execution. To achieve this, there are
several mutual exclusion OSL interfaces that must be properly
implemented with the native host OS primitives to ensure that mutual
exclusion and synchronization can be performed correctly. Although
dependent on the correct implementation of these interfaces, the ACPI
Core Subsystem is otherwise fully reentrant and supports multiple
threads throughout the component, with the exception of the AML
interpreter, as explained below.

Because of the constraints of the ACPI specification, there is a major
limitation on the concurrency that can be achieved within the AML
interpreter portion of the subsystem. The specification states that at
most one control method can be actually executing AML code at any given
time. If a control method blocks (an event that can occur only under a
few limited conditions), another method may begin execution. However, it
can be said that the specification precludes the concurrent execution of
control methods. Therefore, the AML interpreter itself is essentially a
single-threaded component of the ACPI subsystem. Serialization of both
internal and external requests for execution of control methods is
performed and managed by the front-end of the interpreter.

And section 3.3.2
3.3.2	Control Method Execution
Most of the multithread support within the ACPI subsystem is implemented
using traditional locks and mutexes around critical (shared) data areas.
However, the AML interpreter design is different in that the ACPI
specification defines a special threading behavior for the execution of
control methods. The design implements the following portion of the ACPI
specification that defines a partially multithreaded AML interpreter in
four sentences:
A control method can use other internal, or well-defined, control
methods to accomplish the task at hand, which can include defined
control methods provided by the operating software. Interpretation of a
Control Method is not preemptive, but can block. When a control method
does block, the operating software can initiate or continue the
execution of a different control method. A control method can only
assume that access to global objects is exclusive for any period the
control method does not block.

3.3.2.1	Control Method Blocking
First of all, how can a control method block? This is a fairly
exhaustive list of the possibilities:
1.	Executes the Sleep() ASL opcode
2.	Executes the Acquire() ASL opcode and the request cannot be
immediately satisfied
3.	Executes the Wait() ASL opcode and the request cannot be
immediately satisfied
4.	Attempts to acquire the Global Lock (via Operation Region
access, etc), but must wait
5.	Attempts to execute a control method that is serialized and
already executing (or is blocked), or has reached its concurrency limit
6.	Invokes the host debugger via a write to the debug object or
executes the BreakPoint() ASL opcode
7.	Accesses an Operation Region which results in a dispatch to a
user-installed handler that blocks on I/O or other long-term operation
8.	A Notify AML opcode results in a dispatch to a user-installed
handler that blocks in a similar way


---------------------


The "handler" in question is the code that implements #7 above. A good
example of this is the EC operation region, which can be very slow.

Essentially, the implementation of 1-8 above is to unlock the
interpreter for the duration of the event.

There's more information about this in the ACPICA document, which is
available at

http://www.acpica.org/documentation/



>-----Original Message-----
>From: Daniel Walker [mailto:dwalker@mvista.com]
>Sent: Thursday, August 07, 2008 2:09 PM
>To: Moore, Robert
>Cc: linux-acpi@vger.kernel.org; Andi Kleen; Matthew Wilcox; Peter
Zijlstra;
>Zhao, Yakui; Dave Chinner; Ingo Molnar
>Subject: RE: [PATCH 4/5] acpi: remove interpreter lock
>
>On Thu, 2008-08-07 at 13:32 -0700, Moore, Robert wrote:
>> I believe the point was to allow the handler to execute methods, even
>> from other threads and to not block the interpreter for an unknown
>> amount of time.
>>
>
>Could you elaborate on this more, I'm not following you.. What do you
>mean by "handler"? The name "interpreter lock" to me sort of indicates
>that it's protecting the interpreter. So I'm guessing the point was to
>stop multiple threads from being inside the interpreter at once.
>
>If that's the case it doesn't make sense to release the mutex while
your
>inside the interpreter. That means another thread could then enter and
>the protection doesn't hold.
>
>It looked like most if the interpreter functions allocate their own
>memory, then use that memory without sharing it with other instances of
>the interpreter. So it's all contained into the one thread.
>
>You also have the mutexes embedded in the AML , which seem to indicate
>the AML is thread safe (might be a big assumption tho).
>
>Daniel

RE: [PATCH 4/5] acpi: remove interpreter lock

[Daniel Walker]

On Thu, 2008-08-07 at 14:30 -0700, Moore, Robert wrote:
> Yes, we lock the interpreter for major parts of control method execution
> in order to implement the multithreading model of the ACPI
> specification.
> 
> Here are a couple of sections from the ACPICA programmer reference that
> explain the ACPICA multithreading, control method execution, reentrancy,
> etc.

Thanks for the lengthy response.

> 3.3.2.1	Control Method Blocking
> First of all, how can a control method block? This is a fairly
> exhaustive list of the possibilities:
> 1.	Executes the Sleep() ASL opcode
> 2.	Executes the Acquire() ASL opcode and the request cannot be
> immediately satisfied
> 3.	Executes the Wait() ASL opcode and the request cannot be
> immediately satisfied
> 4.	Attempts to acquire the Global Lock (via Operation Region
> access, etc), but must wait
> 5.	Attempts to execute a control method that is serialized and
> already executing (or is blocked), or has reached its concurrency limit
> 6.	Invokes the host debugger via a write to the debug object or
> executes the BreakPoint() ASL opcode
> 7.	Accesses an Operation Region which results in a dispatch to a
> user-installed handler that blocks on I/O or other long-term operation
> 8.	A Notify AML opcode results in a dispatch to a user-installed
> handler that blocks in a similar way


Ok .. Well it looks like the deadlock that lockdep is warning on is
fixed with #2 or #3 .. If you release the interpreter before sleeping on
an Acquire() ASL opcode then you "fix" the deadlock that could happen by
having different locking orders of the interpreter lock with other
mutexes.

I'm not sure how to train lockdep to understand this tho , Ingo? Peter?
you guys have any thoughts?

I assume it's not safe to remove the interpreter lock given the text
that you quoted?

Daniel

Re: [PATCH 4/5] acpi: remove interpreter lock

[Andi Kleen]

Daniel Walker wrote:
> Lockdep has identified this lock as a source of potential deadlock in
> acpi. After reviewing the lock it's not clear what it's protecting. I
> invite anyone who might know what the point of the lock is to describe it.
> 
> In the mean time I removed the lock completely which, so far, hasn't had
> any ill effects on my test machines.

As Bob pointed out this lock is required by the ACPI specification. So I'm
not going to apply this patch.

However of course we still have to do something about the lockdep warnings
when the lock is dropped/reaquired.

The big question is if deadlocks can happen really or not. AFAIK we didn't
have a clear answer on that yet.

-Andi