Skip to content

Commit 8d59349

Browse files
Erik Schmaussrafaeljw
Erik Schmauss
authored and
rafaeljw
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. =======================================+============================= IRQ handler (top-half) |IRQ polling =======================================+============================= acpi_ev_detect_gpe() | LOCK() | READ (GPE0-7 enable/status registers)| ^^^^^^^^^^^^ROOT CAUSE^^^^^^^^^^^^^^^| Walk GPE0 | UNLOCK() |LOCK() Invoke GPE0 RAW_HANDLER |READ (GPE1 enable/status bit) |acpi_ev_gpe_dispatch(irq=false) | CLEAR (GPE1 enable bit) | CLEAR (GPE1 status bit) LOCK() |UNLOCK() Walk GPE1 +============================= acpi_ev_gpe_dispatch(irq=true) |IRQ polling (defer) CLEAR (GPE1 enable bit) +============================= CLEAR (GPE1 status bit) |acpi_ev_async_execute_gpe_method() Walk others | Evaluate GPE1 _Exx fi | acpi_ev_async_enable_gpe() UNLOCK() | LOCK() =======================================+ SET (GPE enable bit) IRQ handler (bottom-half) | UNLOCK() =======================================+ acpi_ev_async_execute_gpe_method() | Evaluate GPE1 _Exx | acpi_ev_async_enable_gpe() | LOCK() | SET (GPE1 enable bit) | UNLOCK() | =======================================+============================= If acpi_ev_detect_gpe() is only invoked from the IRQ context, there won't be more than one _Lxx/_Exx evaluations for one status bit flagging if the IRQ handlers controlled by the underlying IRQ chip/driver (ex. APIC) are run in serial. Note that, this is a known potential gap and we had an approach, locking entire non-raw-handler processes in the top-half IRQ handler and handling all raw-handlers out of the locked loop to be friendly to those IRQ chip/driver. But the approach is too complicated while the issue is not so real, thus ACPICA treated such issue (if any) as a parallelism/quality issue of the underlying IRQ chip/driver to stop putting it on the radar. Bug in link #1 is suspiciously reflecting the same cause, and if so, it can also be fixed by this simpler approach. But it will be no excuse an ACPICA problem now if ACPICA starts to poll IRQs itself. In the changed scenario, _Exx will be evaluated from the task context due to new ACPICA provided "polling after enabling GPEs" mechanism. And the above figure uses edge-triggered GPEs demonstrating the possibility of evaluating _Exx twice for one status bit flagging. As a conclusion, there is now an increased chance of evaluating _Lxx/_Exx more than once for one status bit flagging. However this is still not a real problem if the _Lxx/_Exx checks the 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 wrongly duplicated by the underlying IRQ delivery mechanisms. But _Exx may have very low quality BIOS by BIOS to trigger real 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 GPEs' status bits. Thus the hardware GPE architecture has already provided us with the mechanism of implementing such parallelism. So we can lock around one GPE handling process to achieve the parallelism: 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 rather than raw handler only during the top-half IRQ handler. Lv Zheng. Link: https://bugzilla.kernel.org/show_bug.cgi?id=196703 [#1] Signed-off-by: Lv Zheng <[email protected]> Signed-off-by: Erik Schmauss <[email protected]> Signed-off-by: Rafael J. Wysocki <[email protected]>
1 parent 18996f2 commit 8d59349

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)