From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758797AbcLAIAp (ORCPT ); Thu, 1 Dec 2016 03:00:45 -0500 Received: from mga04.intel.com ([192.55.52.120]:63211 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750847AbcLAIAn (ORCPT ); Thu, 1 Dec 2016 03:00:43 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,723,1477983600"; d="scan'208";a="12032669" From: "Zheng, Lv" To: "Rafael J. Wysocki" CC: "Wysocki, Rafael J" , "Rafael J. Wysocki" , "Brown, Len" , Lv Zheng , Linux Kernel Mailing List , ACPI Devel Maling List , "Moore, Robert" Subject: RE: [PATCH 04/11] ACPICA: Events: Fix acpi_ev_initialize_region() return value Thread-Topic: [PATCH 04/11] ACPICA: Events: Fix acpi_ev_initialize_region() return value Thread-Index: AQHSStpc8QL1H8lp/0aj0zXDW+061aDxoVuAgAEYkjA= Date: Thu, 1 Dec 2016 08:00:39 +0000 Message-ID: <1AE640813FDE7649BE1B193DEA596E886A297699@SHSMSX101.ccr.corp.intel.com> References: In-Reply-To: Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZjZmNzVlMTYtN2QxMC00MGZhLTlkMGQtNWRiNWYyYzQzYjI2IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6ImhTYTY0N0luUlVuVzJncU5Iazk3T1FQZzF3XC9GdXFBbWZOSlZoRWg0RjJNPSJ9 x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id uB180nwK031474 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 04/11] ACPICA: Events: Fix acpi_ev_initialize_region() return value > > On Wed, Nov 30, 2016 at 8:21 AM, Lv Zheng wrote: > > ACPICA commit 543342ab7a676f4eb0c9f100d349388a84dff0e8 > > > > 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. The exception can be seen in > > acpi_ds_initialize_objects() when certain table loading mode is chosen. > > > > This patch also removes useless acpi_ns_locked from acpi_ev_initialize_region() > > as this function will always be invoked with interpreter lock held now, and > > the lock granularity has been tuned to lock around _REG execution, thus it > > is now handled by acpi_ex_exit_interpreter(). Lv Zheng. > > > > Link: https://github.com/acpica/acpica/commit/543342ab > > Signed-off-by: Lv Zheng > > Signed-off-by: Bob Moore > > --- > > 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 @@ union acpi_operand_object *acpi_ev_find_region_handler(acpi_adr_space_type > > 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)); > > This hunk doesn't apply for me. > > We have acpi_ex_exit_interpreter() / acpi_ex_enter_interpreter() > around the acpi_ev_initialize_region() in linux-next. This commit only changes returning value. Lock changes are not included. We never invokes acpi_ev_initialize_region with acpi_ns_locked=true. So all if (acpi_ns_locked) code pieces are useless and deleted in this commit. > > > 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 @@ static u8 acpi_ev_is_pci_root_bridge(struct acpi_namespace_node *node) > > * FUNCTION: acpi_ev_initialize_region > > * > > * PARAMETERS: region_obj - Region we are initializing > > - * acpi_ns_locked - Is namespace locked? > > * > > * RETURN: Status > > * > > @@ -497,19 +496,28 @@ static u8 acpi_ev_is_pci_root_bridge(struct acpi_namespace_node *node) > > * 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 @@ static u8 acpi_ev_is_pci_root_bridge(struct acpi_namespace_node *node) > > 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(); > > And this also doesn't apply, because we don't invoke > acpi_ex_exit_interpreter() / acpi_ex_enter_interpreter() around the > acpi_ev_execute_reg_method() call in linux-next. > acpi_ex_exit_interpreter() / acpi_ex_enter_interpreter() are not-modified-lines. Please check again. Unlock before _REG is the minimum requirement as ACPICA will reacquire interpreter lock when _REG is evaluated. Before applying the following commit (which is in the upstream): https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8633db6b02 ACPICA unlocks the entire walk namespace process. However this change did nothing to the locks. Only changed the returning value of acpi_ev_initialize_region(). Always returning AE_OK instead of AE_NOT_EXIST. Other than this, changes are no-ops. Thanks and best regards Lv > > > - > > - 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 @@ static u8 acpi_ev_is_pci_root_bridge(struct acpi_namespace_node *node) > > 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); > > } > > -- > > 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