[PATCH] ACPI / ACPICA: Defer enabling of runtime GPEs (v2)

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Monday, August 30, 2010, Bjorn Helgaas wrote:
> On Saturday, August 28, 2010 05:10:56 pm Rafael J. Wysocki wrote:
> > Bob, Matthew,
> > 
> > The patch below implements the idea we've been recently discussing off-list,
> > to only enable "runtime" GPEs after the ACPI namespace have been scanned and
> > we know what GPEs are pointed to by _PRW methods.
> > 
> > It is on top of the current mainline Linux kernel (2.6.36-rc2+) which contains
> > my patches that removed the _PRW scan from the ACPICA code and modified
> > the GPE handler installation/removal.
> > 
> > Fortunately, the patch turns out to be rather straightforward, although I have
> > one reservation.  Namely, I'm not sure if any GPEs can be added after
> > acpi_scan_init() has returned, in which case we may want to enable them
> > as soon as they are found and some code to do that will be necessary.
> 
> Isn't it possible (at least in theory) to hot-add an ACPI0006 GPE Block
> Device after acpi_scan_init()?

Yes, in theory it is possible, but I'm not sure we handle that at all.

Just in case, though, I reworked the patch so that the "hot-added" GPEs
are automatically enabled if they are added after the initial namespace scan.
The new version is appended.

> > ---
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > Subject: ACPI / ACPICA: Defer enabling of runtime GPEs
> > 
> > The current ACPI GPEs initialization code has a problem that it
> > enables some GPEs pointed to by device _PRW methods and are generally
> > intended for signaling wakeup events (system or device wakeup).
> > These GPEs are then almost immediately disabled by the ACPI namespace
> > scanning code with the help of acpi_gpe_can_wake(), but it would be
> > better no to enable them at all until really necessary.
> 
> Typo:    ^ I think you meant "not"

That's correct, thanks.  It's fixed in the changelog below.

> > Modify the initialization of GPEs so that the ones that have
> > associated _Lxx or _Exx methods and are not pointed to by any _PRW
> > methods will be enabled after the namespace scan is complete.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: ACPI / ACPICA: Defer enabling of runtime GPEs (v2)

The current ACPI GPEs initialization code has a problem that it
enables some GPEs pointed to by device _PRW methods, generally
intended for signaling wakeup events (system or device wakeup).
These GPEs are then almost immediately disabled by the ACPI namespace
scanning code with the help of acpi_gpe_can_wake(), but it would be
better not to enable them at all until really necessary.

Modify the initialization of GPEs so that the ones that have
associated _Lxx or _Exx methods and are not pointed to by any _PRW
methods will be enabled after the namespace scan is complete.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/acpica/acevents.h  |    5 ++-
 drivers/acpi/acpica/acglobal.h  |    1 
 drivers/acpi/acpica/aclocal.h   |    1 
 drivers/acpi/acpica/evevent.c   |   41 -----------------------------
 drivers/acpi/acpica/evgpeblk.c  |   33 +++++++----------------
 drivers/acpi/acpica/evgpeinit.c |    2 -
 drivers/acpi/acpica/evxface.c   |   19 +++++++------
 drivers/acpi/acpica/evxfevnt.c  |   56 +++++++++++++++++++++++++++-------------
 drivers/acpi/acpica/utglobal.c  |    1 
 drivers/acpi/acpica/utxface.c   |   13 ---------
 drivers/acpi/bus.c              |    1 
 include/acpi/acpixf.h           |    2 +
 12 files changed, 70 insertions(+), 105 deletions(-)

Index: linux-2.6/drivers/acpi/acpica/evxfevnt.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/evxfevnt.c
+++ linux-2.6/drivers/acpi/acpica/evxfevnt.c
@@ -379,21 +379,12 @@ acpi_status acpi_gpe_can_wake(acpi_handl
 	/* Ensure that we have a valid GPE number */
 
 	gpe_event_info = acpi_ev_get_gpe_event_info(gpe_device, gpe_number);
-	if (!gpe_event_info) {
+	if (gpe_event_info) {
+		gpe_event_info->flags |= ACPI_GPE_CAN_WAKE;
+	} else {
 		status = AE_BAD_PARAMETER;
-		goto unlock_and_exit;
-	}
-
-	if (gpe_event_info->flags & ACPI_GPE_CAN_WAKE) {
-		goto unlock_and_exit;
 	}
 
-	gpe_event_info->flags |= ACPI_GPE_CAN_WAKE;
-	if (gpe_event_info->flags & ACPI_GPE_DISPATCH_METHOD) {
-		(void)acpi_raw_disable_gpe(gpe_event_info);
-	}
-
-unlock_and_exit:
 	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
 	return_ACPI_STATUS(status);
 }
@@ -651,7 +642,7 @@ acpi_install_gpe_block(acpi_handle gpe_d
 		       struct acpi_generic_address *gpe_block_address,
 		       u32 register_count, u32 interrupt_number)
 {
-	acpi_status status;
+	acpi_status status = AE_OK;
 	union acpi_operand_object *obj_desc;
 	struct acpi_namespace_node *node;
 	struct acpi_gpe_block_info *gpe_block;
@@ -715,10 +706,6 @@ acpi_install_gpe_block(acpi_handle gpe_d
 
 	obj_desc->device.gpe_block = gpe_block;
 
-	/* Enable the runtime GPEs in the new block */
-
-	status = acpi_ev_initialize_gpe_block(node, gpe_block);
-
       unlock_and_exit:
 	(void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
 	return_ACPI_STATUS(status);
@@ -924,3 +911,38 @@ acpi_status acpi_enable_all_runtime_gpes
 
 	return_ACPI_STATUS(status);
 }
+
+/******************************************************************************
+ *
+ * FUNCTION:    acpi_start_gpes
+ *
+ * PARAMETERS:  None
+ *
+ * RETURN:      None
+ *
+ * DESCRIPTION: Enable all GPEs that have associated _Lxx or _Exx methods and
+ *              are not pointed to by any device _PRW methods indicating that
+ *              these GPEs are generally intended for system or device wakeup
+ *              (such GPEs have to be enabled directly when the devices whose
+ *              _PRW methods point to them are set up for wakeup signaling).
+ *
+ ******************************************************************************/
+
+acpi_status acpi_start_gpes(void)
+{
+	acpi_status status;
+
+	ACPI_FUNCTION_TRACE(acpi_start_gpes);
+
+	status = acpi_ut_acquire_mutex(ACPI_MTX_EVENTS);
+	if (ACPI_FAILURE(status)) {
+		return_ACPI_STATUS(status);
+	}
+
+	acpi_auto_enable_gpes = TRUE;
+	status = acpi_ev_walk_gpe_list(acpi_ev_initialize_gpe_block, NULL);
+
+	(void)acpi_ut_release_mutex(ACPI_MTX_EVENTS);
+
+	return_ACPI_STATUS(status);
+}
Index: linux-2.6/drivers/acpi/acpica/acevents.h
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/acevents.h
+++ linux-2.6/drivers/acpi/acpica/acevents.h
@@ -105,8 +105,9 @@ acpi_ev_create_gpe_block(struct acpi_nam
 			 struct acpi_gpe_block_info **return_gpe_block);
 
 acpi_status
-acpi_ev_initialize_gpe_block(struct acpi_namespace_node *gpe_device,
-			     struct acpi_gpe_block_info *gpe_block);
+acpi_ev_initialize_gpe_block(struct acpi_gpe_xrupt_info *gpe_xrupt_info,
+			     struct acpi_gpe_block_info *gpe_block,
+			     void *ignored);
 
 acpi_status acpi_ev_delete_gpe_block(struct acpi_gpe_block_info *gpe_block);
 
Index: linux-2.6/drivers/acpi/acpica/evgpeblk.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/evgpeblk.c
+++ linux-2.6/drivers/acpi/acpica/evgpeblk.c
@@ -434,14 +434,14 @@ acpi_ev_create_gpe_block(struct acpi_nam
  ******************************************************************************/
 
 acpi_status
-acpi_ev_initialize_gpe_block(struct acpi_namespace_node *gpe_device,
-			     struct acpi_gpe_block_info *gpe_block)
+acpi_ev_initialize_gpe_block(struct acpi_gpe_xrupt_info *gpe_xrupt_info,
+			     struct acpi_gpe_block_info *gpe_block,
+			     void *ignored)
 {
 	acpi_status status;
 	struct acpi_gpe_event_info *gpe_event_info;
 	u32 gpe_enabled_count;
 	u32 gpe_index;
-	u32 gpe_number;
 	u32 i;
 	u32 j;
 
@@ -454,15 +454,12 @@ acpi_ev_initialize_gpe_block(struct acpi
 	}
 
 	/*
-	 * Enable all GPEs that have a corresponding method.  Any other GPEs
-	 * within this block must be enabled via the acpi_enable_gpe interface.
+	 * Enable all GPEs that have a corresponding method and have the
+	 * ACPI_GPE_CAN_WAKE flag unset.  Any other GPEs within this block must
+	 * be enabled via the acpi_enable_gpe() interface.
 	 */
 	gpe_enabled_count = 0;
 
-	if (gpe_device == acpi_gbl_fadt_gpe_device) {
-		gpe_device = NULL;
-	}
-
 	for (i = 0; i < gpe_block->register_count; i++) {
 		for (j = 0; j < ACPI_GPE_REGISTER_WIDTH; j++) {
 
@@ -470,27 +467,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;
 
 			/* Ignore GPEs that have no corresponding _Lxx/_Exx method */
 
-			if (!(gpe_event_info->flags & ACPI_GPE_DISPATCH_METHOD)) {
+			if (!(gpe_event_info->flags & ACPI_GPE_DISPATCH_METHOD)
+			    || (gpe_event_info->flags & ACPI_GPE_CAN_WAKE)) {
 				continue;
 			}
 
-			/*
-			 * If the GPE has already been enabled for runtime
-			 * signaling, make sure it remains enabled, but do not
-			 * increment its reference counter.
-			 */
-			status = gpe_event_info->runtime_count ?
-				acpi_ev_enable_gpe(gpe_event_info) :
-				acpi_enable_gpe(gpe_device, gpe_number);
-
+			status = acpi_raw_enable_gpe(gpe_event_info);
 			if (ACPI_FAILURE(status)) {
 				ACPI_EXCEPTION((AE_INFO, status,
-						"Could not enable GPE 0x%02X",
-						gpe_number));
+					"Could not enable GPE 0x%02X",
+					gpe_index + gpe_block->block_base_number));
 				continue;
 			}
 
Index: linux-2.6/include/acpi/acpixf.h
===================================================================
--- linux-2.6.orig/include/acpi/acpixf.h
+++ linux-2.6/include/acpi/acpixf.h
@@ -308,6 +308,8 @@ acpi_install_gpe_block(acpi_handle gpe_d
 
 acpi_status acpi_remove_gpe_block(acpi_handle gpe_device);
 
+acpi_status acpi_start_gpes(void);
+
 /*
  * Resource interfaces
  */
Index: linux-2.6/drivers/acpi/acpica/aclocal.h
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/aclocal.h
+++ linux-2.6/drivers/acpi/acpica/aclocal.h
@@ -413,6 +413,7 @@ struct acpi_handler_info {
 	void *context;		/* Context to be passed to handler */
 	struct acpi_namespace_node *method_node;	/* Method node for this GPE level (saved) */
 	u8 orig_flags;		/* Original misc info about this GPE */
+	u8 orig_enabled;	/* Set if the GPE was originally enabled */
 };
 
 union acpi_gpe_dispatch_info {
Index: linux-2.6/drivers/acpi/acpica/evevent.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/evevent.c
+++ linux-2.6/drivers/acpi/acpica/evevent.c
@@ -95,47 +95,6 @@ acpi_status acpi_ev_initialize_events(vo
 
 /*******************************************************************************
  *
- * FUNCTION:    acpi_ev_install_fadt_gpes
- *
- * PARAMETERS:  None
- *
- * RETURN:      Status
- *
- * DESCRIPTION: Completes initialization of the FADT-defined GPE blocks
- *              (0 and 1). The HW must be fully initialized at this point,
- *              including global lock support.
- *
- ******************************************************************************/
-
-acpi_status acpi_ev_install_fadt_gpes(void)
-{
-	acpi_status status;
-
-	ACPI_FUNCTION_TRACE(ev_install_fadt_gpes);
-
-	/* Namespace must be locked */
-
-	status = acpi_ut_acquire_mutex(ACPI_MTX_NAMESPACE);
-	if (ACPI_FAILURE(status)) {
-		return (status);
-	}
-
-	/* FADT GPE Block 0 */
-
-	(void)acpi_ev_initialize_gpe_block(acpi_gbl_fadt_gpe_device,
-					   acpi_gbl_gpe_fadt_blocks[0]);
-
-	/* FADT GPE Block 1 */
-
-	(void)acpi_ev_initialize_gpe_block(acpi_gbl_fadt_gpe_device,
-					   acpi_gbl_gpe_fadt_blocks[1]);
-
-	(void)acpi_ut_release_mutex(ACPI_MTX_NAMESPACE);
-	return_ACPI_STATUS(AE_OK);
-}
-
-/*******************************************************************************
- *
  * FUNCTION:    acpi_ev_install_xrupt_handlers
  *
  * PARAMETERS:  None
Index: linux-2.6/drivers/acpi/acpica/utxface.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/utxface.c
+++ linux-2.6/drivers/acpi/acpica/utxface.c
@@ -290,19 +290,6 @@ acpi_status acpi_initialize_objects(u32 
 	}
 
 	/*
-	 * Complete the GPE initialization for the GPE blocks defined in the FADT
-	 * (GPE block 0 and 1).
-	 *
-	 * NOTE: Currently, there seems to be no need to run the _REG methods
-	 * before enabling the GPEs.
-	 */
-	if (!(flags & ACPI_NO_EVENT_INIT)) {
-		status = acpi_ev_install_fadt_gpes();
-		if (ACPI_FAILURE(status))
-			return (status);
-	}
-
-	/*
 	 * Empty the caches (delete the cached objects) on the assumption that
 	 * the table load filled them up more than they will be at runtime --
 	 * thus wasting non-paged memory.
Index: linux-2.6/drivers/acpi/bus.c
===================================================================
--- linux-2.6.orig/drivers/acpi/bus.c
+++ linux-2.6/drivers/acpi/bus.c
@@ -1032,6 +1032,7 @@ static int __init acpi_init(void)
 	dmi_check_system(power_nocheck_dmi_table);
 
 	acpi_scan_init();
+	acpi_start_gpes();
 	acpi_ec_init();
 	acpi_power_init();
 	acpi_sysfs_init();
Index: linux-2.6/drivers/acpi/acpica/evxface.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/evxface.c
+++ linux-2.6/drivers/acpi/acpica/evxface.c
@@ -793,15 +793,16 @@ acpi_install_gpe_handler(acpi_handle gpe
 			(ACPI_GPE_XRUPT_TYPE_MASK | ACPI_GPE_DISPATCH_MASK);
 
 	/*
-	 * If the GPE is associated with a method and it cannot wake up the
-	 * system from sleep states, it was enabled automatically during
-	 * initialization, so it has to be disabled now to avoid spurious
-	 * execution of the handler.
+	 * If the GPE is associated with a method, it might have been enabled
+	 * automatically during initialization, in which case it has to be
+	 * disabled now to avoid spurious execution of the handler.
 	 */
 
 	if ((handler->orig_flags & ACPI_GPE_DISPATCH_METHOD)
-	    && !(gpe_event_info->flags & ACPI_GPE_CAN_WAKE))
+	    && gpe_event_info->runtime_count) {
+		handler->orig_enabled = 1;
 		(void)acpi_raw_disable_gpe(gpe_event_info);
+	}
 
 	/* Install the handler */
 
@@ -904,13 +905,13 @@ acpi_remove_gpe_handler(acpi_handle gpe_
 	gpe_event_info->flags |= handler->orig_flags;
 
 	/*
-	 * If the GPE was previously associated with a method and it cannot wake
-	 * up the system from sleep states, it should be enabled at this point
-	 * to restore the post-initialization configuration.
+	 * If the GPE was previously associated with a method and it was
+	 * enabled, it should be enabled at this point to restore the
+	 * post-initialization configuration.
 	 */
 
 	if ((handler->orig_flags & ACPI_GPE_DISPATCH_METHOD)
-	    && !(gpe_event_info->flags & ACPI_GPE_CAN_WAKE))
+	    && handler->orig_enabled)
 		(void)acpi_raw_enable_gpe(gpe_event_info);
 
 	/* Now we can free the handler object */
Index: linux-2.6/drivers/acpi/acpica/acglobal.h
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/acglobal.h
+++ linux-2.6/drivers/acpi/acpica/acglobal.h
@@ -364,6 +364,7 @@ ACPI_EXTERN struct acpi_fixed_event_hand
 ACPI_EXTERN struct acpi_gpe_xrupt_info *acpi_gbl_gpe_xrupt_list_head;
 ACPI_EXTERN struct acpi_gpe_block_info
 *acpi_gbl_gpe_fadt_blocks[ACPI_MAX_GPE_BLOCKS];
+ACPI_EXTERN u8 acpi_auto_enable_gpes;
 
 /*****************************************************************************
  *
Index: linux-2.6/drivers/acpi/acpica/utglobal.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/utglobal.c
+++ linux-2.6/drivers/acpi/acpica/utglobal.c
@@ -766,6 +766,7 @@ acpi_status acpi_ut_init_globals(void)
 	acpi_gbl_gpe_fadt_blocks[0] = NULL;
 	acpi_gbl_gpe_fadt_blocks[1] = NULL;
 	acpi_current_gpe_count = 0;
+	acpi_auto_enable_gpes = FALSE;
 
 	/* Global handlers */
 
Index: linux-2.6/drivers/acpi/acpica/evgpeinit.c
===================================================================
--- linux-2.6.orig/drivers/acpi/acpica/evgpeinit.c
+++ linux-2.6/drivers/acpi/acpica/evgpeinit.c
@@ -239,7 +239,7 @@ void acpi_ev_update_gpes(acpi_owner_id t
 	walk_info.owner_id = table_owner_id;
 	walk_info.execute_by_owner_id = TRUE;
 	walk_info.count = 0;
-	walk_info.enable_this_gpe = TRUE;
+	walk_info.enable_this_gpe = acpi_auto_enable_gpes;
 
 	/* Walk the interrupt level descriptor list */