* [PATCH 1/6] ACPICA: Dispatcher: Fix order issue of method termination
2016-10-25 5:20 [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes Lv Zheng
@ 2016-10-25 5:21 ` Lv Zheng
2016-10-25 5:21 ` [PATCH 2/6] ACPICA: Dispatcher: Fix an unbalanced lock exit path in acpi_ds_auto_serialize_method() Lv Zheng
` (6 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Lv Zheng @ 2016-10-25 5:21 UTC (permalink / raw)
To: Rafael J . Wysocki, Rafael J . Wysocki, Len Brown
Cc: linux-kernel, linux-acpi, Lv Zheng
The last step of the method termination should be the end of the method
serialization. Otherwise, the steps happening after it will face the race
issues that cannot be protected by the method serialization mechanism.
This patch fixes this issue by moving the per-method-object deletion code
prior than the end of the method serialization. Otherwise, the possible
race issues may result in AE_ALREADY_EXISTS error in a parallel
environment.
Reported-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
drivers/acpi/acpica/dsmethod.c | 40 ++++++++++++++++++++--------------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/acpi/acpica/dsmethod.c b/drivers/acpi/acpica/dsmethod.c
index 32e9ddc..c4028a8 100644
--- a/drivers/acpi/acpica/dsmethod.c
+++ b/drivers/acpi/acpica/dsmethod.c
@@ -731,26 +731,6 @@ acpi_ds_terminate_control_method(union acpi_operand_object *method_desc,
acpi_ds_method_data_delete_all(walk_state);
/*
- * If method is serialized, release the mutex and restore the
- * current sync level for this thread
- */
- if (method_desc->method.mutex) {
-
- /* Acquisition Depth handles recursive calls */
-
- method_desc->method.mutex->mutex.acquisition_depth--;
- if (!method_desc->method.mutex->mutex.acquisition_depth) {
- walk_state->thread->current_sync_level =
- method_desc->method.mutex->mutex.
- original_sync_level;
-
- acpi_os_release_mutex(method_desc->method.
- mutex->mutex.os_mutex);
- method_desc->method.mutex->mutex.thread_id = 0;
- }
- }
-
- /*
* Delete any namespace objects created anywhere within the
* namespace by the execution of this method. Unless:
* 1) This method is a module-level executable code method, in which
@@ -786,6 +766,26 @@ acpi_ds_terminate_control_method(union acpi_operand_object *method_desc,
~ACPI_METHOD_MODIFIED_NAMESPACE;
}
}
+
+ /*
+ * If method is serialized, release the mutex and restore the
+ * current sync level for this thread
+ */
+ if (method_desc->method.mutex) {
+
+ /* Acquisition Depth handles recursive calls */
+
+ method_desc->method.mutex->mutex.acquisition_depth--;
+ if (!method_desc->method.mutex->mutex.acquisition_depth) {
+ walk_state->thread->current_sync_level =
+ method_desc->method.mutex->mutex.
+ original_sync_level;
+
+ acpi_os_release_mutex(method_desc->method.
+ mutex->mutex.os_mutex);
+ method_desc->method.mutex->mutex.thread_id = 0;
+ }
+ }
}
/* Decrement the thread count on the method */
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 2/6] ACPICA: Dispatcher: Fix an unbalanced lock exit path in acpi_ds_auto_serialize_method()
2016-10-25 5:20 [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes Lv Zheng
2016-10-25 5:21 ` [PATCH 1/6] ACPICA: Dispatcher: Fix order issue of method termination Lv Zheng
@ 2016-10-25 5:21 ` Lv Zheng
2016-10-25 5:21 ` [PATCH 3/6] ACPICA: Dispatcher: Tune interpreter lock around acpi_ev_initialize_region() Lv Zheng
` (5 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Lv Zheng @ 2016-10-25 5:21 UTC (permalink / raw)
To: Rafael J . Wysocki, Rafael J . Wysocki, Len Brown
Cc: linux-kernel, linux-acpi, Lv Zheng
There is a lock unbalanced exit path in acpi_ds_initialize_method(),
this patch corrects it. Lv Zheng.
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
drivers/acpi/acpica/dsmethod.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/acpica/dsmethod.c b/drivers/acpi/acpica/dsmethod.c
index c4028a8..5997e59 100644
--- a/drivers/acpi/acpica/dsmethod.c
+++ b/drivers/acpi/acpica/dsmethod.c
@@ -128,7 +128,7 @@ acpi_ds_auto_serialize_method(struct acpi_namespace_node *node,
if (ACPI_FAILURE(status)) {
acpi_ds_delete_walk_state(walk_state);
acpi_ps_free_op(op);
- return_ACPI_STATUS(status);
+ goto unlock;
}
walk_state->descending_callback = acpi_ds_detect_named_opcodes;
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 3/6] ACPICA: Dispatcher: Tune interpreter lock around acpi_ev_initialize_region()
2016-10-25 5:20 [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes Lv Zheng
2016-10-25 5:21 ` [PATCH 1/6] ACPICA: Dispatcher: Fix order issue of method termination Lv Zheng
2016-10-25 5:21 ` [PATCH 2/6] ACPICA: Dispatcher: Fix an unbalanced lock exit path in acpi_ds_auto_serialize_method() Lv Zheng
@ 2016-10-25 5:21 ` Lv Zheng
2016-10-25 5:21 ` [PATCH 4/6] ACPICA: Events: Cleanup acpi_ev_initialize_region() Lv Zheng
` (4 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Lv Zheng @ 2016-10-25 5:21 UTC (permalink / raw)
To: Rafael J . Wysocki, Rafael J . Wysocki, Len Brown
Cc: linux-kernel, linux-acpi, Lv Zheng
In code path of acpi_ev_initialize_region(), there are namespace modification
code unlocked. This patch tunes the code to make sure such modification are
locked. Lv Zheng.
Reported-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
drivers/acpi/acpica/dsinit.c | 11 +++--------
drivers/acpi/acpica/dsmethod.c | 12 +++---------
drivers/acpi/acpica/dswload2.c | 2 --
drivers/acpi/acpica/evrgnini.c | 3 +++
drivers/acpi/acpica/nsload.c | 2 ++
5 files changed, 11 insertions(+), 19 deletions(-)
diff --git a/drivers/acpi/acpica/dsinit.c b/drivers/acpi/acpica/dsinit.c
index f1e6dcc..54d48b9 100644
--- a/drivers/acpi/acpica/dsinit.c
+++ b/drivers/acpi/acpica/dsinit.c
@@ -46,6 +46,7 @@
#include "acdispat.h"
#include "acnamesp.h"
#include "actables.h"
+#include "acinterp.h"
#define _COMPONENT ACPI_DISPATCHER
ACPI_MODULE_NAME("dsinit")
@@ -214,23 +215,17 @@ acpi_ds_initialize_objects(u32 table_index,
/* Walk entire namespace from the supplied root */
- status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
- if (ACPI_FAILURE(status)) {
- return_ACPI_STATUS(status);
- }
-
/*
* We don't use acpi_walk_namespace since we do not want to acquire
* the namespace reader lock.
*/
status =
acpi_ns_walk_namespace(ACPI_TYPE_ANY, start_node, ACPI_UINT32_MAX,
- ACPI_NS_WALK_UNLOCK, acpi_ds_init_one_object,
- NULL, &info, NULL);
+ 0, acpi_ds_init_one_object, NULL, &info,
+ NULL);
if (ACPI_FAILURE(status)) {
ACPI_EXCEPTION((AE_INFO, status, "During WalkNamespace"));
}
- (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
status = acpi_get_table_by_index(table_index, &table);
if (ACPI_FAILURE(status)) {
diff --git a/drivers/acpi/acpica/dsmethod.c b/drivers/acpi/acpica/dsmethod.c
index 5997e59..2b3210f 100644
--- a/drivers/acpi/acpica/dsmethod.c
+++ b/drivers/acpi/acpica/dsmethod.c
@@ -99,14 +99,11 @@ acpi_ds_auto_serialize_method(struct acpi_namespace_node *node,
"Method auto-serialization parse [%4.4s] %p\n",
acpi_ut_get_node_name(node), node));
- acpi_ex_enter_interpreter();
-
/* Create/Init a root op for the method parse tree */
op = acpi_ps_alloc_op(AML_METHOD_OP, obj_desc->method.aml_start);
if (!op) {
- status = AE_NO_MEMORY;
- goto unlock;
+ return_ACPI_STATUS(AE_NO_MEMORY);
}
acpi_ps_set_name(op, node->name.integer);
@@ -118,8 +115,7 @@ acpi_ds_auto_serialize_method(struct acpi_namespace_node *node,
acpi_ds_create_walk_state(node->owner_id, NULL, NULL, NULL);
if (!walk_state) {
acpi_ps_free_op(op);
- status = AE_NO_MEMORY;
- goto unlock;
+ return_ACPI_STATUS(AE_NO_MEMORY);
}
status = acpi_ds_init_aml_walk(walk_state, op, node,
@@ -128,7 +124,7 @@ acpi_ds_auto_serialize_method(struct acpi_namespace_node *node,
if (ACPI_FAILURE(status)) {
acpi_ds_delete_walk_state(walk_state);
acpi_ps_free_op(op);
- goto unlock;
+ return_ACPI_STATUS(status);
}
walk_state->descending_callback = acpi_ds_detect_named_opcodes;
@@ -138,8 +134,6 @@ acpi_ds_auto_serialize_method(struct acpi_namespace_node *node,
status = acpi_ps_parse_aml(walk_state);
acpi_ps_delete_parse_tree(op);
-unlock:
- acpi_ex_exit_interpreter();
return_ACPI_STATUS(status);
}
diff --git a/drivers/acpi/acpica/dswload2.c b/drivers/acpi/acpica/dswload2.c
index 028b22a..e362182 100644
--- a/drivers/acpi/acpica/dswload2.c
+++ b/drivers/acpi/acpica/dswload2.c
@@ -607,11 +607,9 @@ acpi_status acpi_ds_load2_end_op(struct acpi_walk_state *walk_state)
}
}
- acpi_ex_exit_interpreter();
status =
acpi_ev_initialize_region
(acpi_ns_get_attached_object(node), FALSE);
- acpi_ex_enter_interpreter();
if (ACPI_FAILURE(status)) {
/*
diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c
index 3843f1f..75ddd16 100644
--- a/drivers/acpi/acpica/evrgnini.c
+++ b/drivers/acpi/acpica/evrgnini.c
@@ -45,6 +45,7 @@
#include "accommon.h"
#include "acevents.h"
#include "acnamesp.h"
+#include "acinterp.h"
#define _COMPONENT ACPI_EVENTS
ACPI_MODULE_NAME("evrgnini")
@@ -597,9 +598,11 @@ acpi_ev_initialize_region(union acpi_operand_object *region_obj,
}
}
+ acpi_ex_exit_interpreter();
status =
acpi_ev_execute_reg_method(region_obj,
ACPI_REG_CONNECT);
+ acpi_ex_enter_interpreter();
if (acpi_ns_locked) {
status =
diff --git a/drivers/acpi/acpica/nsload.c b/drivers/acpi/acpica/nsload.c
index 334d3c5..d1f2014 100644
--- a/drivers/acpi/acpica/nsload.c
+++ b/drivers/acpi/acpica/nsload.c
@@ -137,7 +137,9 @@ unlock:
ACPI_DEBUG_PRINT((ACPI_DB_INFO,
"**** Begin Table Object Initialization\n"));
+ acpi_ex_enter_interpreter();
status = acpi_ds_initialize_objects(table_index, node);
+ acpi_ex_exit_interpreter();
ACPI_DEBUG_PRINT((ACPI_DB_INFO,
"**** Completed Table Object Initialization\n"));
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 4/6] ACPICA: Events: Cleanup acpi_ev_initialize_region()
2016-10-25 5:20 [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes Lv Zheng
` (2 preceding siblings ...)
2016-10-25 5:21 ` [PATCH 3/6] ACPICA: Dispatcher: Tune interpreter lock around acpi_ev_initialize_region() Lv Zheng
@ 2016-10-25 5:21 ` Lv Zheng
2016-10-25 5:21 ` [PATCH 5/6] ACPICA: Tables: Cleanup acpi_tb_install_and_load_table() Lv Zheng
` (3 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Lv Zheng @ 2016-10-25 5:21 UTC (permalink / raw)
To: Rafael J . Wysocki, Rafael J . Wysocki, Len Brown
Cc: linux-kernel, linux-acpi, Lv Zheng
This patch changes acpi_ev_initialize_region(), stop returning AE_NOT_EXIST
from it so that, not only in acpi_ds_load2_end_op(), but all places invoking
this function won't emit exceptions.
This patch also removes acpi_ns_locked from acpi_ev_initialize_region() as we
have tuned table/namespace mutexes to be independent to each other, and
decided to deal with namespace node deletion stuffs with new design other
than using the 2 locks (because using them are not working correctly). Thus
this function won't be invoked with the namespace lock held.
No functional changes. Lv Zheng.
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
drivers/acpi/acpica/acevents.h | 4 +--
drivers/acpi/acpica/dsopcode.c | 2 +-
drivers/acpi/acpica/dswload2.c | 13 +---------
drivers/acpi/acpica/evrgnini.c | 59 +++++++++++++++++-------------------------
4 files changed, 27 insertions(+), 51 deletions(-)
diff --git a/drivers/acpi/acpica/acevents.h b/drivers/acpi/acpica/acevents.h
index 92fa47c..8a0049d 100644
--- a/drivers/acpi/acpica/acevents.h
+++ b/drivers/acpi/acpica/acevents.h
@@ -243,9 +243,7 @@ acpi_ev_default_region_setup(acpi_handle handle,
u32 function,
void *handler_context, void **region_context);
-acpi_status
-acpi_ev_initialize_region(union acpi_operand_object *region_obj,
- u8 acpi_ns_locked);
+acpi_status acpi_ev_initialize_region(union acpi_operand_object *region_obj);
/*
* evsci - SCI (System Control Interrupt) handling/dispatch
diff --git a/drivers/acpi/acpica/dsopcode.c b/drivers/acpi/acpica/dsopcode.c
index 4cc9d98..77fd7c8 100644
--- a/drivers/acpi/acpica/dsopcode.c
+++ b/drivers/acpi/acpica/dsopcode.c
@@ -84,7 +84,7 @@ acpi_status acpi_ds_initialize_region(acpi_handle obj_handle)
/* Namespace is NOT locked */
- status = acpi_ev_initialize_region(obj_desc, FALSE);
+ status = acpi_ev_initialize_region(obj_desc);
return (status);
}
diff --git a/drivers/acpi/acpica/dswload2.c b/drivers/acpi/acpica/dswload2.c
index e362182..651f35a 100644
--- a/drivers/acpi/acpica/dswload2.c
+++ b/drivers/acpi/acpica/dswload2.c
@@ -609,18 +609,7 @@ acpi_status acpi_ds_load2_end_op(struct acpi_walk_state *walk_state)
status =
acpi_ev_initialize_region
- (acpi_ns_get_attached_object(node), FALSE);
-
- if (ACPI_FAILURE(status)) {
- /*
- * If AE_NOT_EXIST is returned, it is not fatal
- * because many regions get created before a handler
- * is installed for said region.
- */
- if (AE_NOT_EXIST == status) {
- status = AE_OK;
- }
- }
+ (acpi_ns_get_attached_object(node));
break;
case AML_NAME_OP:
diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c
index 75ddd16..a909225 100644
--- a/drivers/acpi/acpica/evrgnini.c
+++ b/drivers/acpi/acpica/evrgnini.c
@@ -479,7 +479,6 @@ acpi_ev_default_region_setup(acpi_handle handle,
* FUNCTION: acpi_ev_initialize_region
*
* PARAMETERS: region_obj - Region we are initializing
- * acpi_ns_locked - Is namespace locked?
*
* RETURN: Status
*
@@ -497,19 +496,28 @@ acpi_ev_default_region_setup(acpi_handle handle,
* MUTEX: Interpreter should be unlocked, because we may run the _REG
* method for this region.
*
+ * NOTE: Possible incompliance:
+ * There is a behavior conflict in automatic _REG execution:
+ * 1. When the interpreter is evaluating a method, we can only
+ * automatically run _REG for the following case:
+ * operation_region (OPR1, 0x80, 0x1000010, 0x4)
+ * 2. When the interpreter is loading a table, we can also
+ * automatically run _REG for the following case:
+ * operation_region (OPR1, 0x80, 0x1000010, 0x4)
+ * Though this may not be compliant to the de-facto standard, the
+ * logic is kept in order not to trigger regressions. And keeping
+ * this logic should be taken care by the caller of this function.
+ *
******************************************************************************/
-acpi_status
-acpi_ev_initialize_region(union acpi_operand_object *region_obj,
- u8 acpi_ns_locked)
+acpi_status acpi_ev_initialize_region(union acpi_operand_object *region_obj)
{
union acpi_operand_object *handler_obj;
union acpi_operand_object *obj_desc;
acpi_adr_space_type space_id;
struct acpi_namespace_node *node;
- acpi_status status;
- ACPI_FUNCTION_TRACE_U32(ev_initialize_region, acpi_ns_locked);
+ ACPI_FUNCTION_TRACE(ev_initialize_region);
if (!region_obj) {
return_ACPI_STATUS(AE_BAD_PARAMETER);
@@ -580,39 +588,17 @@ acpi_ev_initialize_region(union acpi_operand_object *region_obj,
handler_obj, region_obj,
obj_desc));
- status =
- acpi_ev_attach_region(handler_obj,
- region_obj,
- acpi_ns_locked);
+ (void)acpi_ev_attach_region(handler_obj,
+ region_obj, FALSE);
/*
* Tell all users that this region is usable by
* running the _REG method
*/
- if (acpi_ns_locked) {
- status =
- acpi_ut_release_mutex
- (ACPI_MTX_NAMESPACE);
- if (ACPI_FAILURE(status)) {
- return_ACPI_STATUS(status);
- }
- }
-
acpi_ex_exit_interpreter();
- status =
- acpi_ev_execute_reg_method(region_obj,
- ACPI_REG_CONNECT);
+ (void)acpi_ev_execute_reg_method(region_obj,
+ ACPI_REG_CONNECT);
acpi_ex_enter_interpreter();
-
- if (acpi_ns_locked) {
- status =
- acpi_ut_acquire_mutex
- (ACPI_MTX_NAMESPACE);
- if (ACPI_FAILURE(status)) {
- return_ACPI_STATUS(status);
- }
- }
-
return_ACPI_STATUS(AE_OK);
}
}
@@ -622,12 +608,15 @@ acpi_ev_initialize_region(union acpi_operand_object *region_obj,
node = node->parent;
}
- /* If we get here, there is no handler for this region */
-
+ /*
+ * If we get here, there is no handler for this region. This is not
+ * fatal because many regions get created before a handler is installed
+ * for said region.
+ */
ACPI_DEBUG_PRINT((ACPI_DB_OPREGION,
"No handler for RegionType %s(%X) (RegionObj %p)\n",
acpi_ut_get_region_name(space_id), space_id,
region_obj));
- return_ACPI_STATUS(AE_NOT_EXIST);
+ return_ACPI_STATUS(AE_OK);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 5/6] ACPICA: Tables: Cleanup acpi_tb_install_and_load_table()
2016-10-25 5:20 [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes Lv Zheng
` (3 preceding siblings ...)
2016-10-25 5:21 ` [PATCH 4/6] ACPICA: Events: Cleanup acpi_ev_initialize_region() Lv Zheng
@ 2016-10-25 5:21 ` Lv Zheng
2016-10-25 5:21 ` [PATCH 6/6] ACPICA: Tables: Add acpi_tb_unload_table() Lv Zheng
` (2 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Lv Zheng @ 2016-10-25 5:21 UTC (permalink / raw)
To: Rafael J . Wysocki, Rafael J . Wysocki, Len Brown
Cc: linux-kernel, linux-acpi, Lv Zheng
acpi_tb_install_and_load_table() can invoke acpi_tb_load_table() to eliminate
redundant code.
No functional change. Lv Zheng.
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
drivers/acpi/acpica/actables.h | 3 +--
drivers/acpi/acpica/exconfig.c | 7 +++----
drivers/acpi/acpica/tbdata.c | 45 +++++-------------------------------------
drivers/acpi/acpica/tbxfload.c | 7 +++----
4 files changed, 12 insertions(+), 50 deletions(-)
diff --git a/drivers/acpi/acpica/actables.h b/drivers/acpi/acpica/actables.h
index e85953b..1850005 100644
--- a/drivers/acpi/acpica/actables.h
+++ b/drivers/acpi/acpica/actables.h
@@ -127,8 +127,7 @@ acpi_status
acpi_tb_load_table(u32 table_index, struct acpi_namespace_node *parent_node);
acpi_status
-acpi_tb_install_and_load_table(struct acpi_table_header *table,
- acpi_physical_address address,
+acpi_tb_install_and_load_table(acpi_physical_address address,
u8 flags, u8 override, u32 *table_index);
void acpi_tb_terminate(void);
diff --git a/drivers/acpi/acpica/exconfig.c b/drivers/acpi/acpica/exconfig.c
index 718428b..8b8d620 100644
--- a/drivers/acpi/acpica/exconfig.c
+++ b/drivers/acpi/acpica/exconfig.c
@@ -437,10 +437,9 @@ acpi_ex_load_op(union acpi_operand_object *obj_desc,
ACPI_INFO(("Dynamic OEM Table Load:"));
acpi_ex_exit_interpreter();
- status =
- acpi_tb_install_and_load_table(table, ACPI_PTR_TO_PHYSADDR(table),
- ACPI_TABLE_ORIGIN_INTERNAL_VIRTUAL,
- TRUE, &table_index);
+ status = acpi_tb_install_and_load_table(ACPI_PTR_TO_PHYSADDR(table),
+ ACPI_TABLE_ORIGIN_INTERNAL_VIRTUAL,
+ TRUE, &table_index);
acpi_ex_enter_interpreter();
if (ACPI_FAILURE(status)) {
diff --git a/drivers/acpi/acpica/tbdata.c b/drivers/acpi/acpica/tbdata.c
index d9ca8c2..4cbfa30 100644
--- a/drivers/acpi/acpica/tbdata.c
+++ b/drivers/acpi/acpica/tbdata.c
@@ -832,9 +832,9 @@ acpi_tb_load_table(u32 table_index, struct acpi_namespace_node *parent_node)
*
* FUNCTION: acpi_tb_install_and_load_table
*
- * PARAMETERS: table - Pointer to the table
- * address - Physical address of the table
+ * PARAMETERS: address - Physical address of the table
* flags - Allocation flags of the table
+ * override - Whether override should be performed
* table_index - Where table index is returned
*
* RETURN: Status
@@ -844,15 +844,13 @@ acpi_tb_load_table(u32 table_index, struct acpi_namespace_node *parent_node)
******************************************************************************/
acpi_status
-acpi_tb_install_and_load_table(struct acpi_table_header *table,
- acpi_physical_address address,
+acpi_tb_install_and_load_table(acpi_physical_address address,
u8 flags, u8 override, u32 *table_index)
{
acpi_status status;
u32 i;
- acpi_owner_id owner_id;
- ACPI_FUNCTION_TRACE(acpi_load_table);
+ ACPI_FUNCTION_TRACE(tb_install_and_load_table);
(void)acpi_ut_acquire_mutex(ACPI_MTX_TABLES);
@@ -864,41 +862,8 @@ acpi_tb_install_and_load_table(struct acpi_table_header *table,
goto unlock_and_exit;
}
- /*
- * Note: Now table is "INSTALLED", it must be validated before
- * using.
- */
- status = acpi_tb_validate_table(&acpi_gbl_root_table_list.tables[i]);
- if (ACPI_FAILURE(status)) {
- goto unlock_and_exit;
- }
-
(void)acpi_ut_release_mutex(ACPI_MTX_TABLES);
- status = acpi_ns_load_table(i, acpi_gbl_root_node);
-
- /* Execute any module-level code that was found in the table */
-
- if (!acpi_gbl_parse_table_as_term_list
- && acpi_gbl_group_module_level_code) {
- acpi_ns_exec_module_code_list();
- }
-
- /*
- * Update GPEs for any new _Lxx/_Exx methods. Ignore errors. The host is
- * responsible for discovering any new wake GPEs by running _PRW methods
- * that may have been loaded by this table.
- */
- status = acpi_tb_get_owner_id(i, &owner_id);
- if (ACPI_SUCCESS(status)) {
- acpi_ev_update_gpes(owner_id);
- }
-
- /* Invoke table handler if present */
-
- if (acpi_gbl_table_handler) {
- (void)acpi_gbl_table_handler(ACPI_TABLE_EVENT_LOAD, table,
- acpi_gbl_table_handler_context);
- }
+ status = acpi_tb_load_table(i, acpi_gbl_root_node);
(void)acpi_ut_acquire_mutex(ACPI_MTX_TABLES);
unlock_and_exit:
diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c
index 5569f63..35d3f9b 100644
--- a/drivers/acpi/acpica/tbxfload.c
+++ b/drivers/acpi/acpica/tbxfload.c
@@ -326,10 +326,9 @@ acpi_status acpi_load_table(struct acpi_table_header *table)
/* Install the table and load it into the namespace */
ACPI_INFO(("Host-directed Dynamic ACPI Table Load:"));
- status =
- acpi_tb_install_and_load_table(table, ACPI_PTR_TO_PHYSADDR(table),
- ACPI_TABLE_ORIGIN_EXTERNAL_VIRTUAL,
- FALSE, &table_index);
+ status = acpi_tb_install_and_load_table(ACPI_PTR_TO_PHYSADDR(table),
+ ACPI_TABLE_ORIGIN_EXTERNAL_VIRTUAL,
+ FALSE, &table_index);
return_ACPI_STATUS(status);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 6/6] ACPICA: Tables: Add acpi_tb_unload_table()
2016-10-25 5:20 [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes Lv Zheng
` (4 preceding siblings ...)
2016-10-25 5:21 ` [PATCH 5/6] ACPICA: Tables: Cleanup acpi_tb_install_and_load_table() Lv Zheng
@ 2016-10-25 5:21 ` Lv Zheng
2016-10-26 0:51 ` [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes Rafael J. Wysocki
2016-10-26 7:39 ` [PATCH v2 0/3] ACPICA: Interpreter: Fix regressions in lock order improvement Lv Zheng
7 siblings, 0 replies; 15+ messages in thread
From: Lv Zheng @ 2016-10-25 5:21 UTC (permalink / raw)
To: Rafael J . Wysocki, Rafael J . Wysocki, Len Brown
Cc: linux-kernel, linux-acpi, Lv Zheng
This patch introduces acpi_tb_unload_table() to eliminate redundant code from
acpi_ex_unload_table() and acpi_unload_parent_table().
No functional change. Lv Zheng.
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
drivers/acpi/acpica/actables.h | 2 ++
drivers/acpi/acpica/exconfig.c | 35 +-----------------------------
drivers/acpi/acpica/tbdata.c | 48 ++++++++++++++++++++++++++++++++++++++++++
drivers/acpi/acpica/tbxfload.c | 31 +--------------------------
4 files changed, 52 insertions(+), 64 deletions(-)
diff --git a/drivers/acpi/acpica/actables.h b/drivers/acpi/acpica/actables.h
index 1850005..7dd527f 100644
--- a/drivers/acpi/acpica/actables.h
+++ b/drivers/acpi/acpica/actables.h
@@ -130,6 +130,8 @@ acpi_status
acpi_tb_install_and_load_table(acpi_physical_address address,
u8 flags, u8 override, u32 *table_index);
+acpi_status acpi_tb_unload_table(u32 table_index);
+
void acpi_tb_terminate(void);
acpi_status acpi_tb_delete_namespace_by_owner(u32 table_index);
diff --git a/drivers/acpi/acpica/exconfig.c b/drivers/acpi/acpica/exconfig.c
index 8b8d620..c32c782 100644
--- a/drivers/acpi/acpica/exconfig.c
+++ b/drivers/acpi/acpica/exconfig.c
@@ -499,7 +499,6 @@ acpi_status acpi_ex_unload_table(union acpi_operand_object *ddb_handle)
acpi_status status = AE_OK;
union acpi_operand_object *table_desc = ddb_handle;
u32 table_index;
- struct acpi_table_header *table;
ACPI_FUNCTION_TRACE(ex_unload_table);
@@ -536,39 +535,7 @@ acpi_status acpi_ex_unload_table(union acpi_operand_object *ddb_handle)
* strict order requirement against it.
*/
acpi_ex_exit_interpreter();
-
- /* Ensure the table is still loaded */
-
- if (!acpi_tb_is_table_loaded(table_index)) {
- status = AE_NOT_EXIST;
- goto lock_and_exit;
- }
-
- /* Invoke table handler if present */
-
- if (acpi_gbl_table_handler) {
- status = acpi_get_table_by_index(table_index, &table);
- if (ACPI_SUCCESS(status)) {
- (void)acpi_gbl_table_handler(ACPI_TABLE_EVENT_UNLOAD,
- table,
- acpi_gbl_table_handler_context);
- }
- }
-
- /* Delete the portion of the namespace owned by this table */
-
- status = acpi_tb_delete_namespace_by_owner(table_index);
- if (ACPI_FAILURE(status)) {
- goto lock_and_exit;
- }
-
- (void)acpi_tb_release_owner_id(table_index);
- acpi_tb_set_table_loaded_flag(table_index, FALSE);
-
-lock_and_exit:
-
- /* Re-acquire the interpreter lock */
-
+ status = acpi_tb_unload_table(table_index);
acpi_ex_enter_interpreter();
/*
diff --git a/drivers/acpi/acpica/tbdata.c b/drivers/acpi/acpica/tbdata.c
index 4cbfa30..82b0b57 100644
--- a/drivers/acpi/acpica/tbdata.c
+++ b/drivers/acpi/acpica/tbdata.c
@@ -871,3 +871,51 @@ unlock_and_exit:
(void)acpi_ut_release_mutex(ACPI_MTX_TABLES);
return_ACPI_STATUS(status);
}
+
+/*******************************************************************************
+ *
+ * FUNCTION: acpi_tb_unload_table
+ *
+ * PARAMETERS: table_index - Table index
+ *
+ * RETURN: Status
+ *
+ * DESCRIPTION: Unload an ACPI table
+ *
+ ******************************************************************************/
+
+acpi_status acpi_tb_unload_table(u32 table_index)
+{
+ acpi_status status = AE_OK;
+ struct acpi_table_header *table;
+
+ ACPI_FUNCTION_TRACE(tb_unload_table);
+
+ /* Ensure the table is still loaded */
+
+ if (!acpi_tb_is_table_loaded(table_index)) {
+ return_ACPI_STATUS(AE_NOT_EXIST);
+ }
+
+ /* Invoke table handler if present */
+
+ if (acpi_gbl_table_handler) {
+ status = acpi_get_table_by_index(table_index, &table);
+ if (ACPI_SUCCESS(status)) {
+ (void)acpi_gbl_table_handler(ACPI_TABLE_EVENT_UNLOAD,
+ table,
+ acpi_gbl_table_handler_context);
+ }
+ }
+
+ /* Delete the portion of the namespace owned by this table */
+
+ status = acpi_tb_delete_namespace_by_owner(table_index);
+ if (ACPI_FAILURE(status)) {
+ return_ACPI_STATUS(status);
+ }
+
+ (void)acpi_tb_release_owner_id(table_index);
+ acpi_tb_set_table_loaded_flag(table_index, FALSE);
+ return_ACPI_STATUS(status);
+}
diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c
index 35d3f9b..940254c 100644
--- a/drivers/acpi/acpica/tbxfload.c
+++ b/drivers/acpi/acpica/tbxfload.c
@@ -404,37 +404,8 @@ acpi_status acpi_unload_parent_table(acpi_handle object)
break;
}
- /* Ensure the table is actually loaded */
-
(void)acpi_ut_release_mutex(ACPI_MTX_TABLES);
- if (!acpi_tb_is_table_loaded(i)) {
- status = AE_NOT_EXIST;
- (void)acpi_ut_acquire_mutex(ACPI_MTX_TABLES);
- break;
- }
-
- /* Invoke table handler if present */
-
- if (acpi_gbl_table_handler) {
- (void)acpi_gbl_table_handler(ACPI_TABLE_EVENT_UNLOAD,
- acpi_gbl_root_table_list.
- tables[i].pointer,
- acpi_gbl_table_handler_context);
- }
-
- /*
- * Delete all namespace objects owned by this table. Note that
- * these objects can appear anywhere in the namespace by virtue
- * of the AML "Scope" operator. Thus, we need to track ownership
- * by an ID, not simply a position within the hierarchy.
- */
- status = acpi_tb_delete_namespace_by_owner(i);
- if (ACPI_FAILURE(status)) {
- break;
- }
-
- status = acpi_tb_release_owner_id(i);
- acpi_tb_set_table_loaded_flag(i, FALSE);
+ status = acpi_tb_unload_table(i);
(void)acpi_ut_acquire_mutex(ACPI_MTX_TABLES);
break;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes
2016-10-25 5:20 [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes Lv Zheng
` (5 preceding siblings ...)
2016-10-25 5:21 ` [PATCH 6/6] ACPICA: Tables: Add acpi_tb_unload_table() Lv Zheng
@ 2016-10-26 0:51 ` Rafael J. Wysocki
2016-10-26 6:17 ` Zheng, Lv
2016-10-26 7:39 ` [PATCH v2 0/3] ACPICA: Interpreter: Fix regressions in lock order improvement Lv Zheng
7 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2016-10-26 0:51 UTC (permalink / raw)
To: Lv Zheng
Cc: Rafael J . Wysocki, Rafael J . Wysocki, Len Brown,
Linux Kernel Mailing List, ACPI Devel Maling List, Lv Zheng
On Tue, Oct 25, 2016 at 7:20 AM, Lv Zheng <zetalog@gmail.com> wrote:
> This patchset improves ACPICA intepreter lock order fixes. Including
> several urgent regression fixes [PATCH 0-3].
OK, thanks!
So patches [4-6/6] appear to be cleanups and I'd prefer them to be
applied in a usual way (ie. via the upstream ACPICA).
I'd like to take the [1-3/6] as fixes for 4.9-rc3 though, but for that
I need you to tell me which mainline kernel commits are fixed by them.
IOW, what should I put into the Fixes: tags.
[In the future, if you post a regression fix, please always add a
FIxes: tag to it pointing to the commit being fixed.]
> Patches tested with customized ACPI table where _PS0/_PS3 methods are
> customized to invoke a serialized control method which creates named
> objects. When pm_async=yes, AE_ALREADY_EXISTS can be seen in suspend/resume
> process. This is an existing issue, triggered in 4.9-rc1 by ACPICA
> interpreter lock order fixes, and can be fixed by [PATCH 1] in this series.
>
> Lv Zheng (6):
> ACPICA: Dispatcher: Fix order issue of method termination
> ACPICA: Dispatcher: Fix an unbalanced lock exit path in
> acpi_ds_auto_serialize_method()
> ACPICA: Dispatcher: Tune interpreter lock around
> acpi_ev_initialize_region()
> ACPICA: Events: Cleanup acpi_ev_initialize_region()
> ACPICA: Tables: Cleanup acpi_tb_install_and_load_table()
> ACPICA: Tables: Add acpi_tb_unload_table()
Thanks,
Rafael
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes
2016-10-26 0:51 ` [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes Rafael J. Wysocki
@ 2016-10-26 6:17 ` Zheng, Lv
2016-10-26 7:05 ` Zheng, Lv
0 siblings, 1 reply; 15+ messages in thread
From: Zheng, Lv @ 2016-10-26 6:17 UTC (permalink / raw)
To: Rafael J. Wysocki, Lv Zheng
Cc: Wysocki, Rafael J, Rafael J . Wysocki, Brown, Len,
Linux Kernel Mailing List, ACPI Devel Maling List
Hi, Rafael
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> Wysocki
> Subject: Re: [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes
>
> On Tue, Oct 25, 2016 at 7:20 AM, Lv Zheng <zetalog@gmail.com> wrote:
> > This patchset improves ACPICA intepreter lock order fixes. Including
> > several urgent regression fixes [PATCH 0-3].
>
> OK, thanks!
>
> So patches [4-6/6] appear to be cleanups and I'd prefer them to be
> applied in a usual way (ie. via the upstream ACPICA).
I think PATCH 4 is also an urgent fix.
On certain table loading mode (we have 3 now).
When acpi_ds_initialize_objects() is invoked, acpi_ds_initialize_region() will be invoked.
While in other modes, it will be invoked in acpi_ds_load2_end_op(), so no-op in acpi_ds_initialize_objects().
When it is not no-op in acpi_ds_initialize_objects(), the wrong returning value becomes an exception preventing the table from being correctly loaded/initialized.
[PATCH 5-6] are cleanups.
>
> I'd like to take the [1-3/6] as fixes for 4.9-rc3 though, but for that
> I need you to tell me which mainline kernel commits are fixed by them.
>
> IOW, what should I put into the Fixes: tags.
>
> [In the future, if you post a regression fix, please always add a
> FIxes: tag to it pointing to the commit being fixed.]
OK, I'll add the Fixes tag and re-send the patches.
Thanks and best regards
Lv
>
> > Patches tested with customized ACPI table where _PS0/_PS3 methods are
> > customized to invoke a serialized control method which creates named
> > objects. When pm_async=yes, AE_ALREADY_EXISTS can be seen in suspend/resume
> > process. This is an existing issue, triggered in 4.9-rc1 by ACPICA
> > interpreter lock order fixes, and can be fixed by [PATCH 1] in this series.
> >
> > Lv Zheng (6):
> > ACPICA: Dispatcher: Fix order issue of method termination
> > ACPICA: Dispatcher: Fix an unbalanced lock exit path in
> > acpi_ds_auto_serialize_method()
> > ACPICA: Dispatcher: Tune interpreter lock around
> > acpi_ev_initialize_region()
> > ACPICA: Events: Cleanup acpi_ev_initialize_region()
> > ACPICA: Tables: Cleanup acpi_tb_install_and_load_table()
> > ACPICA: Tables: Add acpi_tb_unload_table()
>
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes
2016-10-26 6:17 ` Zheng, Lv
@ 2016-10-26 7:05 ` Zheng, Lv
0 siblings, 0 replies; 15+ messages in thread
From: Zheng, Lv @ 2016-10-26 7:05 UTC (permalink / raw)
To: Zheng, Lv, Rafael J. Wysocki, Lv Zheng
Cc: Wysocki, Rafael J, Rafael J . Wysocki, Brown, Len,
Linux Kernel Mailing List, ACPI Devel Maling List
Hi, Rafael
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Zheng,
> Lv
> Sent: Tuesday, October 25, 2016 11:17 PM
> To: Rafael J. Wysocki <rafael@kernel.org>; Lv Zheng <zetalog@gmail.com>
> Cc: Wysocki, Rafael J <rafael.j.wysocki@intel.com>; Rafael J . Wysocki <rjw@rjwysocki.net>; Brown, Len
> <len.brown@intel.com>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>; ACPI Devel Maling
> List <linux-acpi@vger.kernel.org>
> Subject: RE: [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes
>
> Hi, Rafael
>
> > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael
> J.
> > Wysocki
> > Subject: Re: [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes
> >
> > On Tue, Oct 25, 2016 at 7:20 AM, Lv Zheng <zetalog@gmail.com> wrote:
> > > This patchset improves ACPICA intepreter lock order fixes. Including
> > > several urgent regression fixes [PATCH 0-3].
> >
> > OK, thanks!
> >
> > So patches [4-6/6] appear to be cleanups and I'd prefer them to be
> > applied in a usual way (ie. via the upstream ACPICA).
>
> I think PATCH 4 is also an urgent fix.
> On certain table loading mode (we have 3 now).
> When acpi_ds_initialize_objects() is invoked, acpi_ds_initialize_region() will be invoked.
> While in other modes, it will be invoked in acpi_ds_load2_end_op(), so no-op in
> acpi_ds_initialize_objects().
>
> When it is not no-op in acpi_ds_initialize_objects(), the wrong returning value becomes an exception
> preventing the table from being correctly loaded/initialized.
I'll stop including PATCH 4 in the regression fix series.
I cannot find the original triggering case right here right now.
I'll think it's not urgent.
Sorry for the noise.
Thanks
Lv
>
> [PATCH 5-6] are cleanups.
>
> >
> > I'd like to take the [1-3/6] as fixes for 4.9-rc3 though, but for that
> > I need you to tell me which mainline kernel commits are fixed by them.
> >
> > IOW, what should I put into the Fixes: tags.
> >
> > [In the future, if you post a regression fix, please always add a
> > FIxes: tag to it pointing to the commit being fixed.]
>
> OK, I'll add the Fixes tag and re-send the patches.
>
> Thanks and best regards
> Lv
>
> >
> > > Patches tested with customized ACPI table where _PS0/_PS3 methods are
> > > customized to invoke a serialized control method which creates named
> > > objects. When pm_async=yes, AE_ALREADY_EXISTS can be seen in suspend/resume
> > > process. This is an existing issue, triggered in 4.9-rc1 by ACPICA
> > > interpreter lock order fixes, and can be fixed by [PATCH 1] in this series.
> > >
> > > Lv Zheng (6):
> > > ACPICA: Dispatcher: Fix order issue of method termination
> > > ACPICA: Dispatcher: Fix an unbalanced lock exit path in
> > > acpi_ds_auto_serialize_method()
> > > ACPICA: Dispatcher: Tune interpreter lock around
> > > acpi_ev_initialize_region()
> > > ACPICA: Events: Cleanup acpi_ev_initialize_region()
> > > ACPICA: Tables: Cleanup acpi_tb_install_and_load_table()
> > > ACPICA: Tables: Add acpi_tb_unload_table()
> >
> > Thanks,
> > Rafael
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> \x04�{.n�+�������+%��lzwm��b�맲��r��zX��\x16��(��\x17��ܨ}���Ơz�&j:+v���\r����zZ+��+zf���h���~����i���z�\x1e�w���?��
> ��&�)ߢ^[f
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 0/3] ACPICA: Interpreter: Fix regressions in lock order improvement
2016-10-25 5:20 [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes Lv Zheng
` (6 preceding siblings ...)
2016-10-26 0:51 ` [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes Rafael J. Wysocki
@ 2016-10-26 7:39 ` Lv Zheng
2016-10-26 7:40 ` [PATCH v2 1/3] ACPICA: Dispatcher: Fix order issue of method termination Lv Zheng
` (3 more replies)
7 siblings, 4 replies; 15+ messages in thread
From: Lv Zheng @ 2016-10-26 7:39 UTC (permalink / raw)
To: Rafael J . Wysocki, Rafael J . Wysocki, Len Brown
Cc: linux-kernel, linux-acpi, Lv Zheng
This patchset fixes regressions in ACPICA intepreter lock order
improvement.
Patches tested with customized ACPI table where _PS0/_PS3 methods are
customized to invoke a serialized control method which creates named
objects. When pm_async=yes, AE_ALREADY_EXISTS can be seen in suspend/resume
process. This is an existing issue, triggered in 4.9-rc1 by ACPICA
interpreter lock order fixes, and can be fixed by [PATCH 1] in this series.
v2:
1. Remove non-regression-fixes.
2. Add "Fixes" tags.
Lv Zheng (3):
ACPICA: Dispatcher: Fix order issue of method termination
ACPICA: Dispatcher: Fix an unbalanced lock exit path in
acpi_ds_auto_serialize_method()
ACPICA: Dispatcher: Tune interpreter lock around
acpi_ev_initialize_region()
drivers/acpi/acpica/dsinit.c | 11 +++-------
drivers/acpi/acpica/dsmethod.c | 50 +++++++++++++++++++-----------------------
drivers/acpi/acpica/dswload2.c | 2 --
drivers/acpi/acpica/evrgnini.c | 3 +++
drivers/acpi/acpica/nsload.c | 2 ++
5 files changed, 30 insertions(+), 38 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v2 1/3] ACPICA: Dispatcher: Fix order issue of method termination
2016-10-26 7:39 ` [PATCH v2 0/3] ACPICA: Interpreter: Fix regressions in lock order improvement Lv Zheng
@ 2016-10-26 7:40 ` Lv Zheng
2016-10-26 7:40 ` [PATCH v2 2/3] ACPICA: Dispatcher: Fix an unbalanced lock exit path in acpi_ds_auto_serialize_method() Lv Zheng
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Lv Zheng @ 2016-10-26 7:40 UTC (permalink / raw)
To: Rafael J . Wysocki, Rafael J . Wysocki, Len Brown
Cc: linux-kernel, linux-acpi, Lv Zheng
The last step of the method termination should be the end of the method
serialization. Otherwise, the steps happening after it will face the race
issues that cannot be protected by the method serialization mechanism.
This patch fixes this issue by moving the per-method-object deletion code
prior than the end of the method serialization. Otherwise, the possible
race issues may result in AE_ALREADY_EXISTS error in a parallel
environment. Reported by Imre Deak. Fixed by Lv Zheng.
Fixes: 74f51b80a0c4 ("ACPICA: Namespace: Fix dynamic table loading issues")
Reported-and-tested-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
drivers/acpi/acpica/dsmethod.c | 40 ++++++++++++++++++++--------------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/acpi/acpica/dsmethod.c b/drivers/acpi/acpica/dsmethod.c
index 32e9ddc..c4028a8 100644
--- a/drivers/acpi/acpica/dsmethod.c
+++ b/drivers/acpi/acpica/dsmethod.c
@@ -731,26 +731,6 @@ acpi_ds_terminate_control_method(union acpi_operand_object *method_desc,
acpi_ds_method_data_delete_all(walk_state);
/*
- * If method is serialized, release the mutex and restore the
- * current sync level for this thread
- */
- if (method_desc->method.mutex) {
-
- /* Acquisition Depth handles recursive calls */
-
- method_desc->method.mutex->mutex.acquisition_depth--;
- if (!method_desc->method.mutex->mutex.acquisition_depth) {
- walk_state->thread->current_sync_level =
- method_desc->method.mutex->mutex.
- original_sync_level;
-
- acpi_os_release_mutex(method_desc->method.
- mutex->mutex.os_mutex);
- method_desc->method.mutex->mutex.thread_id = 0;
- }
- }
-
- /*
* Delete any namespace objects created anywhere within the
* namespace by the execution of this method. Unless:
* 1) This method is a module-level executable code method, in which
@@ -786,6 +766,26 @@ acpi_ds_terminate_control_method(union acpi_operand_object *method_desc,
~ACPI_METHOD_MODIFIED_NAMESPACE;
}
}
+
+ /*
+ * If method is serialized, release the mutex and restore the
+ * current sync level for this thread
+ */
+ if (method_desc->method.mutex) {
+
+ /* Acquisition Depth handles recursive calls */
+
+ method_desc->method.mutex->mutex.acquisition_depth--;
+ if (!method_desc->method.mutex->mutex.acquisition_depth) {
+ walk_state->thread->current_sync_level =
+ method_desc->method.mutex->mutex.
+ original_sync_level;
+
+ acpi_os_release_mutex(method_desc->method.
+ mutex->mutex.os_mutex);
+ method_desc->method.mutex->mutex.thread_id = 0;
+ }
+ }
}
/* Decrement the thread count on the method */
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 2/3] ACPICA: Dispatcher: Fix an unbalanced lock exit path in acpi_ds_auto_serialize_method()
2016-10-26 7:39 ` [PATCH v2 0/3] ACPICA: Interpreter: Fix regressions in lock order improvement Lv Zheng
2016-10-26 7:40 ` [PATCH v2 1/3] ACPICA: Dispatcher: Fix order issue of method termination Lv Zheng
@ 2016-10-26 7:40 ` Lv Zheng
2016-10-26 7:42 ` [PATCH v2 3/3] ACPICA: Dispatcher: Tune interpreter lock around acpi_ev_initialize_region() Lv Zheng
2016-10-27 10:22 ` [PATCH v2 0/3] ACPICA: Interpreter: Fix regressions in lock order improvement Rafael J. Wysocki
3 siblings, 0 replies; 15+ messages in thread
From: Lv Zheng @ 2016-10-26 7:40 UTC (permalink / raw)
To: Rafael J . Wysocki, Rafael J . Wysocki, Len Brown
Cc: linux-kernel, linux-acpi, Lv Zheng
There is a lock unbalanced exit path in acpi_ds_initialize_method(),
this patch corrects it. Lv Zheng.
Fixes: 441ad11d078f ("ACPICA: Dispatcher: Fix a mutex issue for method auto serialization")
Tested-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
drivers/acpi/acpica/dsmethod.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/acpica/dsmethod.c b/drivers/acpi/acpica/dsmethod.c
index c4028a8..5997e59 100644
--- a/drivers/acpi/acpica/dsmethod.c
+++ b/drivers/acpi/acpica/dsmethod.c
@@ -128,7 +128,7 @@ acpi_ds_auto_serialize_method(struct acpi_namespace_node *node,
if (ACPI_FAILURE(status)) {
acpi_ds_delete_walk_state(walk_state);
acpi_ps_free_op(op);
- return_ACPI_STATUS(status);
+ goto unlock;
}
walk_state->descending_callback = acpi_ds_detect_named_opcodes;
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 3/3] ACPICA: Dispatcher: Tune interpreter lock around acpi_ev_initialize_region()
2016-10-26 7:39 ` [PATCH v2 0/3] ACPICA: Interpreter: Fix regressions in lock order improvement Lv Zheng
2016-10-26 7:40 ` [PATCH v2 1/3] ACPICA: Dispatcher: Fix order issue of method termination Lv Zheng
2016-10-26 7:40 ` [PATCH v2 2/3] ACPICA: Dispatcher: Fix an unbalanced lock exit path in acpi_ds_auto_serialize_method() Lv Zheng
@ 2016-10-26 7:42 ` Lv Zheng
2016-10-27 10:22 ` [PATCH v2 0/3] ACPICA: Interpreter: Fix regressions in lock order improvement Rafael J. Wysocki
3 siblings, 0 replies; 15+ messages in thread
From: Lv Zheng @ 2016-10-26 7:42 UTC (permalink / raw)
To: Rafael J . Wysocki, Rafael J . Wysocki, Len Brown
Cc: linux-kernel, linux-acpi, Lv Zheng
In code path of acpi_ev_initialize_region(), there are namespace modification
code unlocked. This patch tunes the code to make sure such modification are
locked. Lv Zheng.
Fixes: 74f51b80a0c4 ("ACPICA: Namespace: Fix dynamic table loading issues")
Tested-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
drivers/acpi/acpica/dsinit.c | 11 +++--------
drivers/acpi/acpica/dsmethod.c | 12 +++---------
drivers/acpi/acpica/dswload2.c | 2 --
drivers/acpi/acpica/evrgnini.c | 3 +++
drivers/acpi/acpica/nsload.c | 2 ++
5 files changed, 11 insertions(+), 19 deletions(-)
diff --git a/drivers/acpi/acpica/dsinit.c b/drivers/acpi/acpica/dsinit.c
index f1e6dcc..54d48b9 100644
--- a/drivers/acpi/acpica/dsinit.c
+++ b/drivers/acpi/acpica/dsinit.c
@@ -46,6 +46,7 @@
#include "acdispat.h"
#include "acnamesp.h"
#include "actables.h"
+#include "acinterp.h"
#define _COMPONENT ACPI_DISPATCHER
ACPI_MODULE_NAME("dsinit")
@@ -214,23 +215,17 @@ acpi_ds_initialize_objects(u32 table_index,
/* Walk entire namespace from the supplied root */
- status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
- if (ACPI_FAILURE(status)) {
- return_ACPI_STATUS(status);
- }
-
/*
* We don't use acpi_walk_namespace since we do not want to acquire
* the namespace reader lock.
*/
status =
acpi_ns_walk_namespace(ACPI_TYPE_ANY, start_node, ACPI_UINT32_MAX,
- ACPI_NS_WALK_UNLOCK, acpi_ds_init_one_object,
- NULL, &info, NULL);
+ 0, acpi_ds_init_one_object, NULL, &info,
+ NULL);
if (ACPI_FAILURE(status)) {
ACPI_EXCEPTION((AE_INFO, status, "During WalkNamespace"));
}
- (void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
status = acpi_get_table_by_index(table_index, &table);
if (ACPI_FAILURE(status)) {
diff --git a/drivers/acpi/acpica/dsmethod.c b/drivers/acpi/acpica/dsmethod.c
index 5997e59..2b3210f 100644
--- a/drivers/acpi/acpica/dsmethod.c
+++ b/drivers/acpi/acpica/dsmethod.c
@@ -99,14 +99,11 @@ acpi_ds_auto_serialize_method(struct acpi_namespace_node *node,
"Method auto-serialization parse [%4.4s] %p\n",
acpi_ut_get_node_name(node), node));
- acpi_ex_enter_interpreter();
-
/* Create/Init a root op for the method parse tree */
op = acpi_ps_alloc_op(AML_METHOD_OP, obj_desc->method.aml_start);
if (!op) {
- status = AE_NO_MEMORY;
- goto unlock;
+ return_ACPI_STATUS(AE_NO_MEMORY);
}
acpi_ps_set_name(op, node->name.integer);
@@ -118,8 +115,7 @@ acpi_ds_auto_serialize_method(struct acpi_namespace_node *node,
acpi_ds_create_walk_state(node->owner_id, NULL, NULL, NULL);
if (!walk_state) {
acpi_ps_free_op(op);
- status = AE_NO_MEMORY;
- goto unlock;
+ return_ACPI_STATUS(AE_NO_MEMORY);
}
status = acpi_ds_init_aml_walk(walk_state, op, node,
@@ -128,7 +124,7 @@ acpi_ds_auto_serialize_method(struct acpi_namespace_node *node,
if (ACPI_FAILURE(status)) {
acpi_ds_delete_walk_state(walk_state);
acpi_ps_free_op(op);
- goto unlock;
+ return_ACPI_STATUS(status);
}
walk_state->descending_callback = acpi_ds_detect_named_opcodes;
@@ -138,8 +134,6 @@ acpi_ds_auto_serialize_method(struct acpi_namespace_node *node,
status = acpi_ps_parse_aml(walk_state);
acpi_ps_delete_parse_tree(op);
-unlock:
- acpi_ex_exit_interpreter();
return_ACPI_STATUS(status);
}
diff --git a/drivers/acpi/acpica/dswload2.c b/drivers/acpi/acpica/dswload2.c
index 028b22a..e362182 100644
--- a/drivers/acpi/acpica/dswload2.c
+++ b/drivers/acpi/acpica/dswload2.c
@@ -607,11 +607,9 @@ acpi_status acpi_ds_load2_end_op(struct acpi_walk_state *walk_state)
}
}
- acpi_ex_exit_interpreter();
status =
acpi_ev_initialize_region
(acpi_ns_get_attached_object(node), FALSE);
- acpi_ex_enter_interpreter();
if (ACPI_FAILURE(status)) {
/*
diff --git a/drivers/acpi/acpica/evrgnini.c b/drivers/acpi/acpica/evrgnini.c
index 3843f1f..75ddd16 100644
--- a/drivers/acpi/acpica/evrgnini.c
+++ b/drivers/acpi/acpica/evrgnini.c
@@ -45,6 +45,7 @@
#include "accommon.h"
#include "acevents.h"
#include "acnamesp.h"
+#include "acinterp.h"
#define _COMPONENT ACPI_EVENTS
ACPI_MODULE_NAME("evrgnini")
@@ -597,9 +598,11 @@ acpi_ev_initialize_region(union acpi_operand_object *region_obj,
}
}
+ acpi_ex_exit_interpreter();
status =
acpi_ev_execute_reg_method(region_obj,
ACPI_REG_CONNECT);
+ acpi_ex_enter_interpreter();
if (acpi_ns_locked) {
status =
diff --git a/drivers/acpi/acpica/nsload.c b/drivers/acpi/acpica/nsload.c
index 334d3c5..d1f2014 100644
--- a/drivers/acpi/acpica/nsload.c
+++ b/drivers/acpi/acpica/nsload.c
@@ -137,7 +137,9 @@ acpi_ns_load_table(u32 table_index, struct acpi_namespace_node *node)
ACPI_DEBUG_PRINT((ACPI_DB_INFO,
"**** Begin Table Object Initialization\n"));
+ acpi_ex_enter_interpreter();
status = acpi_ds_initialize_objects(table_index, node);
+ acpi_ex_exit_interpreter();
ACPI_DEBUG_PRINT((ACPI_DB_INFO,
"**** Completed Table Object Initialization\n"));
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 0/3] ACPICA: Interpreter: Fix regressions in lock order improvement
2016-10-26 7:39 ` [PATCH v2 0/3] ACPICA: Interpreter: Fix regressions in lock order improvement Lv Zheng
` (2 preceding siblings ...)
2016-10-26 7:42 ` [PATCH v2 3/3] ACPICA: Dispatcher: Tune interpreter lock around acpi_ev_initialize_region() Lv Zheng
@ 2016-10-27 10:22 ` Rafael J. Wysocki
3 siblings, 0 replies; 15+ messages in thread
From: Rafael J. Wysocki @ 2016-10-27 10:22 UTC (permalink / raw)
To: Lv Zheng
Cc: Rafael J . Wysocki, Rafael J . Wysocki, Len Brown,
Linux Kernel Mailing List, ACPI Devel Maling List, Lv Zheng
On Wed, Oct 26, 2016 at 9:39 AM, Lv Zheng <zetalog@gmail.com> wrote:
> This patchset fixes regressions in ACPICA intepreter lock order
> improvement.
>
> Patches tested with customized ACPI table where _PS0/_PS3 methods are
> customized to invoke a serialized control method which creates named
> objects. When pm_async=yes, AE_ALREADY_EXISTS can be seen in suspend/resume
> process. This is an existing issue, triggered in 4.9-rc1 by ACPICA
> interpreter lock order fixes, and can be fixed by [PATCH 1] in this series.
>
> v2:
> 1. Remove non-regression-fixes.
> 2. Add "Fixes" tags.
>
> Lv Zheng (3):
> ACPICA: Dispatcher: Fix order issue of method termination
> ACPICA: Dispatcher: Fix an unbalanced lock exit path in
> acpi_ds_auto_serialize_method()
> ACPICA: Dispatcher: Tune interpreter lock around
> acpi_ev_initialize_region()
Thanks!
I'm queuing up this series for the next ACPI pull request.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 15+ messages in thread