Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit c50f13c6 authored by Rafael J. Wysocki's avatar Rafael J. Wysocki
Browse files

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: default avatarLv Zheng <lv.zheng@intel.com>
Signed-off-by: default avatarRafael J. Wysocki <rafael.j.wysocki@intel.com>
parent e5874591

drivers/acpi/acpica/aclocal.h

+1 −0
Original line number Diff line number Diff line
@@ -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 */

};



/*

drivers/acpi/acpica/evgpe.c

+3 −3
Original line number Diff line number Diff line
@@ -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);

drivers/acpi/acpica/hwgpe.c

+43 −10
Original line number Diff line number Diff line
@@ -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);

		}

include/acpi/actypes.h

+4 −0
Original line number Diff line number Diff line
@@ -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