lkml.org 
[lkml]   [2016]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [PATCH 1/3] ACPICA: Events: Introduce acpi_block_gpe()/acpi_unblock_gpe()/acpi_control_gpe_handling() to allow administrative GPE enabling/disabling
Date
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

\
 
 \ /
  Last update: 2016-06-08 00:01    [W:1.690 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site