ACPICA: Namespace: Fix dynamic table loading issues
authorLv Zheng <lv.zheng@intel.com>
Wed, 7 Sep 2016 06:07:10 +0000 (14:07 +0800)
committerRafael J. Wysocki <rafael.j.wysocki@intel.com>
Sat, 10 Sep 2016 00:43:02 +0000 (02:43 +0200)
ACPICA commit 767ee53354e0c4b7e8e7c57c6dd7bf569f0d52bb

There are issues related to the namespace/interpreter locks, which causes
several ACPI functionalities not specification compliant. The lock issues
were detectec when we were trying to fix the functionalities (please see
Link # [1] for the details).

What's the lock issues? Let's first look into the namespace/interpreter
lock usages inside of the object evaluation and the table loading which are
the key AML interpretion code paths:
Table loading:
acpi_ns_load_table
L(Namespace)
acpi_ns_parse_table
acpi_ns_one_complete_parse(LOAD_PASS1/LOAD_PASS2)
acpi_ds_load1_begion_op
acpi_ds_load1_end_op
acpi_ds_load2_begion_op
acpi_ds_load2_end_op
U(Namespace)
Object evaluation:
acpi_ns_evaluate
L(Interpreter)
acpi_ps_execute_method
acpi_ds_exec_begin_op
acpi_ds_exec_end_op
U(Interpreter)
acpi_ns_load_table
L(Namespace)
U(Namespace)
acpi_ev_initialize_region
L(Namespace)
U(Namespace)
address_space.Setup
address_space.Handler
acpi_os_wait_semaphore
acpi_os_acquire_mutex
acpi_os_sleep
L(Interpreter)
U(Interpreter)
L(Interpreter)
acpi_ex_resolve_node_to_value
U(Interpreter)
acpi_ns_check_return_value
Where:
  1. L(Interpreter) means acquire(MTX_INTERPRETER);
  2. U(Interpreter) means release(MTX_INTERPRETER);
  3. L(Namespace) means acquire(MTX_NAMESPACE);
  4. U(Namespace) means release(MTX_NAMESPACE);

We can see that acpi_ns_exec_module_code() (which invokes acpi_ns_evaluate) is
implemented in a deferred way just in order to avoid to reacquire the
namespace lock. This is in fact the root cause of many other ACPICA issues:
1. We now know for sure that the module code should be executed right in
   place by the Windows AML interpreter. So in the current design, if
   the region initializations/accesses or the table loadings (where the
   namespace surely should be locked again) happening during the table
   loading period, dead lock could happen because ACPICA never unlocks the
   namespace during the AML interpretion.
2. ACPICA interpreter just ensures that all static namespace nodes (named
   objects created during the acpi_load_tables()) are created
   (acpi_ns_lookup()) with the correct lock held, but doesn't ensure that
   the named objects created by the control method are created with the
   same correct lock held. It requires the control methods to be executed
   in a serial way after "loading a table", that's why ACPICA requires
   method auto serialization.

This patch fixes these software design issues by extending interpreter
enter/exit APIs to hold both interpreter/namespace locks to ensure the lock
order correctness, so that we can get these code paths:
Table loading:
acpi_ns_load_table
E(Interpreter)
acpi_ns_parse_table
acpi_ns_one_complete_parse
acpi_ns_execute_table
X(Interpreter)
acpi_ns_load_table
acpi_ev_initialize_region
address_space.Setup
address_space.Handler
acpi_os_wait_semaphore
acpi_os_acquire_mutex
acpi_os_sleep
E(Interpreter)
X(Interpreter)
Object evaluation:
acpi_ns_evaluate
E(Interpreter)
acpi_ps_execute_method
X(Interpreter)
acpi_ns_load_table
acpi_ev_initialize_region
address_space.Setup
address_space.Handler
acpi_os_wait_semaphore
acpi_os_acquire_mutex
acpi_os_sleep
E(Interpreter)
X(Interpreter)
Where:
  1. E(Interpreter) means acquire(MTX_INTERPRETER, MTX_NAMESPACE);
  2. X(Interpreter) means release(MTX_NAMESPACE, MTX_INTERPRETER);

After this change, we can see:
1. All namespace nodes creations are locked by the namespace lock.
2. All namespace nodes referencing are locked with the same lock.
3. But we also can notice a defact that, all namespace nodes deletions
   could be affected by this change. As a consequence,
   acpi_ns_delete_namespace_subtree() may delete a static namespace node that
   is still referenced by the interpreter (for example, the parser scopes).
Currently, we needn't worry about the last defact because in ACPICA, table
unloading is not fully functioning, its design strictly relies on the fact
that when the namespace deletion happens, either the AML table or the OSPMs
should have been notified and thus either the AML table or the OSPMs
shouldn't reference deletion-related namespace nodes during the namespace
deletion. And this change still works with the above restrictions applied.
While making this a-step-forward helps us to correct the wrong grammar to
pull many things back to the correct rail. And pulling things back to the
correct rail in return makes it possible for us to support fully
functioning table unloading after doing many cleanups.

While this patch is generated, all namespace locks are examined to ensure
that they can meet either of the following pattens:
1. L(Namespace)
   U(Namespace)
2. E(Interpreter)
   X(Interpreter)
3. E(Interpreter)
   X(Interpreter)
   L(Namespace)
   U(Namespace)
   E(Interpreter)
   X(Interpreter)
We ensure this by adding X(Interpreter)/E(Interpreter) or removing
U(Namespace)/L(Namespace) for those currently are executed in the following
order:
   E(Interpreter)
   L(Namespace)
   U(Namespace)
   X(Interpreter)
And adding E(Interpreter)/X(Interpreter) for those currently are executed
in the following order:
   X(Interpreter)
   E(Interpreter)

Originally, the interpreter lock is held for the execution AML opcodes, the
namespace lock is held for the named object creation AML opcodes. Since
they are actually same in MS interpreter (can all be executed during the
table loading), we can combine the 2 locks and tune the locking code better
in this way. Lv Zheng.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=153541
Link: https://bugzilla.kernel.org/show_bug.cgi?id=121701
Link: https://bugs.acpica.org/show_bug.cgi?id=1323
Link: https://github.com/acpica/acpica/commit/767ee533
Reported-and-tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reported-and-tested-by: Greg White <gwhite@kupulau.com>
Reported-and-tested-by: Dutch Guy <lucht_piloot@gmx.net>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Signed-off-by: Bob Moore <robert.moore@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
drivers/acpi/acpica/dsmethod.c
drivers/acpi/acpica/dswload2.c
drivers/acpi/acpica/exconfig.c
drivers/acpi/acpica/exoparg1.c
drivers/acpi/acpica/extrace.c
drivers/acpi/acpica/exutils.c
drivers/acpi/acpica/nsload.c
drivers/acpi/acpica/nsparse.c
drivers/acpi/acpica/psxface.c
drivers/acpi/acpica/utaddress.c

index 47c7b52a519c8d25054f0627da222c39fa8b8fd3..25b387a6d8be17dcde66a72f864b99edff63445c 100644 (file)
@@ -757,8 +757,10 @@ acpi_ds_terminate_control_method(union acpi_operand_object *method_desc,
 
                        /* Delete any direct children of (created by) this method */
 
+                       (void)acpi_ex_exit_interpreter();
                        acpi_ns_delete_namespace_subtree(walk_state->
                                                         method_node);
+                       (void)acpi_ex_enter_interpreter();
 
                        /*
                         * Delete any objects that were created by this method
@@ -769,9 +771,11 @@ acpi_ds_terminate_control_method(union acpi_operand_object *method_desc,
                         */
                        if (method_desc->method.
                            info_flags & ACPI_METHOD_MODIFIED_NAMESPACE) {
+                               (void)acpi_ex_exit_interpreter();
                                acpi_ns_delete_namespace_by_owner(method_desc->
                                                                  method.
                                                                  owner_id);
+                               (void)acpi_ex_enter_interpreter();
                                method_desc->method.info_flags &=
                                    ~ACPI_METHOD_MODIFIED_NAMESPACE;
                        }
index 762db3fa70e0f94158dd858781805cf615b2f937..028b22a3154ebb888245d030bd38db6ce04cb3ab 100644 (file)
@@ -605,16 +605,13 @@ acpi_status acpi_ds_load2_end_op(struct acpi_walk_state *walk_state)
                                if (ACPI_FAILURE(status)) {
                                        return_ACPI_STATUS(status);
                                }
-
-                               acpi_ex_exit_interpreter();
                        }
 
+                       acpi_ex_exit_interpreter();
                        status =
                            acpi_ev_initialize_region
                            (acpi_ns_get_attached_object(node), FALSE);
-                       if (walk_state->method_node) {
-                               acpi_ex_enter_interpreter();
-                       }
+                       acpi_ex_enter_interpreter();
 
                        if (ACPI_FAILURE(status)) {
                                /*
index 74dd5bac84229b7a573a30631c2c2e306e634cd4..578d5c832325fbe8ee53153310f1da46ebb4d20d 100644 (file)
@@ -198,9 +198,10 @@ acpi_ex_load_table_op(struct acpi_walk_state *walk_state,
                 * Find the node referenced by the root_path_string. This is the
                 * location within the namespace where the table will be loaded.
                 */
-               status =
-                   acpi_ns_get_node(start_node, operand[3]->string.pointer,
-                                    ACPI_NS_SEARCH_PARENT, &parent_node);
+               status = acpi_ns_get_node_unlocked(start_node,
+                                                  operand[3]->string.pointer,
+                                                  ACPI_NS_SEARCH_PARENT,
+                                                  &parent_node);
                if (ACPI_FAILURE(status)) {
                        return_ACPI_STATUS(status);
                }
@@ -220,9 +221,10 @@ acpi_ex_load_table_op(struct acpi_walk_state *walk_state,
 
                /* Find the node referenced by the parameter_path_string */
 
-               status =
-                   acpi_ns_get_node(start_node, operand[4]->string.pointer,
-                                    ACPI_NS_SEARCH_PARENT, &parameter_node);
+               status = acpi_ns_get_node_unlocked(start_node,
+                                                  operand[4]->string.pointer,
+                                                  ACPI_NS_SEARCH_PARENT,
+                                                  &parameter_node);
                if (ACPI_FAILURE(status)) {
                        return_ACPI_STATUS(status);
                }
index 6ae19cb26eb2eb0be6343ef435d0cc2151482010..007300433cdea4edaf31c54306cdb6ff6d9400a3 100644 (file)
@@ -891,14 +891,16 @@ acpi_status acpi_ex_opcode_1A_0T_1R(struct acpi_walk_state *walk_state)
                                 *    Field, so we need to resolve the node to a value.
                                 */
                                status =
-                                   acpi_ns_get_node(walk_state->scope_info->
-                                                    scope.node,
-                                                    operand[0]->string.pointer,
-                                                    ACPI_NS_SEARCH_PARENT,
-                                                    ACPI_CAST_INDIRECT_PTR
-                                                    (struct
-                                                     acpi_namespace_node,
-                                                     &return_desc));
+                                   acpi_ns_get_node_unlocked(walk_state->
+                                                             scope_info->scope.
+                                                             node,
+                                                             operand[0]->
+                                                             string.pointer,
+                                                             ACPI_NS_SEARCH_PARENT,
+                                                             ACPI_CAST_INDIRECT_PTR
+                                                             (struct
+                                                              acpi_namespace_node,
+                                                              &return_desc));
                                if (ACPI_FAILURE(status)) {
                                        goto cleanup;
                                }
index b52e84841c1a0d30ec37cd8cd19d0a476461718a..c9ca82610d776ae7e3d46385c00168b0f5ca80c4 100644 (file)
@@ -201,7 +201,6 @@ acpi_ex_start_trace_method(struct acpi_namespace_node *method_node,
                           union acpi_operand_object *obj_desc,
                           struct acpi_walk_state *walk_state)
 {
-       acpi_status status;
        char *pathname = NULL;
        u8 enabled = FALSE;
 
@@ -211,11 +210,6 @@ acpi_ex_start_trace_method(struct acpi_namespace_node *method_node,
                pathname = acpi_ns_get_normalized_pathname(method_node, TRUE);
        }
 
-       status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
-       if (ACPI_FAILURE(status)) {
-               goto exit;
-       }
-
        enabled = acpi_ex_interpreter_trace_enabled(pathname);
        if (enabled && !acpi_gbl_trace_method_object) {
                acpi_gbl_trace_method_object = obj_desc;
@@ -233,9 +227,6 @@ acpi_ex_start_trace_method(struct acpi_namespace_node *method_node,
                }
        }
 
-       (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
-
-exit:
        if (enabled) {
                ACPI_TRACE_POINT(ACPI_TRACE_AML_METHOD, TRUE,
                                 obj_desc ? obj_desc->method.aml_start : NULL,
@@ -267,7 +258,6 @@ acpi_ex_stop_trace_method(struct acpi_namespace_node *method_node,
                          union acpi_operand_object *obj_desc,
                          struct acpi_walk_state *walk_state)
 {
-       acpi_status status;
        char *pathname = NULL;
        u8 enabled;
 
@@ -277,26 +267,14 @@ acpi_ex_stop_trace_method(struct acpi_namespace_node *method_node,
                pathname = acpi_ns_get_normalized_pathname(method_node, TRUE);
        }
 
-       status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
-       if (ACPI_FAILURE(status)) {
-               goto exit_path;
-       }
-
        enabled = acpi_ex_interpreter_trace_enabled(NULL);
 
-       (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
-
        if (enabled) {
                ACPI_TRACE_POINT(ACPI_TRACE_AML_METHOD, FALSE,
                                 obj_desc ? obj_desc->method.aml_start : NULL,
                                 pathname);
        }
 
-       status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
-       if (ACPI_FAILURE(status)) {
-               goto exit_path;
-       }
-
        /* Check whether the tracer should be stopped */
 
        if (acpi_gbl_trace_method_object == obj_desc) {
@@ -312,9 +290,6 @@ acpi_ex_stop_trace_method(struct acpi_namespace_node *method_node,
                acpi_gbl_trace_method_object = NULL;
        }
 
-       (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
-
-exit_path:
        if (pathname) {
                ACPI_FREE(pathname);
        }
index 425f13372e68f3fe57c8b58cfb1d2948d54886a1..a8b857a7e9fb63291fcd5fb2dbf914b079b6cb08 100644 (file)
@@ -94,6 +94,10 @@ void acpi_ex_enter_interpreter(void)
                ACPI_ERROR((AE_INFO,
                            "Could not acquire AML Interpreter mutex"));
        }
+       status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
+       if (ACPI_FAILURE(status)) {
+               ACPI_ERROR((AE_INFO, "Could not acquire AML Namespace mutex"));
+       }
 
        return_VOID;
 }
@@ -127,6 +131,10 @@ void acpi_ex_exit_interpreter(void)
 
        ACPI_FUNCTION_TRACE(ex_exit_interpreter);
 
+       status = acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
+       if (ACPI_FAILURE(status)) {
+               ACPI_ERROR((AE_INFO, "Could not release AML Namespace mutex"));
+       }
        status = acpi_ut_release_mutex(ACPI_MTX_INTERPRETER);
        if (ACPI_FAILURE(status)) {
                ACPI_ERROR((AE_INFO,
index 2daa9a093c56510483b8373e72b84196f27dd674..334d3c5ba617dc23a46198bdd3ccaafe9b12223f 100644 (file)
@@ -46,6 +46,7 @@
 #include "acnamesp.h"
 #include "acdispat.h"
 #include "actables.h"
+#include "acinterp.h"
 
 #define _COMPONENT          ACPI_NAMESPACE
 ACPI_MODULE_NAME("nsload")
@@ -78,20 +79,6 @@ acpi_ns_load_table(u32 table_index, struct acpi_namespace_node *node)
 
        ACPI_FUNCTION_TRACE(ns_load_table);
 
-       /*
-        * Parse the table and load the namespace with all named
-        * objects found within. Control methods are NOT parsed
-        * at this time. In fact, the control methods cannot be
-        * parsed until the entire namespace is loaded, because
-        * if a control method makes a forward reference (call)
-        * to another control method, we can't continue parsing
-        * because we don't know how many arguments to parse next!
-        */
-       status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
-       if (ACPI_FAILURE(status)) {
-               return_ACPI_STATUS(status);
-       }
-
        /* If table already loaded into namespace, just return */
 
        if (acpi_tb_is_table_loaded(table_index)) {
@@ -107,6 +94,15 @@ acpi_ns_load_table(u32 table_index, struct acpi_namespace_node *node)
                goto unlock;
        }
 
+       /*
+        * Parse the table and load the namespace with all named
+        * objects found within. Control methods are NOT parsed
+        * at this time. In fact, the control methods cannot be
+        * parsed until the entire namespace is loaded, because
+        * if a control method makes a forward reference (call)
+        * to another control method, we can't continue parsing
+        * because we don't know how many arguments to parse next!
+        */
        status = acpi_ns_parse_table(table_index, node);
        if (ACPI_SUCCESS(status)) {
                acpi_tb_set_table_loaded_flag(table_index, TRUE);
@@ -120,7 +116,6 @@ acpi_ns_load_table(u32 table_index, struct acpi_namespace_node *node)
                 * exist. This target of Scope must already exist in the
                 * namespace, as per the ACPI specification.
                 */
-               (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
                acpi_ns_delete_namespace_by_owner(acpi_gbl_root_table_list.
                                                  tables[table_index].owner_id);
 
@@ -129,8 +124,6 @@ acpi_ns_load_table(u32 table_index, struct acpi_namespace_node *node)
        }
 
 unlock:
-       (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
-
        if (ACPI_FAILURE(status)) {
                return_ACPI_STATUS(status);
        }
index e51012b90118756500c9d0ee0b225bb7e4ea5b4c..4f14e9205bff75b841aebdb897cc6f498b1d551d 100644 (file)
@@ -47,6 +47,7 @@
 #include "acparser.h"
 #include "acdispat.h"
 #include "actables.h"
+#include "acinterp.h"
 
 #define _COMPONENT          ACPI_NAMESPACE
 ACPI_MODULE_NAME("nsparse")
@@ -234,7 +235,9 @@ acpi_ns_one_complete_parse(u32 pass_number,
 
        ACPI_DEBUG_PRINT((ACPI_DB_PARSE,
                          "*PARSE* pass %u parse\n", pass_number));
+       acpi_ex_enter_interpreter();
        status = acpi_ps_parse_aml(walk_state);
+       acpi_ex_exit_interpreter();
 
 cleanup:
        acpi_ps_delete_parse_tree(parse_root);
index 22a52c28c04829d6be1004c8156736474de04965..8d7e5b59b5984858099fb36cadb47526b2572ac5 100644 (file)
@@ -308,7 +308,9 @@ acpi_status acpi_ps_execute_table(struct acpi_evaluate_info *info)
        /*
         * Parse the AML, walk_state will be deleted by parse_aml
         */
+       acpi_ex_enter_interpreter();
        status = acpi_ps_parse_aml(walk_state);
+       acpi_ex_exit_interpreter();
        walk_state = NULL;
 
 cleanup:
index c986ec66a118d5999f07d47c5c8784bb691188a8..433d822798b6a742924fdaedf5aa206d7f62cdaa 100644 (file)
@@ -77,7 +77,6 @@ acpi_ut_add_address_range(acpi_adr_space_type space_id,
                          u32 length, struct acpi_namespace_node *region_node)
 {
        struct acpi_address_range *range_info;
-       acpi_status status;
 
        ACPI_FUNCTION_TRACE(ut_add_address_range);
 
@@ -97,12 +96,6 @@ acpi_ut_add_address_range(acpi_adr_space_type space_id,
        range_info->end_address = (address + length - 1);
        range_info->region_node = region_node;
 
-       status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
-       if (ACPI_FAILURE(status)) {
-               ACPI_FREE(range_info);
-               return_ACPI_STATUS(status);
-       }
-
        range_info->next = acpi_gbl_address_range_list[space_id];
        acpi_gbl_address_range_list[space_id] = range_info;
 
@@ -112,7 +105,6 @@ acpi_ut_add_address_range(acpi_adr_space_type space_id,
                          ACPI_FORMAT_UINT64(address),
                          ACPI_FORMAT_UINT64(range_info->end_address)));
 
-       (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
        return_ACPI_STATUS(AE_OK);
 }