Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1507980 > unrolled thread

[PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes Lv Zheng <zetalog@gmail.com> - 2016-10-25 07:30 +0200
    [PATCH 2/6] ACPICA: Dispatcher: Fix an unbalanced lock exit path in acpi_ds_auto_serialize_method() Lv Zheng <zetalog@gmail.com> - 2016-10-25 07:30 +0200
    [PATCH 4/6] ACPICA: Events: Cleanup acpi_ev_initialize_region() Lv Zheng <zetalog@gmail.com> - 2016-10-25 07:30 +0200
    Re: [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes "Rafael J. Wysocki" <rafael@kernel.org> - 2016-10-26 03:00 +0200
      RE: [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes "Zheng, Lv" <lv.zheng@intel.com> - 2016-10-26 08:20 +0200
        RE: [PATCH 0/6] ACPICA: Interpreter: Improve lock order fixes "Zheng, Lv" <lv.zheng@intel.com> - 2016-10-26 09:10 +0200
    [PATCH v2 3/3] ACPICA: Dispatcher: Tune interpreter lock around acpi_ev_initialize_region() Lv Zheng <zetalog@gmail.com> - 2016-10-26 09:50 +0200
    [PATCH v2 2/3] ACPICA: Dispatcher: Fix an unbalanced lock exit path in acpi_ds_auto_serialize_method() Lv Zheng <zetalog@gmail.com> - 2016-10-26 09:50 +0200
    [PATCH v2 0/3] ACPICA: Interpreter: Fix regressions in lock order improvement Lv Zheng <zetalog@gmail.com> - 2016-10-26 09:50 +0200
      [PATCH v2 1/3] ACPICA: Dispatcher: Fix order issue of method termination Lv Zheng <zetalog@gmail.com> - 2016-10-26 09:50 +0200

#1507980 — [PATCH 0/6] 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(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1507982 — [PATCH 2/6] ACPICA: Dispatcher: Fix an unbalanced lock exit path in acpi_ds_auto_serialize_method()

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

[toc] | [prev] | [next] | [standalone]

#1507984 — [PATCH 4/6] ACPICA: Events: Cleanup acpi_ev_initialize_region()

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

[toc] | [prev] | [next] | [standalone]

#1508730

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

[toc] | [prev] | [next] | [standalone]

#1508816

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

[toc] | [prev] | [next] | [standalone]

#1508856

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
> �{.n�+�������+%��lzwm��b�맲��r��zX����(����ܨ}���Ơz�&j:+v���
����zZ+��+zf���h���~����i���z��w���?��
> ��&�)ߢf

[toc] | [prev] | [next] | [standalone]

#1508886 — [PATCH v2 3/3] ACPICA: Dispatcher: Tune interpreter lock around acpi_ev_initialize_region()

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

[toc] | [prev] | [next] | [standalone]

#1508887 — [PATCH v2 2/3] ACPICA: Dispatcher: Fix an unbalanced lock exit path in acpi_ds_auto_serialize_method()

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

[toc] | [prev] | [next] | [standalone]

#1508888 — [PATCH v2 0/3] ACPICA: Interpreter: Fix regressions in lock order improvement

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

[toc] | [prev] | [next] | [standalone]

#1508889 — [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.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web