ACPICA: 20161117 Release

ACPICA commit bc481e758e54f7644fd0b657119ca7763d8b6a9c

This is a back port result of the following commit:
Commit: 8633db6b027952449e155a316f4ae3a530bbe18f
Subject: ACPICA: Dispatcher: Fix interpreter locking around acpi_ev_initialize_region()

Link https://github.com/acpica/acpica/commit/bc481e75

Signed-off-by Lv Zheng <[email protected]>

Signed-off-by Bob Moore <[email protected]>

---
 drivers/acpi/acpica/dsinit.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/acpi/acpica/dsinit.c +0 -0

index 54d48b9..5de3f10 100644
@@ -221,8 +221,8 @@
         */

From: [email protected] [mailto:[email protected]] On Behalf Of Rafael J. Wysocki
Subject: Re: [PATCH 02/11] ACPICA: Back port of "ACPICA: Dispatcher: Tune interpreter lock around AcpiEvInitializeRegion()"

Quoted 25 lines · Lv Zheng

ACPICA commit bc481e758e54f7644fd0b657119ca7763d8b6a9c

This is a back port result of the following commit:
Commit: 8633db6b027952449e155a316f4ae3a530bbe18f
Subject: ACPICA: Dispatcher: Fix interpreter locking around acpi_ev_initialize_region()

Link https://github.com/acpica/acpica/commit/bc481e75

Signed-off-by Lv Zheng <[email protected]>

Signed-off-by Bob Moore <[email protected]>

---
 drivers/acpi/acpica/dsinit.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/acpi/acpica/dsinit.c +2 -2

index 54d48b9..5de3f10 100644
@@ -221,8 +221,8 @@
         */
        status =
            acpi_ns_walk_namespace(ACPI_TYPE_ANY, start_node, ACPI_UINT32_MAX,
-                                  0, acpi_ds_init_one_object, NULL, &info,
-                                  NULL);
+                                  ACPI_NS_WALK_NO_UNLOCK,
+                                  acpi_ds_init_one_object, NULL, &info, NULL);

drivers/acpi/acpica/dsinit.c

Quoted 4 lines

drivers/acpi/acpica/dsinit.c +0 -1

        if (ACPI_FAILURE(status)) {
                ACPI_EXCEPTION((AE_INFO, status, "During WalkNamespace"));
        }
--

This isn't necessary IMO, the current code linux-next code looks like the change has been made in there already AFAICS (please double check, though).

I'm skipping this patch.

Hi, Rafael

Quoted 30 lines

From: [email protected] [mailto:[email protected]] On Behalf Of Rafael J. Wysocki
Subject: Re: [PATCH 02/11] ACPICA: Back port of "ACPICA: Dispatcher: Tune interpreter lock around AcpiEvInitializeRegion()"

Quoted 25 lines · Lv Zheng

ACPICA commit bc481e758e54f7644fd0b657119ca7763d8b6a9c

This is a back port result of the following commit:
Commit: 8633db6b027952449e155a316f4ae3a530bbe18f
Subject: ACPICA: Dispatcher: Fix interpreter locking around acpi_ev_initialize_region()

Link https://github.com/acpica/acpica/commit/bc481e75

Signed-off-by Lv Zheng <[email protected]>

Signed-off-by Bob Moore <[email protected]>

---
 drivers/acpi/acpica/dsinit.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/acpi/acpica/dsinit.c +2 -2

index 54d48b9..5de3f10 100644
@@ -221,8 +221,8 @@
         */
        status =
            acpi_ns_walk_namespace(ACPI_TYPE_ANY, start_node, ACPI_UINT32_MAX,
-                                  0, acpi_ds_init_one_object, NULL, &info,
-                                  NULL);
+                                  ACPI_NS_WALK_NO_UNLOCK,
+                                  acpi_ds_init_one_object, NULL, &info, NULL);

Quoted 8 lines

drivers/acpi/acpica/dsinit.c

Quoted 4 lines

drivers/acpi/acpica/dsinit.c +0 -1

        if (ACPI_FAILURE(status)) {
                ACPI_EXCEPTION((AE_INFO, status, "During WalkNamespace"));
        }
--

This isn't necessary IMO, the current code linux-next code looks like the change has been made in there already AFAICS (please double check, though).

The fix was in Linux, however, when it is back ported to ACPICA, Bob asked me to do this change. Using ACPI_NS_WALK_NO_UNLOCK instead of meaningless 0.
So during this release cycle, this change is detected out as the only difference of the back ported commit.

Quoted 1 line

Quoted 1 line

I'm skipping this patch.

If this is skipped, it leaves us 14 lines divergences. Hope we can have this kind of divergences eliminated.

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 <[email protected]>

Signed-off-by Bob Moore <[email protected]>

---
 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

index 92fa47c..8a0049d 100644
@@ -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

drivers/acpi/acpica/dsopcode.c +1 -1

index 4cc9d98..77fd7c8 100644
@@ -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 +0 -3

index e362182..651f35a 100644
@@ -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)) {

drivers/acpi/acpica/dswload2.c +0 -0

                        break;
 
                case AML_NAME_OP:

drivers/acpi/acpica/evrgnini.c +16 -16

index 75ddd16..a909225 100644
@@ -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);

drivers/acpi/acpica/evrgnini.c +4 -12

-
-                               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.

From: [email protected] [mailto:[email protected]] On Behalf Of Rafael J. Wysocki
Subject: Re: [PATCH 04/11] ACPICA: Events: Fix acpi_ev_initialize_region() return value

Quoted 61 lines · Lv Zheng

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 <[email protected]>

Signed-off-by Bob Moore <[email protected]>

---
 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

index 92fa47c..8a0049d 100644
@@ -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

drivers/acpi/acpica/dsopcode.c +1 -1

index 4cc9d98..77fd7c8 100644
@@ -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 +0 -3

index e362182..651f35a 100644
@@ -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)) {

drivers/acpi/acpica/dswload2.c acpi_status acpi_ds_load2_end_op(struct acpi_walk_state *walk_state) · line 612

Quoted 10 lines

drivers/acpi/acpica/dswload2.c +1 -9

-                               /*
-                                *  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.

Quoted 77 lines

drivers/acpi/acpica/dswload2.c +0 -0

                        break;
 
                case AML_NAME_OP:

drivers/acpi/acpica/evrgnini.c +16 -21

index 75ddd16..a909225 100644
@@ -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,

drivers/acpi/acpica/evrgnini.c static u8 acpi_ev_is_pci_root_bridge(struct acpi_namespace_node *node) · line 601

Quoted 4 lines

drivers/acpi/acpica/evrgnini.c +2 -1

-                                                              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.

Quoted 24 lines

drivers/acpi/acpica/evrgnini.c +4 -12

-
-                               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.

drivers/acpi/acpica/evrgnini.c static u8 acpi_ev_is_pci_root_bridge(struct acpi_namespace_node *node)

Quoted 10 lines

drivers/acpi/acpica/evrgnini.c +2 -2

+        */
        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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi, Rafael

Quoted 79 lines

From: [email protected] [mailto:[email protected]] On Behalf Of Rafael J. Wysocki
Subject: Re: [PATCH 04/11] ACPICA: Events: Fix acpi_ev_initialize_region() return value

Quoted 61 lines · Lv Zheng

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 <[email protected]>

Signed-off-by Bob Moore <[email protected]>

---
 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

index 92fa47c..8a0049d 100644
@@ -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

drivers/acpi/acpica/dsopcode.c +1 -1

index 4cc9d98..77fd7c8 100644
@@ -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 +0 -3

index e362182..651f35a 100644
@@ -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)) {

drivers/acpi/acpica/dswload2.c acpi_status acpi_ds_load2_end_op(struct acpi_walk_state *walk_state) · line 612

Quoted 10 lines

drivers/acpi/acpica/dswload2.c +1 -9

-                               /*
-                                *  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.

Quoted 2 lines

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.

Quoted 77 lines

Quoted 77 lines

drivers/acpi/acpica/dswload2.c +0 -0

                        break;
 
                case AML_NAME_OP:

drivers/acpi/acpica/evrgnini.c +16 -21

index 75ddd16..a909225 100644
@@ -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,

Quoted 9 lines

drivers/acpi/acpica/evrgnini.c static u8 acpi_ev_is_pci_root_bridge(struct acpi_namespace_node *node) · line 601

Quoted 4 lines

drivers/acpi/acpica/evrgnini.c +2 -1

-                                                              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.

ACPICA commit cac6790954d4d752a083e6122220b8a22febcd07

This patch back ports Linux acpi_get_table_with_size() and early_acpi_os_unmap_memory() into ACPICA upstream to reduce divergences.

The 2 APIs are used by Linux as table management APIs for long time, it contains a hidden logic that during the early stage, the mapped tables should be unmapped before the early stage ends.

During the early stage, tables are handled by the following sequence:
acpi_get_table_with_size();
parse the table
early_acpi_os_unmap_memory();
During the late stage, tables are handled by the following sequence:
acpi_get_table();
parse the table
Linux uses acpi_gbl_permanent_mmap to distinguish the early stage and the late stage.

The reasoning of introducing acpi_get_table_with_size() is: ACPICA will remember the early mapped pointer in acpi_get_table() and Linux isn't able to prevent ACPICA from using the wrong early mapped pointer during the late stage as there is no API provided from ACPICA to be an inverse of acpi_get_table() to forget the early mapped pointer.

But how ACPICA can work with the early/late stage requirement? Inside of ACPICA, tables are ensured to be remained in "INSTALLED" state during the early stage, and they are carefully not transitioned to "VALIDATED" state until the late stage. So the same logic is in fact implemented inside of ACPICA in a different way. The gap is only that the feature is not provided to the OSPMs in an accessible external API style.

It then is possible to fix the gap by providing an inverse of acpi_get_table() from ACPICA, so that the two Linux sequences can be combined:
acpi_get_table();
parse the table
acpi_put_table();
In order to work easier with the current Linux code, acpi_get_table() and acpi_put_table() is implemented in a usage counting based style:
1. When the usage count of the table is increased from 0 to 1, table is

    mapped and .Pointer is set with the mapping address (VALIDATED);

2. When the usage count of the table is decreased from 1 to 0, .Pointer is unset and the mapping address is unmapped (INVALIDATED).
So that we can deploy the new APIs to Linux with minimal effort by just invoking acpi_get_table() in acpi_get_table_with_size() and invoking acpi_put_table() in early_acpi_os_unmap_memory(). Lv Zheng.

Link https://github.com/acpica/acpica/commit/cac67909

Signed-off-by Lv Zheng <[email protected]>

Signed-off-by Bob Moore <[email protected]>

Quoted 48 lines · Lv Zheng

ACPICA commit cac6790954d4d752a083e6122220b8a22febcd07

This patch back ports Linux acpi_get_table_with_size() and early_acpi_os_unmap_memory() into ACPICA upstream to reduce divergences.

The 2 APIs are used by Linux as table management APIs for long time, it contains a hidden logic that during the early stage, the mapped tables should be unmapped before the early stage ends.

During the early stage, tables are handled by the following sequence:
acpi_get_table_with_size();
parse the table
early_acpi_os_unmap_memory();
During the late stage, tables are handled by the following sequence:
acpi_get_table();
parse the table
Linux uses acpi_gbl_permanent_mmap to distinguish the early stage and the late stage.

The reasoning of introducing acpi_get_table_with_size() is: ACPICA will remember the early mapped pointer in acpi_get_table() and Linux isn't able to prevent ACPICA from using the wrong early mapped pointer during the late stage as there is no API provided from ACPICA to be an inverse of acpi_get_table() to forget the early mapped pointer.

But how ACPICA can work with the early/late stage requirement? Inside of ACPICA, tables are ensured to be remained in "INSTALLED" state during the early stage, and they are carefully not transitioned to "VALIDATED" state until the late stage. So the same logic is in fact implemented inside of ACPICA in a different way. The gap is only that the feature is not provided to the OSPMs in an accessible external API style.

It then is possible to fix the gap by providing an inverse of acpi_get_table() from ACPICA, so that the two Linux sequences can be combined:
acpi_get_table();
parse the table
acpi_put_table();
In order to work easier with the current Linux code, acpi_get_table() and acpi_put_table() is implemented in a usage counting based style:
1. When the usage count of the table is increased from 0 to 1, table is

    mapped and .Pointer is set with the mapping address (VALIDATED);

2. When the usage count of the table is decreased from 1 to 0, .Pointer is unset and the mapping address is unmapped (INVALIDATED).
So that we can deploy the new APIs to Linux with minimal effort by just invoking acpi_get_table() in acpi_get_table_with_size() and invoking acpi_put_table() in early_acpi_os_unmap_memory(). Lv Zheng.

Quoted 3 lines

Link https://github.com/acpica/acpica/commit/cac67909

Signed-off-by Lv Zheng <[email protected]>

Signed-off-by Bob Moore <[email protected]>

This commit in -next (071b39575679 ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel) causes a regression in my nfit/nvdimm test environment. The nfit produced by QEMU no longer results in a nvdimm bus being created.

I have not root caused it, but I'm using the following command line options to create an nfit in qemu-2.6. Reverting the commit leads compile failures.

Quoted 58 lines · Dan Williams

Quoted 48 lines · Lv Zheng

ACPICA commit cac6790954d4d752a083e6122220b8a22febcd07

This patch back ports Linux acpi_get_table_with_size() and early_acpi_os_unmap_memory() into ACPICA upstream to reduce divergences.

The 2 APIs are used by Linux as table management APIs for long time, it contains a hidden logic that during the early stage, the mapped tables should be unmapped before the early stage ends.

During the early stage, tables are handled by the following sequence:
acpi_get_table_with_size();
parse the table
early_acpi_os_unmap_memory();
During the late stage, tables are handled by the following sequence:
acpi_get_table();
parse the table
Linux uses acpi_gbl_permanent_mmap to distinguish the early stage and the late stage.

The reasoning of introducing acpi_get_table_with_size() is: ACPICA will remember the early mapped pointer in acpi_get_table() and Linux isn't able to prevent ACPICA from using the wrong early mapped pointer during the late stage as there is no API provided from ACPICA to be an inverse of acpi_get_table() to forget the early mapped pointer.

But how ACPICA can work with the early/late stage requirement? Inside of ACPICA, tables are ensured to be remained in "INSTALLED" state during the early stage, and they are carefully not transitioned to "VALIDATED" state until the late stage. So the same logic is in fact implemented inside of ACPICA in a different way. The gap is only that the feature is not provided to the OSPMs in an accessible external API style.

It then is possible to fix the gap by providing an inverse of acpi_get_table() from ACPICA, so that the two Linux sequences can be combined:
acpi_get_table();
parse the table
acpi_put_table();
In order to work easier with the current Linux code, acpi_get_table() and acpi_put_table() is implemented in a usage counting based style:
1. When the usage count of the table is increased from 0 to 1, table is

    mapped and .Pointer is set with the mapping address (VALIDATED);

2. When the usage count of the table is decreased from 1 to 0, .Pointer is unset and the mapping address is unmapped (INVALIDATED).
So that we can deploy the new APIs to Linux with minimal effort by just invoking acpi_get_table() in acpi_get_table_with_size() and invoking acpi_put_table() in early_acpi_os_unmap_memory(). Lv Zheng.

Quoted 3 lines

Link https://github.com/acpica/acpica/commit/cac67909

Signed-off-by Lv Zheng <[email protected]>

Signed-off-by Bob Moore <[email protected]>

This commit in -next (071b39575679 ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel) causes a regression in my nfit/nvdimm test environment. The nfit produced by QEMU no longer results in a nvdimm bus being created.

Quoted 3 lines

I have not root caused it, but I'm using the following command line options to create an nfit in qemu-2.6. Reverting the commit leads compile failures.

Would the build problems go away if you reverted "ACPICA: Tables:
Allow FADT to be customized with virtual address" (linux-next commit cf334d3174f9) in addition to it?

From: Dan Williams [mailto:[email protected]]
Subject: Re: [PATCH 08/11] ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel

Quoted 59 lines · Rafael J. Wysocki

Quoted 58 lines · Dan Williams

Quoted 48 lines · Lv Zheng

ACPICA commit cac6790954d4d752a083e6122220b8a22febcd07

This patch back ports Linux acpi_get_table_with_size() and early_acpi_os_unmap_memory() into ACPICA upstream to reduce divergences.

The 2 APIs are used by Linux as table management APIs for long time, it contains a hidden logic that during the early stage, the mapped tables should be unmapped before the early stage ends.

During the early stage, tables are handled by the following sequence:
acpi_get_table_with_size();
parse the table
early_acpi_os_unmap_memory();
During the late stage, tables are handled by the following sequence:
acpi_get_table();
parse the table
Linux uses acpi_gbl_permanent_mmap to distinguish the early stage and the late stage.

The reasoning of introducing acpi_get_table_with_size() is: ACPICA will remember the early mapped pointer in acpi_get_table() and Linux isn't able to prevent ACPICA from using the wrong early mapped pointer during the late stage as there is no API provided from ACPICA to be an inverse of acpi_get_table() to forget the early mapped pointer.

But how ACPICA can work with the early/late stage requirement? Inside of ACPICA, tables are ensured to be remained in "INSTALLED" state during the early stage, and they are carefully not transitioned to "VALIDATED" state until the late stage. So the same logic is in fact implemented inside of ACPICA in a different way. The gap is only that the feature is not provided to the OSPMs in an accessible external API style.

It then is possible to fix the gap by providing an inverse of acpi_get_table() from ACPICA, so that the two Linux sequences can be combined:
acpi_get_table();
parse the table
acpi_put_table();
In order to work easier with the current Linux code, acpi_get_table() and acpi_put_table() is implemented in a usage counting based style:
1. When the usage count of the table is increased from 0 to 1, table is

    mapped and .Pointer is set with the mapping address (VALIDATED);

2. When the usage count of the table is decreased from 1 to 0, .Pointer is unset and the mapping address is unmapped (INVALIDATED).
So that we can deploy the new APIs to Linux with minimal effort by just invoking acpi_get_table() in acpi_get_table_with_size() and invoking acpi_put_table() in early_acpi_os_unmap_memory(). Lv Zheng.

Quoted 3 lines

Link https://github.com/acpica/acpica/commit/cac67909

Signed-off-by Lv Zheng <[email protected]>

Signed-off-by Bob Moore <[email protected]>

This commit in -next (071b39575679 ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel) causes a regression in my nfit/nvdimm test environment. The nfit produced by QEMU no longer results in a nvdimm bus being created.

Quoted 7 lines

Quoted 3 lines

I have not root caused it, but I'm using the following command line options to create an nfit in qemu-2.6. Reverting the commit leads compile failures.

Would the build problems go away if you reverted "ACPICA: Tables:
Allow FADT to be customized with virtual address" (linux-next commit cf334d3174f9) in addition to it?

Yes, reverting those two commits gets me back to a functional environment:

Revert "ACPICA: Tables: Allow FADT to be customized with virtual address" Revert "ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_un

Hi, Rafael and Dan

Quoted 74 lines

From: Dan Williams [mailto:[email protected]]
Subject: Re: [PATCH 08/11] ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel

Quoted 59 lines · Rafael J. Wysocki

Quoted 58 lines · Dan Williams

Quoted 48 lines · Lv Zheng

ACPICA commit cac6790954d4d752a083e6122220b8a22febcd07

This patch back ports Linux acpi_get_table_with_size() and early_acpi_os_unmap_memory() into ACPICA upstream to reduce divergences.

The 2 APIs are used by Linux as table management APIs for long time, it contains a hidden logic that during the early stage, the mapped tables should be unmapped before the early stage ends.

During the early stage, tables are handled by the following sequence:
acpi_get_table_with_size();
parse the table
early_acpi_os_unmap_memory();
During the late stage, tables are handled by the following sequence:
acpi_get_table();
parse the table
Linux uses acpi_gbl_permanent_mmap to distinguish the early stage and the late stage.

The reasoning of introducing acpi_get_table_with_size() is: ACPICA will remember the early mapped pointer in acpi_get_table() and Linux isn't able to prevent ACPICA from using the wrong early mapped pointer during the late stage as there is no API provided from ACPICA to be an inverse of acpi_get_table() to forget the early mapped pointer.

But how ACPICA can work with the early/late stage requirement? Inside of ACPICA, tables are ensured to be remained in "INSTALLED" state during the early stage, and they are carefully not transitioned to "VALIDATED" state until the late stage. So the same logic is in fact implemented inside of ACPICA in a different way. The gap is only that the feature is not provided to the OSPMs in an accessible external API style.

It then is possible to fix the gap by providing an inverse of acpi_get_table() from ACPICA, so that the two Linux sequences can be combined:
acpi_get_table();
parse the table
acpi_put_table();
In order to work easier with the current Linux code, acpi_get_table() and acpi_put_table() is implemented in a usage counting based style:
1. When the usage count of the table is increased from 0 to 1, table is

    mapped and .Pointer is set with the mapping address (VALIDATED);

2. When the usage count of the table is decreased from 1 to 0, .Pointer is unset and the mapping address is unmapped (INVALIDATED).
So that we can deploy the new APIs to Linux with minimal effort by just invoking acpi_get_table() in acpi_get_table_with_size() and invoking acpi_put_table() in early_acpi_os_unmap_memory(). Lv Zheng.

Quoted 3 lines

Link https://github.com/acpica/acpica/commit/cac67909

Signed-off-by Lv Zheng <[email protected]>

Signed-off-by Bob Moore <[email protected]>

This commit in -next (071b39575679 ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel) causes a regression in my nfit/nvdimm test environment. The nfit produced by QEMU no longer results in a nvdimm bus being created.

Quoted 7 lines

Quoted 3 lines

I have not root caused it, but I'm using the following command line options to create an nfit in qemu-2.6. Reverting the commit leads compile failures.

Would the build problems go away if you reverted "ACPICA: Tables:
Allow FADT to be customized with virtual address" (linux-next commit cf334d3174f9) in addition to it?

Yes, reverting those two commits gets me back to a functional environment:

Quoted 3 lines

Revert "ACPICA: Tables: Allow FADT to be customized with virtual address" Revert "ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_un

To Dan:
It seems in drivers/acpi/nfit/core.c.
The returned table size is used by the NFIT code. I think it should be changed to use table_header->length.

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Dan
Williams
Sent: Friday, December 9, 2016 10:05 AM
To: Zheng, Lv <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>; Wysocki, Rafael J <[email protected]>; Rafael J. Wysocki <[email protected]>; Brown, Len <[email protected]>; Lv Zheng <[email protected]>; Linux Kernel Mailing List <[email protected]>; Linux ACPI <[email protected]>; Moore,
Robert <[email protected]>; [email protected] <[email protected]>
Subject: Re: [PATCH 08/11] ACPICA: Tables: Back port acpi_get_table_with_size() and
early_acpi_os_unmap_memory() from Linux kernel

Quoted 76 lines · Lv

Hi, Rafael and Dan

Quoted 74 lines

From: Dan Williams [mailto:[email protected]]
Subject: Re: [PATCH 08/11] ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel

Quoted 59 lines · Rafael J. Wysocki

Quoted 58 lines · Dan Williams

Quoted 48 lines · Lv Zheng

ACPICA commit cac6790954d4d752a083e6122220b8a22febcd07

This patch back ports Linux acpi_get_table_with_size() and early_acpi_os_unmap_memory() into ACPICA upstream to reduce divergences.

The 2 APIs are used by Linux as table management APIs for long time, it contains a hidden logic that during the early stage, the mapped tables should be unmapped before the early stage ends.

During the early stage, tables are handled by the following sequence:
acpi_get_table_with_size();
parse the table
early_acpi_os_unmap_memory();
During the late stage, tables are handled by the following sequence:
acpi_get_table();
parse the table
Linux uses acpi_gbl_permanent_mmap to distinguish the early stage and the late stage.

The reasoning of introducing acpi_get_table_with_size() is: ACPICA will remember the early mapped pointer in acpi_get_table() and Linux isn't able to prevent ACPICA from using the wrong early mapped pointer during the late stage as there is no API provided from ACPICA to be an inverse of acpi_get_table() to forget the early mapped pointer.

But how ACPICA can work with the early/late stage requirement? Inside of ACPICA, tables are ensured to be remained in "INSTALLED" state during the early stage, and they are carefully not transitioned to "VALIDATED" state until the late stage. So the same logic is in fact implemented inside of ACPICA in a different way. The gap is only that the feature is not provided to the OSPMs in an accessible external API style.

It then is possible to fix the gap by providing an inverse of acpi_get_table() from ACPICA, so that the two Linux sequences can be combined:
acpi_get_table();
parse the table
acpi_put_table();
In order to work easier with the current Linux code, acpi_get_table() and acpi_put_table() is implemented in a usage counting based style:
1. When the usage count of the table is increased from 0 to 1, table is

    mapped and .Pointer is set with the mapping address (VALIDATED);

2. When the usage count of the table is decreased from 1 to 0, .Pointer is unset and the mapping address is unmapped (INVALIDATED).
So that we can deploy the new APIs to Linux with minimal effort by just invoking acpi_get_table() in acpi_get_table_with_size() and invoking acpi_put_table() in early_acpi_os_unmap_memory(). Lv Zheng.

Quoted 3 lines

Link https://github.com/acpica/acpica/commit/cac67909

Signed-off-by Lv Zheng <[email protected]>

Signed-off-by Bob Moore <[email protected]>

This commit in -next (071b39575679 ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel) causes a regression in my nfit/nvdimm test environment. The nfit produced by QEMU no longer results in a nvdimm bus being created.

Quoted 7 lines

Quoted 3 lines

I have not root caused it, but I'm using the following command line options to create an nfit in qemu-2.6. Reverting the commit leads compile failures.

Would the build problems go away if you reverted "ACPICA: Tables:
Allow FADT to be customized with virtual address" (linux-next commit cf334d3174f9) in addition to it?

Yes, reverting those two commits gets me back to a functional environment:

Quoted 8 lines

Quoted 3 lines

Revert "ACPICA: Tables: Allow FADT to be customized with virtual address" Revert "ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_un

To Dan:
It seems in drivers/acpi/nfit/core.c.
The returned table size is used by the NFIT code. I think it should be changed to use table_header->length.

Does the acpi core already validate that table_header->length is correct? i.e. is is possible that a broken implementation could have the wrong length in the header? I was assuming that was the purpose of the _with_size(), but maybe I was wrong?

--
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

From: [email protected] [mailto:[email protected]] On Behalf Of Dan Williams
Subject: Re: [PATCH 08/11] ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel

Quoted 76 lines · Lv

Hi, Rafael and Dan

Quoted 74 lines

From: Dan Williams [mailto:[email protected]]
Subject: Re: [PATCH 08/11] ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel

Quoted 59 lines · Rafael J. Wysocki

Quoted 58 lines · Dan Williams

Quoted 48 lines · Lv Zheng

ACPICA commit cac6790954d4d752a083e6122220b8a22febcd07

This patch back ports Linux acpi_get_table_with_size() and early_acpi_os_unmap_memory() into ACPICA upstream to reduce divergences.

The 2 APIs are used by Linux as table management APIs for long time, it contains a hidden logic that during the early stage, the mapped tables should be unmapped before the early stage ends.

During the early stage, tables are handled by the following sequence:
acpi_get_table_with_size();
parse the table
early_acpi_os_unmap_memory();
During the late stage, tables are handled by the following sequence:
acpi_get_table();
parse the table
Linux uses acpi_gbl_permanent_mmap to distinguish the early stage and the late stage.

The reasoning of introducing acpi_get_table_with_size() is: ACPICA will remember the early mapped pointer in acpi_get_table() and Linux isn't able to prevent ACPICA from using the wrong early mapped pointer during the late stage as there is no API provided from ACPICA to be an inverse of acpi_get_table() to forget the early mapped pointer.

But how ACPICA can work with the early/late stage requirement? Inside of ACPICA, tables are ensured to be remained in "INSTALLED" state during the early stage, and they are carefully not transitioned to "VALIDATED" state until the late stage. So the same logic is in fact implemented inside of ACPICA in a different way. The gap is only that the feature is not provided to the OSPMs in an accessible external API style.

It then is possible to fix the gap by providing an inverse of acpi_get_table() from ACPICA, so that the two Linux sequences can be combined:
acpi_get_table();
parse the table
acpi_put_table();
In order to work easier with the current Linux code, acpi_get_table() and acpi_put_table() is implemented in a usage counting based style:
1. When the usage count of the table is increased from 0 to 1, table is

    mapped and .Pointer is set with the mapping address (VALIDATED);

2. When the usage count of the table is decreased from 1 to 0, .Pointer is unset and the mapping address is unmapped (INVALIDATED).
So that we can deploy the new APIs to Linux with minimal effort by just invoking acpi_get_table() in acpi_get_table_with_size() and invoking acpi_put_table() in early_acpi_os_unmap_memory(). Lv Zheng.

Quoted 3 lines

Link https://github.com/acpica/acpica/commit/cac67909

Signed-off-by Lv Zheng <[email protected]>

Signed-off-by Bob Moore <[email protected]>

This commit in -next (071b39575679 ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel) causes a regression in my nfit/nvdimm test environment. The nfit produced by QEMU no longer results in a nvdimm bus being created.

Quoted 7 lines

Quoted 3 lines

I have not root caused it, but I'm using the following command line options to create an nfit in qemu-2.6. Reverting the commit leads compile failures.

Would the build problems go away if you reverted "ACPICA: Tables:
Allow FADT to be customized with virtual address" (linux-next commit cf334d3174f9) in addition to it?

Yes, reverting those two commits gets me back to a functional environment:

Quoted 8 lines

Quoted 3 lines

Revert "ACPICA: Tables: Allow FADT to be customized with virtual address" Revert "ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_un

To Dan:
It seems in drivers/acpi/nfit/core.c.
The returned table size is used by the NFIT code. I think it should be changed to use table_header->length.

Does the acpi core already validate that table_header->length is correct? i.e. is is possible that a broken implementation could have the wrong length in the header? I was assuming that was the purpose of the _with_size(), but maybe I was wrong?

--
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 and Dan

Quoted 72 lines

From: Dan Williams [mailto:[email protected]]
Subject: Re: [PATCH 08/11] ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel

Quoted 59 lines · Rafael J. Wysocki

Quoted 58 lines · Dan Williams

Quoted 48 lines · Lv Zheng

ACPICA commit cac6790954d4d752a083e6122220b8a22febcd07

This patch back ports Linux acpi_get_table_with_size() and early_acpi_os_unmap_memory() into ACPICA upstream to reduce divergences.

The 2 APIs are used by Linux as table management APIs for long time, it contains a hidden logic that during the early stage, the mapped tables should be unmapped before the early stage ends.

During the early stage, tables are handled by the following sequence:
acpi_get_table_with_size();
parse the table
early_acpi_os_unmap_memory();
During the late stage, tables are handled by the following sequence:
acpi_get_table();
parse the table
Linux uses acpi_gbl_permanent_mmap to distinguish the early stage and the late stage.

The reasoning of introducing acpi_get_table_with_size() is: ACPICA will remember the early mapped pointer in acpi_get_table() and Linux isn't able to prevent ACPICA from using the wrong early mapped pointer during the late stage as there is no API provided from ACPICA to be an inverse of acpi_get_table() to forget the early mapped pointer.

But how ACPICA can work with the early/late stage requirement? Inside of ACPICA, tables are ensured to be remained in "INSTALLED" state during the early stage, and they are carefully not transitioned to "VALIDATED" state until the late stage. So the same logic is in fact implemented inside of ACPICA in a different way. The gap is only that the feature is not provided to the OSPMs in an accessible external API style.

It then is possible to fix the gap by providing an inverse of acpi_get_table() from ACPICA, so that the two Linux sequences can be combined:
acpi_get_table();
parse the table
acpi_put_table();
In order to work easier with the current Linux code, acpi_get_table() and acpi_put_table() is implemented in a usage counting based style:
1. When the usage count of the table is increased from 0 to 1, table is

    mapped and .Pointer is set with the mapping address (VALIDATED);

2. When the usage count of the table is decreased from 1 to 0, .Pointer is unset and the mapping address is unmapped (INVALIDATED).
So that we can deploy the new APIs to Linux with minimal effort by just invoking acpi_get_table() in acpi_get_table_with_size() and invoking acpi_put_table() in early_acpi_os_unmap_memory(). Lv Zheng.

Quoted 3 lines

Link https://github.com/acpica/acpica/commit/cac67909

Signed-off-by Lv Zheng <[email protected]>

Signed-off-by Bob Moore <[email protected]>

This commit in -next (071b39575679 ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel) causes a regression in my nfit/nvdimm test environment. The nfit produced by QEMU no longer results in a nvdimm bus being created.

Quoted 7 lines

Quoted 3 lines

I have not root caused it, but I'm using the following command line options to create an nfit in qemu-2.6. Reverting the commit leads compile failures.

Would the build problems go away if you reverted "ACPICA: Tables:
Allow FADT to be customized with virtual address" (linux-next commit cf334d3174f9) in addition to it?

Quoted 5 lines

Yes, reverting those two commits gets me back to a functional environment:

Revert "ACPICA: Tables: Allow FADT to be customized with virtual address" Revert "ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_un

To Dan:
It seems in drivers/acpi/nfit/core.c.
The returned table size is used by the NFIT code. I think it should be changed to use table_header->length.

To Rafael:
I can offer a quick fix for this by returning table_header->length from acpi_get_table_with_size().

From: [email protected] [mailto:[email protected]] On Behalf Of Rafael J. Wysocki
Subject: Re: [PATCH 08/11] ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel

Quoted 80 lines · Lv

Hi, Rafael and Dan

Quoted 72 lines

From: Dan Williams [mailto:[email protected]]
Subject: Re: [PATCH 08/11] ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel

Quoted 59 lines · Rafael J. Wysocki

Quoted 58 lines · Dan Williams

Quoted 48 lines · Lv Zheng

ACPICA commit cac6790954d4d752a083e6122220b8a22febcd07

This patch back ports Linux acpi_get_table_with_size() and early_acpi_os_unmap_memory() into ACPICA upstream to reduce divergences.

The 2 APIs are used by Linux as table management APIs for long time, it contains a hidden logic that during the early stage, the mapped tables should be unmapped before the early stage ends.

During the early stage, tables are handled by the following sequence:
acpi_get_table_with_size();
parse the table
early_acpi_os_unmap_memory();
During the late stage, tables are handled by the following sequence:
acpi_get_table();
parse the table
Linux uses acpi_gbl_permanent_mmap to distinguish the early stage and the late stage.

The reasoning of introducing acpi_get_table_with_size() is: ACPICA will remember the early mapped pointer in acpi_get_table() and Linux isn't able to prevent ACPICA from using the wrong early mapped pointer during the late stage as there is no API provided from ACPICA to be an inverse of acpi_get_table() to forget the early mapped pointer.

But how ACPICA can work with the early/late stage requirement? Inside of ACPICA, tables are ensured to be remained in "INSTALLED" state during the early stage, and they are carefully not transitioned to "VALIDATED" state until the late stage. So the same logic is in fact implemented inside of ACPICA in a different way. The gap is only that the feature is not provided to the OSPMs in an accessible external API style.

It then is possible to fix the gap by providing an inverse of acpi_get_table() from ACPICA, so that the two Linux sequences can be combined:
acpi_get_table();
parse the table
acpi_put_table();
In order to work easier with the current Linux code, acpi_get_table() and acpi_put_table() is implemented in a usage counting based style:
1. When the usage count of the table is increased from 0 to 1, table is

    mapped and .Pointer is set with the mapping address (VALIDATED);

2. When the usage count of the table is decreased from 1 to 0, .Pointer is unset and the mapping address is unmapped (INVALIDATED).
So that we can deploy the new APIs to Linux with minimal effort by just invoking acpi_get_table() in acpi_get_table_with_size() and invoking acpi_put_table() in early_acpi_os_unmap_memory(). Lv Zheng.

Quoted 3 lines

Link https://github.com/acpica/acpica/commit/cac67909

Signed-off-by Lv Zheng <[email protected]>

Signed-off-by Bob Moore <[email protected]>

This commit in -next (071b39575679 ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel) causes a regression in my nfit/nvdimm test environment. The nfit produced by QEMU no longer results in a nvdimm bus being created.

Quoted 7 lines

Quoted 3 lines

I have not root caused it, but I'm using the following command line options to create an nfit in qemu-2.6. Reverting the commit leads compile failures.

Would the build problems go away if you reverted "ACPICA: Tables:
Allow FADT to be customized with virtual address" (linux-next commit cf334d3174f9) in addition to it?

Quoted 5 lines

Yes, reverting those two commits gets me back to a functional environment:

Revert "ACPICA: Tables: Allow FADT to be customized with virtual address" Revert "ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_un

Quoted 7 lines

To Dan:
It seems in drivers/acpi/nfit/core.c.
The returned table size is used by the NFIT code. I think it should be changed to use table_header->length.

To Rafael:
I can offer a quick fix for this by returning table_header->length from acpi_get_table_with_size().

OK

I've dropped the problematic ACPICA commit for now (along with the one that depended on it) to prevent the issue from going in.

The change that you are suggesting would defeat the purpose of what the NFIT code does.

From: [email protected] [mailto:[email protected]] On Behalf Of Dan Williams
Subject: Re: [PATCH 08/11] ACPICA: Tables: Back port acpi_get_table_with_size() and
early_acpi_os_unmap_memory() from Linux kernel

Quoted 48 lines · Lv Zheng

ACPICA commit cac6790954d4d752a083e6122220b8a22febcd07

This patch back ports Linux acpi_get_table_with_size() and early_acpi_os_unmap_memory() into ACPICA upstream to reduce divergences.

The 2 APIs are used by Linux as table management APIs for long time, it contains a hidden logic that during the early stage, the mapped tables should be unmapped before the early stage ends.

During the early stage, tables are handled by the following sequence:
acpi_get_table_with_size();
parse the table
early_acpi_os_unmap_memory();
During the late stage, tables are handled by the following sequence:
acpi_get_table();
parse the table
Linux uses acpi_gbl_permanent_mmap to distinguish the early stage and the late stage.

The reasoning of introducing acpi_get_table_with_size() is: ACPICA will remember the early mapped pointer in acpi_get_table() and Linux isn't able to prevent ACPICA from using the wrong early mapped pointer during the late stage as there is no API provided from ACPICA to be an inverse of acpi_get_table() to forget the early mapped pointer.

But how ACPICA can work with the early/late stage requirement? Inside of ACPICA, tables are ensured to be remained in "INSTALLED" state during the early stage, and they are carefully not transitioned to "VALIDATED" state until the late stage. So the same logic is in fact implemented inside of ACPICA in a different way. The gap is only that the feature is not provided to the OSPMs in an accessible external API style.

It then is possible to fix the gap by providing an inverse of acpi_get_table() from ACPICA, so that the two Linux sequences can be combined:
acpi_get_table();
parse the table
acpi_put_table();
In order to work easier with the current Linux code, acpi_get_table() and acpi_put_table() is implemented in a usage counting based style:
1. When the usage count of the table is increased from 0 to 1, table is

    mapped and .Pointer is set with the mapping address (VALIDATED);

2. When the usage count of the table is decreased from 1 to 0, .Pointer is unset and the mapping address is unmapped (INVALIDATED).
So that we can deploy the new APIs to Linux with minimal effort by just invoking acpi_get_table() in acpi_get_table_with_size() and invoking acpi_put_table() in early_acpi_os_unmap_memory(). Lv Zheng.

Quoted 3 lines

Link https://github.com/acpica/acpica/commit/cac67909

Signed-off-by Lv Zheng <[email protected]>

Signed-off-by Bob Moore <[email protected]>

This commit in -next (071b39575679 ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel) causes a regression in my nfit/nvdimm test environment. The nfit produced by QEMU no longer results in a nvdimm bus being created.

I have not root caused it, but I'm using the following command line options to create an nfit in qemu-2.6. Reverting the commit leads compile failures.

qemu=$HOME/git/qemu/build/x86_64-softmmu/qemu-system-x86_64 mem=$HOME/mem
label_size=$((128*1024)) mem_size=$(((3*1024*1024*1024) + (64 * 1024 *1024))) IMAGE=$HOME/ahci.img

kvm=(

        $qemu
        -enable-kvm
        -cpu kvm64
        -kernel $kernel
        -initrd $initrd
        -m 12G,slots=3,maxmem=40G

        -machine pc-i440fx-2.4,accel=kvm,usb=off,vmport=off,nvdimm
        -cpu SandyBridge
        -smp 2
        -netdev tap,id=hostnet0,ifname=tap0,script=no,downscript=no
        -device

virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:b7:a1:ad,bus=pci.0,addr=0x7
-object
memory-backend-file,id=mem1,share,mem-path=${mem},size=$((label_size + mem_size))

        -device nvdimm,memdev=mem1,id=nv1,label-size=${label_size}
        -device ahci,id=sata0,bus=pci.0,addr=0x8
        -drive file=$IMAGE,if=none,id=drive-sata0-0-0,format=raw
        -device ide-hd,bus=sata0.0,drive=drive-sata0-0-0,id=sata0-0-0
        -boot order=nc
        -no-reboot
        -watchdog i6300esb
        -rtc base=localtime
        -serial stdio
        -display none
        -monitor null

)

Hi, Dan

Good to see you here!

Quoted 53 lines

From: [email protected] [mailto:[email protected]] On Behalf Of Dan Williams
Subject: Re: [PATCH 08/11] ACPICA: Tables: Back port acpi_get_table_with_size() and
early_acpi_os_unmap_memory() from Linux kernel

Quoted 48 lines · Lv Zheng

ACPICA commit cac6790954d4d752a083e6122220b8a22febcd07

This patch back ports Linux acpi_get_table_with_size() and early_acpi_os_unmap_memory() into ACPICA upstream to reduce divergences.

The 2 APIs are used by Linux as table management APIs for long time, it contains a hidden logic that during the early stage, the mapped tables should be unmapped before the early stage ends.

During the early stage, tables are handled by the following sequence:
acpi_get_table_with_size();
parse the table
early_acpi_os_unmap_memory();
During the late stage, tables are handled by the following sequence:
acpi_get_table();
parse the table
Linux uses acpi_gbl_permanent_mmap to distinguish the early stage and the late stage.

The reasoning of introducing acpi_get_table_with_size() is: ACPICA will remember the early mapped pointer in acpi_get_table() and Linux isn't able to prevent ACPICA from using the wrong early mapped pointer during the late stage as there is no API provided from ACPICA to be an inverse of acpi_get_table() to forget the early mapped pointer.

But how ACPICA can work with the early/late stage requirement? Inside of ACPICA, tables are ensured to be remained in "INSTALLED" state during the early stage, and they are carefully not transitioned to "VALIDATED" state until the late stage. So the same logic is in fact implemented inside of ACPICA in a different way. The gap is only that the feature is not provided to the OSPMs in an accessible external API style.

It then is possible to fix the gap by providing an inverse of acpi_get_table() from ACPICA, so that the two Linux sequences can be combined:
acpi_get_table();
parse the table
acpi_put_table();
In order to work easier with the current Linux code, acpi_get_table() and acpi_put_table() is implemented in a usage counting based style:
1. When the usage count of the table is increased from 0 to 1, table is

    mapped and .Pointer is set with the mapping address (VALIDATED);

2. When the usage count of the table is decreased from 1 to 0, .Pointer is unset and the mapping address is unmapped (INVALIDATED).
So that we can deploy the new APIs to Linux with minimal effort by just invoking acpi_get_table() in acpi_get_table_with_size() and invoking acpi_put_table() in early_acpi_os_unmap_memory(). Lv Zheng.

Quoted 8 lines

Quoted 3 lines

Link https://github.com/acpica/acpica/commit/cac67909

Signed-off-by Lv Zheng <[email protected]>

Signed-off-by Bob Moore <[email protected]>

This commit in -next (071b39575679 ACPICA: Tables: Back port acpi_get_table_with_size() and early_acpi_os_unmap_memory() from Linux kernel) causes a regression in my nfit/nvdimm test environment. The nfit produced by QEMU no longer results in a nvdimm bus being created.

This commit is almost a no-op, unless some code in kernel is using the length field returned by old acpi_get_table_with_size().

Quoted 39 lines

I have not root caused it, but I'm using the following command line options to create an nfit in qemu-2.6. Reverting the commit leads compile failures.

qemu=$HOME/git/qemu/build/x86_64-softmmu/qemu-system-x86_64 mem=$HOME/mem
label_size=$((128*1024)) mem_size=$(((3*1024*1024*1024) + (64 * 1024 *1024))) IMAGE=$HOME/ahci.img

kvm=(

        $qemu
        -enable-kvm
        -cpu kvm64
        -kernel $kernel
        -initrd $initrd
        -m 12G,slots=3,maxmem=40G

        -machine pc-i440fx-2.4,accel=kvm,usb=off,vmport=off,nvdimm
        -cpu SandyBridge
        -smp 2
        -netdev tap,id=hostnet0,ifname=tap0,script=no,downscript=no
        -device

virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:b7:a1:ad,bus=pci.0,addr=0x7
-object
memory-backend-file,id=mem1,share,mem-path=${mem},size=$((label_size + mem_size))

        -device nvdimm,memdev=mem1,id=nv1,label-size=${label_size}
        -device ahci,id=sata0,bus=pci.0,addr=0x8
        -drive file=$IMAGE,if=none,id=drive-sata0-0-0,format=raw
        -device ide-hd,bus=sata0.0,drive=drive-sata0-0-0,id=sata0-0-0
        -boot order=nc
        -no-reboot
        -watchdog i6300esb
        -rtc base=localtime
        -serial stdio
        -display none
        -monitor null

Quoted 1 line

)

Let me file a kernel Bugzilla bug to track this issue:
https://bugzilla.kernel.org/show_bug.cgi?id=189891
And see if we can quickly fix it.

Could you also point me the NFIT code that I should take a look at. Thanks in advance.

The returned size is still used by the drivers.

Reported-by Dan Williams <[email protected]>

Cc Dan Williams <[email protected]>

Signed-off-by Lv Zheng <[email protected]>

---
 drivers/acpi/osl.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/acpi/osl.c +0 -2

index 5bef0f65..adf1ec4 100644
@@ -445,8 +445,12 @@ void __ref acpi_os_unmap_memory(void *virt, acpi_size size)
 
        status = acpi_get_table(signature, instance, out_table);
        if (ACPI_SUCCESS(status)) {
-               /* No longer used by early_acpi_os_unmap_memory() */
-               *tbl_size = 0;

From: [email protected] [mailto:[email protected]] On Behalf Of Rafael J. Wysocki
Subject: Re: [PATCH] ACPI / OSL: Fix a regression by returning table size via acpi_get_table_with_size()

Quoted 27 lines · Lv Zheng

The returned size is still used by the drivers.

Reported-by Dan Williams <[email protected]>

Cc Dan Williams <[email protected]>

Signed-off-by Lv Zheng <[email protected]>

---
 drivers/acpi/osl.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/acpi/osl.c +6 -2

index 5bef0f65..adf1ec4 100644
@@ -445,8 +445,12 @@ void __ref acpi_os_unmap_memory(void *virt, acpi_size size)
 
        status = acpi_get_table(signature, instance, out_table);
        if (ACPI_SUCCESS(status)) {
-               /* No longer used by early_acpi_os_unmap_memory() */
-               *tbl_size = 0;
+               /*
+                * No longer used by early_acpi_os_unmap_memory(), but still
+                * used by the ACPI table drivers.
+                */
+               if (*out_table)
+                       *tbl_size = (*out_table)->length;
        }
 

drivers/acpi/osl.c void __ref acpi_os_unmap_memory(void *virt, acpi_size size)

Quoted 2 lines

drivers/acpi/osl.c +0 -1

        return (status);
--

The changelog doesn't explain anything. Please say (a) what the problem is and (b) how it is being addressed by your patch.

The following wrong commit triggers issues in acpi_get_table_with_size() users:
Subject: ACPICA: Tables: Back port acpi_get_table_with_size() and
early_acpi_os_unmap_memory() from Linux kernel

The function is invented to be a replacement of acpi_get_table() during early stage so that the early mapped pointer will not be stored in ACPICA core and thus the late stage acpi_get_table() won't return a wrong pointer. However the mapping size is returned just because it is required by early_acpi_os_unmap_memory() to unmap the pointer during early stage.

As the mapping size equals to the acpi_table_header.length (see acpi_tb_init_table_descriptor() and acpi_tb_validate_table()), when such a convenient result is returned, driver code will start to use it instead of accessing acpi_table_header to obtain the length.

So the commit can trigger problems in such drivers as it returns 0 now. And it did bring a trouble to drivers/acpi/nfit/core.c. So before cleaning up the drivers, we should keep the old semantics of the API.

This patch fixes the wrong commit by returning the acpi_table_header.length from acpi_get_table_with_size().

Reported-by Dan Williams <[email protected]>

Cc Dan Williams <[email protected]>

Signed-off-by Lv Zheng <[email protected]>

---
 drivers/acpi/osl.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

drivers/acpi/osl.c +10 -3

index 5bef0f65..d0b273f 100644
@@ -445,8 +445,16 @@ void __ref acpi_os_unmap_memory(void *virt, acpi_size size)
 
        status = acpi_get_table(signature, instance, out_table);
        if (ACPI_SUCCESS(status)) {
-               /* No longer used by early_acpi_os_unmap_memory() */
-               *tbl_size = 0;
+               /*
+                * "tbl_size" is no longer used by
+                * early_acpi_os_unmap_memory(), but is still used by the
+                * ACPI table drivers. So sets it to the length of the
+                * table when the tbl_size is requested.
+                * "out_table" is not sanity checked as AE_BAD_PARAMETER
+                * is returned if it is NULL.
+                */
+               if (tbl_size && *out_table)
+                       *tbl_size = (*out_table)->length;
        }
 
        return (status);
--

1.7.10

From: [email protected] [mailto:[email protected]] On Behalf Of Rafael J. Wysocki
Subject: Re: [PATCH v2] ACPI / OSL: Fix a regression by returning acpi_table_header.length instead of 0 from acpi_get_table_with_size()

Quoted 16 lines · Lv Zheng

The following wrong commit triggers issues in acpi_get_table_with_size() users:
Subject: ACPICA: Tables: Back port acpi_get_table_with_size() and
early_acpi_os_unmap_memory() from Linux kernel

The function is invented to be a replacement of acpi_get_table() during early stage so that the early mapped pointer will not be stored in ACPICA core and thus the late stage acpi_get_table() won't return a wrong pointer. However the mapping size is returned just because it is required by early_acpi_os_unmap_memory() to unmap the pointer during early stage.

As the mapping size equals to the acpi_table_header.length (see acpi_tb_init_table_descriptor() and acpi_tb_validate_table()), when such a convenient result is returned, driver code will start to use it instead of accessing acpi_table_header to obtain the length.

Quoted 6 lines

So the commit can trigger problems in such drivers as it returns 0 now. And it did bring a trouble to drivers/acpi/nfit/core.c. So before cleaning up the drivers, we should keep the old semantics of the API.

This patch fixes the wrong commit by returning the acpi_table_header.length from acpi_get_table_with_size().

Well, the way this works isn't particularly straightforward.

Wouldn't it be better to simply drop the check (if it is redundant) from the NFIT driver?

Quoted 32 lines

Reported-by Dan Williams <[email protected]>

Cc Dan Williams <[email protected]>

Signed-off-by Lv Zheng <[email protected]>

---
 drivers/acpi/osl.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

drivers/acpi/osl.c +10 -3

index 5bef0f65..d0b273f 100644
@@ -445,8 +445,16 @@ void __ref acpi_os_unmap_memory(void *virt, acpi_size size)
 
        status = acpi_get_table(signature, instance, out_table);
        if (ACPI_SUCCESS(status)) {
-               /* No longer used by early_acpi_os_unmap_memory() */
-               *tbl_size = 0;
+               /*
+                * "tbl_size" is no longer used by
+                * early_acpi_os_unmap_memory(), but is still used by the
+                * ACPI table drivers. So sets it to the length of the
+                * table when the tbl_size is requested.
+                * "out_table" is not sanity checked as AE_BAD_PARAMETER
+                * is returned if it is NULL.
+                */
+               if (tbl_size && *out_table)
+                       *tbl_size = (*out_table)->length;
        }
 
        return (status);
--

Quoted 2 lines

1.7.10

Thanks,
Rafael

Hi,

Quoted 25 lines

From: [email protected] [mailto:[email protected]] On Behalf Of Rafael J. Wysocki
Subject: Re: [PATCH v2] ACPI / OSL: Fix a regression by returning acpi_table_header.length instead of 0 from acpi_get_table_with_size()

Quoted 20 lines · Lv Zheng

The following wrong commit triggers issues in acpi_get_table_with_size() users:
Subject: ACPICA: Tables: Back port acpi_get_table_with_size() and
early_acpi_os_unmap_memory() from Linux kernel

The function is invented to be a replacement of acpi_get_table() during early stage so that the early mapped pointer will not be stored in ACPICA core and thus the late stage acpi_get_table() won't return a wrong pointer. However the mapping size is returned just because it is required by early_acpi_os_unmap_memory() to unmap the pointer during early stage.

As the mapping size equals to the acpi_table_header.length (see acpi_tb_init_table_descriptor() and acpi_tb_validate_table()), when such a convenient result is returned, driver code will start to use it instead of accessing acpi_table_header to obtain the length.

So the commit can trigger problems in such drivers as it returns 0 now. And it did bring a trouble to drivers/acpi/nfit/core.c. So before cleaning up the drivers, we should keep the old semantics of the API.

Quoted 4 lines

Quoted 2 lines

This patch fixes the wrong commit by returning the acpi_table_header.length from acpi_get_table_with_size().

Well, the way this works isn't particularly straightforward.

I don't know what's your preference here?
Shall we drop the 2 wrong commits, and merge fixed ones to replace them in linux-pm/linux-next?

Or can we just merge this fix patch on top of the wrong commits?

Quoted 3 lines

Wouldn't it be better to simply drop the check (if it is redundant) from the NFIT driver?

At this point, I'm not sure if NFIT is the only driver broken by the wrong commit.

My plan was:
1. Keep old behavior working by still providing acpi_get_table_with_size()/early_acpi_os_unmap_memory();

2. Use a set of patches to cleanup acpi_get_table_with_size()/early_acpi_os_unmap_memory() users, So that we can carefully examine every caller, split strange use cases into separate commits to avoid regressions;
3. Remove acpi_get_table_with_size() and early_acpi_os_unmap_memory() if there is no driver using them.