ACPICA: Interpreter: Improve lock order fixes
Subsystems (2)
cover ACPICA: Interpreter: Improve lock order fixes
This patchset improves ACPICA intepreter lock order fixes. Including several urgent regression fixes [PATCH 0-3].
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()
drivers/acpi/acpica/acevents.h | 4 +-- drivers/acpi/acpica/actables.h | 5 +-- drivers/acpi/acpica/dsinit.c | 11 ++---- drivers/acpi/acpica/dsmethod.c | 50 ++++++++++++-------------- drivers/acpi/acpica/dsopcode.c | 2 +- drivers/acpi/acpica/dswload2.c | 15 +------- drivers/acpi/acpica/evrgnini.c | 62 ++++++++++++++------------------ drivers/acpi/acpica/exconfig.c | 42 +++------------------- drivers/acpi/acpica/nsload.c | 2 ++ drivers/acpi/acpica/tbdata.c | 81 ++++++++++++++++++++++++------------------ drivers/acpi/acpica/tbxfload.c | 38 +++----------------- 11 files changed, 115 insertions(+), 197 deletions(-)
Signature · 1 line
2.7.4
PATCH 1/6 ACPICA: Dispatcher: Fix order issue of method termination
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.
--- drivers/acpi/acpica/dsmethod.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-)
drivers/acpi/acpica/dsmethod.c +20 -20
@@ -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 */
Signature · 1 line
2.7.4
PATCH 2/6 ACPICA: Dispatcher: Fix an unbalanced lock exit path in acpi_ds_auto_serialize_method()
In reply to Lv ZhengThere is a lock unbalanced exit path in acpi_ds_initialize_method(), this patch corrects it. Lv Zheng.
--- drivers/acpi/acpica/dsmethod.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/acpi/acpica/dsmethod.c +1 -1
@@ -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;
Signature · 1 line
2.7.4
PATCH 3/6 ACPICA: Dispatcher: Tune interpreter lock around acpi_ev_initialize_region()
In reply to Lv ZhengIn 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.
--- 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(-)
drivers/acpi/acpica/dsinit.c +3 -8
@@ -46,6 +46,7 @@#include "acdispat.h" #include "acnamesp.h" #include "actables.h" +#include "acinterp.h" #define _COMPONENT ACPI_DISPATCHERACPI_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)) {
drivers/acpi/acpica/dsmethod.c +3 -9
@@ -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); }
drivers/acpi/acpica/dswload2.c +0 -2
@@ -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)) {/*
drivers/acpi/acpica/evrgnini.c +3 -0
@@ -45,6 +45,7 @@#include "accommon.h" #include "acevents.h" #include "acnamesp.h" +#include "acinterp.h" #define _COMPONENT ACPI_EVENTSACPI_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 =
drivers/acpi/acpica/nsload.c +2 -0
@@ -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" ));
Signature · 1 line
2.7.4
PATCH 4/6 ACPICA: Events: Cleanup acpi_ev_initialize_region()
In reply to Lv ZhengThis 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.
--- 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(-)
drivers/acpi/acpica/acevents.h +1 -3
@@ -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
drivers/acpi/acpica/dsopcode.c +1 -1
@@ -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); }
drivers/acpi/acpica/dswload2.c +1 -12
@@ -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 :
drivers/acpi/acpica/evrgnini.c +24 -35
101 lines not on this page · raw message
Signature · 1 line
2.7.4
PATCH 5/6 ACPICA: Tables: Cleanup acpi_tb_install_and_load_table()
In reply to Lv Zhengacpi_tb_install_and_load_table() can invoke acpi_tb_load_table() to eliminate redundant code.
No functional change. Lv Zheng.
--- 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(-)
drivers/acpi/acpica/actables.h +1 -2
@@ -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 );
drivers/acpi/acpica/exconfig.c +3 -4
@@ -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)) {
drivers/acpi/acpica/tbdata.c +5 -40
70 lines not on this page · raw message
drivers/acpi/acpica/tbxfload.c +3 -4
@@ -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); }
Signature · 1 line
2.7.4
PATCH 6/6 ACPICA: Tables: Add acpi_tb_unload_table()
In reply to Lv ZhengThis 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.
--- 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(-)
drivers/acpi/acpica/actables.h +2 -0
@@ -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);
drivers/acpi/acpica/exconfig.c +1 -34
@@ -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 ();/*
drivers/acpi/acpica/tbdata.c +48 -0
51 lines not on this page · raw message
drivers/acpi/acpica/tbxfload.c +1 -30
@@ -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 ; }
Signature · 1 line
2.7.4
Rafael J. Wysocki
maintainerQuoted 2 lines · Lv Zheng
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.]
Quoted 6 lines
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_EXISTScan 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.
Quoted 9 lines
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: Cleanupacpi_ev_initialize_region()
ACPICA: Tables: Cleanupacpi_tb_install_and_load_table()
ACPICA: Tables: Addacpi_tb_unload_table()
Thanks,
Rafael
Hi, Rafael
Quoted 4 lines
From: [email protected] [mailto:[email protected]] On Behalf Of Rafael J. Wysocki
Subject: Re: [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes
Quoted 8 lines
Quoted 2 lines · Lv Zheng
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.
Quoted 8 lines
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
Quoted 23 lines
Quoted 7 lines
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_EXISTScan 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.Quoted 9 lines
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: Cleanupacpi_ev_initialize_region()
ACPICA: Tables: Cleanupacpi_tb_install_and_load_table()
ACPICA: Tables: Addacpi_tb_unload_table()Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Rafael
Quoted 25 lines
From: [email protected] [mailto:[email protected]] On Behalf Of Zheng, Lv
Sent: Tuesday, October 25, 2016 11:17 PM
To: Rafael J. Wysocki <[email protected]>; Lv Zheng <[email protected]>
Cc: Wysocki, Rafael J <[email protected]>; Rafael J . Wysocki <[email protected]>; Brown, Len <[email protected]>; Linux Kernel Mailing List <[email protected]>; ACPI Devel Maling List <[email protected]>
Subject: RE: [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixesHi, Rafael
Quoted 1 line
From: [email protected] [mailto:[email protected]] On Behalf Of Rafael
J.
Quoted 11 lines
Wysocki
Subject: Re: [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixesQuoted 2 lines · Lv Zheng
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).
Quoted 8 lines
I think PATCH 4 is also an urgent fix.
On certain table loading mode (we have 3 now).
Whenacpi_ds_initialize_objects()is invoked,acpi_ds_initialize_region()will be invoked. While in other modes, it will be invoked inacpi_ds_load2_end_op(), so no-op inacpi_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
Quoted 42 lines
[PATCH 5-6] are cleanups.
Quoted 8 lines
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
LvQuoted 17 lines
Quoted 7 lines
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_EXISTScan 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.Quoted 9 lines
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: Cleanupacpi_ev_initialize_region()
ACPICA: Tables: Cleanupacpi_tb_install_and_load_table()
ACPICA: Tables: Addacpi_tb_unload_table()Quoted 6 lines
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html�{.n�+�������+%��lzwm��b�맲��r��zX����(����ܨ}���Ơz�&j:+v��� ����zZ+��+zf���h���~����i���z��w���?�� ��&�)ߢf
v2 cover ACPICA: Interpreter: Fix regressions in lock order improvement
In reply to Lv ZhengThis 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(-)
Signature · 1 line
2.7.4
PATCH v2 1/3 ACPICA: Dispatcher: Fix order issue of method termination
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.
--- drivers/acpi/acpica/dsmethod.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-)
drivers/acpi/acpica/dsmethod.c +20 -20
@@ -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 */
Signature · 1 line
2.7.4
PATCH v2 2/3 ACPICA: Dispatcher: Fix an unbalanced lock exit path in acpi_ds_auto_serialize_method()
In reply to Lv ZhengThere is a lock unbalanced exit path in acpi_ds_initialize_method(), this patch corrects it. Lv Zheng.
--- drivers/acpi/acpica/dsmethod.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/acpi/acpica/dsmethod.c +1 -1
@@ -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;
Signature · 1 line
2.7.4
PATCH v2 3/3 ACPICA: Dispatcher: Tune interpreter lock around acpi_ev_initialize_region()
In reply to Lv ZhengIn 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.
--- 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(-)
drivers/acpi/acpica/dsinit.c +3 -8
@@ -46,6 +46,7 @@#include "acdispat.h" #include "acnamesp.h" #include "actables.h" +#include "acinterp.h" #define _COMPONENT ACPI_DISPATCHERACPI_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)) {
drivers/acpi/acpica/dsmethod.c +3 -9
@@ -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); }
drivers/acpi/acpica/dswload2.c +0 -2
@@ -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)) {/*
drivers/acpi/acpica/evrgnini.c +3 -0
@@ -45,6 +45,7 @@#include "accommon.h" #include "acevents.h" #include "acnamesp.h" +#include "acinterp.h" #define _COMPONENT ACPI_EVENTSACPI_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 =
drivers/acpi/acpica/nsload.c +2 -0
@@ -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" ));
Signature · 1 line
2.7.4
Rafael J. Wysocki
maintainerRe: [PATCH v2 0/3] ACPICA: Interpreter: Fix regressions in lock order improvement
In reply to Lv ZhengQuoted 13 lines · 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_EXISTScan 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.
Quoted 6 lines
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