ACPI / ACPICA: Fix problems related to GPE reference counting

PATCH New Rafael J. Wysocki 2 participants 15 messages Jun 7, 2010 – Jun 9, 2010

Subsystems (2)

ACPI ACPI component architecture (acpica)

lore

Share & tools

lore

Rafael J. Wysocki

author

2010-06-07 22:23 UTC #0 lore raw

PATCH 1/1 ACPI / ACPICA: Fix problems related to GPE reference counting

There are a few problems with the recently introduced GPE reference counting mechanism.

First of all, we overlooked the fact that acpi_ev_update_gpes() could have enabled GPEs before acpi_ev_initialize_gpe_block() was called and some GPEs are enabled twice during the initialization as a result.

Second, acpi_set_gpe() won't enable wakeup-only GPEs, because it attempts to do that with the help of acpi_ev_enable_gpe() which, in turn, calls acpi_hw_write_gpe_enable_reg() that will only enable the GPE if its bit is set in the register's enable_for_run. Worse yet, acpi_hw_write_gpe_enable_reg() always writes the entire enable_for_run mask into the register, so in theory it may enable more GPEs than just the one we want to enable (for example, it may enable GPEs in the same register that were disabled by acpi_hw_low_disable_gpe() previously).

Next, the enable_for_run and enable_for_wake masks of the GPE registers only change when the GPEs' runtime_count and wakeup_count fields change from zero to nonzero or the other way around, so it doesn't make sense to update the enable_for_run and enable_for_wake masks anywhere outside of acpi_enable_gpe() and acpi_disable_gpe().

Finally, the sysfs interface allowing user space to disable/enable GPEs doesn't work correctly, because a GPE disabled this way may be re-enabled shortly by acpi_ev_asynch_enable_gpe() if its bit is set in its register's enable_for_run mask.

To address these problems, call acpi_ev_update_gpe_enable_masks() directly from acpi_enable_gpe() and acpi_disable_gpe() where the GPE's runtime_count and wakeup_count counters change in such a way that its register's enable_for_run and enable_for_wake masks have to be updated. Change acpi_hw_low_disable_gpe() into acpi_hw_low_set_gpe() that can set as well as clear the appropriate bit in the GPE's register and use it wherever necessary.

Add a new action constant, ACPI_GPE_CHECK_AND_ENABLE, that will cause acpi_hw_low_set_gpe() to check if the GPE's bit in its register's enable_for_run mask is set before attempting to enable the GPE and remove acpi_hw_write_gpe_enable_reg() which isn't necessary any more. Remove acpi_ev_enable_gpe() and acpi_ev_disable_gpe() that aren't necessary too.

Make acpi_ev_initialize_gpe_block() check if a GPE has already been enabled and avoid calling acpi_enable_gpe() for it if that's the case. Also make the sysfs GPE interface use acpi_enable_gpe() and acpi_disable_gpe() instead of acpi_set_gpe() so that GPE reference counters are taken into account by it.

In addition to that, to reduce code duplication, move the computation of a GPE's register bit mask to a separate function and call it wherever that computation has to be carried out.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/acpica/acevents.h |    4 -
 drivers/acpi/acpica/achware.h  |    7 +-
 drivers/acpi/acpica/evgpe.c    |  118 ++---------------------------------------
 drivers/acpi/acpica/evgpeblk.c |   25 ++++++++
 drivers/acpi/acpica/evxface.c  |    2 
 drivers/acpi/acpica/evxfevnt.c |   59 ++++++++++++++++++--
 drivers/acpi/acpica/hwgpe.c    |  118 +++++++++++++++++++++++------------------
 drivers/acpi/system.c          |    6 +-
 include/acpi/actypes.h         |    3 -
 9 files changed, 161 insertions(+), 181 deletions(-)

drivers/acpi/acpica/hwgpe.c +67 ~~-51~~

Index: linux-2.6/drivers/acpi/acpica/hwgpe.c
===================================================================
@@ -57,21 +57,45 @@ acpi_hw_enable_wakeup_gpe_block(struct a
 
 /******************************************************************************
  *
- * FUNCTION:	acpi_hw_low_disable_gpe
+ * FUNCTION:	acpi_hw_gpe_register_bit
+ *
+ * PARAMETERS:	gpe_event_info	    - Info block for the GPE
+ *		gpe_register_info   - Info block for the GPE register
+ *
+ * RETURN:	Status
+ *
+ * DESCRIPTION:	Compute the enable mask with one bit corresponding to the given
+ *		GPE set.
+ *
+ ******************************************************************************/
+
+u32 acpi_hw_gpe_register_bit(struct acpi_gpe_event_info *gpe_event_info,
+			     struct acpi_gpe_register_info *gpe_register_info)
+{
+	return (u32)1 << (gpe_event_info->gpe_number -
+				gpe_register_info->base_gpe_number);
+}
+
+/******************************************************************************
+ *
+ * FUNCTION:	acpi_hw_low_set_gpe
  *
  * PARAMETERS:	gpe_event_info	    - Info block for the GPE to be disabled
+ *		action		    - Enable or disable
  *
  * RETURN:	Status
  *
- * DESCRIPTION: Disable a single GPE in the enable register.
+ * DESCRIPTION: Enable or disable a single GPE in its enable register.
  *
  ******************************************************************************/
 
-acpi_status acpi_hw_low_disable_gpe(struct acpi_gpe_event_info *gpe_event_info)
+acpi_status acpi_hw_low_set_gpe(struct acpi_gpe_event_info *gpe_event_info,
+				u8 action)
 {
 	struct acpi_gpe_register_info *gpe_register_info;
 	acpi_status status;
 	u32 enable_mask;
+	u32 register_bit;
 
 	/* Get the info block for the entire GPE register */

@@ -80,6 +104,17 @@ acpi_status acpi_hw_low_disable_gpe(stru
 		return (AE_NOT_EXIST);
 	}
 
+	register_bit = acpi_hw_gpe_register_bit(gpe_event_info,
+						gpe_register_info);
+
+	if (action == ACPI_GPE_CHECK_AND_ENABLE) {
+		if (register_bit & gpe_register_info->enable_for_run) {
+			action = ACPI_GPE_ENABLE;
+		} else {
+			return (AE_BAD_PARAMETER);
+		}
+	}
+
 	/* Get current value of the enable register that contains this GPE */
 
 	status = acpi_hw_read(&enable_mask, &gpe_register_info->enable_address);

@@ -87,11 +122,22 @@ acpi_status acpi_hw_low_disable_gpe(stru
 		return (status);
 	}
 
-	/* Clear just the bit that corresponds to this GPE */
+	/* Set ot clear just the bit that corresponds to this GPE */
+
+	switch (action) {
+	case ACPI_GPE_ENABLE:
+		ACPI_SET_BIT(enable_mask, register_bit);
+		break;
+
+	case ACPI_GPE_DISABLE:
+		ACPI_CLEAR_BIT(enable_mask, register_bit);
+		break;
+
+	default:
+		ACPI_ERROR((AE_INFO, "Invalid action\n"));
+		return (AE_BAD_PARAMETER);
+	}
 
-	ACPI_CLEAR_BIT(enable_mask, ((u32)1 <<
-				     (gpe_event_info->gpe_number -
-				      gpe_register_info->base_gpe_number)));
 
 	/* Write the updated enable mask */

@@ -101,23 +147,21 @@ acpi_status acpi_hw_low_disable_gpe(stru
 
 /******************************************************************************
  *
- * FUNCTION:    acpi_hw_write_gpe_enable_reg
+ * FUNCTION:    acpi_hw_clear_gpe
  *
- * PARAMETERS:  gpe_event_info      - Info block for the GPE to be enabled
+ * PARAMETERS:  gpe_event_info      - Info block for the GPE to be cleared
  *
  * RETURN:      Status
  *
- * DESCRIPTION: Write a GPE enable register.  Note: The bit for this GPE must
- *              already be cleared or set in the parent register
- *              enable_for_run mask.
+ * DESCRIPTION: Clear the status bit for a single GPE.
  *
  ******************************************************************************/
 
-acpi_status
-acpi_hw_write_gpe_enable_reg(struct acpi_gpe_event_info * gpe_event_info)
+acpi_status acpi_hw_clear_gpe(struct acpi_gpe_event_info * gpe_event_info)
 {
 	struct acpi_gpe_register_info *gpe_register_info;
 	acpi_status status;
+	u32 register_bit;
 
 	ACPI_FUNCTION_ENTRY();

@@ -128,43 +172,15 @@ acpi_hw_write_gpe_enable_reg(struct acpi
 		return (AE_NOT_EXIST);
 	}
 
-	/* Write the entire GPE (runtime) enable register */
-
-	status = acpi_hw_write(gpe_register_info->enable_for_run,
-			       &gpe_register_info->enable_address);
-
-	return (status);
-}
-
-/******************************************************************************
- *
- * FUNCTION:    acpi_hw_clear_gpe
- *
- * PARAMETERS:  gpe_event_info      - Info block for the GPE to be cleared
- *
- * RETURN:      Status
- *
- * DESCRIPTION: Clear the status bit for a single GPE.
- *
- ******************************************************************************/
-
-acpi_status acpi_hw_clear_gpe(struct acpi_gpe_event_info * gpe_event_info)
-{
-	acpi_status status;
-	u8 register_bit;
-
-	ACPI_FUNCTION_ENTRY();
-
-	register_bit = (u8)(1 <<
-			    (gpe_event_info->gpe_number -
-			     gpe_event_info->register_info->base_gpe_number));
+	register_bit = acpi_hw_gpe_register_bit(gpe_event_info,
+						gpe_register_info);
 
 	/*
 	 * Write a one to the appropriate bit in the status register to
 	 * clear this GPE.
 	 */
 	status = acpi_hw_write(register_bit,
-			       &gpe_event_info->register_info->status_address);
+				&gpe_register_info->status_address);
 
 	return (status);
 }

@@ -187,7 +203,7 @@ acpi_hw_get_gpe_status(struct acpi_gpe_e
 		       acpi_event_status * event_status)
 {
 	u32 in_byte;
-	u8 register_bit;
+	u32 register_bit;
 	struct acpi_gpe_register_info *gpe_register_info;
 	acpi_status status;
 	acpi_event_status local_event_status = 0;

@@ -201,12 +217,12 @@ acpi_hw_get_gpe_status(struct acpi_gpe_e
 	/* Get the info block for the entire GPE register */
 
 	gpe_register_info = gpe_event_info->register_info;
+	if (!gpe_register_info) {
+		return (AE_NOT_EXIST);
+	}
 
-	/* Get the register bitmask for this GPE */
-
-	register_bit = (u8)(1 <<
-			    (gpe_event_info->gpe_number -
-			     gpe_event_info->register_info->base_gpe_number));
+	register_bit = acpi_hw_gpe_register_bit(gpe_event_info,
+						gpe_register_info);
 
 	/* GPE currently enabled? (enabled for runtime?) */

drivers/acpi/acpica/achware.h +4 -3

Index: linux-2.6/drivers/acpi/acpica/achware.h
===================================================================
@@ -90,10 +90,11 @@ acpi_status acpi_hw_write_port(acpi_io_a
 /*
  * hwgpe - GPE support
  */
-acpi_status acpi_hw_low_disable_gpe(struct acpi_gpe_event_info *gpe_event_info);
+u32 acpi_hw_gpe_register_bit(struct acpi_gpe_event_info *gpe_event_info,
+			     struct acpi_gpe_register_info *gpe_register_info);
 
-acpi_status
-acpi_hw_write_gpe_enable_reg(struct acpi_gpe_event_info *gpe_event_info);
+acpi_status acpi_hw_low_set_gpe(struct acpi_gpe_event_info *gpe_event_info,
+				u8 action);
 
 acpi_status
 acpi_hw_disable_gpe_block(struct acpi_gpe_xrupt_info *gpe_xrupt_info,

drivers/acpi/acpica/evgpe.c +6 ~~-112~~

160 lines not on this page · raw message

drivers/acpi/acpica/evxfevnt.c +53 -6

98 lines not on this page · raw message

drivers/acpi/acpica/evgpeblk.c +24 -1

Index: linux-2.6/drivers/acpi/acpica/evgpeblk.c
===================================================================
@@ -500,6 +500,30 @@ acpi_ev_initialize_gpe_block(struct acpi
 
 			gpe_index = (i * ACPI_GPE_REGISTER_WIDTH) + j;
 			gpe_event_info = &gpe_block->event_info[gpe_index];
+			gpe_number = gpe_index + gpe_block->block_base_number;
+
+			/*
+			 * Ensure that the register bit corresponding to this
+			 * GPE in its register's enable masks is up to date.
+			 */
+			status = acpi_ev_update_gpe_enable_masks(gpe_event_info);
+			if (ACPI_FAILURE(status)) {
+				ACPI_ERROR((AE_INFO, "Failed to update enable "
+					"masks for GPE %02X\n", gpe_number));
+				continue;
+			}
+
+			/*
+			 * If the GPE has already been enabled for runtime
+			 * signaling, make sure it remains enabled, but do not
+			 * increment its reference counter.
+			 */
+			if (gpe_event_info->runtime_count) {
+				acpi_set_gpe(gpe_device, gpe_number,
+						ACPI_GPE_ENABLE);
+				gpe_enabled_count++;
+				continue;
+			}
 
 			if (gpe_event_info->flags & ACPI_GPE_CAN_WAKE) {
 				wake_gpe_count++;

@@ -516,7 +540,6 @@ acpi_ev_initialize_gpe_block(struct acpi
 
 			/* Enable this GPE */
 
-			gpe_number = gpe_index + gpe_block->block_base_number;
 			status = acpi_enable_gpe(gpe_device, gpe_number,
 						 ACPI_GPE_TYPE_RUNTIME);
 			if (ACPI_FAILURE(status)) {

include/acpi/actypes.h +2 -1

Index: linux-2.6/include/acpi/actypes.h
===================================================================
@@ -663,10 +663,11 @@ typedef u32 acpi_event_status;
 #define ACPI_GPE_MAX                    0xFF
 #define ACPI_NUM_GPE                    256
 
-/* Actions for acpi_set_gpe */
+/* Actions for acpi_set_gpe and acpi_hw_low_set_gpe */
 
 #define ACPI_GPE_ENABLE                 0
 #define ACPI_GPE_DISABLE                1
+#define ACPI_GPE_CHECK_AND_ENABLE       2
 
 /* gpe_types for acpi_enable_gpe and acpi_disable_gpe */

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

Index: linux-2.6/drivers/acpi/acpica/evxface.c
===================================================================
@@ -721,7 +721,7 @@ acpi_install_gpe_handler(acpi_handle gpe
 
 	/* Disable the GPE before installing the handler */
 
-	status = acpi_ev_disable_gpe(gpe_event_info);
+	status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_DISABLE);
 	if (ACPI_FAILURE (status)) {
 		goto unlock_and_exit;
 	}

drivers/acpi/acpica/acevents.h +0 -4

Index: linux-2.6/drivers/acpi/acpica/acevents.h
===================================================================
@@ -80,10 +80,6 @@ u32 acpi_ev_gpe_detect(struct acpi_gpe_x
 acpi_status
 acpi_ev_update_gpe_enable_masks(struct acpi_gpe_event_info *gpe_event_info);
 
-acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info);
-
-acpi_status acpi_ev_disable_gpe(struct acpi_gpe_event_info *gpe_event_info);
-
 struct acpi_gpe_event_info *acpi_ev_get_gpe_event_info(acpi_handle gpe_device,
 						       u32 gpe_number);

drivers/acpi/system.c +4 -2

Index: linux-2.6/drivers/acpi/system.c
===================================================================
@@ -388,10 +388,12 @@ static ssize_t counter_set(struct kobjec
 	if (index < num_gpes) {
 		if (!strcmp(buf, "disable\n") &&
 				(status & ACPI_EVENT_FLAG_ENABLED))
-			result = acpi_set_gpe(handle, index, ACPI_GPE_DISABLE);
+			result = acpi_disable_gpe(handle, index,
+						ACPI_GPE_TYPE_RUNTIME);
 		else if (!strcmp(buf, "enable\n") &&
 				!(status & ACPI_EVENT_FLAG_ENABLED))
-			result = acpi_set_gpe(handle, index, ACPI_GPE_ENABLE);
+			result = acpi_enable_gpe(handle, index,
+						ACPI_GPE_TYPE_RUNTIME);
 		else if (!strcmp(buf, "clear\n") &&
 				(status & ACPI_EVENT_FLAG_SET))
 			result = acpi_clear_gpe(handle, index);

Rafael J. Wysocki

author

2010-06-08 08:46 UTC #1 lore raw

cover ACPI / ACPICA: Fix problems related to GPE reference counting

Hi,

I decided to split this big patch into a series of smaller ones addressing one issue each. I think all of the issues addressed by [2-5/5] may be regarded as regressions from 2.6.33, because that kernel didn't have these problems.

[1/5] - Not really a fix, but [2/5] depends on it and I don't see a reason to
duplicate that thing even more.
[2/5] - Low-level GPE manipulation fix
[3/5] - Fix possible issue with re-enabling a GPE after event handling [4/5] - Fix the initialization of GPEs
[5/5] - Fix GPE sysfs interface

Please review, possibly apply.

Thanks,
Rafael

Rafael J. Wysocki

author

2010-06-08 09:00 UTC #2 lore raw

cover ACPI / ACPICA: Fix problems related to GPE reference counting

In reply to Rafael J. Wysocki

Hi,

Please review, possibly apply.

Thanks,
Rafael

Rafael J. Wysocki

author

2010-06-08 08:48 UTC #3 lore raw

PATCH 1/5 ACPI / ACPICA: Use helper function for computing GPE masks

From: Rafael J. Wysocki <[email protected]>

In quite a few places ACPICA needs to compute a GPE enable mask with only one bit, corresponding to a given GPE, set. Currently, that computation is always open coded which leads to unnecessary code duplication. Fix this by introducing a helper function for computing one-bit GPE enable masks and using it where appropriate.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/acpica/achware.h |    3 ++
 drivers/acpi/acpica/evgpe.c   |    7 ++---
 drivers/acpi/acpica/hwgpe.c   |   52 ++++++++++++++++++++++++++++++++----------
 3 files changed, 46 insertions(+), 16 deletions(-)

drivers/acpi/acpica/achware.h +3 -0

Index: linux-2.6/drivers/acpi/acpica/achware.h
===================================================================
@@ -90,6 +90,9 @@ acpi_status acpi_hw_write_port(acpi_io_a
 /*
  * hwgpe - GPE support
  */
+u32 acpi_hw_gpe_register_bit(struct acpi_gpe_event_info *gpe_event_info,
+			     struct acpi_gpe_register_info *gpe_register_info);
+
 acpi_status acpi_hw_low_disable_gpe(struct acpi_gpe_event_info *gpe_event_info);
 
 acpi_status

drivers/acpi/acpica/hwgpe.c +40 ~~-12~~

98 lines not on this page · raw message

drivers/acpi/acpica/evgpe.c +3 -4

Index: linux-2.6/drivers/acpi/acpica/evgpe.c
===================================================================
@@ -69,7 +69,7 @@ acpi_status
 acpi_ev_update_gpe_enable_masks(struct acpi_gpe_event_info *gpe_event_info)
 {
 	struct acpi_gpe_register_info *gpe_register_info;
-	u8 register_bit;
+	u32 register_bit;
 
 	ACPI_FUNCTION_TRACE(ev_update_gpe_enable_masks);

@@ -78,9 +78,8 @@ acpi_ev_update_gpe_enable_masks(struct a
 		return_ACPI_STATUS(AE_NOT_EXIST);
 	}
 
-	register_bit = (u8)
-	    (1 <<
-	     (gpe_event_info->gpe_number - gpe_register_info->base_gpe_number));
+	register_bit = acpi_hw_gpe_register_bit(gpe_event_info,
+						gpe_register_info);
 
 	/* Clear the wake/run bits up front */

Rafael J. Wysocki

author

2010-06-08 09:00 UTC #4 lore raw

PATCH 1/5 ACPI / ACPICA: Use helper function for computing GPE masks

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/acpica/achware.h |    3 ++
 drivers/acpi/acpica/evgpe.c   |    7 ++---
 drivers/acpi/acpica/hwgpe.c   |   52 ++++++++++++++++++++++++++++++++----------
 3 files changed, 46 insertions(+), 16 deletions(-)

drivers/acpi/acpica/achware.h +3 -0

Index: linux-2.6/drivers/acpi/acpica/achware.h
===================================================================
@@ -90,6 +90,9 @@ acpi_status acpi_hw_write_port(acpi_io_a
 /*
  * hwgpe - GPE support
  */
+u32 acpi_hw_gpe_register_bit(struct acpi_gpe_event_info *gpe_event_info,
+			     struct acpi_gpe_register_info *gpe_register_info);
+
 acpi_status acpi_hw_low_disable_gpe(struct acpi_gpe_event_info *gpe_event_info);
 
 acpi_status

drivers/acpi/acpica/hwgpe.c +40 ~~-12~~

98 lines not on this page · raw message

drivers/acpi/acpica/evgpe.c +3 -4

Index: linux-2.6/drivers/acpi/acpica/evgpe.c
===================================================================
@@ -69,7 +69,7 @@ acpi_status
 acpi_ev_update_gpe_enable_masks(struct acpi_gpe_event_info *gpe_event_info)
 {
 	struct acpi_gpe_register_info *gpe_register_info;
-	u8 register_bit;
+	u32 register_bit;
 
 	ACPI_FUNCTION_TRACE(ev_update_gpe_enable_masks);

@@ -78,9 +78,8 @@ acpi_ev_update_gpe_enable_masks(struct a
 		return_ACPI_STATUS(AE_NOT_EXIST);
 	}
 
-	register_bit = (u8)
-	    (1 <<
-	     (gpe_event_info->gpe_number - gpe_register_info->base_gpe_number));
+	register_bit = acpi_hw_gpe_register_bit(gpe_event_info,
+						gpe_register_info);
 
 	/* Clear the wake/run bits up front */

Rafael J. Wysocki

author

2010-06-08 08:49 UTC #5 lore raw

PATCH 2/5 ACPI / ACPICA: Fix low-level GPE manipulation code

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

ACPICA uses acpi_ev_enable_gpe() for enabling GPEs at the low level, which is incorrect, because this function only enables the GPE if the corresponding bit in its enable register's enable_for_run mask is set. This causes acpi_set_gpe() to work incorrectly if used for enabling GPEs that were not previously enabled with acpi_enable_gpe(). As a result, among other things, wakeup-only GPEs are never enabled by acpi_enable_wakeup_device(), so the devices that use them are unable to wake up the system.

To fix this issue remove acpi_ev_enable_gpe() and its counterpart acpi_ev_disable_gpe() and replace acpi_hw_low_disable_gpe() with acpi_hw_low_set_gpe() that will be used instead to manipulate GPE enable bits at the low level. Make the users of acpi_ev_enable_gpe() and acpi_ev_disable_gpe() call acpi_hw_low_set_gpe() instead and make sure that GPE enable masks are only updated by acpi_enable_gpe() and acpi_disable_gpe() when GPE reference counters change from 0 to 1 and from 1 to 0, respectively.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/acpica/acevents.h |    4 -
 drivers/acpi/acpica/achware.h  |    3 -
 drivers/acpi/acpica/evgpe.c    |  108 -----------------------------------------
 drivers/acpi/acpica/evxface.c  |    2 
 drivers/acpi/acpica/evxfevnt.c |   59 ++++++++++++++++++++--
 drivers/acpi/acpica/hwgpe.c    |   26 +++++++--
 include/acpi/actypes.h         |    2 
 7 files changed, 80 insertions(+), 124 deletions(-)

drivers/acpi/acpica/hwgpe.c +21 -5

Index: linux-2.6/drivers/acpi/acpica/hwgpe.c
===================================================================
@@ -78,23 +78,27 @@ u32 acpi_hw_gpe_register_bit(struct acpi
 
 /******************************************************************************
  *
- * FUNCTION:	acpi_hw_low_disable_gpe
+ * FUNCTION:	acpi_hw_low_set_gpe
  *
  * PARAMETERS:	gpe_event_info	    - Info block for the GPE to be disabled
+ *		action		    - Enable or disable
  *
  * RETURN:	Status
  *
- * DESCRIPTION: Disable a single GPE in the enable register.
+ * DESCRIPTION: Enable or disable a single GPE in its enable register.
  *
  ******************************************************************************/
 
-acpi_status acpi_hw_low_disable_gpe(struct acpi_gpe_event_info *gpe_event_info)
+acpi_status
+acpi_hw_low_set_gpe(struct acpi_gpe_event_info *gpe_event_info, u8 action)
 {
 	struct acpi_gpe_register_info *gpe_register_info;
 	acpi_status status;
 	u32 enable_mask;
 	u32 register_bit;
 
+	ACPI_FUNCTION_ENTRY();
+
 	/* Get the info block for the entire GPE register */
 
 	gpe_register_info = gpe_event_info->register_info;

@@ -109,11 +113,23 @@ acpi_status acpi_hw_low_disable_gpe(stru
 		return (status);
 	}
 
-	/* Clear just the bit that corresponds to this GPE */
+	/* Set ot clear just the bit that corresponds to this GPE */
 
 	register_bit = acpi_hw_gpe_register_bit(gpe_event_info,
 						gpe_register_info);
-	ACPI_CLEAR_BIT(enable_mask, register_bit);
+	switch (action) {
+	case ACPI_GPE_ENABLE:
+		ACPI_SET_BIT(enable_mask, register_bit);
+		break;
+
+	case ACPI_GPE_DISABLE:
+		ACPI_CLEAR_BIT(enable_mask, register_bit);
+		break;
+
+	default:
+		ACPI_ERROR((AE_INFO, "Invalid action\n"));
+		return (AE_BAD_PARAMETER);
+	}
 
 	/* Write the updated enable mask */

drivers/acpi/acpica/achware.h +2 -1

Index: linux-2.6/drivers/acpi/acpica/achware.h
===================================================================
@@ -93,7 +93,8 @@ acpi_status acpi_hw_write_port(acpi_io_a
 u32 acpi_hw_gpe_register_bit(struct acpi_gpe_event_info *gpe_event_info,
 			     struct acpi_gpe_register_info *gpe_register_info);
 
-acpi_status acpi_hw_low_disable_gpe(struct acpi_gpe_event_info *gpe_event_info);
+acpi_status
+acpi_hw_low_set_gpe(struct acpi_gpe_event_info *gpe_event_info, u8 action);
 
 acpi_status
 acpi_hw_write_gpe_enable_reg(struct acpi_gpe_event_info *gpe_event_info);

drivers/acpi/acpica/evgpe.c +2 ~~-106~~

132 lines not on this page · raw message

drivers/acpi/acpica/evxfevnt.c +53 -6

98 lines not on this page · raw message

include/acpi/actypes.h +1 -1

Index: linux-2.6/include/acpi/actypes.h
===================================================================
@@ -663,7 +663,7 @@ typedef u32 acpi_event_status;
 #define ACPI_GPE_MAX                    0xFF
 #define ACPI_NUM_GPE                    256
 
-/* Actions for acpi_set_gpe */
+/* Actions for acpi_set_gpe and acpi_hw_low_set_gpe */
 
 #define ACPI_GPE_ENABLE                 0
 #define ACPI_GPE_DISABLE                1

drivers/acpi/acpica/acevents.h +0 -4

Index: linux-2.6/drivers/acpi/acpica/acevents.h
===================================================================
@@ -80,10 +80,6 @@ u32 acpi_ev_gpe_detect(struct acpi_gpe_x
 acpi_status
 acpi_ev_update_gpe_enable_masks(struct acpi_gpe_event_info *gpe_event_info);
 
-acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info);
-
-acpi_status acpi_ev_disable_gpe(struct acpi_gpe_event_info *gpe_event_info);
-
 struct acpi_gpe_event_info *acpi_ev_get_gpe_event_info(acpi_handle gpe_device,
 						       u32 gpe_number);

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

Index: linux-2.6/drivers/acpi/acpica/evxface.c
===================================================================
@@ -721,7 +721,7 @@ acpi_install_gpe_handler(acpi_handle gpe
 
 	/* Disable the GPE before installing the handler */
 
-	status = acpi_ev_disable_gpe(gpe_event_info);
+	status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_DISABLE);
 	if (ACPI_FAILURE (status)) {
 		goto unlock_and_exit;
 	}

Rafael J. Wysocki

author

2010-06-08 09:00 UTC #6 lore raw

PATCH 2/5 ACPI / ACPICA: Fix low-level GPE manipulation code

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/acpica/acevents.h |    4 -
 drivers/acpi/acpica/achware.h  |    3 -
 drivers/acpi/acpica/evgpe.c    |  108 -----------------------------------------
 drivers/acpi/acpica/evxface.c  |    2 
 drivers/acpi/acpica/evxfevnt.c |   59 ++++++++++++++++++++--
 drivers/acpi/acpica/hwgpe.c    |   26 +++++++--
 include/acpi/actypes.h         |    2 
 7 files changed, 80 insertions(+), 124 deletions(-)

drivers/acpi/acpica/hwgpe.c +21 -5

Index: linux-2.6/drivers/acpi/acpica/hwgpe.c
===================================================================
@@ -78,23 +78,27 @@ u32 acpi_hw_gpe_register_bit(struct acpi
 
 /******************************************************************************
  *
- * FUNCTION:	acpi_hw_low_disable_gpe
+ * FUNCTION:	acpi_hw_low_set_gpe
  *
  * PARAMETERS:	gpe_event_info	    - Info block for the GPE to be disabled
+ *		action		    - Enable or disable
  *
  * RETURN:	Status
  *
- * DESCRIPTION: Disable a single GPE in the enable register.
+ * DESCRIPTION: Enable or disable a single GPE in its enable register.
  *
  ******************************************************************************/
 
-acpi_status acpi_hw_low_disable_gpe(struct acpi_gpe_event_info *gpe_event_info)
+acpi_status
+acpi_hw_low_set_gpe(struct acpi_gpe_event_info *gpe_event_info, u8 action)
 {
 	struct acpi_gpe_register_info *gpe_register_info;
 	acpi_status status;
 	u32 enable_mask;
 	u32 register_bit;
 
+	ACPI_FUNCTION_ENTRY();
+
 	/* Get the info block for the entire GPE register */
 
 	gpe_register_info = gpe_event_info->register_info;

@@ -109,11 +113,23 @@ acpi_status acpi_hw_low_disable_gpe(stru
 		return (status);
 	}
 
-	/* Clear just the bit that corresponds to this GPE */
+	/* Set ot clear just the bit that corresponds to this GPE */
 
 	register_bit = acpi_hw_gpe_register_bit(gpe_event_info,
 						gpe_register_info);
-	ACPI_CLEAR_BIT(enable_mask, register_bit);
+	switch (action) {
+	case ACPI_GPE_ENABLE:
+		ACPI_SET_BIT(enable_mask, register_bit);
+		break;
+
+	case ACPI_GPE_DISABLE:
+		ACPI_CLEAR_BIT(enable_mask, register_bit);
+		break;
+
+	default:
+		ACPI_ERROR((AE_INFO, "Invalid action\n"));
+		return (AE_BAD_PARAMETER);
+	}
 
 	/* Write the updated enable mask */

drivers/acpi/acpica/achware.h +2 -1

Index: linux-2.6/drivers/acpi/acpica/achware.h
===================================================================
@@ -93,7 +93,8 @@ acpi_status acpi_hw_write_port(acpi_io_a
 u32 acpi_hw_gpe_register_bit(struct acpi_gpe_event_info *gpe_event_info,
 			     struct acpi_gpe_register_info *gpe_register_info);
 
-acpi_status acpi_hw_low_disable_gpe(struct acpi_gpe_event_info *gpe_event_info);
+acpi_status
+acpi_hw_low_set_gpe(struct acpi_gpe_event_info *gpe_event_info, u8 action);
 
 acpi_status
 acpi_hw_write_gpe_enable_reg(struct acpi_gpe_event_info *gpe_event_info);

drivers/acpi/acpica/evgpe.c +2 ~~-106~~

132 lines not on this page · raw message

drivers/acpi/acpica/evxfevnt.c +53 -6

98 lines not on this page · raw message

include/acpi/actypes.h +1 -1

Index: linux-2.6/include/acpi/actypes.h
===================================================================
@@ -663,7 +663,7 @@ typedef u32 acpi_event_status;
 #define ACPI_GPE_MAX                    0xFF
 #define ACPI_NUM_GPE                    256
 
-/* Actions for acpi_set_gpe */
+/* Actions for acpi_set_gpe and acpi_hw_low_set_gpe */
 
 #define ACPI_GPE_ENABLE                 0
 #define ACPI_GPE_DISABLE                1

drivers/acpi/acpica/acevents.h +0 -4

Index: linux-2.6/drivers/acpi/acpica/acevents.h
===================================================================
@@ -80,10 +80,6 @@ u32 acpi_ev_gpe_detect(struct acpi_gpe_x
 acpi_status
 acpi_ev_update_gpe_enable_masks(struct acpi_gpe_event_info *gpe_event_info);
 
-acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info);
-
-acpi_status acpi_ev_disable_gpe(struct acpi_gpe_event_info *gpe_event_info);
-
 struct acpi_gpe_event_info *acpi_ev_get_gpe_event_info(acpi_handle gpe_device,
 						       u32 gpe_number);

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

Index: linux-2.6/drivers/acpi/acpica/evxface.c
===================================================================
@@ -721,7 +721,7 @@ acpi_install_gpe_handler(acpi_handle gpe
 
 	/* Disable the GPE before installing the handler */
 
-	status = acpi_ev_disable_gpe(gpe_event_info);
+	status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_DISABLE);
 	if (ACPI_FAILURE (status)) {
 		goto unlock_and_exit;
 	}

Rafael J. Wysocki

author

2010-06-08 08:49 UTC #7 lore raw

PATCH 3/5 ACPI / ACPICA: Avoid writing full enable masks to GPE registers

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

ACPICA uses acpi_hw_write_gpe_enable_reg() to re-enable a GPE after an event signaled by it has been handled. However, this function writes the entire GPE enable mask to the GPE's enable register which may not be correct. Namely, if one of the other GPEs in the same register was previously enabled by acpi_enable_gpe() and subsequently disabled using acpi_set_gpe(), acpi_hw_write_gpe_enable_reg() will re-enable it along with the target GPE.

To fix this issue rework acpi_hw_write_gpe_enable_reg() so that it calls acpi_hw_low_set_gpe() with a special action value, ACPI_GPE_COND_ENABLE, that will make it only enable the GPE if the corresponding bit in its register's enable_for_run mask is set.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/acpica/hwgpe.c |   18 +++++-------------
 include/acpi/actypes.h      |    1 +
 2 files changed, 6 insertions(+), 13 deletions(-)

drivers/acpi/acpica/hwgpe.c +5 ~~-13~~

Index: linux-2.6/drivers/acpi/acpica/hwgpe.c
===================================================================
@@ -118,6 +118,10 @@ acpi_hw_low_set_gpe(struct acpi_gpe_even
 	register_bit = acpi_hw_gpe_register_bit(gpe_event_info,
 						gpe_register_info);
 	switch (action) {
+	case ACPI_GPE_COND_ENABLE:
+		if (!(register_bit & gpe_register_info->enable_for_run))
+			return (AE_BAD_PARAMETER);
+
 	case ACPI_GPE_ENABLE:
 		ACPI_SET_BIT(enable_mask, register_bit);
 		break;

@@ -154,23 +158,11 @@ acpi_hw_low_set_gpe(struct acpi_gpe_even
 acpi_status
 acpi_hw_write_gpe_enable_reg(struct acpi_gpe_event_info * gpe_event_info)
 {
-	struct acpi_gpe_register_info *gpe_register_info;
 	acpi_status status;
 
 	ACPI_FUNCTION_ENTRY();
 
-	/* Get the info block for the entire GPE register */
-
-	gpe_register_info = gpe_event_info->register_info;
-	if (!gpe_register_info) {
-		return (AE_NOT_EXIST);
-	}
-
-	/* Write the entire GPE (runtime) enable register */
-
-	status = acpi_hw_write(gpe_register_info->enable_for_run,
-			       &gpe_register_info->enable_address);
-
+	status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_COND_ENABLE);
 	return (status);
 }

include/acpi/actypes.h +1 -0

Index: linux-2.6/include/acpi/actypes.h
===================================================================
@@ -667,6 +667,7 @@ typedef u32 acpi_event_status;
 
 #define ACPI_GPE_ENABLE                 0
 #define ACPI_GPE_DISABLE                1
+#define ACPI_GPE_COND_ENABLE            2
 
 /* gpe_types for acpi_enable_gpe and acpi_disable_gpe */

Rafael J. Wysocki

author

2010-06-08 08:49 UTC #8 lore raw

PATCH 3/5 ACPI / ACPICA: Avoid writing full enable masks to GPE registers

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/acpica/hwgpe.c |   18 +++++-------------
 include/acpi/actypes.h      |    1 +
 2 files changed, 6 insertions(+), 13 deletions(-)

drivers/acpi/acpica/hwgpe.c +5 ~~-13~~

Index: linux-2.6/drivers/acpi/acpica/hwgpe.c
===================================================================
@@ -118,6 +118,10 @@ acpi_hw_low_set_gpe(struct acpi_gpe_even
 	register_bit = acpi_hw_gpe_register_bit(gpe_event_info,
 						gpe_register_info);
 	switch (action) {
+	case ACPI_GPE_COND_ENABLE:
+		if (!(register_bit & gpe_register_info->enable_for_run))
+			return (AE_BAD_PARAMETER);
+
 	case ACPI_GPE_ENABLE:
 		ACPI_SET_BIT(enable_mask, register_bit);
 		break;

@@ -154,23 +158,11 @@ acpi_hw_low_set_gpe(struct acpi_gpe_even
 acpi_status
 acpi_hw_write_gpe_enable_reg(struct acpi_gpe_event_info * gpe_event_info)
 {
-	struct acpi_gpe_register_info *gpe_register_info;
 	acpi_status status;
 
 	ACPI_FUNCTION_ENTRY();
 
-	/* Get the info block for the entire GPE register */
-
-	gpe_register_info = gpe_event_info->register_info;
-	if (!gpe_register_info) {
-		return (AE_NOT_EXIST);
-	}
-
-	/* Write the entire GPE (runtime) enable register */
-
-	status = acpi_hw_write(gpe_register_info->enable_for_run,
-			       &gpe_register_info->enable_address);
-
+	status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_COND_ENABLE);
 	return (status);
 }

include/acpi/actypes.h +1 -0

Index: linux-2.6/include/acpi/actypes.h
===================================================================
@@ -667,6 +667,7 @@ typedef u32 acpi_event_status;
 
 #define ACPI_GPE_ENABLE                 0
 #define ACPI_GPE_DISABLE                1
+#define ACPI_GPE_COND_ENABLE            2
 
 /* gpe_types for acpi_enable_gpe and acpi_disable_gpe */

Rafael J. Wysocki

author

2010-06-08 08:50 UTC #9 lore raw

PATCH 4/5 ACPI / ACPICA: Fix GPE initialization

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

While developing the GPE reference counting code we overlooked the fact that acpi_ev_update_gpes() could have enabled GPEs before acpi_ev_initialize_gpe_block() was called. As a result, some GPEs are enabled twice during the initialization.

To fix this issue avoid calling acpi_enable_gpe() from acpi_ev_initialize_gpe_block() for the GPEs that have nonzero runtime reference counters.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/acpica/evgpeblk.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

drivers/acpi/acpica/evgpeblk.c +13 -1

Index: linux-2.6/drivers/acpi/acpica/evgpeblk.c
===================================================================
@@ -500,6 +500,19 @@ acpi_ev_initialize_gpe_block(struct acpi
 
 			gpe_index = (i * ACPI_GPE_REGISTER_WIDTH) + j;
 			gpe_event_info = &gpe_block->event_info[gpe_index];
+			gpe_number = gpe_index + gpe_block->block_base_number;
+
+			/*
+			 * If the GPE has already been enabled for runtime
+			 * signaling, make sure it remains enabled, but do not
+			 * increment its reference counter.
+			 */
+			if (gpe_event_info->runtime_count) {
+				acpi_set_gpe(gpe_device, gpe_number,
+						ACPI_GPE_ENABLE);
+				gpe_enabled_count++;
+				continue;
+			}
 
 			if (gpe_event_info->flags & ACPI_GPE_CAN_WAKE) {
 				wake_gpe_count++;

@@ -516,7 +529,6 @@ acpi_ev_initialize_gpe_block(struct acpi
 
 			/* Enable this GPE */
 
-			gpe_number = gpe_index + gpe_block->block_base_number;
 			status = acpi_enable_gpe(gpe_device, gpe_number,
 						 ACPI_GPE_TYPE_RUNTIME);
 			if (ACPI_FAILURE(status)) {

Rafael J. Wysocki

author

2010-06-08 08:50 UTC #10 lore raw

PATCH 4/5 ACPI / ACPICA: Fix GPE initialization

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

To fix this issue avoid calling acpi_enable_gpe() from acpi_ev_initialize_gpe_block() for the GPEs that have nonzero runtime reference counters.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/acpi/acpica/evgpeblk.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

drivers/acpi/acpica/evgpeblk.c +13 -1

Index: linux-2.6/drivers/acpi/acpica/evgpeblk.c
===================================================================
@@ -500,6 +500,19 @@ acpi_ev_initialize_gpe_block(struct acpi
 
 			gpe_index = (i * ACPI_GPE_REGISTER_WIDTH) + j;
 			gpe_event_info = &gpe_block->event_info[gpe_index];
+			gpe_number = gpe_index + gpe_block->block_base_number;
+
+			/*
+			 * If the GPE has already been enabled for runtime
+			 * signaling, make sure it remains enabled, but do not
+			 * increment its reference counter.
+			 */
+			if (gpe_event_info->runtime_count) {
+				acpi_set_gpe(gpe_device, gpe_number,
+						ACPI_GPE_ENABLE);
+				gpe_enabled_count++;
+				continue;
+			}
 
 			if (gpe_event_info->flags & ACPI_GPE_CAN_WAKE) {
 				wake_gpe_count++;

@@ -516,7 +529,6 @@ acpi_ev_initialize_gpe_block(struct acpi
 
 			/* Enable this GPE */
 
-			gpe_number = gpe_index + gpe_block->block_base_number;
 			status = acpi_enable_gpe(gpe_device, gpe_number,
 						 ACPI_GPE_TYPE_RUNTIME);
 			if (ACPI_FAILURE(status)) {

Rafael J. Wysocki

author

2010-06-08 08:50 UTC #11 lore raw

PATCH 5/5 ACPI / ACPICA: Fix sysfs GPE interface

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

The sysfs interface allowing user space to disable/enable GPEs doesn't work correctly, because a GPE disabled this way will be re-enabled shortly by acpi_ev_asynch_enable_gpe() if it was previosuly enabled by acpi_enable_gpe() (in which case the corresponding bit in its enable register's enable_for_run mask is set).

To address this issue make the sysfs GPE interface use acpi_enable_gpe() and acpi_disable_gpe() instead of acpi_set_gpe() so that GPE reference counters are modified by it along with the values of GPE enable registers.

Signed-off-by Rafael J. Wysocki <[email protected]>

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

drivers/acpi/system.c +4 -2

Index: linux-2.6/drivers/acpi/system.c
===================================================================
@@ -388,10 +388,12 @@ static ssize_t counter_set(struct kobjec
 	if (index < num_gpes) {
 		if (!strcmp(buf, "disable\n") &&
 				(status & ACPI_EVENT_FLAG_ENABLED))
-			result = acpi_set_gpe(handle, index, ACPI_GPE_DISABLE);
+			result = acpi_disable_gpe(handle, index,
+						ACPI_GPE_TYPE_RUNTIME);
 		else if (!strcmp(buf, "enable\n") &&
 				!(status & ACPI_EVENT_FLAG_ENABLED))
-			result = acpi_set_gpe(handle, index, ACPI_GPE_ENABLE);
+			result = acpi_enable_gpe(handle, index,
+						ACPI_GPE_TYPE_RUNTIME);
 		else if (!strcmp(buf, "clear\n") &&
 				(status & ACPI_EVENT_FLAG_SET))
 			result = acpi_clear_gpe(handle, index);

Rafael J. Wysocki

author

2010-06-08 08:50 UTC #12 lore raw

PATCH 5/5 ACPI / ACPICA: Fix sysfs GPE interface

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

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

drivers/acpi/system.c +4 -2

Index: linux-2.6/drivers/acpi/system.c
===================================================================
@@ -388,10 +388,12 @@ static ssize_t counter_set(struct kobjec
 	if (index < num_gpes) {
 		if (!strcmp(buf, "disable\n") &&
 				(status & ACPI_EVENT_FLAG_ENABLED))
-			result = acpi_set_gpe(handle, index, ACPI_GPE_DISABLE);
+			result = acpi_disable_gpe(handle, index,
+						ACPI_GPE_TYPE_RUNTIME);
 		else if (!strcmp(buf, "enable\n") &&
 				!(status & ACPI_EVENT_FLAG_ENABLED))
-			result = acpi_set_gpe(handle, index, ACPI_GPE_ENABLE);
+			result = acpi_enable_gpe(handle, index,
+						ACPI_GPE_TYPE_RUNTIME);
 		else if (!strcmp(buf, "clear\n") &&
 				(status & ACPI_EVENT_FLAG_SET))
 			result = acpi_clear_gpe(handle, index);

Len Brown

reviewer

2010-06-09 04:53 UTC #13 lore raw

In reply to Rafael J. Wysocki

series applied to acpi-test

thanks,
Len Brown, Intel Open Source Technology Center

Len Brown

reviewer

2010-06-09 04:53 UTC #14 lore raw

In reply to Rafael J. Wysocki

series applied to acpi-test

thanks,
Len Brown, Intel Open Source Technology Center