* [PATCH 0/2] ACPI / PM: Fixes related to GPE race conditions
@ 2014-12-01 1:52 Rafael J. Wysocki
2014-12-01 1:53 ` [PATCH 1/2] ACPICA: Save current masks of enabled GPEs after enable register writes Rafael J. Wysocki
2014-12-01 1:54 ` [PATCH 2/2][Resend] ACPI / sleep: Drain outstanding events after disabling multiple GPEs Rafael J. Wysocki
0 siblings, 2 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2014-12-01 1:52 UTC (permalink / raw)
To: ACPI Devel Maling List; +Cc: Robert Moore, Lv Zheng, Linux PM list
Hi,
The following two patches fix race conditions related to ACPI GPEs and PM.
The first one if for a race between wholesale disabling of GPEs (during
system suspend and shutdown) and the handling of level-triggered GPEs. The
race is that the latter may re-enable a GPE disabled by the former under
specific conditions, which is not good.
The second one is to prevent suspend-to-idle from leaking GPE handling
into the late code (that has been sent once already).
Thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] ACPICA: Save current masks of enabled GPEs after enable register writes
2014-12-01 1:52 [PATCH 0/2] ACPI / PM: Fixes related to GPE race conditions Rafael J. Wysocki
@ 2014-12-01 1:53 ` Rafael J. Wysocki
2014-12-01 5:03 ` Zheng, Lv
2014-12-01 1:54 ` [PATCH 2/2][Resend] ACPI / sleep: Drain outstanding events after disabling multiple GPEs Rafael J. Wysocki
1 sibling, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2014-12-01 1:53 UTC (permalink / raw)
To: ACPI Devel Maling List; +Cc: Robert Moore, Lv Zheng, Linux PM list
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
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.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/acpi/acpica/aclocal.h | 1
drivers/acpi/acpica/evgpe.c | 6 ++--
drivers/acpi/acpica/hwgpe.c | 53 ++++++++++++++++++++++++++++++++++--------
include/acpi/actypes.h | 4 +++
4 files changed, 51 insertions(+), 13 deletions(-)
Index: linux-pm/drivers/acpi/acpica/aclocal.h
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/aclocal.h
+++ linux-pm/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 */
};
/*
Index: linux-pm/drivers/acpi/acpica/hwgpe.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/hwgpe.c
+++ linux-pm/drivers/acpi/acpica/hwgpe.c
@@ -115,12 +115,12 @@ acpi_hw_low_set_gpe(struct acpi_gpe_even
/* 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_even
/* 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);
}
@@ -262,6 +265,32 @@ acpi_hw_get_gpe_status(struct acpi_gpe_e
/******************************************************************************
*
+ * 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
*
* PARAMETERS: gpe_xrupt_info - GPE Interrupt info
@@ -287,8 +316,8 @@ acpi_hw_disable_gpe_block(struct acpi_gp
/* 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
{
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 a
{
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 a
*/
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);
}
Index: linux-pm/drivers/acpi/acpica/evgpe.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/evgpe.c
+++ linux-pm/drivers/acpi/acpica/evgpe.c
@@ -134,7 +134,7 @@ acpi_status acpi_ev_enable_gpe(struct ac
/* 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
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 ac
/*
* 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);
Index: linux-pm/include/acpi/actypes.h
===================================================================
--- linux-pm.orig/include/acpi/actypes.h
+++ linux-pm/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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2][Resend] ACPI / sleep: Drain outstanding events after disabling multiple GPEs
2014-12-01 1:52 [PATCH 0/2] ACPI / PM: Fixes related to GPE race conditions Rafael J. Wysocki
2014-12-01 1:53 ` [PATCH 1/2] ACPICA: Save current masks of enabled GPEs after enable register writes Rafael J. Wysocki
@ 2014-12-01 1:54 ` Rafael J. Wysocki
1 sibling, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2014-12-01 1:54 UTC (permalink / raw)
To: ACPI Devel Maling List; +Cc: Robert Moore, Lv Zheng, Linux PM list
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
After multiple GPEs have been disabled at the low level in one go,
like when acpi_disable_all_gpes() is called, we should always drain
all of the outstanding events from them, or interesting races become
possible.
For this reason, call acpi_os_wait_events_complete() after
acpi_enable_all_wakeup_gpes() and acpi_disable_all_gpes() in
acpi_freeze_prepare() and acpi_power_off_prepare(), respectively.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/acpi/sleep.c | 2 ++
1 file changed, 2 insertions(+)
Index: linux-pm/drivers/acpi/sleep.c
===================================================================
--- linux-pm.orig/drivers/acpi/sleep.c
+++ linux-pm/drivers/acpi/sleep.c
@@ -630,6 +630,7 @@ static int acpi_freeze_begin(void)
static int acpi_freeze_prepare(void)
{
acpi_enable_all_wakeup_gpes();
+ acpi_os_wait_events_complete();
enable_irq_wake(acpi_gbl_FADT.sci_interrupt);
return 0;
}
@@ -825,6 +826,7 @@ static void acpi_power_off_prepare(void)
/* Prepare to power off the system */
acpi_sleep_prepare(ACPI_STATE_S5);
acpi_disable_all_gpes();
+ acpi_os_wait_events_complete();
}
static void acpi_power_off(void)
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 1/2] ACPICA: Save current masks of enabled GPEs after enable register writes
2014-12-01 1:53 ` [PATCH 1/2] ACPICA: Save current masks of enabled GPEs after enable register writes Rafael J. Wysocki
@ 2014-12-01 5:03 ` Zheng, Lv
2014-12-01 22:33 ` Rafael J. Wysocki
0 siblings, 1 reply; 6+ messages in thread
From: Zheng, Lv @ 2014-12-01 5:03 UTC (permalink / raw)
To: Rafael J. Wysocki, ACPI Devel Maling List; +Cc: Moore, Robert, Linux PM list
Hi, Rafael
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Monday, December 01, 2014 9:53 AM
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> 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.
Both the solution and the patch looks OK.
You'll still need the following fix to ensure the atomicity of acpi_ev_asynch_enable_gpe():
https://bugzilla.kernel.org/attachment.cgi?id=157391
Acked-by: Lv Zheng <lv.zheng@intel.com>
Best regards
-Lv
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/acpi/acpica/aclocal.h | 1
> drivers/acpi/acpica/evgpe.c | 6 ++--
> drivers/acpi/acpica/hwgpe.c | 53 ++++++++++++++++++++++++++++++++++--------
> include/acpi/actypes.h | 4 +++
> 4 files changed, 51 insertions(+), 13 deletions(-)
>
> Index: linux-pm/drivers/acpi/acpica/aclocal.h
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/aclocal.h
> +++ linux-pm/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 */
> };
>
> /*
> Index: linux-pm/drivers/acpi/acpica/hwgpe.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/hwgpe.c
> +++ linux-pm/drivers/acpi/acpica/hwgpe.c
> @@ -115,12 +115,12 @@ acpi_hw_low_set_gpe(struct acpi_gpe_even
> /* 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_even
> /* 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);
> }
>
> @@ -262,6 +265,32 @@ acpi_hw_get_gpe_status(struct acpi_gpe_e
>
> /******************************************************************************
> *
> + * 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
> *
> * PARAMETERS: gpe_xrupt_info - GPE Interrupt info
> @@ -287,8 +316,8 @@ acpi_hw_disable_gpe_block(struct acpi_gp
> /* 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
> {
> 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 a
> {
> 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 a
> */
>
> 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);
> }
> Index: linux-pm/drivers/acpi/acpica/evgpe.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/evgpe.c
> +++ linux-pm/drivers/acpi/acpica/evgpe.c
> @@ -134,7 +134,7 @@ acpi_status acpi_ev_enable_gpe(struct ac
>
> /* 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
> 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 ac
>
> /*
> * 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);
> Index: linux-pm/include/acpi/actypes.h
> ===================================================================
> --- linux-pm.orig/include/acpi/actypes.h
> +++ linux-pm/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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ACPICA: Save current masks of enabled GPEs after enable register writes
2014-12-01 5:03 ` Zheng, Lv
@ 2014-12-01 22:33 ` Rafael J. Wysocki
2014-12-02 0:56 ` Zheng, Lv
0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2014-12-01 22:33 UTC (permalink / raw)
To: Zheng, Lv; +Cc: ACPI Devel Maling List, Moore, Robert, Linux PM list
On Monday, December 01, 2014 05:03:04 AM Zheng, Lv wrote:
> Hi, Rafael
>
> > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > Sent: Monday, December 01, 2014 9:53 AM
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > 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.
>
> Both the solution and the patch looks OK.
> You'll still need the following fix to ensure the atomicity of acpi_ev_asynch_enable_gpe():
> https://bugzilla.kernel.org/attachment.cgi?id=157391
>
> Acked-by: Lv Zheng <lv.zheng@intel.com>
Thanks!
Can you please submit the patch from
https://bugzilla.kernel.org/attachment.cgi?id=157391
to linux-acpi officially?
I'd like it to be included into 3.19 as a fix.
Rafael
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 1/2] ACPICA: Save current masks of enabled GPEs after enable register writes
2014-12-01 22:33 ` Rafael J. Wysocki
@ 2014-12-02 0:56 ` Zheng, Lv
0 siblings, 0 replies; 6+ messages in thread
From: Zheng, Lv @ 2014-12-02 0:56 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: ACPI Devel Maling List, Moore, Robert, Linux PM list
Hi,
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Tuesday, December 02, 2014 6:33 AM
>
> On Monday, December 01, 2014 05:03:04 AM Zheng, Lv wrote:
> > Hi, Rafael
> >
> > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > Sent: Monday, December 01, 2014 9:53 AM
> > >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > 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.
> >
> > Both the solution and the patch looks OK.
> > You'll still need the following fix to ensure the atomicity of acpi_ev_asynch_enable_gpe():
> > https://bugzilla.kernel.org/attachment.cgi?id=157391
> >
> > Acked-by: Lv Zheng <lv.zheng@intel.com>
>
> Thanks!
>
> Can you please submit the patch from
> https://bugzilla.kernel.org/attachment.cgi?id=157391
> to linux-acpi officially?
>
> I'd like it to be included into 3.19 as a fix.
OK. I'll post it to this thread.
Thanks and best regards
-Lv
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-12-02 0:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-01 1:52 [PATCH 0/2] ACPI / PM: Fixes related to GPE race conditions Rafael J. Wysocki
2014-12-01 1:53 ` [PATCH 1/2] ACPICA: Save current masks of enabled GPEs after enable register writes Rafael J. Wysocki
2014-12-01 5:03 ` Zheng, Lv
2014-12-01 22:33 ` Rafael J. Wysocki
2014-12-02 0:56 ` Zheng, Lv
2014-12-01 1:54 ` [PATCH 2/2][Resend] ACPI / sleep: Drain outstanding events after disabling multiple GPEs Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).