[PATCH 1/4] mutex: add mutex_lock_timeout()

[PATCH 1/4] mutex: 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                   |   70 +++++++++++++++++++++++++++++++++-----
 7 files changed, 160 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..902be79 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,22 @@ 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 +314,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 +345,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,
+			    0, _RET_IP_);
 }
 
 static noinline int __sched
@@ -326,7 +367,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,
+				   0, _RET_IP_);
 }
 
 static noinline int __sched
@@ -334,7 +376,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, 0, _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_UNINTERRUPTIBLE, timeout,
+				   0, _RET_IP_);
 }
 #endif
 
-- 
1.5.5.1.32.gba7d2

[PATCH 2/4] 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 acpi os mutex
implementation.

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

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 235a138..e6f7337 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -871,6 +871,99 @@ 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_DO_NOT_WAIT) {
+		if (mutex_trylock(mutex))
+			return status;
+		else
+			return -ETIME;
+	}
+
+	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/4] 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   |    4 ++++
 drivers/acpi/namespace/nsaccess.c  |    5 +++++
 drivers/acpi/utilities/utmutex.c   |    6 ++++++
 4 files changed, 19 insertions(+), 0 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..044a780 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);
 
@@ -240,6 +241,9 @@ acpi_status acpi_ex_create_mutex(struct acpi_walk_state *walk_state)
 		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/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/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;
-- 
1.5.5.1.32.gba7d2

[PATCH 4/4] 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.

Cc: linux-acpi@vger.kernel.org
Signed-off-by: Daniel Walker <dwalker@mvista.com>
---
 drivers/acpi/osl.c |   56 ++++++++++++++++++++++++++-------------------------
 1 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index e6f7337..63de45e 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,
-			  units));
+	ACPI_DEBUG_PRINT((ACPI_DB_MUTEX, "Signaling completion[%p|%d]\n",
+			 handle, units));
 
-	up(sem);
+	complete(comp);
 
 	return AE_OK;
 }
-- 
1.5.5.1.32.gba7d2

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

[Matthew Wilcox]

On Tue, Aug 26, 2008 at 11:59:49AM -0700, Daniel Walker wrote:
> The semaphore usage in ACPI is more like completions. The ASL

Huh?  They are semaphores.  They're not 'more like completions' at all.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"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 4/4] acpi: semaphore removal

[Daniel Walker]

On Tue, 2008-08-26 at 13:13 -0600, Matthew Wilcox wrote:
> On Tue, Aug 26, 2008 at 11:59:49AM -0700, Daniel Walker wrote:
> > The semaphore usage in ACPI is more like completions. The ASL
> 
> Huh?  They are semaphores.  They're not 'more like completions' at all.

You can clearly make a completion out of a semaphore, but we have a
completion API .. ACPI is using locked semaphores, and essentially
re-making completions with the semaphore API..

Daniel

Re: [PATCH 1/4] mutex: add mutex_lock_timeout()

[Andi Kleen]

Daniel,

My decision on this patchkit is to reject it for now, because:

- I'm worried about the long term maintenance impact of doing full
lockdep checking on AML controlled locks. Since I'm keeping ACPI
only temporarily I don't want to leave an potentially problematic
legacy.
- I fail to see the advantage of implementing semaphores using conditions.

However what you can do is to ask Len again when he's back. Ultimately
it is his decision and he might decide that he can deal with AML lockdep
issues longer term.

Don't think it makes all that much sense to resubmit the completion
patch though. It's unrelated to the other patches anyways (not sure
why you mix them together)

-Andi

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

[Matthew Wilcox]

On Tue, Aug 26, 2008 at 12:30:46PM -0700, Daniel Walker wrote:
> On Tue, 2008-08-26 at 13:13 -0600, Matthew Wilcox wrote:
> > On Tue, Aug 26, 2008 at 11:59:49AM -0700, Daniel Walker wrote:
> > > The semaphore usage in ACPI is more like completions. The ASL
> > 
> > Huh?  They are semaphores.  They're not 'more like completions' at all.
> 
> You can clearly make a completion out of a semaphore, but we have a
> completion API .. ACPI is using locked semaphores, and essentially
> re-making completions with the semaphore API..

What makes you think that?

executer/excreate.c:    status = acpi_os_create_semaphore(ACPI_NO_UNIT_LIMIT, 0,
executer/exsystem.c:        acpi_os_create_semaphore(ACPI_NO_UNIT_LIMIT, 0, &temp_semaphore);
namespace/nsaccess.c:                                       acpi_os_create_semaphore(1, 0,
osl.c:acpi_os_create_semaphore(u32 max_units, u32 initial_units, acpi_handle * handle)

All users set 'initial_units' to 0.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"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/4] mutex: add mutex_lock_timeout()

[Daniel Walker]

On Tue, 2008-08-26 at 21:32 +0200, Andi Kleen wrote:
> Daniel,
> 
> My decision on this patchkit is to reject it for now, because:
> 
> - I'm worried about the long term maintenance impact of doing full
> lockdep checking on AML controlled locks. Since I'm keeping ACPI
> only temporarily I don't want to leave an potentially problematic
> legacy.

I think we have to allow some testing before there could be claim that
there would be a major problem.

> However what you can do is to ask Len again when he's back. Ultimately
> it is his decision and he might decide that he can deal with AML lockdep
> issues longer term.

For instance these changes could go into linux-next until the 2.6.29
merge window .. Len should be back by then, and we should have a much
better idea what kind of problems may exist, if any..

> Don't think it makes all that much sense to resubmit the completion
> patch though. It's unrelated to the other patches anyways (not sure
> why you mix them together)

It's all related .. I removed the semaphores in ACPI which are actually
mutexes in the first three patches. Your left with only semaphores that
are locked.. Locked semaphores are really completions, so it's natural
to convert them.

I would have liked to change the name in ACPI from "semaphore" to
"completion" , but you and Bob seemed to have some serious objections to
it, w.r.t other operating systems that are using that ACPI code.

Daniel

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

[Daniel Walker]

On Tue, 2008-08-26 at 13:50 -0600, Matthew Wilcox wrote:
> On Tue, Aug 26, 2008 at 12:30:46PM -0700, Daniel Walker wrote:
> > On Tue, 2008-08-26 at 13:13 -0600, Matthew Wilcox wrote:
> > > On Tue, Aug 26, 2008 at 11:59:49AM -0700, Daniel Walker wrote:
> > > > The semaphore usage in ACPI is more like completions. The ASL
> > > 
> > > Huh?  They are semaphores.  They're not 'more like completions' at all.
> > 
> > You can clearly make a completion out of a semaphore, but we have a
> > completion API .. ACPI is using locked semaphores, and essentially
> > re-making completions with the semaphore API..
> 
> What makes you think that?
> 
> executer/excreate.c:    status = acpi_os_create_semaphore(ACPI_NO_UNIT_LIMIT, 0,
> executer/exsystem.c:        acpi_os_create_semaphore(ACPI_NO_UNIT_LIMIT, 0, &temp_semaphore);
> namespace/nsaccess.c:                                       acpi_os_create_semaphore(1, 0,
> osl.c:acpi_os_create_semaphore(u32 max_units, u32 initial_units, acpi_handle * handle)
> 
> All users set 'initial_units' to 0.
> 

We have from ACPI,

acpi_status
acpi_os_create_semaphore(u32 max_units, u32 initial_units, acpi_handle * handle)
{
        struct semaphore *sem = NULL;
...
        sema_init(sem, initial_units);



Then from semaphore.h,

#define init_MUTEX(sem)         sema_init(sem, 1)
#define init_MUTEX_LOCKED(sem)  sema_init(sem, 0)

So initial units of 0 means make it locked initially .. Initialize to 1
would be a regular unlocked mutex ..

Daniel

Re: [PATCH 1/4] mutex: add mutex_lock_timeout()

[Andi Kleen]

>> However what you can do is to ask Len again when he's back. Ultimately
>> it is his decision and he might decide that he can deal with AML lockdep
>> issues longer term.
> 
> For instance these changes could go into linux-next until the 2.6.29
> merge window .. Len should be back by then, and we should have a much
> better idea what kind of problems may exist, if any..

Sorry I'm not convinced that linux-next testing can resolve that.
It doesn't really have enough hardware/tester coverage. Also linux-next
is really only for stuff that is going to be merged, and from
my current perspective it's not.

>> Don't think it makes all that much sense to resubmit the completion
>> patch though. It's unrelated to the other patches anyways (not sure
>> why you mix them together)
> 
> It's all related .. 

I don't think it is. You keep claiming that but it's just not true.
You have not so far brought up a single argument why the semaphores
should be changed to completions.

-Andi

Re: [PATCH 1/4] mutex: add mutex_lock_timeout()

[Daniel Walker]

On Tue, 2008-08-26 at 23:13 +0200, Andi Kleen wrote:
> >> However what you can do is to ask Len again when he's back. Ultimately
> >> it is his decision and he might decide that he can deal with AML lockdep
> >> issues longer term.
> > 
> > For instance these changes could go into linux-next until the 2.6.29
> > merge window .. Len should be back by then, and we should have a much
> > better idea what kind of problems may exist, if any..
> 
> Sorry I'm not convinced that linux-next testing can resolve that.
> It doesn't really have enough hardware/tester coverage. Also linux-next
> is really only for stuff that is going to be merged, and from
> my current perspective it's not.

What form of testing do you suggest? I only have access to so many
machines..

> >> Don't think it makes all that much sense to resubmit the completion
> >> patch though. It's unrelated to the other patches anyways (not sure
> >> why you mix them together)
> > 
> > It's all related .. 
> 
> I don't think it is. You keep claiming that but it's just not true.
> You have not so far brought up a single argument why the semaphores
> should be changed to completions.

The over arching goal is to remove semaphores from the kernel. AFAIK
there is broad support for that, and it has been discusses.. ACPI uses
semaphores like mutexes, which I changed to actually use mutexes. ACPI
also uses semaphores as completions , which I've changed to just
directly use completions.

Using semaphores has the side effect that you don't know for sure how
the semaphore is being used. It could be a completion, it could be a
mutex, it could be something else completely.. With ACPI this was hard
to figure out .. ACPI locking is not that readable, and not that easy to
understand..

By using completions your making your code more readable. By using
mutexes, you get faster, and more readable code. Your also allowing your
locking to be checked by lockdep.

Daniel