ACPICA: Save current masks of enabled GPEs after enable register writes

There is a race condition between acpi_hw_disable_all_gpes() or
acpi_enable_all_wakeup_gpes() and acpi_ev_asynch_enable_gpe() such
that if the latter wins the race, it may mistakenly enable a GPE
disabled by the former.  This may lead to premature system wakeups
during system suspend and potentially to more serious consequences.

The source of the problem is how acpi_hw_low_set_gpe() works when
passed ACPI_GPE_CONDITIONAL_ENABLE as the second argument.  In that
case, the GPE will be enabled if the corresponding bit is set in the
enable_for_run mask of the GPE enable register containing that bit.
However, acpi_hw_disable_all_gpes() and acpi_enable_all_wakeup_gpes()
don't modify the enable_for_run masks of GPE registers when writing
to them.  In consequence, if acpi_ev_asynch_enable_gpe(), which
eventually calls acpi_hw_low_set_gpe() with the second argument
equal to ACPI_GPE_CONDITIONAL_ENABLE, is executed in parallel with
one of these functions, it may reverse changes made by them.

To fix the problem, introduce a new enable_mask field in struct
acpi_gpe_register_info in which to store the current mask of
enabled GPEs and modify acpi_hw_low_set_gpe() to take this
mask into account instead of enable_for_run when its second
argument is equal to ACPI_GPE_CONDITIONAL_ENABLE.  Also modify
the low-level routines called by acpi_hw_disable_all_gpes(),
acpi_enable_all_wakeup_gpes() and acpi_enable_all_runtime_gpes()
to update the enable_mask masks of GPE registers after all
(successful) writes to those registers.

Acked-by: Lv Zheng <lv.zheng@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
index 80c74e90f3ccc93260c395bc5e7dde267c735f54..680d23bbae7c4ec293df73a01966a29a345fb6ce 100644 (file)
--- a/drivers/acpi/acpica/aclocal.h
+++ b/drivers/acpi/acpica/aclocal.h
@@ -454,6 +454,7 @@ struct acpi_gpe_register_info {
        u16 base_gpe_number;    /* Base GPE number for this register */
        u8 enable_for_wake;     /* GPEs to keep enabled when sleeping */
        u8 enable_for_run;      /* GPEs to keep enabled when running */
+       u8 enable_mask;         /* Current mask of enabled GPEs */
 };
 
 /*

diff --git a/drivers/acpi/acpica/evgpe.c b/drivers/acpi/acpica/evgpe.c
index 2095dfb72bcb3c9fd7a48512c3024d58e09e6c76..81db932b15ee811125175a9a1041dc7c3933648c 100644 (file)
--- a/drivers/acpi/acpica/evgpe.c
+++ b/drivers/acpi/acpica/evgpe.c
@@ -134,7 +134,7 @@ acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info)
 
        /* Enable the requested GPE */
 
-       status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE);
+       status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE_SAVE);
        return_ACPI_STATUS(status);
 }
 
@@ -213,7 +213,7 @@ acpi_ev_remove_gpe_reference(struct acpi_gpe_event_info *gpe_event_info)
                if (ACPI_SUCCESS(status)) {
                        status =
                            acpi_hw_low_set_gpe(gpe_event_info,
-                                               ACPI_GPE_DISABLE);
+                                               ACPI_GPE_DISABLE_SAVE);
                }
 
                if (ACPI_FAILURE(status)) {
@@ -655,7 +655,7 @@ acpi_status acpi_ev_finish_gpe(struct acpi_gpe_event_info * gpe_event_info)
 
        /*
         * Enable this GPE, conditionally. This means that the GPE will
-        * only be physically enabled if the enable_for_run bit is set
+        * only be physically enabled if the enable_mask bit is set
         * in the event_info.
         */
        (void)acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_CONDITIONAL_ENABLE);

diff --git a/drivers/acpi/acpica/hwgpe.c b/drivers/acpi/acpica/hwgpe.c
index 48ac7b7b59cdcf7edf954897d1ec1336dd0534bb..494027f5c06771d7b97472a8222f5ed5716b0cb8 100644 (file)
--- a/drivers/acpi/acpica/hwgpe.c
+++ b/drivers/acpi/acpica/hwgpe.c
@@ -115,12 +115,12 @@ acpi_hw_low_set_gpe(struct acpi_gpe_event_info *gpe_event_info, u32 action)
        /* Set or clear just the bit that corresponds to this GPE */
 
        register_bit = acpi_hw_get_gpe_register_bit(gpe_event_info);
-       switch (action) {
+       switch (action & ~ACPI_GPE_SAVE_MASK) {
        case ACPI_GPE_CONDITIONAL_ENABLE:
 
-               /* Only enable if the enable_for_run bit is set */
+               /* Only enable if the corresponding enable_mask bit is set */
 
-               if (!(register_bit & gpe_register_info->enable_for_run)) {
+               if (!(register_bit & gpe_register_info->enable_mask)) {
                        return (AE_BAD_PARAMETER);
                }
 
@@ -145,6 +145,9 @@ acpi_hw_low_set_gpe(struct acpi_gpe_event_info *gpe_event_info, u32 action)
        /* Write the updated enable mask */
 
        status = acpi_hw_write(enable_mask, &gpe_register_info->enable_address);
+       if (ACPI_SUCCESS(status) && (action & ACPI_GPE_SAVE_MASK)) {
+               gpe_register_info->enable_mask = enable_mask;
+       }
        return (status);
 }
 
@@ -260,6 +263,32 @@ acpi_hw_get_gpe_status(struct acpi_gpe_event_info * gpe_event_info,
        return (AE_OK);
 }
 
+/******************************************************************************
+ *
+ * FUNCTION:    acpi_hw_gpe_enable_write
+ *
+ * PARAMETERS:  enable_mask         - Bit mask to write to the GPE register
+ *              gpe_register_info   - Gpe Register info
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Write the enable mask byte to the given GPE register.
+ *
+ ******************************************************************************/
+
+static acpi_status
+acpi_hw_gpe_enable_write(u8 enable_mask,
+                        struct acpi_gpe_register_info *gpe_register_info)
+{
+       acpi_status status;
+
+       status = acpi_hw_write(enable_mask, &gpe_register_info->enable_address);
+       if (ACPI_SUCCESS(status)) {
+               gpe_register_info->enable_mask = enable_mask;
+       }
+       return (status);
+}
+
 /******************************************************************************
  *
  * FUNCTION:    acpi_hw_disable_gpe_block
@@ -287,8 +316,8 @@ acpi_hw_disable_gpe_block(struct acpi_gpe_xrupt_info *gpe_xrupt_info,
                /* Disable all GPEs in this register */
 
                status =
-                   acpi_hw_write(0x00,
-                                 &gpe_block->register_info[i].enable_address);
+                   acpi_hw_gpe_enable_write(0x00,
+                                            &gpe_block->register_info[i]);
                if (ACPI_FAILURE(status)) {
                        return (status);
                }
@@ -355,21 +384,23 @@ acpi_hw_enable_runtime_gpe_block(struct acpi_gpe_xrupt_info *gpe_xrupt_info,
 {
        u32 i;
        acpi_status status;
+       struct acpi_gpe_register_info *gpe_register_info;
 
        /* NOTE: assumes that all GPEs are currently disabled */
 
        /* Examine each GPE Register within the block */
 
        for (i = 0; i < gpe_block->register_count; i++) {
-               if (!gpe_block->register_info[i].enable_for_run) {
+               gpe_register_info = &gpe_block->register_info[i];
+               if (!gpe_register_info->enable_for_run) {
                        continue;
                }
 
                /* Enable all "runtime" GPEs in this register */
 
                status =
-                   acpi_hw_write(gpe_block->register_info[i].enable_for_run,
-                                 &gpe_block->register_info[i].enable_address);
+                   acpi_hw_gpe_enable_write(gpe_register_info->enable_for_run,
+                                            gpe_register_info);
                if (ACPI_FAILURE(status)) {
                        return (status);
                }
@@ -399,10 +430,12 @@ acpi_hw_enable_wakeup_gpe_block(struct acpi_gpe_xrupt_info *gpe_xrupt_info,
 {
        u32 i;
        acpi_status status;
+       struct acpi_gpe_register_info *gpe_register_info;
 
        /* Examine each GPE Register within the block */
 
        for (i = 0; i < gpe_block->register_count; i++) {
+               gpe_register_info = &gpe_block->register_info[i];
 
                /*
                 * Enable all "wake" GPEs in this register and disable the
@@ -410,8 +443,8 @@ acpi_hw_enable_wakeup_gpe_block(struct acpi_gpe_xrupt_info *gpe_xrupt_info,
                 */
 
                status =
-                   acpi_hw_write(gpe_block->register_info[i].enable_for_wake,
-                                 &gpe_block->register_info[i].enable_address);
+                   acpi_hw_gpe_enable_write(gpe_register_info->enable_for_wake,
+                                            gpe_register_info);
                if (ACPI_FAILURE(status)) {
                        return (status);
                }

diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index 7000e66f768eabd9893484d22b7b86055b8c471f..bbef17368e49fb65c0870f54ab59e388a76e4e62 100644 (file)
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -736,6 +736,10 @@ typedef u32 acpi_event_status;
 #define ACPI_GPE_ENABLE                 0
 #define ACPI_GPE_DISABLE                1
 #define ACPI_GPE_CONDITIONAL_ENABLE     2
+#define ACPI_GPE_SAVE_MASK              4
+
+#define ACPI_GPE_ENABLE_SAVE            (ACPI_GPE_ENABLE | ACPI_GPE_SAVE_MASK)
+#define ACPI_GPE_DISABLE_SAVE           (ACPI_GPE_DISABLE | ACPI_GPE_SAVE_MASK)
 
 /*
  * GPE info flags - Per GPE