Skip to content

Commit

Permalink
ACPICA: Namespace: Fix namespace/interpreter lock ordering
Browse files Browse the repository at this point in the history
There is a lock order issue in acpi_load_tables(). The namespace lock
is held before holding the interpreter lock.

With ACPI_MUTEX_DEBUG enabled in the kernel, this is printed to the
log during boot:

  [    0.885699] ACPI Error: Invalid acquire order: Thread 405884224 owns [ACPI_MTX_Namespace], wants [ACPI_MTX_Interpreter] (20160422/utmutex-263)
  [    0.885881] ACPI Error: Could not acquire AML Interpreter mutex (20160422/exutils-95)
  [    0.893846] ACPI Error: Mutex [0x0] is not acquired, cannot release (20160422/utmutex-326)
  [    0.894019] ACPI Error: Could not release AML Interpreter mutex (20160422/exutils-133)

The issue has been introduced by the following commit:

  Commit: 2f38b1b
  ACPICA Commit: bfe03ffcde8ed56a7eae38ea0b188aeb12f9c52e
  Subject: ACPICA: Namespace: Fix a regression that MLC support triggers
           dead lock in dynamic table loading

Which fixed a deadlock issue for acpi_ns_load_table() in
acpi_ex_add_table() but didn't take care of the lock order in
acpi_ns_load_table() correctly.

Originally (before the above commit), ACPICA used the
namespace/interpreter locks in the following 2 key code
paths:

 1. Table loading:
 acpi_ns_load_table
	L(Namespace)
		acpi_ns_parse_table
			acpi_ns_one_complete_parse
	U(Namespace)
 2. Object evaluation:
 acpi_ns_evaluate
	L(Interpreter)
	acpi_ps_execute_method
		U(Interpreter)
		acpi_ns_load_table
			L(Namespace)
			U(Namespace)
		acpi_ev_initialize_region
			L(Namespace)
			U(Namespace)
		address_space.setup
			L(Namespace)
			U(Namespace)
		address_space.handler
			L(Namespace)
			U(Namespace)
		acpi_os_wait_semaphore
		acpi_os_acquire_mutex
		acpi_os_sleep
		L(Interpreter)
	U(Interpreter)

During runtime, while acpi_ns_evaluate is called, the lock order is
always Interpreter -> Namespace.

In turn, the problematic commit acquires the locks in the following
order:

 3. Table loading:
 acpi_ns_load_table
	L(Namespace)
		acpi_ns_parse_table
		L(Interpreter)
			acpi_ns_one_complete_parse
		U(Interpreter)
	U(Namespace)

To fix the lock order issue, move the interpreter lock to
acpi_ns_load_table() to ensure the lock order correctness:

 4. Table loading:
 acpi_ns_load_table
	L(Interpreter)
	L(Namespace)
		acpi_ns_parse_table
			acpi_ns_one_complete_parse
	U(Namespace)
	U(Interpreter)

However, this doesn't fix the current design issues related to the
namespace lock. For example, we can notice that in acpi_ns_evaluate(),
outside of acpi_ns_load_table(), the namespace objects may be created
by the named object creation control methods. And the creation of
the method-owned namespace objects are not locked by the namespace
lock. This patch doesn't try to fix such kind of existing issues.

Fixes: 2f38b1b (ACPICA: Namespace: Fix a regression that MLC support triggers dead lock in dynamic table loading)
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
  • Loading branch information
Lv Zheng authored and Rafael J. Wysocki committed Jul 5, 2016
1 parent 2f38b1b commit 4520904
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 8 deletions.
7 changes: 6 additions & 1 deletion drivers/acpi/acpica/nsload.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
#include "acnamesp.h"
#include "acdispat.h"
#include "actables.h"
#include "acinterp.h"

#define _COMPONENT ACPI_NAMESPACE
ACPI_MODULE_NAME("nsload")
Expand Down Expand Up @@ -78,6 +79,8 @@ acpi_ns_load_table(u32 table_index, struct acpi_namespace_node *node)

ACPI_FUNCTION_TRACE(ns_load_table);

acpi_ex_enter_interpreter();

/*
* Parse the table and load the namespace with all named
* objects found within. Control methods are NOT parsed
Expand All @@ -89,7 +92,7 @@ acpi_ns_load_table(u32 table_index, struct acpi_namespace_node *node)
*/
status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
if (ACPI_FAILURE(status)) {
return_ACPI_STATUS(status);
goto unlock_interp;
}

/* If table already loaded into namespace, just return */
Expand Down Expand Up @@ -130,6 +133,8 @@ acpi_ns_load_table(u32 table_index, struct acpi_namespace_node *node)

unlock:
(void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
unlock_interp:
(void)acpi_ex_exit_interpreter();

if (ACPI_FAILURE(status)) {
return_ACPI_STATUS(status);
Expand Down
9 changes: 2 additions & 7 deletions drivers/acpi/acpica/nsparse.c
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
#include "acparser.h"
#include "acdispat.h"
#include "actables.h"
#include "acinterp.h"

#define _COMPONENT ACPI_NAMESPACE
ACPI_MODULE_NAME("nsparse")
Expand Down Expand Up @@ -171,8 +170,6 @@ acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node *start_node)

ACPI_FUNCTION_TRACE(ns_parse_table);

acpi_ex_enter_interpreter();

/*
* AML Parse, pass 1
*
Expand All @@ -188,7 +185,7 @@ acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node *start_node)
status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS1,
table_index, start_node);
if (ACPI_FAILURE(status)) {
goto error_exit;
return_ACPI_STATUS(status);
}

/*
Expand All @@ -204,10 +201,8 @@ acpi_ns_parse_table(u32 table_index, struct acpi_namespace_node *start_node)
status = acpi_ns_one_complete_parse(ACPI_IMODE_LOAD_PASS2,
table_index, start_node);
if (ACPI_FAILURE(status)) {
goto error_exit;
return_ACPI_STATUS(status);
}

error_exit:
acpi_ex_exit_interpreter();
return_ACPI_STATUS(status);
}

0 comments on commit 4520904

Please sign in to comment.