[PATCH 1/3] ACPICA: Events: Introduce acpi_block_gpe()/acpi_unblock_gpe()/acpi_control_gpe_handling() to allow administrative GPE enabling/disabling

[PATCH 1/3] ACPICA: Events: Introduce acpi_block_gpe()/acpi_unblock_gpe()/acpi_control_gpe_handling() to allow administrative GPE enabling/disabling

[Lv Zheng]

There is a facility in Linux, developers can manage GPE enabling/disabling
through /sys/firmware/acpi/interrupts/gpexx. This is mainly for debugging
purposes. Many users expect to use this facility to implement quirks to
disable specific GPEs when there is a gap in Linux causing GPE flood.
This is not working correctly because currently this facility invokes
enabling/disabling counting based GPE driver APIs:
 acpi_enable_gpe()/acpi_disable_gpe()
and the GPE drivers can still affect the count to mess up the GPE
management purposes.

This patch introduces acpi_block_gpe()/acpi_unblock_gpe() to be used in such
situation instead of acpi_enable_gpe()/acpi_disable_gpe().

The idea to implement this is to replace the GPE register EN bit with the
managed value, block EN set/clear operations but record the operation
results in blocked_enabled, so that after the managed state is removed,
restore the saved blocked_enabled back to the GPE register EN bit.

Now OSPMs should be able to use this facility to generate quirks. ACPICA
BZ 1102.

This facility can also be used by the administrator to control the GPE
handling mode during the runtime when the driver is capable of handling the
GPE in both the interrupt mode and the polling mode (for example, the Linux
EC driver). acpi_control_gpe_handling() is offered for this purpose. Lv Zheng.

Link: https://bugs.acpica.org/show_bug.cgi?id=1102
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/acpica/acevents.h |    3 +
 drivers/acpi/acpica/aclocal.h  |    1 +
 drivers/acpi/acpica/evgpe.c    |   84 ++++++++++++++++++++++++
 drivers/acpi/acpica/evxfgpe.c  |  142 ++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/acpica/hwgpe.c    |   24 ++++++-
 include/acpi/acpixf.h          |   23 +++++--
 include/acpi/actypes.h         |   85 +++++++++++++++++++-----
 7 files changed, 336 insertions(+), 26 deletions(-)

diff --git a/drivers/acpi/acpica/acevents.h b/drivers/acpi/acpica/acevents.h
index 77af91c..3a6e4db 100644
--- a/drivers/acpi/acpica/acevents.h
+++ b/drivers/acpi/acpica/acevents.h
@@ -86,6 +86,9 @@ acpi_ev_update_gpe_enable_mask(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_manage_gpe(struct acpi_gpe_event_info *gpe_event_info, u8 action);
+
+acpi_status
 acpi_ev_add_gpe_reference(struct acpi_gpe_event_info *gpe_event_info);
 
 acpi_status
diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
index 13331d7..17a6834 100644
--- a/drivers/acpi/acpica/aclocal.h
+++ b/drivers/acpi/acpica/aclocal.h
@@ -484,6 +484,7 @@ struct acpi_gpe_event_info {
 	u8 flags;		/* Misc info about this GPE */
 	u8 gpe_number;		/* This GPE */
 	u8 runtime_count;	/* References to a run GPE */
+	u8 blocked_enabled;	/* GPE should be enabled but blocked */
 };
 
 /* Information about a GPE register pair, one per each status/enable pair in an array */
diff --git a/drivers/acpi/acpica/evgpe.c b/drivers/acpi/acpica/evgpe.c
index 4b4949c..f821bdd 100644
--- a/drivers/acpi/acpica/evgpe.c
+++ b/drivers/acpi/acpica/evgpe.c
@@ -130,6 +130,90 @@ acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info)
 
 /*******************************************************************************
  *
+ * FUNCTION:    acpi_ev_manage_gpe
+ *
+ * PARAMETERS:  gpe_event_info          - GPE to force enabling/disabling
+ *              action                  - ACPI_GPE_ENABLE, ACPI_GPE_DISABLE or
+ *                                        ACPI_GPE_UNMANAGE
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Unconditionally enable or disable an individual GPE for the
+ *              administrative purposes during the runtime.
+ *
+ ******************************************************************************/
+
+acpi_status
+acpi_ev_manage_gpe(struct acpi_gpe_event_info *gpe_event_info, u8 action)
+{
+	acpi_status status;
+	acpi_event_status event_status;
+
+	ACPI_FUNCTION_TRACE(ev_manage_gpe);
+
+	/* Perform the action */
+
+	switch (action) {
+	case ACPI_GPE_ENABLE:
+	case ACPI_GPE_DISABLE:
+
+		if (!(gpe_event_info->flags & ACPI_GPE_MANAGED_FLAG_MASK)) {
+			status =
+			    acpi_hw_get_gpe_status(gpe_event_info,
+						   &event_status);
+			if (ACPI_FAILURE(status)) {
+				return_ACPI_STATUS(status);
+			}
+			if (event_status & ACPI_EVENT_FLAG_ENABLE_SET) {
+				gpe_event_info->blocked_enabled = TRUE;
+			} else {
+				gpe_event_info->blocked_enabled = FALSE;
+			}
+		}
+
+		/* Reset flags so that acpi_hw_low_set_gpe() can take effective */
+
+		gpe_event_info->flags &= ~ACPI_GPE_MANAGED_FLAG_MASK;
+		if (action == ACPI_GPE_ENABLE) {
+			(void)acpi_hw_low_set_gpe(gpe_event_info,
+						  ACPI_GPE_ENABLE);
+			gpe_event_info->flags |= ACPI_GPE_MANAGED_ENABLED;
+		} else {
+			(void)acpi_hw_low_set_gpe(gpe_event_info,
+						  ACPI_GPE_DISABLE);
+			gpe_event_info->flags |= ACPI_GPE_MANAGED_DISABLED;
+		}
+		break;
+
+	case ACPI_GPE_UNMANAGE:
+
+		if (!(gpe_event_info->flags & ACPI_GPE_MANAGED_FLAG_MASK)) {
+			return_ACPI_STATUS(AE_BAD_PARAMETER);
+		}
+
+		/* Reset flags so that acpi_hw_low_set_gpe() can take effective */
+
+		gpe_event_info->flags &= ~ACPI_GPE_MANAGED_FLAG_MASK;
+		if (gpe_event_info->blocked_enabled) {
+			(void)acpi_hw_low_set_gpe(gpe_event_info,
+						  ACPI_GPE_ENABLE);
+		} else {
+			(void)acpi_hw_low_set_gpe(gpe_event_info,
+						  ACPI_GPE_DISABLE);
+		}
+		gpe_event_info->blocked_enabled = FALSE;
+		break;
+
+	default:
+
+		return_ACPI_STATUS(AE_BAD_PARAMETER);
+	}
+
+	return_ACPI_STATUS(AE_OK);
+}
+
+/*******************************************************************************
+ *
  * FUNCTION:    acpi_ev_add_gpe_reference
  *
  * PARAMETERS:  gpe_event_info          - Add a reference to this GPE
diff --git a/drivers/acpi/acpica/evxfgpe.c b/drivers/acpi/acpica/evxfgpe.c
index 17cfef7..3ad8fa9 100644
--- a/drivers/acpi/acpica/evxfgpe.c
+++ b/drivers/acpi/acpica/evxfgpe.c
@@ -257,6 +257,148 @@ ACPI_EXPORT_SYMBOL(acpi_set_gpe)
 
 /*******************************************************************************
  *
+ * FUNCTION:    acpi_block_gpe
+ *
+ * PARAMETERS:  gpe_device          - Parent GPE Device. NULL for GPE0/GPE1
+ *              gpe_number          - GPE level within the GPE block
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Unconditionally disable an individual GPE.
+ *              This API is typically used by the system administrator to
+ *              block the GPE enabling, ex., prevent the GPE flooding.
+ *
+ ******************************************************************************/
+acpi_status acpi_block_gpe(acpi_handle gpe_device, u32 gpe_number)
+{
+	struct acpi_gpe_event_info *gpe_event_info;
+	acpi_status status;
+	acpi_cpu_flags flags;
+
+	ACPI_FUNCTION_TRACE(acpi_block_gpe);
+
+	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
+
+	/* 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) {
+		status = AE_BAD_PARAMETER;
+		goto unlock_and_exit;
+	}
+
+	status = acpi_ev_manage_gpe(gpe_event_info, ACPI_GPE_DISABLE);
+
+unlock_and_exit:
+	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
+	return_ACPI_STATUS(status);
+}
+
+ACPI_EXPORT_SYMBOL(acpi_block_gpe)
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_unblock_gpe
+ *
+ * PARAMETERS:  gpe_device          - Parent GPE Device. NULL for GPE0/GPE1
+ *              gpe_number          - GPE level within the GPE block
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Stop unconditional GPE disabling.
+ *              This API reverts acpi_block_gpe().
+ *
+ ******************************************************************************/
+acpi_status acpi_unblock_gpe(acpi_handle gpe_device, u32 gpe_number)
+{
+	struct acpi_gpe_event_info *gpe_event_info;
+	acpi_status status;
+	acpi_cpu_flags flags;
+
+	ACPI_FUNCTION_TRACE(acpi_unblock_gpe);
+
+	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
+
+	/* 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) {
+		status = AE_BAD_PARAMETER;
+		goto unlock_and_exit;
+	}
+
+	status = acpi_ev_manage_gpe(gpe_event_info, ACPI_GPE_UNMANAGE);
+
+unlock_and_exit:
+	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
+	return_ACPI_STATUS(status);
+}
+
+ACPI_EXPORT_SYMBOL(acpi_unblock_gpe)
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_control_gpe_handling
+ *
+ * PARAMETERS:  gpe_device          - Parent GPE Device. NULL for GPE0/GPE1
+ *              gpe_number          - GPE level within the GPE block
+ *              use_interrupt_mode  - Allow the GPE to be dispatched in the
+ *                                    interrupt mode
+ *              use_polling_mode    - Allow the GPE to be dispatched in the
+ *                                    polling mode
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Switch the GPE handling mode between the interrupt only mode,
+ *              the polling only mode and the interrupt/polling adaptive mode.
+ *
+ ******************************************************************************/
+acpi_status
+acpi_control_gpe_handling(acpi_handle gpe_device,
+			  u32 gpe_number,
+			  u8 use_interrupt_mode, u8 use_polling_mode)
+{
+	struct acpi_gpe_event_info *gpe_event_info;
+	acpi_status status;
+	acpi_cpu_flags flags;
+	u8 action;
+
+	ACPI_FUNCTION_TRACE(acpi_control_gpe_handling);
+
+	if (!use_interrupt_mode && !use_polling_mode) {
+		return_ACPI_STATUS(AE_BAD_PARAMETER);
+	}
+
+	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
+
+	/* 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) {
+		status = AE_BAD_PARAMETER;
+		goto unlock_and_exit;
+	}
+
+	/* Pick up and peform the correct action */
+
+	action = ACPI_GPE_UNMANAGE;
+	if (!use_interrupt_mode) {
+		action = ACPI_GPE_DISABLE;
+	}
+	if (!use_polling_mode) {
+		action = ACPI_GPE_ENABLE;
+	}
+	status = acpi_ev_manage_gpe(gpe_event_info, action);
+
+unlock_and_exit:
+	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
+	return_ACPI_STATUS(status);
+}
+
+ACPI_EXPORT_SYMBOL(acpi_control_gpe_handling)
+
+/*******************************************************************************
+ *
  * FUNCTION:    acpi_mark_gpe_for_wake
  *
  * PARAMETERS:  gpe_device          - Parent GPE Device. NULL for GPE0/GPE1
diff --git a/drivers/acpi/acpica/hwgpe.c b/drivers/acpi/acpica/hwgpe.c
index bdecd5e..8cdddbe 100644
--- a/drivers/acpi/acpica/hwgpe.c
+++ b/drivers/acpi/acpica/hwgpe.c
@@ -98,7 +98,7 @@ acpi_status
 acpi_hw_low_set_gpe(struct acpi_gpe_event_info *gpe_event_info, u32 action)
 {
 	struct acpi_gpe_register_info *gpe_register_info;
-	acpi_status status;
+	acpi_status status = AE_OK;
 	u32 enable_mask;
 	u32 register_bit;
 
@@ -148,9 +148,21 @@ acpi_hw_low_set_gpe(struct acpi_gpe_event_info *gpe_event_info, u32 action)
 		return (AE_BAD_PARAMETER);
 	}
 
-	/* Write the updated enable mask */
+	/* Check if there is an administrative GPE enabling/disabling */
 
-	status = acpi_hw_write(enable_mask, &gpe_register_info->enable_address);
+	if (gpe_event_info->flags & ACPI_GPE_MANAGED_FLAG_MASK) {
+		if (enable_mask & register_bit) {
+			gpe_event_info->blocked_enabled = TRUE;
+		} else {
+			gpe_event_info->blocked_enabled = FALSE;
+		}
+	} else {
+		/* Write the updated enable mask */
+
+		status =
+		    acpi_hw_write(enable_mask,
+				  &gpe_register_info->enable_address);
+	}
 	return (status);
 }
 
@@ -228,6 +240,12 @@ acpi_hw_get_gpe_status(struct acpi_gpe_event_info *gpe_event_info,
 		local_event_status |= ACPI_EVENT_FLAG_HAS_HANDLER;
 	}
 
+	/* GPE currently managed? (enabled/disabled by the system admin?) */
+
+	if (gpe_event_info->flags & ACPI_GPE_MANAGED_FLAG_MASK) {
+		local_event_status |= ACPI_EVENT_FLAG_MANAGED;
+	}
+
 	/* Get the info block for the entire GPE register */
 
 	gpe_register_info = gpe_event_info->register_info;
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 4e4c214..a0a6544 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -732,15 +732,28 @@ ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
 						u32 gpe_number))
 
 ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
-				acpi_mark_gpe_for_wake(acpi_handle gpe_device,
-						       u32 gpe_number))
+				acpi_block_gpe(acpi_handle gpe_device,
+					       u32 gpe_number))
+
+ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
+				acpi_unblock_gpe(acpi_handle gpe_device,
+						 u32 gpe_number))
 
 ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
-				acpi_setup_gpe_for_wake(acpi_handle
-							parent_device,
-							acpi_handle gpe_device,
+				acpi_control_gpe_handling(acpi_handle
+							  gpe_device,
+							  u32 gpe_number,
+							  u8 use_interrupt_mode,
+							  u8 use_polling_mode))
+ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
+				 acpi_mark_gpe_for_wake(acpi_handle gpe_device,
 							u32 gpe_number))
 ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
+				 acpi_setup_gpe_for_wake(acpi_handle
+							 parent_device,
+							 acpi_handle gpe_device,
+							 u32 gpe_number))
+ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
 				 acpi_set_gpe_wake_mask(acpi_handle gpe_device,
 							u32 gpe_number,
 							u8 action))
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index cb389ef..9f841f2 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -732,16 +732,17 @@ typedef u32 acpi_event_type;
  * The encoding of acpi_event_status is illustrated below.
  * Note that a set bit (1) indicates the property is TRUE
  * (e.g. if bit 0 is set then the event is enabled).
- * +-------------+-+-+-+-+-+
- * |   Bits 31:5 |4|3|2|1|0|
- * +-------------+-+-+-+-+-+
- *          |     | | | | |
- *          |     | | | | +- Enabled?
- *          |     | | | +--- Enabled for wake?
- *          |     | | +----- Status bit set?
- *          |     | +------- Enable bit set?
- *          |     +--------- Has a handler?
- *          +--------------- <Reserved>
+ * +-------------+-+-+-+-+-+-+
+ * |   Bits 31:6 |5|4|3|2|1|0|
+ * +-------------+-+-+-+-+-+-+
+ *          |     | | | | | |
+ *          |     | | | | | +- Enabled?
+ *          |     | | | | +--- Enabled for wake?
+ *          |     | | | +----- Status bit set?
+ *          |     | | +------- Enable bit set?
+ *          |     | +--------- Has a handler?
+ *          |     +----------- Enabling/disabling forced?
+ *          +----------------- <Reserved>
  */
 typedef u32 acpi_event_status;
 
@@ -751,6 +752,7 @@ typedef u32 acpi_event_status;
 #define ACPI_EVENT_FLAG_STATUS_SET      (acpi_event_status) 0x04
 #define ACPI_EVENT_FLAG_ENABLE_SET      (acpi_event_status) 0x08
 #define ACPI_EVENT_FLAG_HAS_HANDLER     (acpi_event_status) 0x10
+#define ACPI_EVENT_FLAG_MANAGED         (acpi_event_status) 0x20
 #define ACPI_EVENT_FLAG_SET             ACPI_EVENT_FLAG_STATUS_SET
 
 /* Actions for acpi_set_gpe, acpi_gpe_wakeup, acpi_hw_low_set_gpe */
@@ -758,17 +760,20 @@ typedef u32 acpi_event_status;
 #define ACPI_GPE_ENABLE                 0
 #define ACPI_GPE_DISABLE                1
 #define ACPI_GPE_CONDITIONAL_ENABLE     2
+#define ACPI_GPE_UNMANAGE               3
 
 /*
  * GPE info flags - Per GPE
- * +-------+-+-+---+
- * |  7:5  |4|3|2:0|
- * +-------+-+-+---+
- *     |    | |  |
- *     |    | |  +-- Type of dispatch:to method, handler, notify, or none
- *     |    | +----- Interrupt type: edge or level triggered
- *     |    +------- Is a Wake GPE
- *     +------------ <Reserved>
+ * +-+-+-+-+-+---+
+ * |7|6|5|4|3|2:0|
+ * +-+-+-+-+-+---+
+ *  | | | | |  |
+ *  | | | | |  +-- Type of dispatch:to method, handler, notify, or none
+ *  | | | | +----- Interrupt type: edge or level triggered
+ *  | | | +------- Is a Wake GPE
+ *  | | +--------- Force GPE enabling
+ *  | +----------- Force GPE disabling
+ *  +------------- <Reserved>
  */
 #define ACPI_GPE_DISPATCH_NONE          (u8) 0x00
 #define ACPI_GPE_DISPATCH_METHOD        (u8) 0x01
@@ -785,6 +790,50 @@ typedef u32 acpi_event_status;
 #define ACPI_GPE_CAN_WAKE               (u8) 0x10
 
 /*
+ * Flags used by the GPE management APIs
+ *
+ * The GPE management APIs are not implemented for the GPE drivers. It is
+ * designed for providing the management purposes out of the GPE drivers'
+ * awareness. The GPE driver APIs should still be working as expected
+ * during the period the GPE management purposes are applied. There are 2
+ * use cases for the flags:
+ *
+ * 1. Prevent the GPE flooding
+ *    These flags can be used to control how the GPE is dispatched by the
+ *    event dispatcher. Note that the ACPI_GPE_MANAGED_DISABLED can also be
+ *    used to prevent the GPE flooding that cannot be prevented due to the
+ *    ACPI_GPE_DISPATCH_NONE sanity check. This kind of the GPE flooding
+ *    happens on the GPE where there is a _Lxx/_Exx prepared by the BIOS
+ *    but the OSPM still cannot handle it correctly by executing the
+ *    method. Such kind of incapability is mostly because of the feature
+ *    gaps in the OSPM:
+ *    ACPI_GPE_MANAGED_ENABLED:  force the event dispatcher to always
+ *                               enable the GPE
+ *    ACPI_GPE_MANAGED_DISABLED: force the event dispatcher to always
+ *                               disable the GPE
+ *                               prevent the GPE from flooding
+ *    acpi_block_gpe()/acpi_unblock_gpe() are the APIs offering the GPE
+ *    flooding prevention feature.
+ *
+ * 2. Control the GPE handling mode
+ *    A GPE driver may be able to handle the GPE both in the interrupt mode
+ *    and in the polling mode. Since the polling mode is a mechanism
+ *    implemented by the driver itself and is not controlled by the event
+ *    dispatcher, this mechanism can be used to switch between the
+ *    interrupt mode (dispatched via the event dispatcher) and the polling
+ *    mode (implemented by the GPE drivers):
+ *    ACPI_GPE_MANAGED_ENABLED:  force the driver to use the interrupt mode
+ *    ACPI_GPE_MANAGED_DISABLED: force the driver to use the polling mode
+ *    None:                      allow the driver to use the
+ *                               interrupt/polling adaptive mode
+ *    acpi_control_gpe_handling() is the API offering the GPE handling mode
+ *    switch feature.
+ */
+#define ACPI_GPE_MANAGED_ENABLED        (u8) 0x20
+#define ACPI_GPE_MANAGED_DISABLED       (u8) 0x40
+#define ACPI_GPE_MANAGED_FLAG_MASK      (u8) 0x60
+
+/*
  * Flags for GPE and Lock interfaces
  */
 #define ACPI_NOT_ISR                    0x1
-- 
1.7.10

Re: [PATCH 1/3] ACPICA: Events: Introduce acpi_block_gpe()/acpi_unblock_gpe()/acpi_control_gpe_handling() to allow administrative GPE enabling/disabling

[Rafael J. Wysocki]

On Monday, May 16, 2016 05:11:11 PM Lv Zheng wrote:
> There is a facility in Linux, developers can manage GPE enabling/disabling
> through /sys/firmware/acpi/interrupts/gpexx. This is mainly for debugging
> purposes. Many users expect to use this facility to implement quirks to
> disable specific GPEs when there is a gap in Linux causing GPE flood.
> This is not working correctly because currently this facility invokes
> enabling/disabling counting based GPE driver APIs:
>  acpi_enable_gpe()/acpi_disable_gpe()
> and the GPE drivers can still affect the count to mess up the GPE
> management purposes.
> 
> This patch introduces acpi_block_gpe()/acpi_unblock_gpe() to be used in such
> situation instead of acpi_enable_gpe()/acpi_disable_gpe().

Up to this point, I agree.

> The idea to implement this is to replace the GPE register EN bit with the
> managed value, block EN set/clear operations but record the operation
> results in blocked_enabled, so that after the managed state is removed,
> restore the saved blocked_enabled back to the GPE register EN bit.

Unfortunately, I don't really follow the above paragraph, so chances are that
whoever is not familiar with the code will not be able to follow it either.

> Now OSPMs should be able to use this facility to generate quirks. ACPICA
> BZ 1102.
> 
> This facility can also be used by the administrator to control the GPE
> handling mode during the runtime when the driver is capable of handling the
> GPE in both the interrupt mode and the polling mode (for example, the Linux
> EC driver). acpi_control_gpe_handling() is offered for this purpose. Lv Zheng.

That is too much.  The patch should focus on the block/unblock functionality.
Anything beyond that should be added later IMO.

> Link: https://bugs.acpica.org/show_bug.cgi?id=1102
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> ---
>  drivers/acpi/acpica/acevents.h |    3 +
>  drivers/acpi/acpica/aclocal.h  |    1 +
>  drivers/acpi/acpica/evgpe.c    |   84 ++++++++++++++++++++++++
>  drivers/acpi/acpica/evxfgpe.c  |  142 ++++++++++++++++++++++++++++++++++++++++
>  drivers/acpi/acpica/hwgpe.c    |   24 ++++++-
>  include/acpi/acpixf.h          |   23 +++++--
>  include/acpi/actypes.h         |   85 +++++++++++++++++++-----
>  7 files changed, 336 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/acevents.h b/drivers/acpi/acpica/acevents.h
> index 77af91c..3a6e4db 100644
> --- a/drivers/acpi/acpica/acevents.h
> +++ b/drivers/acpi/acpica/acevents.h
> @@ -86,6 +86,9 @@ acpi_ev_update_gpe_enable_mask(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_manage_gpe(struct acpi_gpe_event_info *gpe_event_info, u8 action);
> +
> +acpi_status
>  acpi_ev_add_gpe_reference(struct acpi_gpe_event_info *gpe_event_info);
>  
>  acpi_status
> diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
> index 13331d7..17a6834 100644
> --- a/drivers/acpi/acpica/aclocal.h
> +++ b/drivers/acpi/acpica/aclocal.h
> @@ -484,6 +484,7 @@ struct acpi_gpe_event_info {
>  	u8 flags;		/* Misc info about this GPE */
>  	u8 gpe_number;		/* This GPE */
>  	u8 runtime_count;	/* References to a run GPE */
> +	u8 blocked_enabled;	/* GPE should be enabled but blocked */

I'm not sure I'm following the logic here.

What's needed is disabled and blocked.  Why do we need enabled and blocked?

>  };
>  
>  /* Information about a GPE register pair, one per each status/enable pair in an array */
> diff --git a/drivers/acpi/acpica/evgpe.c b/drivers/acpi/acpica/evgpe.c
> index 4b4949c..f821bdd 100644
> --- a/drivers/acpi/acpica/evgpe.c
> +++ b/drivers/acpi/acpica/evgpe.c
> @@ -130,6 +130,90 @@ acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info)
>  
>  /*******************************************************************************
>   *
> + * FUNCTION:    acpi_ev_manage_gpe
> + *
> + * PARAMETERS:  gpe_event_info          - GPE to force enabling/disabling
> + *              action                  - ACPI_GPE_ENABLE, ACPI_GPE_DISABLE or
> + *                                        ACPI_GPE_UNMANAGE
> + *
> + * RETURN:      Status
> + *
> + * DESCRIPTION: Unconditionally enable or disable an individual GPE for the
> + *              administrative purposes during the runtime.
> + *
> + ******************************************************************************/
> +
> +acpi_status
> +acpi_ev_manage_gpe(struct acpi_gpe_event_info *gpe_event_info, u8 action)
> +{
> +	acpi_status status;
> +	acpi_event_status event_status;
> +
> +	ACPI_FUNCTION_TRACE(ev_manage_gpe);
> +
> +	/* Perform the action */
> +
> +	switch (action) {
> +	case ACPI_GPE_ENABLE:
> +	case ACPI_GPE_DISABLE:
> +
> +		if (!(gpe_event_info->flags & ACPI_GPE_MANAGED_FLAG_MASK)) {
> +			status =
> +			    acpi_hw_get_gpe_status(gpe_event_info,
> +						   &event_status);
> +			if (ACPI_FAILURE(status)) {
> +				return_ACPI_STATUS(status);
> +			}
> +			if (event_status & ACPI_EVENT_FLAG_ENABLE_SET) {
> +				gpe_event_info->blocked_enabled = TRUE;
> +			} else {
> +				gpe_event_info->blocked_enabled = FALSE;
> +			}
> +		}
> +
> +		/* Reset flags so that acpi_hw_low_set_gpe() can take effective */
> +
> +		gpe_event_info->flags &= ~ACPI_GPE_MANAGED_FLAG_MASK;
> +		if (action == ACPI_GPE_ENABLE) {
> +			(void)acpi_hw_low_set_gpe(gpe_event_info,
> +						  ACPI_GPE_ENABLE);
> +			gpe_event_info->flags |= ACPI_GPE_MANAGED_ENABLED;
> +		} else {
> +			(void)acpi_hw_low_set_gpe(gpe_event_info,
> +						  ACPI_GPE_DISABLE);
> +			gpe_event_info->flags |= ACPI_GPE_MANAGED_DISABLED;
> +		}
> +		break;
> +
> +	case ACPI_GPE_UNMANAGE:
> +
> +		if (!(gpe_event_info->flags & ACPI_GPE_MANAGED_FLAG_MASK)) {
> +			return_ACPI_STATUS(AE_BAD_PARAMETER);
> +		}
> +
> +		/* Reset flags so that acpi_hw_low_set_gpe() can take effective */
> +
> +		gpe_event_info->flags &= ~ACPI_GPE_MANAGED_FLAG_MASK;
> +		if (gpe_event_info->blocked_enabled) {
> +			(void)acpi_hw_low_set_gpe(gpe_event_info,
> +						  ACPI_GPE_ENABLE);
> +		} else {
> +			(void)acpi_hw_low_set_gpe(gpe_event_info,
> +						  ACPI_GPE_DISABLE);
> +		}
> +		gpe_event_info->blocked_enabled = FALSE;
> +		break;
> +

The code above is really hard to follow.

Maybe the description in the comment above the function should be more, well,
descriptive?

> +	default:
> +
> +		return_ACPI_STATUS(AE_BAD_PARAMETER);
> +	}
> +
> +	return_ACPI_STATUS(AE_OK);
> +}
> +
> +/*******************************************************************************
> + *
>   * FUNCTION:    acpi_ev_add_gpe_reference
>   *
>   * PARAMETERS:  gpe_event_info          - Add a reference to this GPE
> diff --git a/drivers/acpi/acpica/evxfgpe.c b/drivers/acpi/acpica/evxfgpe.c
> index 17cfef7..3ad8fa9 100644
> --- a/drivers/acpi/acpica/evxfgpe.c
> +++ b/drivers/acpi/acpica/evxfgpe.c
> @@ -257,6 +257,148 @@ ACPI_EXPORT_SYMBOL(acpi_set_gpe)
>  
>  /*******************************************************************************
>   *
> + * FUNCTION:    acpi_block_gpe
> + *
> + * PARAMETERS:  gpe_device          - Parent GPE Device. NULL for GPE0/GPE1
> + *              gpe_number          - GPE level within the GPE block
> + *
> + * RETURN:      Status
> + *
> + * DESCRIPTION: Unconditionally disable an individual GPE.
> + *              This API is typically used by the system administrator to
> + *              block the GPE enabling, ex., prevent the GPE flooding.
> + *
> + ******************************************************************************/
> +acpi_status acpi_block_gpe(acpi_handle gpe_device, u32 gpe_number)
> +{
> +	struct acpi_gpe_event_info *gpe_event_info;
> +	acpi_status status;
> +	acpi_cpu_flags flags;
> +
> +	ACPI_FUNCTION_TRACE(acpi_block_gpe);
> +
> +	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
> +
> +	/* 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) {
> +		status = AE_BAD_PARAMETER;
> +		goto unlock_and_exit;
> +	}
> +
> +	status = acpi_ev_manage_gpe(gpe_event_info, ACPI_GPE_DISABLE);

So I would prefer the second argument here to be something like
ACPI_GPE_DISABLE | ACPI_GPE_BLOCK as then the code would be quite
straightforward to understand.

> +
> +unlock_and_exit:
> +	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
> +	return_ACPI_STATUS(status);
> +}
> +
> +ACPI_EXPORT_SYMBOL(acpi_block_gpe)
> +
> +/*******************************************************************************
> + *
> + * FUNCTION:    acpi_unblock_gpe
> + *
> + * PARAMETERS:  gpe_device          - Parent GPE Device. NULL for GPE0/GPE1
> + *              gpe_number          - GPE level within the GPE block
> + *
> + * RETURN:      Status
> + *
> + * DESCRIPTION: Stop unconditional GPE disabling.
> + *              This API reverts acpi_block_gpe().
> + *
> + ******************************************************************************/
> +acpi_status acpi_unblock_gpe(acpi_handle gpe_device, u32 gpe_number)
> +{
> +	struct acpi_gpe_event_info *gpe_event_info;
> +	acpi_status status;
> +	acpi_cpu_flags flags;
> +
> +	ACPI_FUNCTION_TRACE(acpi_unblock_gpe);
> +
> +	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
> +
> +	/* 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) {
> +		status = AE_BAD_PARAMETER;
> +		goto unlock_and_exit;
> +	}
> +
> +	status = acpi_ev_manage_gpe(gpe_event_info, ACPI_GPE_UNMANAGE);

And that is completely unclear to me.

> +
> +unlock_and_exit:
> +	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
> +	return_ACPI_STATUS(status);
> +}
> +
> +ACPI_EXPORT_SYMBOL(acpi_unblock_gpe)
> +
> +/*******************************************************************************
> + *
> + * FUNCTION:    acpi_control_gpe_handling
> + *
> + * PARAMETERS:  gpe_device          - Parent GPE Device. NULL for GPE0/GPE1
> + *              gpe_number          - GPE level within the GPE block
> + *              use_interrupt_mode  - Allow the GPE to be dispatched in the
> + *                                    interrupt mode
> + *              use_polling_mode    - Allow the GPE to be dispatched in the
> + *                                    polling mode
> + *
> + * RETURN:      Status
> + *
> + * DESCRIPTION: Switch the GPE handling mode between the interrupt only mode,
> + *              the polling only mode and the interrupt/polling adaptive mode.
> + *
> + ******************************************************************************/

This is extra functionality that you want to piggy back on a fix for a real issue.

Please separate it from that fix, it doesn't belong here.

> +acpi_status
> +acpi_control_gpe_handling(acpi_handle gpe_device,
> +			  u32 gpe_number,
> +			  u8 use_interrupt_mode, u8 use_polling_mode)
> +{
> +	struct acpi_gpe_event_info *gpe_event_info;
> +	acpi_status status;
> +	acpi_cpu_flags flags;
> +	u8 action;
> +
> +	ACPI_FUNCTION_TRACE(acpi_control_gpe_handling);
> +
> +	if (!use_interrupt_mode && !use_polling_mode) {
> +		return_ACPI_STATUS(AE_BAD_PARAMETER);
> +	}
> +
> +	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
> +
> +	/* 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) {
> +		status = AE_BAD_PARAMETER;
> +		goto unlock_and_exit;
> +	}
> +
> +	/* Pick up and peform the correct action */
> +
> +	action = ACPI_GPE_UNMANAGE;
> +	if (!use_interrupt_mode) {
> +		action = ACPI_GPE_DISABLE;
> +	}
> +	if (!use_polling_mode) {
> +		action = ACPI_GPE_ENABLE;
> +	}
> +	status = acpi_ev_manage_gpe(gpe_event_info, action);
> +
> +unlock_and_exit:
> +	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
> +	return_ACPI_STATUS(status);
> +}
> +
> +ACPI_EXPORT_SYMBOL(acpi_control_gpe_handling)
> +
> +/*******************************************************************************
> + *
>   * FUNCTION:    acpi_mark_gpe_for_wake
>   *
>   * PARAMETERS:  gpe_device          - Parent GPE Device. NULL for GPE0/GPE1
> diff --git a/drivers/acpi/acpica/hwgpe.c b/drivers/acpi/acpica/hwgpe.c
> index bdecd5e..8cdddbe 100644
> --- a/drivers/acpi/acpica/hwgpe.c
> +++ b/drivers/acpi/acpica/hwgpe.c
> @@ -98,7 +98,7 @@ acpi_status
>  acpi_hw_low_set_gpe(struct acpi_gpe_event_info *gpe_event_info, u32 action)
>  {
>  	struct acpi_gpe_register_info *gpe_register_info;
> -	acpi_status status;
> +	acpi_status status = AE_OK;
>  	u32 enable_mask;
>  	u32 register_bit;
>  
> @@ -148,9 +148,21 @@ acpi_hw_low_set_gpe(struct acpi_gpe_event_info *gpe_event_info, u32 action)
>  		return (AE_BAD_PARAMETER);
>  	}
>  
> -	/* Write the updated enable mask */
> +	/* Check if there is an administrative GPE enabling/disabling */
>  
> -	status = acpi_hw_write(enable_mask, &gpe_register_info->enable_address);
> +	if (gpe_event_info->flags & ACPI_GPE_MANAGED_FLAG_MASK) {
> +		if (enable_mask & register_bit) {
> +			gpe_event_info->blocked_enabled = TRUE;
> +		} else {
> +			gpe_event_info->blocked_enabled = FALSE;
> +		}
> +	} else {
> +		/* Write the updated enable mask */
> +
> +		status =
> +		    acpi_hw_write(enable_mask,
> +				  &gpe_register_info->enable_address);
> +	}
>  	return (status);
>  }
>  
> @@ -228,6 +240,12 @@ acpi_hw_get_gpe_status(struct acpi_gpe_event_info *gpe_event_info,
>  		local_event_status |= ACPI_EVENT_FLAG_HAS_HANDLER;
>  	}
>  
> +	/* GPE currently managed? (enabled/disabled by the system admin?) */
> +
> +	if (gpe_event_info->flags & ACPI_GPE_MANAGED_FLAG_MASK) {
> +		local_event_status |= ACPI_EVENT_FLAG_MANAGED;
> +	}
> +
>  	/* Get the info block for the entire GPE register */
>  
>  	gpe_register_info = gpe_event_info->register_info;
> diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> index 4e4c214..a0a6544 100644
> --- a/include/acpi/acpixf.h
> +++ b/include/acpi/acpixf.h
> @@ -732,15 +732,28 @@ ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
>  						u32 gpe_number))
>  
>  ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
> -				acpi_mark_gpe_for_wake(acpi_handle gpe_device,
> -						       u32 gpe_number))
> +				acpi_block_gpe(acpi_handle gpe_device,
> +					       u32 gpe_number))
> +
> +ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
> +				acpi_unblock_gpe(acpi_handle gpe_device,
> +						 u32 gpe_number))
>  
>  ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
> -				acpi_setup_gpe_for_wake(acpi_handle
> -							parent_device,
> -							acpi_handle gpe_device,
> +				acpi_control_gpe_handling(acpi_handle
> +							  gpe_device,
> +							  u32 gpe_number,
> +							  u8 use_interrupt_mode,
> +							  u8 use_polling_mode))
> +ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
> +				 acpi_mark_gpe_for_wake(acpi_handle gpe_device,
>  							u32 gpe_number))
>  ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
> +				 acpi_setup_gpe_for_wake(acpi_handle
> +							 parent_device,
> +							 acpi_handle gpe_device,
> +							 u32 gpe_number))
> +ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
>  				 acpi_set_gpe_wake_mask(acpi_handle gpe_device,
>  							u32 gpe_number,
>  							u8 action))
> diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> index cb389ef..9f841f2 100644
> --- a/include/acpi/actypes.h
> +++ b/include/acpi/actypes.h
> @@ -732,16 +732,17 @@ typedef u32 acpi_event_type;
>   * The encoding of acpi_event_status is illustrated below.
>   * Note that a set bit (1) indicates the property is TRUE
>   * (e.g. if bit 0 is set then the event is enabled).
> - * +-------------+-+-+-+-+-+
> - * |   Bits 31:5 |4|3|2|1|0|
> - * +-------------+-+-+-+-+-+
> - *          |     | | | | |
> - *          |     | | | | +- Enabled?
> - *          |     | | | +--- Enabled for wake?
> - *          |     | | +----- Status bit set?
> - *          |     | +------- Enable bit set?
> - *          |     +--------- Has a handler?
> - *          +--------------- <Reserved>
> + * +-------------+-+-+-+-+-+-+
> + * |   Bits 31:6 |5|4|3|2|1|0|
> + * +-------------+-+-+-+-+-+-+
> + *          |     | | | | | |
> + *          |     | | | | | +- Enabled?
> + *          |     | | | | +--- Enabled for wake?
> + *          |     | | | +----- Status bit set?
> + *          |     | | +------- Enable bit set?
> + *          |     | +--------- Has a handler?
> + *          |     +----------- Enabling/disabling forced?
> + *          +----------------- <Reserved>
>   */
>  typedef u32 acpi_event_status;
>  
> @@ -751,6 +752,7 @@ typedef u32 acpi_event_status;
>  #define ACPI_EVENT_FLAG_STATUS_SET      (acpi_event_status) 0x04
>  #define ACPI_EVENT_FLAG_ENABLE_SET      (acpi_event_status) 0x08
>  #define ACPI_EVENT_FLAG_HAS_HANDLER     (acpi_event_status) 0x10
> +#define ACPI_EVENT_FLAG_MANAGED         (acpi_event_status) 0x20

The meaning of this new flag is unclear.  It probably should be something like
ACPI_EVENT_FLAG_BLOCKED or MASKED meaning that if set, the GPE cannot be
disabled or enabled by usual means (until that flag is cleared).

>  #define ACPI_EVENT_FLAG_SET             ACPI_EVENT_FLAG_STATUS_SET
>  
>  /* Actions for acpi_set_gpe, acpi_gpe_wakeup, acpi_hw_low_set_gpe */
> @@ -758,17 +760,20 @@ typedef u32 acpi_event_status;
>  #define ACPI_GPE_ENABLE                 0
>  #define ACPI_GPE_DISABLE                1
>  #define ACPI_GPE_CONDITIONAL_ENABLE     2
> +#define ACPI_GPE_UNMANAGE               3
>  
>  /*
>   * GPE info flags - Per GPE
> - * +-------+-+-+---+
> - * |  7:5  |4|3|2:0|
> - * +-------+-+-+---+
> - *     |    | |  |
> - *     |    | |  +-- Type of dispatch:to method, handler, notify, or none
> - *     |    | +----- Interrupt type: edge or level triggered
> - *     |    +------- Is a Wake GPE
> - *     +------------ <Reserved>
> + * +-+-+-+-+-+---+
> + * |7|6|5|4|3|2:0|
> + * +-+-+-+-+-+---+
> + *  | | | | |  |
> + *  | | | | |  +-- Type of dispatch:to method, handler, notify, or none
> + *  | | | | +----- Interrupt type: edge or level triggered
> + *  | | | +------- Is a Wake GPE
> + *  | | +--------- Force GPE enabling
> + *  | +----------- Force GPE disabling
> + *  +------------- <Reserved>
>   */
>  #define ACPI_GPE_DISPATCH_NONE          (u8) 0x00
>  #define ACPI_GPE_DISPATCH_METHOD        (u8) 0x01
> @@ -785,6 +790,50 @@ typedef u32 acpi_event_status;
>  #define ACPI_GPE_CAN_WAKE               (u8) 0x10
>  
>  /*
> + * Flags used by the GPE management APIs
> + *
> + * The GPE management APIs are not implemented for the GPE drivers. It is
> + * designed for providing the management purposes out of the GPE drivers'
> + * awareness. The GPE driver APIs should still be working as expected
> + * during the period the GPE management purposes are applied. There are 2
> + * use cases for the flags:
> + *
> + * 1. Prevent the GPE flooding
> + *    These flags can be used to control how the GPE is dispatched by the
> + *    event dispatcher. Note that the ACPI_GPE_MANAGED_DISABLED can also be
> + *    used to prevent the GPE flooding that cannot be prevented due to the
> + *    ACPI_GPE_DISPATCH_NONE sanity check. This kind of the GPE flooding
> + *    happens on the GPE where there is a _Lxx/_Exx prepared by the BIOS
> + *    but the OSPM still cannot handle it correctly by executing the
> + *    method. Such kind of incapability is mostly because of the feature
> + *    gaps in the OSPM:
> + *    ACPI_GPE_MANAGED_ENABLED:  force the event dispatcher to always
> + *                               enable the GPE
> + *    ACPI_GPE_MANAGED_DISABLED: force the event dispatcher to always
> + *                               disable the GPE
> + *                               prevent the GPE from flooding
> + *    acpi_block_gpe()/acpi_unblock_gpe() are the APIs offering the GPE
> + *    flooding prevention feature.
> + *
> + * 2. Control the GPE handling mode
> + *    A GPE driver may be able to handle the GPE both in the interrupt mode
> + *    and in the polling mode. Since the polling mode is a mechanism
> + *    implemented by the driver itself and is not controlled by the event
> + *    dispatcher, this mechanism can be used to switch between the
> + *    interrupt mode (dispatched via the event dispatcher) and the polling
> + *    mode (implemented by the GPE drivers):
> + *    ACPI_GPE_MANAGED_ENABLED:  force the driver to use the interrupt mode
> + *    ACPI_GPE_MANAGED_DISABLED: force the driver to use the polling mode
> + *    None:                      allow the driver to use the
> + *                               interrupt/polling adaptive mode
> + *    acpi_control_gpe_handling() is the API offering the GPE handling mode
> + *    switch feature.
> + */
> +#define ACPI_GPE_MANAGED_ENABLED        (u8) 0x20
> +#define ACPI_GPE_MANAGED_DISABLED       (u8) 0x40
> +#define ACPI_GPE_MANAGED_FLAG_MASK      (u8) 0x60
> +
> +/*
>   * Flags for GPE and Lock interfaces
>   */
>  #define ACPI_NOT_ISR                    0x1

Thanks,
Rafael

RE: [PATCH 1/3] ACPICA: Events: Introduce acpi_block_gpe()/acpi_unblock_gpe()/acpi_control_gpe_handling() to allow administrative GPE enabling/disabling

[Zheng, Lv]

Hi,

> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Subject: Re: [PATCH 1/3] ACPICA: Events: Introduce
> acpi_block_gpe()/acpi_unblock_gpe()/acpi_control_gpe_handling() to
> allow administrative GPE enabling/disabling
> 
> On Monday, May 16, 2016 05:11:11 PM Lv Zheng wrote:
> > There is a facility in Linux, developers can manage GPE enabling/disabling
> > through /sys/firmware/acpi/interrupts/gpexx. This is mainly for
> debugging
> > purposes. Many users expect to use this facility to implement quirks to
> > disable specific GPEs when there is a gap in Linux causing GPE flood.
> > This is not working correctly because currently this facility invokes
> > enabling/disabling counting based GPE driver APIs:
> >  acpi_enable_gpe()/acpi_disable_gpe()
> > and the GPE drivers can still affect the count to mess up the GPE
> > management purposes.
> >
> > This patch introduces acpi_block_gpe()/acpi_unblock_gpe() to be used in
> such
> > situation instead of acpi_enable_gpe()/acpi_disable_gpe().
> 
> Up to this point, I agree.
> 
> > The idea to implement this is to replace the GPE register EN bit with the
> > managed value, block EN set/clear operations but record the operation
> > results in blocked_enabled, so that after the managed state is removed,
> > restore the saved blocked_enabled back to the GPE register EN bit.
> 
> Unfortunately, I don't really follow the above paragraph, so chances are
> that
> whoever is not familiar with the code will not be able to follow it either.
[Lv Zheng] 
I see.
I should stop talking this using "managed".
It is better to use "masked".
As this facility is actually trying to implement the kind of the facility that can be seen in many other IRQ chips - the IRQ MASK bit.

> 
> > Now OSPMs should be able to use this facility to generate quirks. ACPICA
> > BZ 1102.
> >
> > This facility can also be used by the administrator to control the GPE
> > handling mode during the runtime when the driver is capable of handling
> the
> > GPE in both the interrupt mode and the polling mode (for example, the
> Linux
> > EC driver). acpi_control_gpe_handling() is offered for this purpose. Lv
> Zheng.
> 
> That is too much.  The patch should focus on the block/unblock
> functionality.
> Anything beyond that should be added later IMO.
[Lv Zheng] 
OK.
So after examining all of your comments.
I think what I need to improve is eliminating this feature.
That should be able to make the code simpler and thus easier for the others to follow.


> 
> > Link: https://bugs.acpica.org/show_bug.cgi?id=1102
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > ---
> >  drivers/acpi/acpica/acevents.h |    3 +
> >  drivers/acpi/acpica/aclocal.h  |    1 +
> >  drivers/acpi/acpica/evgpe.c    |   84 ++++++++++++++++++++++++
> >  drivers/acpi/acpica/evxfgpe.c  |  142
> ++++++++++++++++++++++++++++++++++++++++
> >  drivers/acpi/acpica/hwgpe.c    |   24 ++++++-
> >  include/acpi/acpixf.h          |   23 +++++--
> >  include/acpi/actypes.h         |   85 +++++++++++++++++++-----
> >  7 files changed, 336 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/acpi/acpica/acevents.h
> b/drivers/acpi/acpica/acevents.h
> > index 77af91c..3a6e4db 100644
> > --- a/drivers/acpi/acpica/acevents.h
> > +++ b/drivers/acpi/acpica/acevents.h
> > @@ -86,6 +86,9 @@ acpi_ev_update_gpe_enable_mask(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_manage_gpe(struct acpi_gpe_event_info *gpe_event_info, u8
> action);
> > +
> > +acpi_status
> >  acpi_ev_add_gpe_reference(struct acpi_gpe_event_info
> *gpe_event_info);
> >
> >  acpi_status
> > diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
> > index 13331d7..17a6834 100644
> > --- a/drivers/acpi/acpica/aclocal.h
> > +++ b/drivers/acpi/acpica/aclocal.h
> > @@ -484,6 +484,7 @@ struct acpi_gpe_event_info {
> >  	u8 flags;		/* Misc info about this GPE */
> >  	u8 gpe_number;		/* This GPE */
> >  	u8 runtime_count;	/* References to a run GPE */
> > +	u8 blocked_enabled;	/* GPE should be enabled but blocked */
> 
> I'm not sure I'm following the logic here.
> 
> What's needed is disabled and blocked.  Why do we need enabled and
> blocked?
[Lv Zheng] 
I just used this variable to track the "expected GPE hardware register's EN bit value" from driver's point of view.
So:
1. If it is TRUE, it means GPE register's EN bit should be flagged from the drivers' point of view.
2. If it is FALSE, it means GPE register's EN bit should be unflagged from the drivers' point view.

When acpi_unblock_gpe() is invoked, I need the value here to be restored to the "GPE hardware register's EN bit".
So it looks like the driver expected "EN bit value" was blocked by this facility.
That's why I use the name of "blocked_enabled".
Is it acceptable now with the justification?

> 
> >  };
> >
> >  /* Information about a GPE register pair, one per each status/enable
> pair in an array */
> > diff --git a/drivers/acpi/acpica/evgpe.c b/drivers/acpi/acpica/evgpe.c
> > index 4b4949c..f821bdd 100644
> > --- a/drivers/acpi/acpica/evgpe.c
> > +++ b/drivers/acpi/acpica/evgpe.c
> > @@ -130,6 +130,90 @@ acpi_status acpi_ev_enable_gpe(struct
> acpi_gpe_event_info *gpe_event_info)
> >
> >
> /*********************************************************
> **********************
> >   *
> > + * FUNCTION:    acpi_ev_manage_gpe
> > + *
> > + * PARAMETERS:  gpe_event_info          - GPE to force enabling/disabling
> > + *              action                  - ACPI_GPE_ENABLE, ACPI_GPE_DISABLE or
> > + *                                        ACPI_GPE_UNMANAGE
> > + *
> > + * RETURN:      Status
> > + *
> > + * DESCRIPTION: Unconditionally enable or disable an individual GPE for
> the
> > + *              administrative purposes during the runtime.
> > + *
> > +
> **********************************************************
> ********************/
> > +
> > +acpi_status
> > +acpi_ev_manage_gpe(struct acpi_gpe_event_info *gpe_event_info, u8
> action)
> > +{
> > +	acpi_status status;
> > +	acpi_event_status event_status;
> > +
> > +	ACPI_FUNCTION_TRACE(ev_manage_gpe);
> > +
> > +	/* Perform the action */
> > +
> > +	switch (action) {
> > +	case ACPI_GPE_ENABLE:
> > +	case ACPI_GPE_DISABLE:
> > +
> > +		if (!(gpe_event_info->flags &
> ACPI_GPE_MANAGED_FLAG_MASK)) {
> > +			status =
> > +			    acpi_hw_get_gpe_status(gpe_event_info,
> > +						   &event_status);
> > +			if (ACPI_FAILURE(status)) {
> > +				return_ACPI_STATUS(status);
> > +			}
> > +			if (event_status & ACPI_EVENT_FLAG_ENABLE_SET)
> {
> > +				gpe_event_info->blocked_enabled = TRUE;
> > +			} else {
> > +				gpe_event_info->blocked_enabled = FALSE;
> > +			}
> > +		}
> > +
> > +		/* Reset flags so that acpi_hw_low_set_gpe() can take
> effective */
> > +
> > +		gpe_event_info->flags &=
> ~ACPI_GPE_MANAGED_FLAG_MASK;
> > +		if (action == ACPI_GPE_ENABLE) {
> > +			(void)acpi_hw_low_set_gpe(gpe_event_info,
> > +						  ACPI_GPE_ENABLE);
> > +			gpe_event_info->flags |=
> ACPI_GPE_MANAGED_ENABLED;
> > +		} else {
> > +			(void)acpi_hw_low_set_gpe(gpe_event_info,
> > +						  ACPI_GPE_DISABLE);
> > +			gpe_event_info->flags |=
> ACPI_GPE_MANAGED_DISABLED;
> > +		}
> > +		break;
> > +
> > +	case ACPI_GPE_UNMANAGE:
> > +
> > +		if (!(gpe_event_info->flags &
> ACPI_GPE_MANAGED_FLAG_MASK)) {
> > +			return_ACPI_STATUS(AE_BAD_PARAMETER);
> > +		}
> > +
> > +		/* Reset flags so that acpi_hw_low_set_gpe() can take
> effective */
> > +
> > +		gpe_event_info->flags &=
> ~ACPI_GPE_MANAGED_FLAG_MASK;
> > +		if (gpe_event_info->blocked_enabled) {
> > +			(void)acpi_hw_low_set_gpe(gpe_event_info,
> > +						  ACPI_GPE_ENABLE);
> > +		} else {
> > +			(void)acpi_hw_low_set_gpe(gpe_event_info,
> > +						  ACPI_GPE_DISABLE);
> > +		}
> > +		gpe_event_info->blocked_enabled = FALSE;
> > +		break;
> > +
> 
> The code above is really hard to follow.
> 
> Maybe the description in the comment above the function should be more,
> well,
> descriptive?
[Lv Zheng] 
OK, I'll improve it.
Also with acpi_control_gpe_handling() deleted, code here should be much simpler.

> 
> > +	default:
> > +
> > +		return_ACPI_STATUS(AE_BAD_PARAMETER);
> > +	}
> > +
> > +	return_ACPI_STATUS(AE_OK);
> > +}
> > +
> >
> +/********************************************************
> ***********************
> > + *
> >   * FUNCTION:    acpi_ev_add_gpe_reference
> >   *
> >   * PARAMETERS:  gpe_event_info          - Add a reference to this GPE
> > diff --git a/drivers/acpi/acpica/evxfgpe.c b/drivers/acpi/acpica/evxfgpe.c
> > index 17cfef7..3ad8fa9 100644
> > --- a/drivers/acpi/acpica/evxfgpe.c
> > +++ b/drivers/acpi/acpica/evxfgpe.c
> > @@ -257,6 +257,148 @@ ACPI_EXPORT_SYMBOL(acpi_set_gpe)
> >
> >
> /*********************************************************
> **********************
> >   *
> > + * FUNCTION:    acpi_block_gpe
> > + *
> > + * PARAMETERS:  gpe_device          - Parent GPE Device. NULL for
> GPE0/GPE1
> > + *              gpe_number          - GPE level within the GPE block
> > + *
> > + * RETURN:      Status
> > + *
> > + * DESCRIPTION: Unconditionally disable an individual GPE.
> > + *              This API is typically used by the system administrator to
> > + *              block the GPE enabling, ex., prevent the GPE flooding.
> > + *
> > +
> **********************************************************
> ********************/
> > +acpi_status acpi_block_gpe(acpi_handle gpe_device, u32 gpe_number)
> > +{
> > +	struct acpi_gpe_event_info *gpe_event_info;
> > +	acpi_status status;
> > +	acpi_cpu_flags flags;
> > +
> > +	ACPI_FUNCTION_TRACE(acpi_block_gpe);
> > +
> > +	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
> > +
> > +	/* 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) {
> > +		status = AE_BAD_PARAMETER;
> > +		goto unlock_and_exit;
> > +	}
> > +
> > +	status = acpi_ev_manage_gpe(gpe_event_info,
> ACPI_GPE_DISABLE);
> 
> So I would prefer the second argument here to be something like
> ACPI_GPE_DISABLE | ACPI_GPE_BLOCK as then the code would be quite
> straightforward to understand.
[Lv Zheng] 
OK.

> 
> > +
> > +unlock_and_exit:
> > +	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
> > +	return_ACPI_STATUS(status);
> > +}
> > +
> > +ACPI_EXPORT_SYMBOL(acpi_block_gpe)
> > +
> >
> +/********************************************************
> ***********************
> > + *
> > + * FUNCTION:    acpi_unblock_gpe
> > + *
> > + * PARAMETERS:  gpe_device          - Parent GPE Device. NULL for
> GPE0/GPE1
> > + *              gpe_number          - GPE level within the GPE block
> > + *
> > + * RETURN:      Status
> > + *
> > + * DESCRIPTION: Stop unconditional GPE disabling.
> > + *              This API reverts acpi_block_gpe().
> > + *
> > +
> **********************************************************
> ********************/
> > +acpi_status acpi_unblock_gpe(acpi_handle gpe_device, u32
> gpe_number)
> > +{
> > +	struct acpi_gpe_event_info *gpe_event_info;
> > +	acpi_status status;
> > +	acpi_cpu_flags flags;
> > +
> > +	ACPI_FUNCTION_TRACE(acpi_unblock_gpe);
> > +
> > +	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
> > +
> > +	/* 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) {
> > +		status = AE_BAD_PARAMETER;
> > +		goto unlock_and_exit;
> > +	}
> > +
> > +	status = acpi_ev_manage_gpe(gpe_event_info,
> ACPI_GPE_UNMANAGE);
> 
> And that is completely unclear to me.
[Lv Zheng] 
The original code implemented 3 states.
Blocked (GPE register's EN bit is not changeable) but disabled (and it is always 0).
Blocked (GPE register's EN bit is not changeable) but enabled (and it is always 1).
Unblocked (GPE register's EN bit is changeable).

So the wording "MANAGE" is really a bad idea, causing all kind of misunderstandings. :)

> 
> > +
> > +unlock_and_exit:
> > +	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
> > +	return_ACPI_STATUS(status);
> > +}
> > +
> > +ACPI_EXPORT_SYMBOL(acpi_unblock_gpe)
> > +
> >
> +/********************************************************
> ***********************
> > + *
> > + * FUNCTION:    acpi_control_gpe_handling
> > + *
> > + * PARAMETERS:  gpe_device          - Parent GPE Device. NULL for
> GPE0/GPE1
> > + *              gpe_number          - GPE level within the GPE block
> > + *              use_interrupt_mode  - Allow the GPE to be dispatched in the
> > + *                                    interrupt mode
> > + *              use_polling_mode    - Allow the GPE to be dispatched in the
> > + *                                    polling mode
> > + *
> > + * RETURN:      Status
> > + *
> > + * DESCRIPTION: Switch the GPE handling mode between the interrupt
> only mode,
> > + *              the polling only mode and the interrupt/polling adaptive
> mode.
> > + *
> > +
> **********************************************************
> ********************/
> 
> This is extra functionality that you want to piggy back on a fix for a real
> issue.
> 
> Please separate it from that fix, it doesn't belong here.
[Lv Zheng] 
OK.

> 
> > +acpi_status
> > +acpi_control_gpe_handling(acpi_handle gpe_device,
> > +			  u32 gpe_number,
> > +			  u8 use_interrupt_mode, u8 use_polling_mode)
> > +{
> > +	struct acpi_gpe_event_info *gpe_event_info;
> > +	acpi_status status;
> > +	acpi_cpu_flags flags;
> > +	u8 action;
> > +
> > +	ACPI_FUNCTION_TRACE(acpi_control_gpe_handling);
> > +
> > +	if (!use_interrupt_mode && !use_polling_mode) {
> > +		return_ACPI_STATUS(AE_BAD_PARAMETER);
> > +	}
> > +
> > +	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
> > +
> > +	/* 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) {
> > +		status = AE_BAD_PARAMETER;
> > +		goto unlock_and_exit;
> > +	}
> > +
> > +	/* Pick up and peform the correct action */
> > +
> > +	action = ACPI_GPE_UNMANAGE;
> > +	if (!use_interrupt_mode) {
> > +		action = ACPI_GPE_DISABLE;
> > +	}
> > +	if (!use_polling_mode) {
> > +		action = ACPI_GPE_ENABLE;
> > +	}
> > +	status = acpi_ev_manage_gpe(gpe_event_info, action);
> > +
> > +unlock_and_exit:
> > +	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
> > +	return_ACPI_STATUS(status);
> > +}
> > +
> > +ACPI_EXPORT_SYMBOL(acpi_control_gpe_handling)
> > +
> >
> +/********************************************************
> ***********************
> > + *
> >   * FUNCTION:    acpi_mark_gpe_for_wake
> >   *
> >   * PARAMETERS:  gpe_device          - Parent GPE Device. NULL for
> GPE0/GPE1
> > diff --git a/drivers/acpi/acpica/hwgpe.c b/drivers/acpi/acpica/hwgpe.c
> > index bdecd5e..8cdddbe 100644
> > --- a/drivers/acpi/acpica/hwgpe.c
> > +++ b/drivers/acpi/acpica/hwgpe.c
> > @@ -98,7 +98,7 @@ acpi_status
> >  acpi_hw_low_set_gpe(struct acpi_gpe_event_info *gpe_event_info,
> u32 action)
> >  {
> >  	struct acpi_gpe_register_info *gpe_register_info;
> > -	acpi_status status;
> > +	acpi_status status = AE_OK;
> >  	u32 enable_mask;
> >  	u32 register_bit;
> >
> > @@ -148,9 +148,21 @@ acpi_hw_low_set_gpe(struct
> acpi_gpe_event_info *gpe_event_info, u32 action)
> >  		return (AE_BAD_PARAMETER);
> >  	}
> >
> > -	/* Write the updated enable mask */
> > +	/* Check if there is an administrative GPE enabling/disabling */
> >
> > -	status = acpi_hw_write(enable_mask, &gpe_register_info-
> >enable_address);
> > +	if (gpe_event_info->flags & ACPI_GPE_MANAGED_FLAG_MASK) {
> > +		if (enable_mask & register_bit) {
> > +			gpe_event_info->blocked_enabled = TRUE;
> > +		} else {
> > +			gpe_event_info->blocked_enabled = FALSE;
> > +		}
> > +	} else {
> > +		/* Write the updated enable mask */
> > +
> > +		status =
> > +		    acpi_hw_write(enable_mask,
> > +				  &gpe_register_info->enable_address);
> > +	}
> >  	return (status);
> >  }
> >
> > @@ -228,6 +240,12 @@ acpi_hw_get_gpe_status(struct
> acpi_gpe_event_info *gpe_event_info,
> >  		local_event_status |= ACPI_EVENT_FLAG_HAS_HANDLER;
> >  	}
> >
> > +	/* GPE currently managed? (enabled/disabled by the system
> admin?) */
> > +
> > +	if (gpe_event_info->flags & ACPI_GPE_MANAGED_FLAG_MASK) {
> > +		local_event_status |= ACPI_EVENT_FLAG_MANAGED;
> > +	}
> > +
> >  	/* Get the info block for the entire GPE register */
> >
> >  	gpe_register_info = gpe_event_info->register_info;
> > diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> > index 4e4c214..a0a6544 100644
> > --- a/include/acpi/acpixf.h
> > +++ b/include/acpi/acpixf.h
> > @@ -732,15 +732,28 @@
> ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
> >  						u32 gpe_number))
> >
> >  ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
> > -				acpi_mark_gpe_for_wake(acpi_handle
> gpe_device,
> > -						       u32 gpe_number))
> > +				acpi_block_gpe(acpi_handle gpe_device,
> > +					       u32 gpe_number))
> > +
> > +ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
> > +				acpi_unblock_gpe(acpi_handle gpe_device,
> > +						 u32 gpe_number))
> >
> >  ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
> > -				acpi_setup_gpe_for_wake(acpi_handle
> > -							parent_device,
> > -							acpi_handle
> gpe_device,
> > +				acpi_control_gpe_handling(acpi_handle
> > +							  gpe_device,
> > +							  u32 gpe_number,
> > +							  u8
> use_interrupt_mode,
> > +							  u8
> use_polling_mode))
> > +ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
> > +				 acpi_mark_gpe_for_wake(acpi_handle
> gpe_device,
> >  							u32 gpe_number))
> >  ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
> > +				 acpi_setup_gpe_for_wake(acpi_handle
> > +							 parent_device,
> > +							 acpi_handle
> gpe_device,
> > +							 u32 gpe_number))
> > +ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
> >  				 acpi_set_gpe_wake_mask(acpi_handle
> gpe_device,
> >  							u32 gpe_number,
> >  							u8 action))
> > diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
> > index cb389ef..9f841f2 100644
> > --- a/include/acpi/actypes.h
> > +++ b/include/acpi/actypes.h
> > @@ -732,16 +732,17 @@ typedef u32 acpi_event_type;
> >   * The encoding of acpi_event_status is illustrated below.
> >   * Note that a set bit (1) indicates the property is TRUE
> >   * (e.g. if bit 0 is set then the event is enabled).
> > - * +-------------+-+-+-+-+-+
> > - * |   Bits 31:5 |4|3|2|1|0|
> > - * +-------------+-+-+-+-+-+
> > - *          |     | | | | |
> > - *          |     | | | | +- Enabled?
> > - *          |     | | | +--- Enabled for wake?
> > - *          |     | | +----- Status bit set?
> > - *          |     | +------- Enable bit set?
> > - *          |     +--------- Has a handler?
> > - *          +--------------- <Reserved>
> > + * +-------------+-+-+-+-+-+-+
> > + * |   Bits 31:6 |5|4|3|2|1|0|
> > + * +-------------+-+-+-+-+-+-+
> > + *          |     | | | | | |
> > + *          |     | | | | | +- Enabled?
> > + *          |     | | | | +--- Enabled for wake?
> > + *          |     | | | +----- Status bit set?
> > + *          |     | | +------- Enable bit set?
> > + *          |     | +--------- Has a handler?
> > + *          |     +----------- Enabling/disabling forced?
> > + *          +----------------- <Reserved>
> >   */
> >  typedef u32 acpi_event_status;
> >
> > @@ -751,6 +752,7 @@ typedef u32 acpi_event_status;
> >  #define ACPI_EVENT_FLAG_STATUS_SET      (acpi_event_status) 0x04
> >  #define ACPI_EVENT_FLAG_ENABLE_SET      (acpi_event_status) 0x08
> >  #define ACPI_EVENT_FLAG_HAS_HANDLER     (acpi_event_status) 0x10
> > +#define ACPI_EVENT_FLAG_MANAGED         (acpi_event_status) 0x20
> 
> The meaning of this new flag is unclear.  It probably should be something
> like
> ACPI_EVENT_FLAG_BLOCKED or MASKED meaning that if set, the GPE
> cannot be
> disabled or enabled by usual means (until that flag is cleared).
[Lv Zheng] 
OK. 

> 
> >  #define ACPI_EVENT_FLAG_SET             ACPI_EVENT_FLAG_STATUS_SET
> >
> >  /* Actions for acpi_set_gpe, acpi_gpe_wakeup, acpi_hw_low_set_gpe
> */
> > @@ -758,17 +760,20 @@ typedef u32 acpi_event_status;
> >  #define ACPI_GPE_ENABLE                 0
> >  #define ACPI_GPE_DISABLE                1
> >  #define ACPI_GPE_CONDITIONAL_ENABLE     2
> > +#define ACPI_GPE_UNMANAGE               3
> >
> >  /*
> >   * GPE info flags - Per GPE
> > - * +-------+-+-+---+
> > - * |  7:5  |4|3|2:0|
> > - * +-------+-+-+---+
> > - *     |    | |  |
> > - *     |    | |  +-- Type of dispatch:to method, handler, notify, or none
> > - *     |    | +----- Interrupt type: edge or level triggered
> > - *     |    +------- Is a Wake GPE
> > - *     +------------ <Reserved>
> > + * +-+-+-+-+-+---+
> > + * |7|6|5|4|3|2:0|
> > + * +-+-+-+-+-+---+
> > + *  | | | | |  |
> > + *  | | | | |  +-- Type of dispatch:to method, handler, notify, or none
> > + *  | | | | +----- Interrupt type: edge or level triggered
> > + *  | | | +------- Is a Wake GPE
> > + *  | | +--------- Force GPE enabling
> > + *  | +----------- Force GPE disabling
> > + *  +------------- <Reserved>
> >   */
> >  #define ACPI_GPE_DISPATCH_NONE          (u8) 0x00
> >  #define ACPI_GPE_DISPATCH_METHOD        (u8) 0x01
> > @@ -785,6 +790,50 @@ typedef u32 acpi_event_status;
> >  #define ACPI_GPE_CAN_WAKE               (u8) 0x10
> >
> >  /*
> > + * Flags used by the GPE management APIs
> > + *
> > + * The GPE management APIs are not implemented for the GPE drivers.
> It is
> > + * designed for providing the management purposes out of the GPE
> drivers'
> > + * awareness. The GPE driver APIs should still be working as expected
> > + * during the period the GPE management purposes are applied. There
> are 2
> > + * use cases for the flags:
> > + *
> > + * 1. Prevent the GPE flooding
> > + *    These flags can be used to control how the GPE is dispatched by the
> > + *    event dispatcher. Note that the ACPI_GPE_MANAGED_DISABLED
> can also be
> > + *    used to prevent the GPE flooding that cannot be prevented due to
> the
> > + *    ACPI_GPE_DISPATCH_NONE sanity check. This kind of the GPE
> flooding
> > + *    happens on the GPE where there is a _Lxx/_Exx prepared by the
> BIOS
> > + *    but the OSPM still cannot handle it correctly by executing the
> > + *    method. Such kind of incapability is mostly because of the feature
> > + *    gaps in the OSPM:
> > + *    ACPI_GPE_MANAGED_ENABLED:  force the event dispatcher to
> always
> > + *                               enable the GPE
> > + *    ACPI_GPE_MANAGED_DISABLED: force the event dispatcher to
> always
> > + *                               disable the GPE
> > + *                               prevent the GPE from flooding
> > + *    acpi_block_gpe()/acpi_unblock_gpe() are the APIs offering the GPE
> > + *    flooding prevention feature.
> > + *
> > + * 2. Control the GPE handling mode
> > + *    A GPE driver may be able to handle the GPE both in the interrupt
> mode
> > + *    and in the polling mode. Since the polling mode is a mechanism
> > + *    implemented by the driver itself and is not controlled by the event
> > + *    dispatcher, this mechanism can be used to switch between the
> > + *    interrupt mode (dispatched via the event dispatcher) and the
> polling
> > + *    mode (implemented by the GPE drivers):
> > + *    ACPI_GPE_MANAGED_ENABLED:  force the driver to use the
> interrupt mode
> > + *    ACPI_GPE_MANAGED_DISABLED: force the driver to use the polling
> mode
> > + *    None:                      allow the driver to use the
> > + *                               interrupt/polling adaptive mode
> > + *    acpi_control_gpe_handling() is the API offering the GPE handling
> mode
> > + *    switch feature.
> > + */
> > +#define ACPI_GPE_MANAGED_ENABLED        (u8) 0x20
> > +#define ACPI_GPE_MANAGED_DISABLED       (u8) 0x40
> > +#define ACPI_GPE_MANAGED_FLAG_MASK      (u8) 0x60
> > +
> > +/*
> >   * Flags for GPE and Lock interfaces
> >   */
> >  #define ACPI_NOT_ISR                    0x1
> 
> Thanks,
> Rafael
[Lv Zheng] 

Thanks for the review.

Best regards
-Lv

Re: [PATCH 1/3] ACPICA: Events: Introduce acpi_block_gpe()/acpi_unblock_gpe()/acpi_control_gpe_handling() to allow administrative GPE enabling/disabling

[Rafael J. Wysocki]

On Wed, Jun 8, 2016 at 9:49 AM, Zheng, Lv <lv.zheng@intel.com> wrote:
> Hi,
>
>> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
>> Subject: Re: [PATCH 1/3] ACPICA: Events: Introduce
>> acpi_block_gpe()/acpi_unblock_gpe()/acpi_control_gpe_handling() to
>> allow administrative GPE enabling/disabling
>>
>> On Monday, May 16, 2016 05:11:11 PM Lv Zheng wrote:
>> > There is a facility in Linux, developers can manage GPE enabling/disabling
>> > through /sys/firmware/acpi/interrupts/gpexx. This is mainly for
>> debugging
>> > purposes. Many users expect to use this facility to implement quirks to
>> > disable specific GPEs when there is a gap in Linux causing GPE flood.
>> > This is not working correctly because currently this facility invokes
>> > enabling/disabling counting based GPE driver APIs:
>> >  acpi_enable_gpe()/acpi_disable_gpe()
>> > and the GPE drivers can still affect the count to mess up the GPE
>> > management purposes.
>> >
>> > This patch introduces acpi_block_gpe()/acpi_unblock_gpe() to be used in
>> such
>> > situation instead of acpi_enable_gpe()/acpi_disable_gpe().
>>
>> Up to this point, I agree.
>>
>> > The idea to implement this is to replace the GPE register EN bit with the
>> > managed value, block EN set/clear operations but record the operation
>> > results in blocked_enabled, so that after the managed state is removed,
>> > restore the saved blocked_enabled back to the GPE register EN bit.
>>
>> Unfortunately, I don't really follow the above paragraph, so chances are
>> that
>> whoever is not familiar with the code will not be able to follow it either.
> [Lv Zheng]
> I see.
> I should stop talking this using "managed".
> It is better to use "masked".
> As this facility is actually trying to implement the kind of the facility that can be seen in many other IRQ chips - the IRQ MASK bit.

This is a somewhat simplified special case of it, though.

>> > Now OSPMs should be able to use this facility to generate quirks. ACPICA
>> > BZ 1102.
>> >
>> > This facility can also be used by the administrator to control the GPE
>> > handling mode during the runtime when the driver is capable of handling
>> the
>> > GPE in both the interrupt mode and the polling mode (for example, the
>> Linux
>> > EC driver). acpi_control_gpe_handling() is offered for this purpose. Lv
>> Zheng.
>>
>> That is too much.  The patch should focus on the block/unblock
>> functionality.
>> Anything beyond that should be added later IMO.
> [Lv Zheng]
> OK.
> So after examining all of your comments.
> I think what I need to improve is eliminating this feature.
> That should be able to make the code simpler and thus easier for the others to follow.

Sounds good!

[PATCH 2/3] ACPI / sys: Update /sys/firmware/acpi/interrupts/gpexx using new GPE forced disabling/enabling mechanism

[Lv Zheng]

Now GPE can be forced enabling/disabling through GPE management APIs, this
patch modifies /sys/firmware/acpi/interrupts/gpexx to use this new
facility.

The "block" command is implemented to invoke acpi_block_gpe() and the
"unblock" command is implemented to invoke acpi_unblock_gpe().
The "force-poll"/"force-irq"/"unforce" command is implemented to invoke
acpi_control_gpe_handling() to switch GPE handling mode (for EC driver
only).

"EN STS" is returned to display the current hardware register status, along
with "!" flag to indicate the register bit unset and "*" flag to indicate
the managed state.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/sleep.c |    2 +-
 drivers/acpi/sysfs.c |   36 ++++++++++++++++++++++++++++++++----
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 7a2e4d4..d00544c 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -555,7 +555,7 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
 
 		acpi_get_event_status(ACPI_EVENT_POWER_BUTTON, &pwr_btn_status);
 
-		if (pwr_btn_status & ACPI_EVENT_FLAG_SET) {
+		if (pwr_btn_status & ACPI_EVENT_FLAG_STATUS_SET) {
 			acpi_clear_event(ACPI_EVENT_POWER_BUTTON);
 			/* Flag for later */
 			pwr_btn_event_pending = true;
diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 4b3a9e2..7f33c90 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -599,6 +599,19 @@ static ssize_t counter_show(struct kobject *kobj,
 	if (result)
 		goto end;
 
+	if (status & ACPI_EVENT_FLAG_ENABLE_SET)
+		size += sprintf(buf + size, "      EN");
+	else
+		size += sprintf(buf + size, "     !EN");
+	if (status & ACPI_EVENT_FLAG_MANAGED)
+		size += sprintf(buf + size, "*");
+	else
+		size += sprintf(buf + size, " ");
+	if (status & ACPI_EVENT_FLAG_STATUS_SET)
+		size += sprintf(buf + size, "     STS");
+	else
+		size += sprintf(buf + size, "    !STS");
+
 	if (!(status & ACPI_EVENT_FLAG_HAS_HANDLER))
 		size += sprintf(buf + size, "   invalid");
 	else if (status & ACPI_EVENT_FLAG_ENABLED)
@@ -656,8 +669,23 @@ static ssize_t counter_set(struct kobject *kobj,
 		else if (!strcmp(buf, "enable\n") &&
 			 !(status & ACPI_EVENT_FLAG_ENABLED))
 			result = acpi_enable_gpe(handle, index);
+		else if (!strcmp(buf, "block\n"))
+			result = acpi_block_gpe(handle, index);
+		else if (!strcmp(buf, "unblock\n") &&
+			 (status & ACPI_EVENT_FLAG_MANAGED))
+			result = acpi_unblock_gpe(handle, index);
+		else if (!strcmp(buf, "force-poll\n"))
+			result = acpi_control_gpe_handling(handle, index,
+							   TRUE, FALSE);
+		else if (!strcmp(buf, "force-irq\n"))
+			result = acpi_control_gpe_handling(handle, index,
+							   FALSE, TRUE);
+		else if (!strcmp(buf, "unforce\n") &&
+			 (status & ACPI_EVENT_FLAG_MANAGED))
+			result = acpi_control_gpe_handling(handle, index,
+							   TRUE, TRUE);
 		else if (!strcmp(buf, "clear\n") &&
-			 (status & ACPI_EVENT_FLAG_SET))
+			 (status & ACPI_EVENT_FLAG_STATUS_SET))
 			result = acpi_clear_gpe(handle, index);
 		else if (!kstrtoul(buf, 0, &tmp))
 			all_counters[index].count = tmp;
@@ -666,13 +694,13 @@ static ssize_t counter_set(struct kobject *kobj,
 	} else if (index < num_gpes + ACPI_NUM_FIXED_EVENTS) {
 		int event = index - num_gpes;
 		if (!strcmp(buf, "disable\n") &&
-		    (status & ACPI_EVENT_FLAG_ENABLED))
+		    (status & ACPI_EVENT_FLAG_ENABLE_SET))
 			result = acpi_disable_event(event, ACPI_NOT_ISR);
 		else if (!strcmp(buf, "enable\n") &&
-			 !(status & ACPI_EVENT_FLAG_ENABLED))
+			 !(status & ACPI_EVENT_FLAG_ENABLE_SET))
 			result = acpi_enable_event(event, ACPI_NOT_ISR);
 		else if (!strcmp(buf, "clear\n") &&
-			 (status & ACPI_EVENT_FLAG_SET))
+			 (status & ACPI_EVENT_FLAG_STATUS_SET))
 			result = acpi_clear_event(event);
 		else if (!kstrtoul(buf, 0, &tmp))
 			all_counters[index].count = tmp;
-- 
1.7.10

[PATCH 3/3] ACPI / sysfs: Provide quirk mechanism to prevent GPE flooding

[Lv Zheng]

Sometimes, the users may require a quirk to be provided from ACPI subsystem
core to prevent a GPE from flooding. Normally, if a GPE cannot be
dispatched, ACPICA core automatically prevents the GPE from firing. But
there are cases the GPE is dispatched by _Lxx/_Exx provided via AML table,
and OSPM is lacking of the knowledge to get _Lxx/_Exx correctly executed to
handle the GPE, thus the GPE flooding may still occur.

This patch provides a quirk mechanism to stop this kind of GPE flooding.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=53071
Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/887793
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/internal.h |    1 +
 drivers/acpi/scan.c     |    1 +
 drivers/acpi/sleep.c    |    2 ++
 drivers/acpi/sysfs.c    |   56 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 60 insertions(+)

diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 9bb0773..d0f1744 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -37,6 +37,7 @@ void acpi_amba_init(void);
 static inline void acpi_amba_init(void) {}
 #endif
 int acpi_sysfs_init(void);
+void acpi_gpe_apply_blocked_gpes(void);
 void acpi_container_init(void);
 void acpi_memory_hotplug_init(void);
 #ifdef	CONFIG_ACPI_HOTPLUG_IOAPIC
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 5f28cf7..5ff366c 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1958,6 +1958,7 @@ int __init acpi_scan_init(void)
 		}
 	}
 
+	acpi_gpe_apply_blocked_gpes();
 	acpi_update_all_gpes();
 
  out:
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index d00544c..daba3ba 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -414,6 +414,7 @@ static void acpi_pm_finish(void)
 		acpi_state);
 	acpi_disable_wakeup_devices(acpi_state);
 	acpi_leave_sleep_state(acpi_state);
+	acpi_gpe_apply_blocked_gpes();
 
 	/* reset firmware waking vector */
 	acpi_set_waking_vector(0);
@@ -774,6 +775,7 @@ static void acpi_pm_thaw(void)
 {
 	acpi_ec_unblock_transactions();
 	acpi_enable_all_runtime_gpes();
+	acpi_gpe_apply_blocked_gpes();
 }
 
 static const struct platform_hibernation_ops acpi_hibernation_ops = {
diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 7f33c90..a2fb524 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -715,6 +715,62 @@ end:
 	return result ? result : size;
 }
 
+/*
+ * A Quirk Mechanism for GPE Flooding Prevention:
+ *
+ * Quirks may be needed to prevent GPE flooding on a specific GPE. The
+ * flooding typically cannot be detected and automatically prevented by
+ * ACPI_GPE_DISPATCH_NONE check because there is a _Lxx/_Exx prepared in
+ * the AML tables. This normally indicates a feature gap in Linux, thus
+ * instead of providing endless quirk tables, we provide a boot parameter
+ * for those who want this quirk. For example, if the users want to prevent
+ * the GPE flooding for GPE 00, they need to specify the following boot
+ * parameter:
+ *   acpi.block_gpe=0x00
+ * The blocking status can be modified by the following runtime controlling
+ * interface:
+ *   echo unblock > /sys/firmware/acpi/interrupts/gpe00
+ */
+
+/*
+ * Currently, the GPE flooding prevention only supports to block the GPEs
+ * numbered from 00 to 63.
+ */
+#define ACPI_BLOCKABLE_GPE_MAX	64
+
+static u64 acpi_blocked_gpes;
+
+static int __init acpi_gpe_set_blocked_gpes(char *val)
+{
+	u8 gpe;
+
+	if (kstrtou8(val, 0, &gpe) || gpe > ACPI_BLOCKABLE_GPE_MAX)
+		return -EINVAL;
+	acpi_blocked_gpes |= ((u64)1<<gpe);
+
+	return 1;
+}
+__setup("acpi_block_gpe=", acpi_gpe_set_blocked_gpes);
+
+void acpi_gpe_apply_blocked_gpes(void)
+{
+	acpi_handle handle;
+	acpi_status status;
+	u8 gpe;
+
+	for (gpe = 0;
+	     gpe < min_t(u8, ACPI_BLOCKABLE_GPE_MAX, acpi_current_gpe_count);
+	     gpe++) {
+		if (acpi_blocked_gpes & ((u64)1<<gpe)) {
+			status = acpi_get_gpe_device(gpe, &handle);
+			if (ACPI_SUCCESS(status)) {
+				pr_info("Blocking GPE 0x%x.\n", gpe);
+				(void)acpi_block_gpe(handle, gpe);
+			}
+		}
+	}
+}
+
 void acpi_irq_stats_init(void)
 {
 	acpi_status status;
-- 
1.7.10

[PATCH v2 0/3] ACPI / gpe: Add GPE masking/unmasking mechanism

[Lv Zheng]

This patchset adds a new mechanism that can be used to mask/unmask GPEs.

Lv Zheng (3):
  ACPICA: Events: Introduce acpi_mask_gpe()/acpi_unmask_gpe() to
    implement GPE masking mechanism
  ACPI / sys: Update /sys/firmware/acpi/interrupts/gpexx using new GPE
    masking mechanism
  ACPI / sysfs: Provide quirk mechanism to prevent GPE flooding

 Documentation/kernel-parameters.txt |   10 ++++
 drivers/acpi/acpica/acevents.h      |    3 ++
 drivers/acpi/acpica/aclocal.h       |    2 +
 drivers/acpi/acpica/evgpe.c         |   57 ++++++++++++++++++++++
 drivers/acpi/acpica/evxfgpe.c       |   43 +++++++++++++++++
 drivers/acpi/acpica/hwgpe.c         |   23 +++++++--
 drivers/acpi/internal.h             |    1 +
 drivers/acpi/scan.c                 |    1 +
 drivers/acpi/sleep.c                |    2 +-
 drivers/acpi/sysfs.c                |   89 +++++++++++++++++++++++++++++++----
 include/acpi/acpixf.h               |    4 ++
 include/acpi/actypes.h              |   39 ++++++++-------
 12 files changed, 242 insertions(+), 32 deletions(-)

-- 
1.7.10

[PATCH v2 1/3] ACPICA: Events: Introduce acpi_mask_gpe()/acpi_unmask_gpe() to implement GPE masking mechanism

[Lv Zheng]

There is a facility in Linux, developers can control the enabling/disabling
of a GPE via /sys/firmware/acpi/interrupts/gpexx. This is mainly for
debugging purposes.

But many users expect to use this facility to implement quirks to mask a
specific GPE when there is a gap in Linux causing this GPE to flood. This
is not working correctly because currently this facility invokes
enabling/disabling counting based GPE driver APIs:
 acpi_enable_gpe()/acpi_disable_gpe()
and the GPE drivers can still affect the count to mess up the GPE
masking purposes.

However, most of the IRQ chip designs allow masking/unmasking IRQs via a
masking bit which is different from the enabled bit to achieve the same
purpose. But the GPE hardware doesn't contain such a feature, this brings
the trouble.

In this patch, we introduce a software mechanism to implement the GPE
masking feature, and acpi_mask_gpe()/acpi_unmask_gpe() are provided to the
OSPMs to mask/unmask GPEs in the above mentioned situation instead of
acpi_enable_gpe()/acpi_disable_gpe(). ACPICA BZ 1102. Lv Zheng.

Link: https://bugs.acpica.org/show_bug.cgi?id=1102
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/acpica/acevents.h |    3 +++
 drivers/acpi/acpica/aclocal.h  |    2 ++
 drivers/acpi/acpica/evgpe.c    |   57 ++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/acpica/evxfgpe.c  |   43 ++++++++++++++++++++++++++++++
 drivers/acpi/acpica/hwgpe.c    |   23 ++++++++++++----
 include/acpi/acpixf.h          |    4 +++
 include/acpi/actypes.h         |   39 ++++++++++++++-------------
 7 files changed, 148 insertions(+), 23 deletions(-)

diff --git a/drivers/acpi/acpica/acevents.h b/drivers/acpi/acpica/acevents.h
index 77af91c..92fa47c 100644
--- a/drivers/acpi/acpica/acevents.h
+++ b/drivers/acpi/acpica/acevents.h
@@ -86,6 +86,9 @@ acpi_ev_update_gpe_enable_mask(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_mask_gpe(struct acpi_gpe_event_info *gpe_event_info, u8 is_masked);
+
+acpi_status
 acpi_ev_add_gpe_reference(struct acpi_gpe_event_info *gpe_event_info);
 
 acpi_status
diff --git a/drivers/acpi/acpica/aclocal.h b/drivers/acpi/acpica/aclocal.h
index 13331d7..dff1207 100644
--- a/drivers/acpi/acpica/aclocal.h
+++ b/drivers/acpi/acpica/aclocal.h
@@ -484,6 +484,7 @@ struct acpi_gpe_event_info {
 	u8 flags;		/* Misc info about this GPE */
 	u8 gpe_number;		/* This GPE */
 	u8 runtime_count;	/* References to a run GPE */
+	u8 disable_for_dispatch;	/* Masked during dispatching */
 };
 
 /* Information about a GPE register pair, one per each status/enable pair in an array */
@@ -494,6 +495,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 mask_for_run;	/* GPEs to keep masked when running */
 	u8 enable_mask;		/* Current mask of enabled GPEs */
 };
 
diff --git a/drivers/acpi/acpica/evgpe.c b/drivers/acpi/acpica/evgpe.c
index 4b4949c..bdb10be 100644
--- a/drivers/acpi/acpica/evgpe.c
+++ b/drivers/acpi/acpica/evgpe.c
@@ -130,6 +130,60 @@ acpi_status acpi_ev_enable_gpe(struct acpi_gpe_event_info *gpe_event_info)
 
 /*******************************************************************************
  *
+ * FUNCTION:    acpi_ev_mask_gpe
+ *
+ * PARAMETERS:  gpe_event_info          - GPE to be blocked/unblocked
+ *              is_masked               - Whether the GPE is masked or not
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Unconditionally mask/unmask a GPE during runtime.
+ *
+ ******************************************************************************/
+
+acpi_status
+acpi_ev_mask_gpe(struct acpi_gpe_event_info *gpe_event_info, u8 is_masked)
+{
+	struct acpi_gpe_register_info *gpe_register_info;
+	u32 register_bit;
+
+	ACPI_FUNCTION_TRACE(ev_mask_gpe);
+
+	gpe_register_info = gpe_event_info->register_info;
+	if (!gpe_register_info) {
+		return_ACPI_STATUS(AE_NOT_EXIST);
+	}
+
+	register_bit = acpi_hw_get_gpe_register_bit(gpe_event_info);
+
+	/* Perform the action */
+
+	if (is_masked) {
+		if (register_bit & gpe_register_info->mask_for_run) {
+			return_ACPI_STATUS(AE_BAD_PARAMETER);
+		}
+
+		(void)acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_DISABLE);
+		ACPI_SET_BIT(gpe_register_info->mask_for_run, (u8)register_bit);
+	} else {
+		if (!(register_bit & gpe_register_info->mask_for_run)) {
+			return_ACPI_STATUS(AE_BAD_PARAMETER);
+		}
+
+		ACPI_CLEAR_BIT(gpe_register_info->mask_for_run,
+			       (u8)register_bit);
+		if (gpe_event_info->runtime_count
+		    && !gpe_event_info->disable_for_dispatch) {
+			(void)acpi_hw_low_set_gpe(gpe_event_info,
+						  ACPI_GPE_ENABLE);
+		}
+	}
+
+	return_ACPI_STATUS(AE_OK);
+}
+
+/*******************************************************************************
+ *
  * FUNCTION:    acpi_ev_add_gpe_reference
  *
  * PARAMETERS:  gpe_event_info          - Add a reference to this GPE
@@ -674,6 +728,7 @@ acpi_status acpi_ev_finish_gpe(struct acpi_gpe_event_info *gpe_event_info)
 	 * in the event_info.
 	 */
 	(void)acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_CONDITIONAL_ENABLE);
+	gpe_event_info->disable_for_dispatch = FALSE;
 	return (AE_OK);
 }
 
@@ -737,6 +792,8 @@ acpi_ev_gpe_dispatch(struct acpi_namespace_node *gpe_device,
 		}
 	}
 
+	gpe_event_info->disable_for_dispatch = TRUE;
+
 	/*
 	 * Dispatch the GPE to either an installed handler or the control
 	 * method associated with this GPE (_Lxx or _Exx). If a handler
diff --git a/drivers/acpi/acpica/evxfgpe.c b/drivers/acpi/acpica/evxfgpe.c
index 17cfef7..d7a3b27 100644
--- a/drivers/acpi/acpica/evxfgpe.c
+++ b/drivers/acpi/acpica/evxfgpe.c
@@ -235,11 +235,13 @@ acpi_status acpi_set_gpe(acpi_handle gpe_device, u32 gpe_number, u8 action)
 	case ACPI_GPE_ENABLE:
 
 		status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_ENABLE);
+		gpe_event_info->disable_for_dispatch = FALSE;
 		break;
 
 	case ACPI_GPE_DISABLE:
 
 		status = acpi_hw_low_set_gpe(gpe_event_info, ACPI_GPE_DISABLE);
+		gpe_event_info->disable_for_dispatch = TRUE;
 		break;
 
 	default:
@@ -257,6 +259,47 @@ ACPI_EXPORT_SYMBOL(acpi_set_gpe)
 
 /*******************************************************************************
  *
+ * FUNCTION:    acpi_mask_gpe
+ *
+ * PARAMETERS:  gpe_device          - Parent GPE Device. NULL for GPE0/GPE1
+ *              gpe_number          - GPE level within the GPE block
+ *              is_masked           - Whether the GPE is masked or not
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Unconditionally mask/unmask the an individual GPE, ex., to
+ *              prevent a GPE flooding.
+ *
+ ******************************************************************************/
+acpi_status acpi_mask_gpe(acpi_handle gpe_device, u32 gpe_number, u8 is_masked)
+{
+	struct acpi_gpe_event_info *gpe_event_info;
+	acpi_status status;
+	acpi_cpu_flags flags;
+
+	ACPI_FUNCTION_TRACE(acpi_mask_gpe);
+
+	flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
+
+	/* 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) {
+		status = AE_BAD_PARAMETER;
+		goto unlock_and_exit;
+	}
+
+	status = acpi_ev_mask_gpe(gpe_event_info, is_masked);
+
+unlock_and_exit:
+	acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
+	return_ACPI_STATUS(status);
+}
+
+ACPI_EXPORT_SYMBOL(acpi_mask_gpe)
+
+/*******************************************************************************
+ *
  * FUNCTION:    acpi_mark_gpe_for_wake
  *
  * PARAMETERS:  gpe_device          - Parent GPE Device. NULL for GPE0/GPE1
diff --git a/drivers/acpi/acpica/hwgpe.c b/drivers/acpi/acpica/hwgpe.c
index bdecd5e..ccea12a 100644
--- a/drivers/acpi/acpica/hwgpe.c
+++ b/drivers/acpi/acpica/hwgpe.c
@@ -98,7 +98,7 @@ acpi_status
 acpi_hw_low_set_gpe(struct acpi_gpe_event_info *gpe_event_info, u32 action)
 {
 	struct acpi_gpe_register_info *gpe_register_info;
-	acpi_status status;
+	acpi_status status = AE_OK;
 	u32 enable_mask;
 	u32 register_bit;
 
@@ -148,9 +148,14 @@ acpi_hw_low_set_gpe(struct acpi_gpe_event_info *gpe_event_info, u32 action)
 		return (AE_BAD_PARAMETER);
 	}
 
-	/* Write the updated enable mask */
+	if (!(register_bit & gpe_register_info->mask_for_run)) {
 
-	status = acpi_hw_write(enable_mask, &gpe_register_info->enable_address);
+		/* Write the updated enable mask */
+
+		status =
+		    acpi_hw_write(enable_mask,
+				  &gpe_register_info->enable_address);
+	}
 	return (status);
 }
 
@@ -242,6 +247,12 @@ acpi_hw_get_gpe_status(struct acpi_gpe_event_info *gpe_event_info,
 		local_event_status |= ACPI_EVENT_FLAG_ENABLED;
 	}
 
+	/* GPE currently masked? (masked for runtime?) */
+
+	if (register_bit & gpe_register_info->mask_for_run) {
+		local_event_status |= ACPI_EVENT_FLAG_MASKED;
+	}
+
 	/* GPE enabled for wake? */
 
 	if (register_bit & gpe_register_info->enable_for_wake) {
@@ -397,6 +408,7 @@ 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;
+	u32 enable_mask;
 
 	/* NOTE: assumes that all GPEs are currently disabled */
 
@@ -410,9 +422,10 @@ acpi_hw_enable_runtime_gpe_block(struct acpi_gpe_xrupt_info *gpe_xrupt_info,
 
 		/* Enable all "runtime" GPEs in this register */
 
+		enable_mask = gpe_register_info->enable_for_run &
+		    ~gpe_register_info->mask_for_run;
 		status =
-		    acpi_hw_gpe_enable_write(gpe_register_info->enable_for_run,
-					     gpe_register_info);
+		    acpi_hw_gpe_enable_write(enable_mask, gpe_register_info);
 		if (ACPI_FAILURE(status)) {
 			return (status);
 		}
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index 4e4c214..7aff081 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -732,6 +732,10 @@ ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
 						u32 gpe_number))
 
 ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
+				acpi_mask_gpe(acpi_handle gpe_device,
+					      u32 gpe_number, u8 is_masked))
+
+ACPI_HW_DEPENDENT_RETURN_STATUS(acpi_status
 				acpi_mark_gpe_for_wake(acpi_handle gpe_device,
 						       u32 gpe_number))
 
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h
index cb389ef..fa4bd6a 100644
--- a/include/acpi/actypes.h
+++ b/include/acpi/actypes.h
@@ -732,16 +732,17 @@ typedef u32 acpi_event_type;
  * The encoding of acpi_event_status is illustrated below.
  * Note that a set bit (1) indicates the property is TRUE
  * (e.g. if bit 0 is set then the event is enabled).
- * +-------------+-+-+-+-+-+
- * |   Bits 31:5 |4|3|2|1|0|
- * +-------------+-+-+-+-+-+
- *          |     | | | | |
- *          |     | | | | +- Enabled?
- *          |     | | | +--- Enabled for wake?
- *          |     | | +----- Status bit set?
- *          |     | +------- Enable bit set?
- *          |     +--------- Has a handler?
- *          +--------------- <Reserved>
+ * +-------------+-+-+-+-+-+-+
+ * |   Bits 31:6 |5|4|3|2|1|0|
+ * +-------------+-+-+-+-+-+-+
+ *          |     | | | | | |
+ *          |     | | | | | +- Enabled?
+ *          |     | | | | +--- Enabled for wake?
+ *          |     | | | +----- Status bit set?
+ *          |     | | +------- Enable bit set?
+ *          |     | +--------- Has a handler?
+ *          |     +----------- Masked?
+ *          +----------------- <Reserved>
  */
 typedef u32 acpi_event_status;
 
@@ -751,6 +752,7 @@ typedef u32 acpi_event_status;
 #define ACPI_EVENT_FLAG_STATUS_SET      (acpi_event_status) 0x04
 #define ACPI_EVENT_FLAG_ENABLE_SET      (acpi_event_status) 0x08
 #define ACPI_EVENT_FLAG_HAS_HANDLER     (acpi_event_status) 0x10
+#define ACPI_EVENT_FLAG_MASKED          (acpi_event_status) 0x20
 #define ACPI_EVENT_FLAG_SET             ACPI_EVENT_FLAG_STATUS_SET
 
 /* Actions for acpi_set_gpe, acpi_gpe_wakeup, acpi_hw_low_set_gpe */
@@ -761,14 +763,15 @@ typedef u32 acpi_event_status;
 
 /*
  * GPE info flags - Per GPE
- * +-------+-+-+---+
- * |  7:5  |4|3|2:0|
- * +-------+-+-+---+
- *     |    | |  |
- *     |    | |  +-- Type of dispatch:to method, handler, notify, or none
- *     |    | +----- Interrupt type: edge or level triggered
- *     |    +------- Is a Wake GPE
- *     +------------ <Reserved>
+ * +---+-+-+-+---+
+ * |7:6|5|4|3|2:0|
+ * +---+-+-+-+---+
+ *   |  | | |  |
+ *   |  | | |  +-- Type of dispatch:to method, handler, notify, or none
+ *   |  | | +----- Interrupt type: edge or level triggered
+ *   |  | +------- Is a Wake GPE
+ *   |  +--------- Is GPE masked by the software GPE masking machanism
+ *   +------------ <Reserved>
  */
 #define ACPI_GPE_DISPATCH_NONE          (u8) 0x00
 #define ACPI_GPE_DISPATCH_METHOD        (u8) 0x01
-- 
1.7.10

Re: [UPDATE PATCH v2 1/3] ACPICA: Events: Introduce acpi_mask_gpe() to implement GPE masking mechanism

[Rafael J. Wysocki]

On Thursday, June 23, 2016 03:05:47 PM Lv Zheng wrote:
> (remove acpi_unmask_gpe() from the patch description)
> 
> There is a facility in Linux, developers can control the enabling/disabling
> of a GPE via /sys/firmware/acpi/interrupts/gpexx. This is mainly for
> debugging purposes.
> 
> But many users expect to use this facility to implement quirks to mask a
> specific GPE when there is a gap in Linux causing this GPE to flood. This
> is not working correctly because currently this facility invokes
> enabling/disabling counting based GPE driver APIs:
>  acpi_enable_gpe()/acpi_disable_gpe()
> and the GPE drivers can still affect the count to mess up the GPE
> masking purposes.
> 
> However, most of the IRQ chip designs allow masking/unmasking IRQs via a
> masking bit which is different from the enabled bit to achieve the same
> purpose. But the GPE hardware doesn't contain such a feature, this brings
> the trouble.
> 
> In this patch, we introduce a software mechanism to implement the GPE
> masking feature, and acpi_mask_gpe() are provided to the OSPMs to
> mask/unmask GPEs in the above mentioned situation instead of
> acpi_enable_gpe()/acpi_disable_gpe(). ACPICA BZ 1102. Lv Zheng.
> 
> Link: https://bugs.acpica.org/show_bug.cgi?id=1102
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>

I've queued up this one and the [2/3] and please see my comments on the [3/3].

Thanks,
Rafael

Re: [UPDATE PATCH v2 1/3] ACPICA: Events: Introduce acpi_mask_gpe() to implement GPE masking mechanism

[Rafael J. Wysocki]

On Monday, July 04, 2016 03:59:07 PM Rafael J. Wysocki wrote:
> On Thursday, June 23, 2016 03:05:47 PM Lv Zheng wrote:
> > (remove acpi_unmask_gpe() from the patch description)
> > 
> > There is a facility in Linux, developers can control the enabling/disabling
> > of a GPE via /sys/firmware/acpi/interrupts/gpexx. This is mainly for
> > debugging purposes.
> > 
> > But many users expect to use this facility to implement quirks to mask a
> > specific GPE when there is a gap in Linux causing this GPE to flood. This
> > is not working correctly because currently this facility invokes
> > enabling/disabling counting based GPE driver APIs:
> >  acpi_enable_gpe()/acpi_disable_gpe()
> > and the GPE drivers can still affect the count to mess up the GPE
> > masking purposes.
> > 
> > However, most of the IRQ chip designs allow masking/unmasking IRQs via a
> > masking bit which is different from the enabled bit to achieve the same
> > purpose. But the GPE hardware doesn't contain such a feature, this brings
> > the trouble.
> > 
> > In this patch, we introduce a software mechanism to implement the GPE
> > masking feature, and acpi_mask_gpe() are provided to the OSPMs to
> > mask/unmask GPEs in the above mentioned situation instead of
> > acpi_enable_gpe()/acpi_disable_gpe(). ACPICA BZ 1102. Lv Zheng.
> > 
> > Link: https://bugs.acpica.org/show_bug.cgi?id=1102
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> 
> I've queued up this one and the [2/3] and please see my comments on the [3/3].

I've decided that it's better if this goes in via upstream ACPICA, so it's not
in the queue any more.

For the time being, I'd like all changes in the ACPICA code to go in via the
upstream.

Thanks,
Rafael

RE: [UPDATE PATCH v2 1/3] ACPICA: Events: Introduce acpi_mask_gpe() to implement GPE masking mechanism

[Zheng, Lv]

Hi, Rafael

> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Subject: Re: [UPDATE PATCH v2 1/3] ACPICA: Events: Introduce
> acpi_mask_gpe() to implement GPE masking mechanism
> 
> On Monday, July 04, 2016 03:59:07 PM Rafael J. Wysocki wrote:
> > On Thursday, June 23, 2016 03:05:47 PM Lv Zheng wrote:
> > > (remove acpi_unmask_gpe() from the patch description)
> > >
> > > There is a facility in Linux, developers can control the
> enabling/disabling
> > > of a GPE via /sys/firmware/acpi/interrupts/gpexx. This is mainly for
> > > debugging purposes.
> > >
> > > But many users expect to use this facility to implement quirks to mask
> a
> > > specific GPE when there is a gap in Linux causing this GPE to flood. This
> > > is not working correctly because currently this facility invokes
> > > enabling/disabling counting based GPE driver APIs:
> > >  acpi_enable_gpe()/acpi_disable_gpe()
> > > and the GPE drivers can still affect the count to mess up the GPE
> > > masking purposes.
> > >
> > > However, most of the IRQ chip designs allow masking/unmasking IRQs
> via a
> > > masking bit which is different from the enabled bit to achieve the same
> > > purpose. But the GPE hardware doesn't contain such a feature, this
> brings
> > > the trouble.
> > >
> > > In this patch, we introduce a software mechanism to implement the
> GPE
> > > masking feature, and acpi_mask_gpe() are provided to the OSPMs to
> > > mask/unmask GPEs in the above mentioned situation instead of
> > > acpi_enable_gpe()/acpi_disable_gpe(). ACPICA BZ 1102. Lv Zheng.
> > >
> > > Link: https://bugs.acpica.org/show_bug.cgi?id=1102
> > > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> >
> > I've queued up this one and the [2/3] and please see my comments on
> the [3/3].
> 
> I've decided that it's better if this goes in via upstream ACPICA, so it's not
> in the queue any more.
> 
> For the time being, I'd like all changes in the ACPICA code to go in via the
> upstream.

[Lv Zheng] 
OK.
Unlike several others, this is not an urgent feature or a feature that ACPICA upstream doesn't have environment to test.
So we needn't to make in the upstream Linux first.

Thanks and best regards
-Lv

[PATCH v2 2/3] ACPI / sys: Update /sys/firmware/acpi/interrupts/gpexx using new GPE masking mechanism

[Lv Zheng]

Now GPE can be masked via new acpi_mask_gpe() API, this patch modifies
/sys/firmware/acpi/interrupts/gpexx to use this new facility.

Writes "mask/unmask" to this file now invoke acpi_mask_gpe().

Reads from this file now returns new "EN/STS" when the corresponding GPE
hardware register's EN/STS bits are flagged, and new "masked/unmasked"
attribute to indicate the status of the masking mechanism.

Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/sleep.c |    2 +-
 drivers/acpi/sysfs.c |   33 +++++++++++++++++++++++++--------
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 7a2e4d4..d00544c 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -555,7 +555,7 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
 
 		acpi_get_event_status(ACPI_EVENT_POWER_BUTTON, &pwr_btn_status);
 
-		if (pwr_btn_status & ACPI_EVENT_FLAG_SET) {
+		if (pwr_btn_status & ACPI_EVENT_FLAG_STATUS_SET) {
 			acpi_clear_event(ACPI_EVENT_POWER_BUTTON);
 			/* Flag for later */
 			pwr_btn_event_pending = true;
diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 4b3a9e2..0303c99 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -599,14 +599,27 @@ static ssize_t counter_show(struct kobject *kobj,
 	if (result)
 		goto end;
 
+	if (status & ACPI_EVENT_FLAG_ENABLE_SET)
+		size += sprintf(buf + size, "  EN");
+	else
+		size += sprintf(buf + size, "    ");
+	if (status & ACPI_EVENT_FLAG_STATUS_SET)
+		size += sprintf(buf + size, " STS");
+	else
+		size += sprintf(buf + size, "    ");
+
 	if (!(status & ACPI_EVENT_FLAG_HAS_HANDLER))
-		size += sprintf(buf + size, "   invalid");
+		size += sprintf(buf + size, " invalid     ");
 	else if (status & ACPI_EVENT_FLAG_ENABLED)
-		size += sprintf(buf + size, "   enabled");
+		size += sprintf(buf + size, " enabled     ");
 	else if (status & ACPI_EVENT_FLAG_WAKE_ENABLED)
-		size += sprintf(buf + size, "   wake_enabled");
+		size += sprintf(buf + size, " wake_enabled");
 	else
-		size += sprintf(buf + size, "   disabled");
+		size += sprintf(buf + size, " disabled    ");
+	if (status & ACPI_EVENT_FLAG_MASKED)
+		size += sprintf(buf + size, " masked  ");
+	else
+		size += sprintf(buf + size, " unmasked");
 
 end:
 	size += sprintf(buf + size, "\n");
@@ -657,8 +670,12 @@ static ssize_t counter_set(struct kobject *kobj,
 			 !(status & ACPI_EVENT_FLAG_ENABLED))
 			result = acpi_enable_gpe(handle, index);
 		else if (!strcmp(buf, "clear\n") &&
-			 (status & ACPI_EVENT_FLAG_SET))
+			 (status & ACPI_EVENT_FLAG_STATUS_SET))
 			result = acpi_clear_gpe(handle, index);
+		else if (!strcmp(buf, "mask\n"))
+			result = acpi_mask_gpe(handle, index, TRUE);
+		else if (!strcmp(buf, "unmask\n"))
+			result = acpi_mask_gpe(handle, index, FALSE);
 		else if (!kstrtoul(buf, 0, &tmp))
 			all_counters[index].count = tmp;
 		else
@@ -666,13 +683,13 @@ static ssize_t counter_set(struct kobject *kobj,
 	} else if (index < num_gpes + ACPI_NUM_FIXED_EVENTS) {
 		int event = index - num_gpes;
 		if (!strcmp(buf, "disable\n") &&
-		    (status & ACPI_EVENT_FLAG_ENABLED))
+		    (status & ACPI_EVENT_FLAG_ENABLE_SET))
 			result = acpi_disable_event(event, ACPI_NOT_ISR);
 		else if (!strcmp(buf, "enable\n") &&
-			 !(status & ACPI_EVENT_FLAG_ENABLED))
+			 !(status & ACPI_EVENT_FLAG_ENABLE_SET))
 			result = acpi_enable_event(event, ACPI_NOT_ISR);
 		else if (!strcmp(buf, "clear\n") &&
-			 (status & ACPI_EVENT_FLAG_SET))
+			 (status & ACPI_EVENT_FLAG_STATUS_SET))
 			result = acpi_clear_event(event);
 		else if (!kstrtoul(buf, 0, &tmp))
 			all_counters[index].count = tmp;
-- 
1.7.10

[PATCH v2 3/3] ACPI / sysfs: Provide quirk mechanism to prevent GPE flooding

[Lv Zheng]

Sometimes, the users may require a quirk to be provided from ACPI subsystem
core to prevent a GPE from flooding. Normally, if a GPE cannot be
dispatched, ACPICA core automatically prevents the GPE from firing. But
there are cases the GPE is dispatched by _Lxx/_Exx provided via AML table,
and OSPM is lacking of the knowledge to get _Lxx/_Exx correctly executed to
handle the GPE, thus the GPE flooding may still occur.

This patch provides a quirk mechanism to stop this kind of GPE flooding.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=53071
Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/887793
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 Documentation/kernel-parameters.txt |   10 +++++++
 drivers/acpi/internal.h             |    1 +
 drivers/acpi/scan.c                 |    1 +
 drivers/acpi/sysfs.c                |   56 +++++++++++++++++++++++++++++++++++
 4 files changed, 68 insertions(+)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 82b42c9..2079b9a 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -274,6 +274,16 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			use by PCI
 			Format: <irq>,<irq>...
 
+	acpi_mask_gpe=  [HW,ACPI]
+			Due to the existence of _Lxx/_Exx, some GPEs triggered
+			by unsupported hardware/firmware features can result in
+                        GPE floodings that cannot be automatically disabled by
+                        the GPE dispatcher.
+			This facility can be used to prevent such uncontrolled
+			GPE floodings.
+			Format: <int>
+			Support masking of GPEs numbered from 0x00 to 0x63.
+
 	acpi_no_auto_serialize	[HW,ACPI]
 			Disable auto-serialization of AML methods
 			AML control methods that contain the opcodes to create
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 27cc7fe..370e184 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -37,6 +37,7 @@ void acpi_amba_init(void);
 static inline void acpi_amba_init(void) {}
 #endif
 int acpi_sysfs_init(void);
+void acpi_gpe_apply_masked_gpes(void);
 void acpi_container_init(void);
 void acpi_memory_hotplug_init(void);
 #ifdef	CONFIG_ACPI_HOTPLUG_IOAPIC
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 5f28cf7..ea70d20 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1958,6 +1958,7 @@ int __init acpi_scan_init(void)
 		}
 	}
 
+	acpi_gpe_apply_masked_gpes();
 	acpi_update_all_gpes();
 
  out:
diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 0303c99..6fabbcf 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -704,6 +704,62 @@ end:
 	return result ? result : size;
 }
 
+/*
+ * A Quirk Mechanism for GPE Flooding Prevention:
+ *
+ * Quirks may be needed to prevent GPE flooding on a specific GPE. The
+ * flooding typically cannot be detected and automatically prevented by
+ * ACPI_GPE_DISPATCH_NONE check because there is a _Lxx/_Exx prepared in
+ * the AML tables. This normally indicates a feature gap in Linux, thus
+ * instead of providing endless quirk tables, we provide a boot parameter
+ * for those who want this quirk. For example, if the users want to prevent
+ * the GPE flooding for GPE 00, they need to specify the following boot
+ * parameter:
+ *   acpi_mask_gpe=0x00
+ * The masking status can be modified by the following runtime controlling
+ * interface:
+ *   echo unmask > /sys/firmware/acpi/interrupts/gpe00
+ */
+
+/*
+ * Currently, the GPE flooding prevention only supports to mask the GPEs
+ * numbered from 00 to 63.
+ */
+#define ACPI_MASKABLE_GPE_MAX	64
+
+static u64 acpi_masked_gpes;
+
+static int __init acpi_gpe_set_masked_gpes(char *val)
+{
+	u8 gpe;
+
+	if (kstrtou8(val, 0, &gpe) || gpe > ACPI_MASKABLE_GPE_MAX)
+		return -EINVAL;
+	acpi_masked_gpes |= ((u64)1<<gpe);
+
+	return 1;
+}
+__setup("acpi_mask_gpe=", acpi_gpe_set_masked_gpes);
+
+void acpi_gpe_apply_masked_gpes(void)
+{
+	acpi_handle handle;
+	acpi_status status;
+	u8 gpe;
+
+	for (gpe = 0;
+	     gpe < min_t(u8, ACPI_MASKABLE_GPE_MAX, acpi_current_gpe_count);
+	     gpe++) {
+		if (acpi_masked_gpes & ((u64)1<<gpe)) {
+			status = acpi_get_gpe_device(gpe, &handle);
+			if (ACPI_SUCCESS(status)) {
+				pr_info("Masking GPE 0x%x.\n", gpe);
+				(void)acpi_mask_gpe(handle, gpe, TRUE);
+			}
+		}
+	}
+}
+
 void acpi_irq_stats_init(void)
 {
 	acpi_status status;
-- 
1.7.10

Re: [PATCH v2 3/3] ACPI / sysfs: Provide quirk mechanism to prevent GPE flooding

[Rafael J. Wysocki]

On Thursday, June 23, 2016 02:20:30 PM Lv Zheng wrote:
> Sometimes, the users may require a quirk to be provided from ACPI subsystem
> core to prevent a GPE from flooding. Normally, if a GPE cannot be
> dispatched, ACPICA core automatically prevents the GPE from firing. But
> there are cases the GPE is dispatched by _Lxx/_Exx provided via AML table,
> and OSPM is lacking of the knowledge to get _Lxx/_Exx correctly executed to
> handle the GPE, thus the GPE flooding may still occur.
> 
> This patch provides a quirk mechanism to stop this kind of GPE flooding.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=53071
> Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/887793
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> ---
>  Documentation/kernel-parameters.txt |   10 +++++++
>  drivers/acpi/internal.h             |    1 +
>  drivers/acpi/scan.c                 |    1 +
>  drivers/acpi/sysfs.c                |   56 +++++++++++++++++++++++++++++++++++
>  4 files changed, 68 insertions(+)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 82b42c9..2079b9a 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -274,6 +274,16 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			use by PCI
>  			Format: <irq>,<irq>...
>  
> +	acpi_mask_gpe=  [HW,ACPI]
> +			Due to the existence of _Lxx/_Exx, some GPEs triggered
> +			by unsupported hardware/firmware features can result in
> +                        GPE floodings that cannot be automatically disabled by
> +                        the GPE dispatcher.
> +			This facility can be used to prevent such uncontrolled
> +			GPE floodings.
> +			Format: <int>
> +			Support masking of GPEs numbered from 0x00 to 0x63.

So what about if someone needs to mask more than one GPE?

The code indicates that acpi_mask_gpe= needs to be used multiple times then,
but it would be good to mention that here too.

> +
>  	acpi_no_auto_serialize	[HW,ACPI]
>  			Disable auto-serialization of AML methods
>  			AML control methods that contain the opcodes to create
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 27cc7fe..370e184 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -37,6 +37,7 @@ void acpi_amba_init(void);
>  static inline void acpi_amba_init(void) {}
>  #endif
>  int acpi_sysfs_init(void);
> +void acpi_gpe_apply_masked_gpes(void);
>  void acpi_container_init(void);
>  void acpi_memory_hotplug_init(void);
>  #ifdef	CONFIG_ACPI_HOTPLUG_IOAPIC
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 5f28cf7..ea70d20 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1958,6 +1958,7 @@ int __init acpi_scan_init(void)
>  		}
>  	}
>  
> +	acpi_gpe_apply_masked_gpes();
>  	acpi_update_all_gpes();
>  
>   out:
> diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
> index 0303c99..6fabbcf 100644
> --- a/drivers/acpi/sysfs.c
> +++ b/drivers/acpi/sysfs.c
> @@ -704,6 +704,62 @@ end:
>  	return result ? result : size;
>  }
>  
> +/*
> + * A Quirk Mechanism for GPE Flooding Prevention:
> + *
> + * Quirks may be needed to prevent GPE flooding on a specific GPE. The
> + * flooding typically cannot be detected and automatically prevented by
> + * ACPI_GPE_DISPATCH_NONE check because there is a _Lxx/_Exx prepared in
> + * the AML tables. This normally indicates a feature gap in Linux, thus
> + * instead of providing endless quirk tables, we provide a boot parameter
> + * for those who want this quirk. For example, if the users want to prevent
> + * the GPE flooding for GPE 00, they need to specify the following boot
> + * parameter:
> + *   acpi_mask_gpe=0x00
> + * The masking status can be modified by the following runtime controlling
> + * interface:
> + *   echo unmask > /sys/firmware/acpi/interrupts/gpe00
> + */
> +
> +/*
> + * Currently, the GPE flooding prevention only supports to mask the GPEs
> + * numbered from 00 to 63.
> + */
> +#define ACPI_MASKABLE_GPE_MAX	64
> +
> +static u64 acpi_masked_gpes;
> +
> +static int __init acpi_gpe_set_masked_gpes(char *val)
> +{
> +	u8 gpe;
> +
> +	if (kstrtou8(val, 0, &gpe) || gpe > ACPI_MASKABLE_GPE_MAX)
> +		return -EINVAL;
> +	acpi_masked_gpes |= ((u64)1<<gpe);

This looks sort of hackish.

In principle you can create a list here and then free it when you do the
actual masking.

Also, why can't the masking be applied right here?

> +
> +	return 1;
> +}
> +__setup("acpi_mask_gpe=", acpi_gpe_set_masked_gpes);
> +
> +void acpi_gpe_apply_masked_gpes(void)
> +{
> +	acpi_handle handle;
> +	acpi_status status;
> +	u8 gpe;
> +
> +	for (gpe = 0;
> +	     gpe < min_t(u8, ACPI_MASKABLE_GPE_MAX, acpi_current_gpe_count);
> +	     gpe++) {
> +		if (acpi_masked_gpes & ((u64)1<<gpe)) {
> +			status = acpi_get_gpe_device(gpe, &handle);
> +			if (ACPI_SUCCESS(status)) {
> +				pr_info("Masking GPE 0x%x.\n", gpe);
> +				(void)acpi_mask_gpe(handle, gpe, TRUE);
> +			}
> +		}
> +	}
> +}
> +
>  void acpi_irq_stats_init(void)
>  {
>  	acpi_status status;
> 

Thanks,
Rafael

[PATCH v3] ACPI / sysfs: Provide quirk mechanism to prevent GPE flooding

[Lv Zheng]

Sometimes, the users may require a quirk to be provided from ACPI subsystem
core to prevent a GPE from flooding. Normally, if a GPE cannot be
dispatched, ACPICA core automatically prevents the GPE from firing. But
there are cases the GPE is dispatched by _Lxx/_Exx provided via AML table,
and OSPM is lacking of the knowledge to get _Lxx/_Exx correctly executed to
handle the GPE, thus the GPE flooding may still occur.

This patch provides a quirk mechanism to stop this kind of GPE flooding.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=53071
Link: https://bugzilla.kernel.org/show_bug.cgi?id=117481
Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/887793
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 Documentation/kernel-parameters.txt |   10 +++++++
 drivers/acpi/internal.h             |    1 +
 drivers/acpi/scan.c                 |    1 +
 drivers/acpi/sysfs.c                |   56 +++++++++++++++++++++++++++++++++++
 4 files changed, 68 insertions(+)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 1f6cecc..12cab01 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -305,6 +305,16 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			use by PCI
 			Format: <irq>,<irq>...
 
+	acpi_mask_gpe=  [HW,ACPI]
+			Due to the existence of _Lxx/_Exx, some GPEs triggered
+			by unsupported hardware/firmware features can result in
+                        GPE floodings that cannot be automatically disabled by
+                        the GPE dispatcher.
+			This facility can be used to prevent such uncontrolled
+			GPE floodings.
+			Format: <int>
+			Support masking of GPEs numbered from 0x00 to 0x7f.
+
 	acpi_no_auto_serialize	[HW,ACPI]
 			Disable auto-serialization of AML methods
 			AML control methods that contain the opcodes to create
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 1b41a27..0c45226 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -37,6 +37,7 @@
 static inline void acpi_amba_init(void) {}
 #endif
 int acpi_sysfs_init(void);
+void acpi_gpe_apply_masked_gpes(void);
 void acpi_container_init(void);
 void acpi_memory_hotplug_init(void);
 #ifdef	CONFIG_ACPI_HOTPLUG_IOAPIC
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 3d1856f..5a2fdf1 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2044,6 +2044,7 @@ int __init acpi_scan_init(void)
 		}
 	}
 
+	acpi_gpe_apply_masked_gpes();
 	acpi_update_all_gpes();
 	acpi_ec_ecdt_start();
 
diff --git a/drivers/acpi/sysfs.c b/drivers/acpi/sysfs.c
index 703c26e..47d7182 100644
--- a/drivers/acpi/sysfs.c
+++ b/drivers/acpi/sysfs.c
@@ -708,6 +708,62 @@ static ssize_t counter_set(struct kobject *kobj,
 	return result ? result : size;
 }
 
+/*
+ * A Quirk Mechanism for GPE Flooding Prevention:
+ *
+ * Quirks may be needed to prevent GPE flooding on a specific GPE. The
+ * flooding typically cannot be detected and automatically prevented by
+ * ACPI_GPE_DISPATCH_NONE check because there is a _Lxx/_Exx prepared in
+ * the AML tables. This normally indicates a feature gap in Linux, thus
+ * instead of providing endless quirk tables, we provide a boot parameter
+ * for those who want this quirk. For example, if the users want to prevent
+ * the GPE flooding for GPE 00, they need to specify the following boot
+ * parameter:
+ *   acpi_mask_gpe=0x00
+ * The masking status can be modified by the following runtime controlling
+ * interface:
+ *   echo unmask > /sys/firmware/acpi/interrupts/gpe00
+ */
+
+/*
+ * Currently, the GPE flooding prevention only supports to mask the GPEs
+ * numbered from 00 to 7f.
+ */
+#define ACPI_MASKABLE_GPE_MAX	0x80
+
+static u64 acpi_masked_gpes;
+
+static int __init acpi_gpe_set_masked_gpes(char *val)
+{
+	u8 gpe;
+
+	if (kstrtou8(val, 0, &gpe) || gpe > ACPI_MASKABLE_GPE_MAX)
+		return -EINVAL;
+	acpi_masked_gpes |= ((u64)1<<gpe);
+
+	return 1;
+}
+__setup("acpi_mask_gpe=", acpi_gpe_set_masked_gpes);
+
+void acpi_gpe_apply_masked_gpes(void)
+{
+	acpi_handle handle;
+	acpi_status status;
+	u8 gpe;
+
+	for (gpe = 0;
+	     gpe < min_t(u8, ACPI_MASKABLE_GPE_MAX, acpi_current_gpe_count);
+	     gpe++) {
+		if (acpi_masked_gpes & ((u64)1<<gpe)) {
+			status = acpi_get_gpe_device(gpe, &handle);
+			if (ACPI_SUCCESS(status)) {
+				pr_info("Masking GPE 0x%x.\n", gpe);
+				(void)acpi_mask_gpe(handle, gpe, TRUE);
+			}
+		}
+	}
+}
+
 void acpi_irq_stats_init(void)
 {
 	acpi_status status;
-- 
1.7.10