Skip to content

Commit 9885704

Browse files
author
Lv Zheng
committed
ACPICA: Events: Add parallel GPE handling support to fix potential redundant _Exx evaluations
There is a risk that a GPE method/handler may be invoked twice. Let's consider a case, both GPE0(RAW_HANDLER) and GPE1(_Exx) is triggered. acpi_ev_detect_gpe() LOCK() READ (GPE0-7 enable/status registers) ^^^^^^^^^^^^ROOT CAUSE^^^^^^^^^^^^^^^ Walk GPE0 UNLOCK() Invoke GPE0 RAW_HANDLER READ (GPE1 enable/status bit) LOCK() acpi_ev_gpe_dispatch() Walk GPE1 CLEAR (GPE1 enable bit) acpi_ev_gpe_dispatch() CLEAR (GPE1 status bit) CLEAR (GPE1 enable bit) Evaluate GPE1 _Exx CLEAR (GPE1 status bit) Evaluate GPE1 _Exx fi UNLOCK() If acpi_ev_detect_gpe() is only invoked from the IRQ context, this situation may not be triggered if the IRQ handlers are controlled by the IRQ chip/driver to be run in serial. But it will be a real problem if _Exx will be evaluated from the task context due to "polling after enabling GPEs". And the above figure just uses edge-triggered GPEs demonstrated such an issue. As a conclusion, there is now an increased chance of evaluating _Lxx/_Exx more than once for one status bit flagging. This is not a problem if the _Lxx/_Exx checks underlying hardware IRQ reasoning and finally just changes the 2nd and the follow-up evaluations into no-ops. Note that _Lxx should always be written in this way as a level-trigger GPE could have it's status be wrongly duplicated by the underlying IRQ delivery mechanisms. But _Exx may have very low quality BIOS by BIOS to trigger issues. For example, trigger duplicated button notifications. To solve this issue, we need to stop reading a bunch of enable/status register bits, but read only one GPE's enable/status bit. And GPE status register's W1C nature ensures that acknowledging one GPE won't affect another GPE's handling process. Thus the hardware GPE architecture has already provided us with the mechanism of implementing such parallelism. So we can actually only lock around one GPE handling to parallelize GPE handling processes: 1. If we can incorporate GPE enable bit check in detection and ensure the atomicity of the following process (top-half IRQ handler): READ (enable/status bit) if (enabled && raised) CLEAR (enable bit) and handle the GPE after this process, we can ensure that we will only invoke GPE handler once for one status bit flagging. 2. In addtion for edge-triggered GPEs, if we can ensure the atomicity of the following process (top-half IRQ handler): READ (enable/status bit) if (enabled && raised) CLEAR (enable bit) CLEAR (status bit) and handle the GPE after this process, we can ensure that we will only invoke GPE handler once for one status bit flagging. By doing a cleanup in this way, we can remove duplicate GPE handling code and ensure that all logics are collected in 1 function. And the function will be safe for both IRQ interrupt and IRQ polling, and will be safe for us to release and re-acquire acpi_gbl_gpe_lock at any time other than the above "top-half IRQ handler" process. Lv Zheng. Signed-off-by: Lv Zheng <lv.zheng@intel.com>
1 parent 8bbf5e8 commit 9885704

File tree

2 files changed

+132
-105
lines changed

2 files changed

+132
-105
lines changed

‎drivers/acpi/acpica/acevents.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,10 @@ struct acpi_gpe_event_info *acpi_ev_low_get_gpe_info(u32 gpe_number,
103103

104104
acpi_status acpi_ev_finish_gpe(struct acpi_gpe_event_info *gpe_event_info);
105105

106+
u32
107+
acpi_ev_detect_gpe(struct acpi_namespace_node *gpe_device,
108+
struct acpi_gpe_event_info *gpe_event_info, u32 gpe_number);
109+
106110
/*
107111
* evgpeblk - Upper-level GPE block support
108112
*/

‎drivers/acpi/acpica/evgpe.c

Lines changed: 128 additions & 105 deletions
Original file line numberDiff line numberDiff line change
@@ -374,17 +374,12 @@ struct acpi_gpe_event_info *acpi_ev_get_gpe_event_info(acpi_handle gpe_device,
374374

375375
u32 acpi_ev_gpe_detect(struct acpi_gpe_xrupt_info *gpe_xrupt_list)
376376
{
377-
acpi_status status;
378377
struct acpi_gpe_block_info *gpe_block;
379378
struct acpi_namespace_node *gpe_device;
380379
struct acpi_gpe_register_info *gpe_register_info;
381380
struct acpi_gpe_event_info *gpe_event_info;
382381
u32 gpe_number;
383-
struct acpi_gpe_handler_info *gpe_handler_info;
384382
u32 int_status = ACPI_INTERRUPT_NOT_HANDLED;
385-
u8 enabled_status_byte;
386-
u64 status_reg;
387-
u64 enable_reg;
388383
acpi_cpu_flags flags;
389384
u32 i;
390385
u32 j;
@@ -441,121 +436,30 @@ u32 acpi_ev_gpe_detect(struct acpi_gpe_xrupt_info *gpe_xrupt_list)
441436
continue;
442437
}
443438

444-
/* Read the Status Register */
445-
446-
status =
447-
acpi_hw_read(&status_reg,
448-
&gpe_register_info->status_address);
449-
if (ACPI_FAILURE(status)) {
450-
goto unlock_and_exit;
451-
}
452-
453-
/* Read the Enable Register */
454-
455-
status =
456-
acpi_hw_read(&enable_reg,
457-
&gpe_register_info->enable_address);
458-
if (ACPI_FAILURE(status)) {
459-
goto unlock_and_exit;
460-
}
461-
462-
ACPI_DEBUG_PRINT((ACPI_DB_INTERRUPTS,
463-
"Read registers for GPE %02X-%02X: Status=%02X, Enable=%02X, "
464-
"RunEnable=%02X, WakeEnable=%02X\n",
465-
gpe_register_info->base_gpe_number,
466-
gpe_register_info->base_gpe_number +
467-
(ACPI_GPE_REGISTER_WIDTH - 1),
468-
(u32)status_reg, (u32)enable_reg,
469-
gpe_register_info->enable_for_run,
470-
gpe_register_info->enable_for_wake));
471-
472-
/* Check if there is anything active at all in this register */
473-
474-
enabled_status_byte = (u8)(status_reg & enable_reg);
475-
if (!enabled_status_byte) {
476-
477-
/* No active GPEs in this register, move on */
478-
479-
continue;
480-
}
481-
482439
/* Now look at the individual GPEs in this byte register */
483440

484441
for (j = 0; j < ACPI_GPE_REGISTER_WIDTH; j++) {
485442

486-
/* Examine one GPE bit */
443+
/* Detect and dispatch one GPE bit */
487444

488445
gpe_event_info =
489446
&gpe_block->
490447
event_info[((acpi_size)i *
491448
ACPI_GPE_REGISTER_WIDTH) + j];
492449
gpe_number =
493450
j + gpe_register_info->base_gpe_number;
494-
495-
if (enabled_status_byte & (1 << j)) {
496-
497-
/* Invoke global event handler if present */
498-
499-
acpi_gpe_count++;
500-
if (acpi_gbl_global_event_handler) {
501-
acpi_gbl_global_event_handler
502-
(ACPI_EVENT_TYPE_GPE,
503-
gpe_device, gpe_number,
504-
acpi_gbl_global_event_handler_context);
505-
}
506-
507-
/* Found an active GPE */
508-
509-
if (ACPI_GPE_DISPATCH_TYPE
510-
(gpe_event_info->flags) ==
511-
ACPI_GPE_DISPATCH_RAW_HANDLER) {
512-
513-
/* Dispatch the event to a raw handler */
514-
515-
gpe_handler_info =
516-
gpe_event_info->dispatch.
517-
handler;
518-
519-
/*
520-
* There is no protection around the namespace node
521-
* and the GPE handler to ensure a safe destruction
522-
* because:
523-
* 1. The namespace node is expected to always
524-
* exist after loading a table.
525-
* 2. The GPE handler is expected to be flushed by
526-
* acpi_os_wait_events_complete() before the
527-
* destruction.
528-
*/
529-
acpi_os_release_lock
530-
(acpi_gbl_gpe_lock, flags);
531-
int_status |=
532-
gpe_handler_info->
533-
address(gpe_device,
534-
gpe_number,
535-
gpe_handler_info->
536-
context);
537-
flags =
538-
acpi_os_acquire_lock
539-
(acpi_gbl_gpe_lock);
540-
} else {
541-
/*
542-
* Dispatch the event to a standard handler or
543-
* method.
544-
*/
545-
int_status |=
546-
acpi_ev_gpe_dispatch
547-
(gpe_device, gpe_event_info,
548-
gpe_number);
549-
}
550-
}
451+
acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
452+
int_status |=
453+
acpi_ev_detect_gpe(gpe_device,
454+
gpe_event_info,
455+
gpe_number);
456+
flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
551457
}
552458
}
553459

554460
gpe_block = gpe_block->next;
555461
}
556462

557-
unlock_and_exit:
558-
559463
acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
560464
return (int_status);
561465
}
@@ -726,6 +630,127 @@ acpi_status acpi_ev_finish_gpe(struct acpi_gpe_event_info *gpe_event_info)
726630
}
727631

728632

633+
/*******************************************************************************
634+
*
635+
* FUNCTION: acpi_ev_detect_gpe
636+
*
637+
* PARAMETERS: gpe_device - Device node. NULL for GPE0/GPE1
638+
* gpe_event_info - Info for this GPE
639+
* gpe_number - Number relative to the parent GPE block
640+
*
641+
* RETURN: INTERRUPT_HANDLED or INTERRUPT_NOT_HANDLED
642+
*
643+
* DESCRIPTION: Detect and dispatch a General Purpose Event to either a function
644+
* (e.g. EC) or method (e.g. _Lxx/_Exx) handler.
645+
* NOTE: GPE is W1C, so it is possible to handle a single GPE from both
646+
* task and irq context in parallel as long as the process to
647+
* detect and mask the GPE is atomic.
648+
* However the atomicity of ACPI_GPE_DISPATCH_RAW_HANDLER is
649+
* dependent on the raw handler itself.
650+
*
651+
******************************************************************************/
652+
653+
u32
654+
acpi_ev_detect_gpe(struct acpi_namespace_node *gpe_device,
655+
struct acpi_gpe_event_info *gpe_event_info, u32 gpe_number)
656+
{
657+
u32 int_status = ACPI_INTERRUPT_NOT_HANDLED;
658+
u8 enabled_status_byte;
659+
u64 status_reg;
660+
u64 enable_reg;
661+
u32 register_bit;
662+
struct acpi_gpe_register_info *gpe_register_info;
663+
struct acpi_gpe_handler_info *gpe_handler_info;
664+
acpi_cpu_flags flags;
665+
acpi_status status;
666+
667+
ACPI_FUNCTION_TRACE(ev_gpe_detect);
668+
669+
flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
670+
671+
/* Get the info block for the entire GPE register */
672+
673+
gpe_register_info = gpe_event_info->register_info;
674+
675+
/* Get the register bitmask for this GPE */
676+
677+
register_bit = acpi_hw_get_gpe_register_bit(gpe_event_info);
678+
679+
/* GPE currently enabled (enable bit == 1)? */
680+
681+
status = acpi_hw_read(&enable_reg, &gpe_register_info->enable_address);
682+
if (ACPI_FAILURE(status)) {
683+
goto error_exit;
684+
}
685+
686+
/* GPE currently active (status bit == 1)? */
687+
688+
status = acpi_hw_read(&status_reg, &gpe_register_info->status_address);
689+
if (ACPI_FAILURE(status)) {
690+
goto error_exit;
691+
}
692+
693+
/* Check if there is anything active at all in this GPE */
694+
695+
ACPI_DEBUG_PRINT((ACPI_DB_INTERRUPTS,
696+
"Read registers for GPE %02X: Status=%02X, Enable=%02X, "
697+
"RunEnable=%02X, WakeEnable=%02X\n",
698+
gpe_number,
699+
(u32)(status_reg & register_bit),
700+
(u32)(enable_reg & register_bit),
701+
gpe_register_info->enable_for_run,
702+
gpe_register_info->enable_for_wake));
703+
704+
enabled_status_byte = (u8)(status_reg & enable_reg);
705+
if (!(enabled_status_byte & register_bit)) {
706+
goto error_exit;
707+
}
708+
709+
/* Invoke global event handler if present */
710+
711+
acpi_gpe_count++;
712+
if (acpi_gbl_global_event_handler) {
713+
acpi_gbl_global_event_handler(ACPI_EVENT_TYPE_GPE,
714+
gpe_device, gpe_number,
715+
acpi_gbl_global_event_handler_context);
716+
}
717+
718+
/* Found an active GPE */
719+
720+
if (ACPI_GPE_DISPATCH_TYPE(gpe_event_info->flags) ==
721+
ACPI_GPE_DISPATCH_RAW_HANDLER) {
722+
723+
/* Dispatch the event to a raw handler */
724+
725+
gpe_handler_info = gpe_event_info->dispatch.handler;
726+
727+
/*
728+
* There is no protection around the namespace node
729+
* and the GPE handler to ensure a safe destruction
730+
* because:
731+
* 1. The namespace node is expected to always
732+
* exist after loading a table.
733+
* 2. The GPE handler is expected to be flushed by
734+
* acpi_os_wait_events_complete() before the
735+
* destruction.
736+
*/
737+
acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
738+
int_status |=
739+
gpe_handler_info->address(gpe_device, gpe_number,
740+
gpe_handler_info->context);
741+
flags = acpi_os_acquire_lock(acpi_gbl_gpe_lock);
742+
} else {
743+
/* Dispatch the event to a standard handler or method. */
744+
745+
int_status |= acpi_ev_gpe_dispatch(gpe_device,
746+
gpe_event_info, gpe_number);
747+
}
748+
749+
error_exit:
750+
acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
751+
return (int_status);
752+
}
753+
729754
/*******************************************************************************
730755
*
731756
* FUNCTION: acpi_ev_gpe_dispatch
@@ -739,8 +764,6 @@ acpi_status acpi_ev_finish_gpe(struct acpi_gpe_event_info *gpe_event_info)
739764
* DESCRIPTION: Dispatch a General Purpose Event to either a function (e.g. EC)
740765
* or method (e.g. _Lxx/_Exx) handler.
741766
*
742-
* This function executes at interrupt level.
743-
*
744767
******************************************************************************/
745768

746769
u32

0 commit comments

Comments
 (0)