Skip to contentSkip to conversation

ACPI / ACPICA: GPE reference counting fixes

PATCH New Rafael J. Wysocki 1 participant 3 messages May 28, 2010

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover ACPI / ACPICA: GPE reference counting fixes

Hi Len,

The following two patches fix a few problems introduced by the recent GPE reference counting changes.

Please review and apply if they make sense to you.

Thanks,
Rafael

#1 lore raw

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

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

First of all, acpi_set_gpe() won't enable wakeup-only GPEs, because it uses acpi_ev_enable_gpe() that, 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).

Second, 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 and the other way around, so it doesn't make sense to update the enable_for_run and enable_for_wake masks outside of acpi_enable_gpe() and acpi_disable_gpe().

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.

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.

---
 drivers/acpi/acpica/acevents.h |    4 -
 drivers/acpi/acpica/achware.h  |    7 +-
 drivers/acpi/acpica/evgpe.c    |  118 ++---------------------------------------
 drivers/acpi/acpica/evgpeblk.c |   13 ++++
 drivers/acpi/acpica/evxface.c  |    2 
 drivers/acpi/acpica/evxfevnt.c |   59 ++++++++++++++++++--
 drivers/acpi/acpica/hwgpe.c    |  118 +++++++++++++++++++++++------------------
 include/acpi/actypes.h         |    3 -
 8 files changed, 145 insertions(+), 179 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 +12 -1
Index: linux-2.6/drivers/acpi/acpica/evgpeblk.c
===================================================================
@@ -500,6 +500,18 @@ 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 are clear in its register's enable masks.
+			 */
+			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 (gpe_event_info->flags & ACPI_GPE_CAN_WAKE) {
 				wake_gpe_count++;
@@ -516,7 +528,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);
 
#2 lore raw

PATCH 2/2 ACPI / ACPICA: Allow GPEs to be force disabled via sysfs

In reply to Rafael J. Wysocki

After the GPE reference counting patches (commits 9630bdd9b15d2f489c646d8bc04b60e53eb5ec78 and cbbc0de700e61d0cdc854d435dbc2ef148de0e00), 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 fix this, introduce two new action values for acpi_set_gpe(), ACPI_GPE_FORCE_ENABLE and ACPI_GPE_FORCE_DISABLE, such that if acpi_set_gpe() is called with ACPI_GPE_FORCE_DISABLE, the GPE is force disabled and can only be re-enabled by calling acpi_set_gpe() with ACPI_GPE_FORCE_ENABLE. Use these new values in the sysfs interface for disabling/enabling GPEs.

---
 drivers/acpi/acpica/evgpe.c    |    5 ++++-
 drivers/acpi/acpica/evxfevnt.c |   24 ++++++++++++++++++++++++
 drivers/acpi/system.c          |    6 ++++--
 include/acpi/actypes.h         |   19 ++++++++++++-------
 4 files changed, 44 insertions(+), 10 deletions(-)
include/acpi/actypes.h +12 -7
Index: linux-2.6/include/acpi/actypes.h
===================================================================
@@ -668,6 +668,8 @@ typedef u32 acpi_event_status;
 #define ACPI_GPE_ENABLE                 0
 #define ACPI_GPE_DISABLE                1
 #define ACPI_GPE_CHECK_AND_ENABLE       2
+#define ACPI_GPE_FORCE_ENABLE           3
+#define ACPI_GPE_FORCE_DISABLE          4
 
 /* gpe_types for acpi_enable_gpe and acpi_disable_gpe */
 
@@ -677,13 +679,14 @@ typedef u32 acpi_event_status;
 
 /*
  * GPE info flags - Per GPE
- * +-------+---+-+-+
- * |  7:4  |3:2|1|0|
- * +-------+---+-+-+
- *     |     |  | |
- *     |     |  | +--- Interrupt type: edge or level triggered
- *     |     |  +----- GPE can wake the system
- *     |     +-------- Type of dispatch:to method, handler, or none
+ * +-------+-+---+-+-+
+ * |  7:5  |4|3:2|1|0|
+ * +-------+-+---+-+-+
+ *     |    |  |  | |
+ *     |    |  |  | +--- Interrupt type: edge or level triggered
+ *     |    |  |  +----- GPE can wake the system
+ *     |    |  +-------- Type of dispatch:to method, handler, or none
+ *     |    +----------- GPE has been forcibly disabled
  *     +-------------- <Reserved>
  */
 #define ACPI_GPE_XRUPT_TYPE_MASK        (u8) 0x01
@@ -697,6 +700,8 @@ typedef u32 acpi_event_status;
 #define ACPI_GPE_DISPATCH_METHOD        (u8) 0x08
 #define ACPI_GPE_DISPATCH_NOT_USED      (u8) 0x00
 
+#define ACPI_GPE_FORCE_DISABLED		(u8) 0x10
+
 /*
  * Flags for GPE and Lock interfaces
  */
drivers/acpi/acpica/evgpe.c +4 -1
Index: linux-2.6/drivers/acpi/acpica/evgpe.c
===================================================================
@@ -417,7 +417,10 @@ static void acpi_ev_asynch_enable_gpe(vo
 	}
 
 	/* Enable this GPE */
-	(void)acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_CHECK_AND_ENABLE);
+	if (!(gpe_event_info->flags & ACPI_GPE_FORCE_DISABLED)) {
+		(void)acpi_hw_low_set_gpe(gpe_event_info,
+					  ACPI_GPE_CHECK_AND_ENABLE);
+	}
 	return_VOID;
 }
 
drivers/acpi/acpica/evxfevnt.c +24 -0
Index: linux-2.6/drivers/acpi/acpica/evxfevnt.c
===================================================================
@@ -281,10 +281,24 @@ acpi_status acpi_set_gpe(acpi_handle gpe
 		status = acpi_clear_and_enable_gpe(gpe_event_info);
 		break;
 
+	case ACPI_GPE_FORCE_ENABLE:
+		status = acpi_clear_and_enable_gpe(gpe_event_info);
+		if (ACPI_SUCCESS(status)) {
+			gpe_event_info->flags &= ~(ACPI_GPE_FORCE_DISABLED);
+		}
+		break;
+
 	case ACPI_GPE_DISABLE:
 		status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_DISABLE);
 		break;
 
+	case ACPI_GPE_FORCE_DISABLE:
+		status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_DISABLE);
+		if (ACPI_SUCCESS(status)) {
+			gpe_event_info->flags |= ACPI_GPE_FORCE_DISABLED;
+		}
+		break;
+
 	default:
 		status = AE_BAD_PARAMETER;
 		break;
@@ -337,6 +351,11 @@ acpi_status acpi_enable_gpe(acpi_handle 
 		goto unlock_and_exit;
 	}
 
+	if (gpe_event_info->flags & ACPI_GPE_FORCE_DISABLED) {
+		status = AE_ERROR;
+		goto unlock_and_exit;
+	}
+
 	if (gpe_type & ACPI_GPE_TYPE_RUNTIME) {
 		if (gpe_event_info->runtime_count == ACPI_UINT8_MAX) {
 			status = AE_LIMIT;	/* Too many references */
@@ -426,6 +445,11 @@ acpi_status acpi_disable_gpe(acpi_handle
 		goto unlock_and_exit;
 	}
 
+	if (gpe_event_info->flags & ACPI_GPE_FORCE_DISABLED) {
+		status = AE_ERROR;
+		goto unlock_and_exit;
+	}
+
 	/* Hardware-disable a runtime GPE on removal of the last reference */
 
 	if (gpe_type & ACPI_GPE_TYPE_RUNTIME) {
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_set_gpe(handle, index,
+						ACPI_GPE_FORCE_DISABLE);
 		else if (!strcmp(buf, "enable\n") &&
 				!(status & ACPI_EVENT_FLAG_ENABLED))
-			result = acpi_set_gpe(handle, index, ACPI_GPE_ENABLE);
+			result = acpi_set_gpe(handle, index,
+						ACPI_GPE_FORCE_ENABLE);
 		else if (!strcmp(buf, "clear\n") &&
 				(status & ACPI_EVENT_FLAG_SET))
 			result = acpi_clear_gpe(handle, index);