Hi,
This is exceptional for at least two reasons.
First, we need it now to work around boot problems on multiple platforms
already seen in the field (which are shipping products).
Second, the ACPICA changes in this series are kind of Linux-specific, because
they are related to how Linux carries out the initialization of devices, which
very well may be different from what the other OSes using ACPICA do.
For these reasons, I'd like to make the following ACPICA changes in Linux
only for the time being and then decide whether or not to take them into
the upstream. If there are major concerns about that, please let me know.
The issue at hand is that some platforms with Thunderbolt controllers won't
boot if there are any Thunderbolt devices connected to them at boot time (if
the devices are connected later, everything works as expected). That turns
out to be related to a complicated sequence of events involving the platform
firmware which needs to happen in exactly the right order at the right time
for things to work and that requires GPEs to be enabled before enumerating
the PCI bus.
The first patch changes ACPICA to check the status of runtime GPEs before
enabling them for the first time in order to avoid missing events (that
is key for edge-triggered GPEs) and to process them early enough.
The second one makes it possible to change the ordering of initialization
between GPEs and devices, and the third one actually changes that ordering.
The patches are based on Mika's work, so kudos to him.
Thanks,
Rafael
From: Rafael J. Wysocki <[email protected]>
On some systems the platform firmware expects GPEs to be enabled
before the enumeration of devices and if that expectation is not
met, the systems in question may not boot in some situations.
For this reason, change the initialization ordering of the ACPI
subsystem to make it enable GPEs before scanning the namespace
for the first time in order to enumerate devices.
Reported-by: Mika Westerberg <[email protected]>
Suggested-by: Mika Westerberg <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/scan.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -2139,6 +2139,10 @@ int __init acpi_scan_init(void)
acpi_get_spcr_uart_addr();
}
+ acpi_gpe_apply_masked_gpes();
+ acpi_update_all_gpes();
+ acpi_ec_ecdt_start();
+
mutex_lock(&acpi_scan_lock);
/*
* Enumerate devices in the ACPI namespace.
@@ -2163,10 +2167,6 @@ int __init acpi_scan_init(void)
}
}
- acpi_gpe_apply_masked_gpes();
- acpi_update_all_gpes();
- acpi_ec_ecdt_start();
-
acpi_scan_initialized = true;
out:
From: Rafael J. Wysocki <[email protected]>
In some cases GPEs are already active when they are enabled by
acpi_ev_initialize_gpe_block() and whatever happens next may depend
on the result of handling the events signaled by them, so the
events should not be discarded (which is what happens currently) and
they should be handled as soon as reasonably possible.
For this reason, modify acpi_ev_initialize_gpe_block() to
dispatch GPEs with the status flag set in-band right after
enabling them.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/acpica/evgpeblk.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
Index: linux-pm/drivers/acpi/acpica/evgpeblk.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/evgpeblk.c
+++ linux-pm/drivers/acpi/acpica/evgpeblk.c
@@ -440,9 +440,11 @@ acpi_ev_initialize_gpe_block(struct acpi
void *ignored)
{
acpi_status status;
+ acpi_event_status event_status;
struct acpi_gpe_event_info *gpe_event_info;
u32 gpe_enabled_count;
u32 gpe_index;
+ u32 gpe_number;
u32 i;
u32 j;
@@ -470,30 +472,38 @@ acpi_ev_initialize_gpe_block(struct acpi
gpe_index = (i * ACPI_GPE_REGISTER_WIDTH) + j;
gpe_event_info = &gpe_block->event_info[gpe_index];
+ gpe_number = gpe_block->block_base_number + gpe_index;
/*
* Ignore GPEs that have no corresponding _Lxx/_Exx method
- * and GPEs that are used to wake the system
+ * and GPEs that are used for wakeup
*/
- if ((ACPI_GPE_DISPATCH_TYPE(gpe_event_info->flags) ==
- ACPI_GPE_DISPATCH_NONE)
- || (ACPI_GPE_DISPATCH_TYPE(gpe_event_info->flags) ==
- ACPI_GPE_DISPATCH_HANDLER)
- || (ACPI_GPE_DISPATCH_TYPE(gpe_event_info->flags) ==
- ACPI_GPE_DISPATCH_RAW_HANDLER)
+ if ((ACPI_GPE_DISPATCH_TYPE(gpe_event_info->flags) !=
+ ACPI_GPE_DISPATCH_METHOD)
|| (gpe_event_info->flags & ACPI_GPE_CAN_WAKE)) {
continue;
}
+ event_status = 0;
+ (void)acpi_hw_get_gpe_status(gpe_event_info,
+ &event_status);
+
status = acpi_ev_add_gpe_reference(gpe_event_info);
if (ACPI_FAILURE(status)) {
ACPI_EXCEPTION((AE_INFO, status,
"Could not enable GPE 0x%02X",
- gpe_index +
- gpe_block->block_base_number));
+ gpe_number));
continue;
}
+ if (event_status & ACPI_EVENT_FLAG_STATUS_SET) {
+ ACPI_INFO(("GPE 0x%02X active on init",
+ gpe_number));
+ (void)acpi_ev_gpe_dispatch(gpe_block->node,
+ gpe_event_info,
+ gpe_number);
+ }
+
gpe_enabled_count++;
}
}
From: Rafael J. Wysocki <[email protected]>
Runtime GPEs have corresponding _Lxx/_Exx methods and are enabled
automatically during the initialization of the ACPI subsystem through
acpi_update_all_gpes() with the assumption that acpi_setup_gpe_for_wake()
will be called in advance for all of the GPEs pointed to by _PRW
objects in the namespace that may be affected by acpi_update_all_gpes().
That is, acpi_ev_initialize_gpe_block() can only be called for a GPE
block after acpi_setup_gpe_for_wake() has been called for all of the
_PRW (wakeup) GPEs in it.
The platform firmware on some systems, however, expects GPEs to be
enabled before the enumeration of devices which is when
acpi_setup_gpe_for_wake() is called and that goes against the above
assumption.
For this reason, introduce a new flag to be set by
acpi_ev_initialize_gpe_block() when automatically enabling a GPE
to indicate to acpi_setup_gpe_for_wake() that it needs to drop the
reference to the GPE coming from acpi_ev_initialize_gpe_block()
and modify acpi_setup_gpe_for_wake() accordingly. These changes
allow acpi_setup_gpe_for_wake() and acpi_ev_initialize_gpe_block()
to be invoked in any order.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/acpica/evgpeblk.c | 2 ++
drivers/acpi/acpica/evxfgpe.c | 8 ++++++++
include/acpi/actypes.h | 3 ++-
3 files changed, 12 insertions(+), 1 deletion(-)
Index: linux-pm/drivers/acpi/acpica/evgpeblk.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/evgpeblk.c
+++ linux-pm/drivers/acpi/acpica/evgpeblk.c
@@ -496,6 +496,8 @@ acpi_ev_initialize_gpe_block(struct acpi
continue;
}
+ gpe_event_info->flags |= ACPI_GPE_AUTO_ENABLED;
+
if (event_status & ACPI_EVENT_FLAG_STATUS_SET) {
ACPI_INFO(("GPE 0x%02X active on init",
gpe_number));
Index: linux-pm/include/acpi/actypes.h
===================================================================
--- linux-pm.orig/include/acpi/actypes.h
+++ linux-pm/include/acpi/actypes.h
@@ -783,7 +783,7 @@ typedef u32 acpi_event_status;
* | | | | +-- 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 mechanism
+ * | +--------- Has been enabled automatically at init time
* +------------ <Reserved>
*/
#define ACPI_GPE_DISPATCH_NONE (u8) 0x00
@@ -799,6 +799,7 @@ typedef u32 acpi_event_status;
#define ACPI_GPE_XRUPT_TYPE_MASK (u8) 0x08
#define ACPI_GPE_CAN_WAKE (u8) 0x10
+#define ACPI_GPE_AUTO_ENABLED (u8) 0x20
/*
* Flags for GPE and Lock interfaces
Index: linux-pm/drivers/acpi/acpica/evxfgpe.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpica/evxfgpe.c
+++ linux-pm/drivers/acpi/acpica/evxfgpe.c
@@ -435,6 +435,14 @@ acpi_setup_gpe_for_wake(acpi_handle wake
*/
gpe_event_info->flags =
(ACPI_GPE_DISPATCH_NOTIFY | ACPI_GPE_LEVEL_TRIGGERED);
+ } else if (gpe_event_info->flags & ACPI_GPE_AUTO_ENABLED) {
+ /*
+ * A reference to this GPE has been added during the GPE block
+ * initialization, so drop it now to prevent the GPE from being
+ * permanently enabled and clear its ACPI_GPE_AUTO_ENABLED flag.
+ */
+ (void)acpi_ev_remove_gpe_reference(gpe_event_info);
+ gpe_event_info->flags &= ~ACPI_GPE_AUTO_ENABLED;
}
/*
Hi, Rafael
> From: Rafael J. Wysocki [mailto:[email protected]]
> Subject: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
>
> From: Rafael J. Wysocki <[email protected]>
>
> In some cases GPEs are already active when they are enabled by
> acpi_ev_initialize_gpe_block() and whatever happens next may depend
> on the result of handling the events signaled by them, so the
> events should not be discarded (which is what happens currently) and
> they should be handled as soon as reasonably possible.
>
> For this reason, modify acpi_ev_initialize_gpe_block() to
> dispatch GPEs with the status flag set in-band right after
> enabling them.
In fact, what we need seems to be invoking acpi_ev_gpe_dispatch()
right after enabling an GPE. So there are 2 conditions related:
1. GPE is enabled for the first time.
2. GPE is initialized.
And we need to make sure that before acpi_update_all_gpes() is invoked,
all GPE EN bits are actually disabled.
What if we do this in this way:
Index: linux-acpica/drivers/acpi/acpica/evxfgpe.c
===================================================================
--- linux-acpica.orig/drivers/acpi/acpica/evxfgpe.c
+++ linux-acpica/drivers/acpi/acpica/evxfgpe.c
@@ -97,6 +97,14 @@ acpi_status acpi_update_all_gpes(void)
unlock_and_exit:
(void)acpi_ut_release_mutex(ACPI_MTX_EVENTS);
+ /*
+ * Poll GPEs to handle already triggered events.
+ * It is not sufficient to trigger edge-triggered GPE with specific
+ * GPE chips, software need to poll once after enabling.
+ */
+ if (acpi_gbl_all_gpes_initialized) {
+ acpi_ev_gpe_detect(acpi_gbl_gpe_xrupt_list_head);
+ }
return_ACPI_STATUS(status);
}
@@ -120,6 +128,7 @@ acpi_status acpi_enable_gpe(acpi_handle
acpi_status status = AE_BAD_PARAMETER;
struct acpi_gpe_event_info *gpe_event_info;
acpi_cpu_flags flags;
+ u8 poll_gpes = FALSE;
ACPI_FUNCTION_TRACE(acpi_enable_gpe);
@@ -135,12 +144,25 @@ acpi_status acpi_enable_gpe(acpi_handle
if (ACPI_GPE_DISPATCH_TYPE(gpe_event_info->flags) !=
ACPI_GPE_DISPATCH_NONE) {
status = acpi_ev_add_gpe_reference(gpe_event_info);
+ if (ACPI_SUCCESS(status) &&
+ gpe_event_info->runtime_count == 1) {
+ poll_gpes = TRUE;
+ }
} else {
status = AE_NO_HANDLER;
}
}
acpi_os_release_lock(acpi_gbl_gpe_lock, flags);
+
+ /*
+ * Poll GPEs to handle already triggered events.
+ * It is not sufficient to trigger edge-triggered GPE with specific
+ * GPE chips, software need to poll once after enabling.
+ */
+ if (poll_gpes && acpi_gbl_all_gpes_initialized) {
+ acpi_ev_gpe_detect(acpi_gbl_gpe_xrupt_list_head);
+ }
return_ACPI_STATUS(status);
}
ACPI_EXPORT_SYMBOL(acpi_enable_gpe)
Index: linux-acpica/drivers/acpi/acpica/utxfinit.c
===================================================================
--- linux-acpica.orig/drivers/acpi/acpica/utxfinit.c
+++ linux-acpica/drivers/acpi/acpica/utxfinit.c
@@ -284,6 +284,13 @@ acpi_status ACPI_INIT_FUNCTION acpi_init
}
/*
+ * Cleanup GPE enabling status to make sure that the GPE settings are
+ * as what the OSPMs expect. This should be done before enumerating
+ * ACPI devices and operation region drivers.
+ */
+ (void)acpi_hw_disable_all_gpes();
+
+ /*
* Initialize all device/region objects in the namespace. This runs
* the device _STA and _INI methods and region _REG methods.
*/
Thanks and best regards
Lv
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/acpi/acpica/evgpeblk.c | 28 +++++++++++++++++++---------
> 1 file changed, 19 insertions(+), 9 deletions(-)
>
> Index: linux-pm/drivers/acpi/acpica/evgpeblk.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/evgpeblk.c
> +++ linux-pm/drivers/acpi/acpica/evgpeblk.c
> @@ -440,9 +440,11 @@ acpi_ev_initialize_gpe_block(struct acpi
> void *ignored)
> {
> acpi_status status;
> + acpi_event_status event_status;
> struct acpi_gpe_event_info *gpe_event_info;
> u32 gpe_enabled_count;
> u32 gpe_index;
> + u32 gpe_number;
> u32 i;
> u32 j;
>
> @@ -470,30 +472,38 @@ acpi_ev_initialize_gpe_block(struct acpi
>
> gpe_index = (i * ACPI_GPE_REGISTER_WIDTH) + j;
> gpe_event_info = &gpe_block->event_info[gpe_index];
> + gpe_number = gpe_block->block_base_number + gpe_index;
>
> /*
> * Ignore GPEs that have no corresponding _Lxx/_Exx method
> - * and GPEs that are used to wake the system
> + * and GPEs that are used for wakeup
> */
> - if ((ACPI_GPE_DISPATCH_TYPE(gpe_event_info->flags) ==
> - ACPI_GPE_DISPATCH_NONE)
> - || (ACPI_GPE_DISPATCH_TYPE(gpe_event_info->flags) ==
> - ACPI_GPE_DISPATCH_HANDLER)
> - || (ACPI_GPE_DISPATCH_TYPE(gpe_event_info->flags) ==
> - ACPI_GPE_DISPATCH_RAW_HANDLER)
> + if ((ACPI_GPE_DISPATCH_TYPE(gpe_event_info->flags) !=
> + ACPI_GPE_DISPATCH_METHOD)
> || (gpe_event_info->flags & ACPI_GPE_CAN_WAKE)) {
> continue;
> }
>
> + event_status = 0;
> + (void)acpi_hw_get_gpe_status(gpe_event_info,
> + &event_status);
> +
> status = acpi_ev_add_gpe_reference(gpe_event_info);
> if (ACPI_FAILURE(status)) {
> ACPI_EXCEPTION((AE_INFO, status,
> "Could not enable GPE 0x%02X",
> - gpe_index +
> - gpe_block->block_base_number));
> + gpe_number));
> continue;
> }
>
> + if (event_status & ACPI_EVENT_FLAG_STATUS_SET) {
> + ACPI_INFO(("GPE 0x%02X active on init",
> + gpe_number));
> + (void)acpi_ev_gpe_dispatch(gpe_block->node,
> + gpe_event_info,
> + gpe_number);
> + }
> +
> gpe_enabled_count++;
> }
> }
Hi, Rafael
For this patch, I have a concern.
> From: Rafael J. Wysocki [mailto:[email protected]]
> Subject: [PATCH 2/3] ACPICA: Make it possible to enable runtime GPEs earlier
>
> From: Rafael J. Wysocki <[email protected]>
>
> Runtime GPEs have corresponding _Lxx/_Exx methods and are enabled
> automatically during the initialization of the ACPI subsystem through
> acpi_update_all_gpes() with the assumption that acpi_setup_gpe_for_wake()
> will be called in advance for all of the GPEs pointed to by _PRW
> objects in the namespace that may be affected by acpi_update_all_gpes().
> That is, acpi_ev_initialize_gpe_block() can only be called for a GPE
> block after acpi_setup_gpe_for_wake() has been called for all of the
> _PRW (wakeup) GPEs in it.
>
> The platform firmware on some systems, however, expects GPEs to be
> enabled before the enumeration of devices which is when
> acpi_setup_gpe_for_wake() is called and that goes against the above
> assumption.
>
> For this reason, introduce a new flag to be set by
> acpi_ev_initialize_gpe_block() when automatically enabling a GPE
> to indicate to acpi_setup_gpe_for_wake() that it needs to drop the
> reference to the GPE coming from acpi_ev_initialize_gpe_block()
> and modify acpi_setup_gpe_for_wake() accordingly. These changes
> allow acpi_setup_gpe_for_wake() and acpi_ev_initialize_gpe_block()
> to be invoked in any order.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/acpi/acpica/evgpeblk.c | 2 ++
> drivers/acpi/acpica/evxfgpe.c | 8 ++++++++
> include/acpi/actypes.h | 3 ++-
> 3 files changed, 12 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/acpi/acpica/evgpeblk.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/evgpeblk.c
> +++ linux-pm/drivers/acpi/acpica/evgpeblk.c
> @@ -496,6 +496,8 @@ acpi_ev_initialize_gpe_block(struct acpi
> continue;
> }
>
> + gpe_event_info->flags |= ACPI_GPE_AUTO_ENABLED;
> +
> if (event_status & ACPI_EVENT_FLAG_STATUS_SET) {
> ACPI_INFO(("GPE 0x%02X active on init",
> gpe_number));
> Index: linux-pm/include/acpi/actypes.h
> ===================================================================
> --- linux-pm.orig/include/acpi/actypes.h
> +++ linux-pm/include/acpi/actypes.h
> @@ -783,7 +783,7 @@ typedef u32 acpi_event_status;
> * | | | | +-- 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 mechanism
> + * | +--------- Has been enabled automatically at init time
> * +------------ <Reserved>
> */
> #define ACPI_GPE_DISPATCH_NONE (u8) 0x00
> @@ -799,6 +799,7 @@ typedef u32 acpi_event_status;
> #define ACPI_GPE_XRUPT_TYPE_MASK (u8) 0x08
>
> #define ACPI_GPE_CAN_WAKE (u8) 0x10
> +#define ACPI_GPE_AUTO_ENABLED (u8) 0x20
>
> /*
> * Flags for GPE and Lock interfaces
> Index: linux-pm/drivers/acpi/acpica/evxfgpe.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/acpica/evxfgpe.c
> +++ linux-pm/drivers/acpi/acpica/evxfgpe.c
> @@ -435,6 +435,14 @@ acpi_setup_gpe_for_wake(acpi_handle wake
> */
> gpe_event_info->flags =
> (ACPI_GPE_DISPATCH_NOTIFY | ACPI_GPE_LEVEL_TRIGGERED);
> + } else if (gpe_event_info->flags & ACPI_GPE_AUTO_ENABLED) {
> + /*
> + * A reference to this GPE has been added during the GPE block
> + * initialization, so drop it now to prevent the GPE from being
> + * permanently enabled and clear its ACPI_GPE_AUTO_ENABLED flag.
> + */
> + (void)acpi_ev_remove_gpe_reference(gpe_event_info);
> + gpe_event_info->flags &= ~ACPI_GPE_AUTO_ENABLED;
The problem is if the GPE is shared, how can we know decrement reference
once can sufficiently convert it into wakeup dispatcher owned GPE?
Thanks and best regards
Lv
Hi, Rafael
> From: [email protected] [mailto:[email protected]] On Behalf Of Rafael J.
> Wysocki
> Subject: [PATCH 3/3] ACPI / scan: Enable GPEs before scanning the namespace
>
> From: Rafael J. Wysocki <[email protected]>
>
> On some systems the platform firmware expects GPEs to be enabled
> before the enumeration of devices and if that expectation is not
> met, the systems in question may not boot in some situations.
>
> For this reason, change the initialization ordering of the ACPI
> subsystem to make it enable GPEs before scanning the namespace
> for the first time in order to enumerate devices.
This indeed is worthy of a try.
Acked-by: Lv Zheng <[email protected]>
Thanks and best regards
Lv
>
> Reported-by: Mika Westerberg <[email protected]>
> Suggested-by: Mika Westerberg <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/acpi/scan.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -2139,6 +2139,10 @@ int __init acpi_scan_init(void)
> acpi_get_spcr_uart_addr();
> }
>
> + acpi_gpe_apply_masked_gpes();
> + acpi_update_all_gpes();
> + acpi_ec_ecdt_start();
> +
> mutex_lock(&acpi_scan_lock);
> /*
> * Enumerate devices in the ACPI namespace.
> @@ -2163,10 +2167,6 @@ int __init acpi_scan_init(void)
> }
> }
>
> - acpi_gpe_apply_masked_gpes();
> - acpi_update_all_gpes();
> - acpi_ec_ecdt_start();
> -
> acpi_scan_initialized = true;
>
> out:
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 10, 2017 at 12:34:23AM +0200, Rafael J. Wysocki wrote:
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -2139,6 +2139,10 @@ int __init acpi_scan_init(void)
> acpi_get_spcr_uart_addr();
> }
>
> + acpi_gpe_apply_masked_gpes();
> + acpi_update_all_gpes();
> + acpi_ec_ecdt_start();
> +
> mutex_lock(&acpi_scan_lock);
> /*
> * Enumerate devices in the ACPI namespace.
I notice this is called from a subsys_initcall(). We scan the PCI bus
much earlier in arch/x86/kernel/early-quirks.c and it would be possible
to identify presence of Thunderbolt host controllers in an early quirk
(using the method of pci_is_thunderbolt_attached()) and, if found,
enable their GPEs or all GPEs.
Just as an aside in case your method doesn't work, I'm not affected by
this issue being a Mac user... ;-)
Thanks,
Lukas
On Thu, Aug 10, 2017 at 07:10:16AM +0200, Lukas Wunner wrote:
> On Thu, Aug 10, 2017 at 12:34:23AM +0200, Rafael J. Wysocki wrote:
> > --- linux-pm.orig/drivers/acpi/scan.c
> > +++ linux-pm/drivers/acpi/scan.c
> > @@ -2139,6 +2139,10 @@ int __init acpi_scan_init(void)
> > acpi_get_spcr_uart_addr();
> > }
> >
> > + acpi_gpe_apply_masked_gpes();
> > + acpi_update_all_gpes();
> > + acpi_ec_ecdt_start();
> > +
> > mutex_lock(&acpi_scan_lock);
> > /*
> > * Enumerate devices in the ACPI namespace.
>
> I notice this is called from a subsys_initcall(). We scan the PCI bus
> much earlier in arch/x86/kernel/early-quirks.c and it would be possible
> to identify presence of Thunderbolt host controllers in an early quirk
> (using the method of pci_is_thunderbolt_attached()) and, if found,
> enable their GPEs or all GPEs.
I don't think we want to differentiate between Thunderbolt controller
and anything else. Point here is that we need to enable GPEs in the same
order than Windows does (before PCI scan) to be on the path that is at
least somehow tested.
> Just as an aside in case your method doesn't work, I'm not affected by
> this issue being a Mac user... ;-)
;-)
On Thu, Aug 10, 2017 at 12:29:12AM +0200, Rafael J. Wysocki wrote:
> Hi,
>
> This is exceptional for at least two reasons.
>
> First, we need it now to work around boot problems on multiple platforms
> already seen in the field (which are shipping products).
>
> Second, the ACPICA changes in this series are kind of Linux-specific, because
> they are related to how Linux carries out the initialization of devices, which
> very well may be different from what the other OSes using ACPICA do.
>
> For these reasons, I'd like to make the following ACPICA changes in Linux
> only for the time being and then decide whether or not to take them into
> the upstream. If there are major concerns about that, please let me know.
>
> The issue at hand is that some platforms with Thunderbolt controllers won't
> boot if there are any Thunderbolt devices connected to them at boot time (if
> the devices are connected later, everything works as expected). That turns
> out to be related to a complicated sequence of events involving the platform
> firmware which needs to happen in exactly the right order at the right time
> for things to work and that requires GPEs to be enabled before enumerating
> the PCI bus.
>
> The first patch changes ACPICA to check the status of runtime GPEs before
> enabling them for the first time in order to avoid missing events (that
> is key for edge-triggered GPEs) and to process them early enough.
>
> The second one makes it possible to change the ordering of initialization
> between GPEs and devices, and the third one actually changes that ordering.
>
> The patches are based on Mika's work, so kudos to him.
Thank you for taking care of this! I've tested the series on Intel Skull
Canyon NUC, Dell XPS 9350, 9550 and 9365, and it fixes the issue on all
of them.
Tested-by: Mika Westerberg <[email protected]>
Hi,
> From: Lukas Wunner [mailto:[email protected]]
> Subject: Re: [PATCH 3/3] ACPI / scan: Enable GPEs before scanning the namespace
>
> On Thu, Aug 10, 2017 at 12:34:23AM +0200, Rafael J. Wysocki wrote:
> > --- linux-pm.orig/drivers/acpi/scan.c
> > +++ linux-pm/drivers/acpi/scan.c
> > @@ -2139,6 +2139,10 @@ int __init acpi_scan_init(void)
> > acpi_get_spcr_uart_addr();
> > }
> >
> > + acpi_gpe_apply_masked_gpes();
> > + acpi_update_all_gpes();
> > + acpi_ec_ecdt_start();
> > +
> > mutex_lock(&acpi_scan_lock);
> > /*
> > * Enumerate devices in the ACPI namespace.
>
> I notice this is called from a subsys_initcall(). We scan the PCI bus
> much earlier in arch/x86/kernel/early-quirks.c and it would be possible
> to identify presence of Thunderbolt host controllers in an early quirk
> (using the method of pci_is_thunderbolt_attached()) and, if found,
> enable their GPEs or all GPEs.
We have 2 choices here:
1. GPE is a part of device enumeration.
GPE must be enabled one by one after making sure that all related
bus/devices are powered on.
But it seems there is no such relationship between GPE and bus/device
in ACPI spec.
However if Windows works in this way, we'll regress by applying this
patch.
2. GPE is not a part of device enumeration.
Then probably we can even move GPE enabling into
acip_initialize_objects(), before calling
acpi_ns_initialize_devices(). As after preparing ACPI namespace,
_Lxx/_Exx is ready, and ACPI subsystem should be able to handle early
GPEs.
And ACPI subsystem's device enumeration starts from
acpi_ns_initialize_devices().
So this patch should be correct in theory. ;)
Thanks and best regards
Lv
>
> Just as an aside in case your method doesn't work, I'm not affected by
> this issue being a Mac user... ;-)
>
> Thanks,
>
> Lukas
On Thursday, August 10, 2017 3:48:58 AM CEST Zheng, Lv wrote:
> Hi, Rafael
>
> > From: Rafael J. Wysocki [mailto:[email protected]]
> > Subject: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> >
> > From: Rafael J. Wysocki <[email protected]>
> >
> > In some cases GPEs are already active when they are enabled by
> > acpi_ev_initialize_gpe_block() and whatever happens next may depend
> > on the result of handling the events signaled by them, so the
> > events should not be discarded (which is what happens currently) and
> > they should be handled as soon as reasonably possible.
> >
> > For this reason, modify acpi_ev_initialize_gpe_block() to
> > dispatch GPEs with the status flag set in-band right after
> > enabling them.
>
> In fact, what we need seems to be invoking acpi_ev_gpe_dispatch()
> right after enabling an GPE. So there are 2 conditions related:
> 1. GPE is enabled for the first time.
> 2. GPE is initialized.
>
> And we need to make sure that before acpi_update_all_gpes() is invoked,
> all GPE EN bits are actually disabled.
But we don't do it today, do we?
And still calling _dispatch() should not be incorrect even if the GPE
has been enabled already at this point. Worst case it just will
queue up the execution of _Lxx/_Exx which may or may not do anything
useful.
And BTW this is all done under acpi_gbl_gpe_lock so acpi_ev_gpe_detect()
will block on it if run concurrently and we've checked the status, so
we know that the GPE *should* be dispatched, so I sort of fail to see
the problem.
Thanks,
Rafael
On Thursday, August 10, 2017 3:52:05 AM CEST Zheng, Lv wrote:
> Hi, Rafael
>
> For this patch, I have a concern.
>
> > From: Rafael J. Wysocki [mailto:[email protected]]
> > Subject: [PATCH 2/3] ACPICA: Make it possible to enable runtime GPEs earlier
> >
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Runtime GPEs have corresponding _Lxx/_Exx methods and are enabled
> > automatically during the initialization of the ACPI subsystem through
> > acpi_update_all_gpes() with the assumption that acpi_setup_gpe_for_wake()
> > will be called in advance for all of the GPEs pointed to by _PRW
> > objects in the namespace that may be affected by acpi_update_all_gpes().
> > That is, acpi_ev_initialize_gpe_block() can only be called for a GPE
> > block after acpi_setup_gpe_for_wake() has been called for all of the
> > _PRW (wakeup) GPEs in it.
> >
> > The platform firmware on some systems, however, expects GPEs to be
> > enabled before the enumeration of devices which is when
> > acpi_setup_gpe_for_wake() is called and that goes against the above
> > assumption.
> >
> > For this reason, introduce a new flag to be set by
> > acpi_ev_initialize_gpe_block() when automatically enabling a GPE
> > to indicate to acpi_setup_gpe_for_wake() that it needs to drop the
> > reference to the GPE coming from acpi_ev_initialize_gpe_block()
> > and modify acpi_setup_gpe_for_wake() accordingly. These changes
> > allow acpi_setup_gpe_for_wake() and acpi_ev_initialize_gpe_block()
> > to be invoked in any order.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > drivers/acpi/acpica/evgpeblk.c | 2 ++
> > drivers/acpi/acpica/evxfgpe.c | 8 ++++++++
> > include/acpi/actypes.h | 3 ++-
> > 3 files changed, 12 insertions(+), 1 deletion(-)
> >
> > Index: linux-pm/drivers/acpi/acpica/evgpeblk.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/acpica/evgpeblk.c
> > +++ linux-pm/drivers/acpi/acpica/evgpeblk.c
> > @@ -496,6 +496,8 @@ acpi_ev_initialize_gpe_block(struct acpi
> > continue;
> > }
> >
> > + gpe_event_info->flags |= ACPI_GPE_AUTO_ENABLED;
> > +
> > if (event_status & ACPI_EVENT_FLAG_STATUS_SET) {
> > ACPI_INFO(("GPE 0x%02X active on init",
> > gpe_number));
> > Index: linux-pm/include/acpi/actypes.h
> > ===================================================================
> > --- linux-pm.orig/include/acpi/actypes.h
> > +++ linux-pm/include/acpi/actypes.h
> > @@ -783,7 +783,7 @@ typedef u32 acpi_event_status;
> > * | | | | +-- 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 mechanism
> > + * | +--------- Has been enabled automatically at init time
> > * +------------ <Reserved>
> > */
> > #define ACPI_GPE_DISPATCH_NONE (u8) 0x00
> > @@ -799,6 +799,7 @@ typedef u32 acpi_event_status;
> > #define ACPI_GPE_XRUPT_TYPE_MASK (u8) 0x08
> >
> > #define ACPI_GPE_CAN_WAKE (u8) 0x10
> > +#define ACPI_GPE_AUTO_ENABLED (u8) 0x20
> >
> > /*
> > * Flags for GPE and Lock interfaces
> > Index: linux-pm/drivers/acpi/acpica/evxfgpe.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/acpica/evxfgpe.c
> > +++ linux-pm/drivers/acpi/acpica/evxfgpe.c
> > @@ -435,6 +435,14 @@ acpi_setup_gpe_for_wake(acpi_handle wake
> > */
> > gpe_event_info->flags =
> > (ACPI_GPE_DISPATCH_NOTIFY | ACPI_GPE_LEVEL_TRIGGERED);
> > + } else if (gpe_event_info->flags & ACPI_GPE_AUTO_ENABLED) {
> > + /*
> > + * A reference to this GPE has been added during the GPE block
> > + * initialization, so drop it now to prevent the GPE from being
> > + * permanently enabled and clear its ACPI_GPE_AUTO_ENABLED flag.
> > + */
> > + (void)acpi_ev_remove_gpe_reference(gpe_event_info);
> > + gpe_event_info->flags &= ~ACPI_GPE_AUTO_ENABLED;
>
> The problem is if the GPE is shared, how can we know decrement reference
> once can sufficiently convert it into wakeup dispatcher owned GPE?
Even if it is shared, the current code will not enable it if it sees
ACPI_GPE_CAN_WAKE set.
We can change that logic, but that should be a separate patch IMO and
this is not related to the problem at hand.
Thanks,
Rafael
Hi, Rafael
> From: Rafael J. Wysocki [mailto:[email protected]]
> Subject: Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
>
> On Thursday, August 10, 2017 3:48:58 AM CEST Zheng, Lv wrote:
> > Hi, Rafael
> >
> > > From: Rafael J. Wysocki [mailto:[email protected]]
> > > Subject: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > >
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > In some cases GPEs are already active when they are enabled by
> > > acpi_ev_initialize_gpe_block() and whatever happens next may depend
> > > on the result of handling the events signaled by them, so the
> > > events should not be discarded (which is what happens currently) and
> > > they should be handled as soon as reasonably possible.
> > >
> > > For this reason, modify acpi_ev_initialize_gpe_block() to
> > > dispatch GPEs with the status flag set in-band right after
> > > enabling them.
> >
> > In fact, what we need seems to be invoking acpi_ev_gpe_dispatch()
> > right after enabling an GPE. So there are 2 conditions related:
> > 1. GPE is enabled for the first time.
> > 2. GPE is initialized.
> >
> > And we need to make sure that before acpi_update_all_gpes() is invoked,
> > all GPE EN bits are actually disabled.
>
> But we don't do it today, do we?
We don't do that.
>
> And still calling _dispatch() should not be incorrect even if the GPE
> has been enabled already at this point. Worst case it just will
> queue up the execution of _Lxx/_Exx which may or may not do anything
> useful.
>
> And BTW this is all done under acpi_gbl_gpe_lock so acpi_ev_gpe_detect()
> will block on it if run concurrently and we've checked the status, so
> we know that the GPE *should* be dispatched, so I sort of fail to see
> the problem.
There is another problem related:
ACPICA clears GPEs before enabling it.
This is proven to be wrong, and we have to fix it:
https://bugzilla.kernel.org/show_bug.cgi?id=196249
without fixing this issue, in this solution, we surely need to save the
GPE STS bit before incrementing GPE reference count, and poll it according
to the saved STS bit. Because if we poll it after enabling, STS bit will
be wrongly cleared.
So if we can do this on top of the "GPE clear" fix, things can be done
in a simpler way - invoke acpi_ev_gpe_detect() after fully initializing
GPEs (as what I pasted).
However I should say - merging "GPE clear" fix might be risky.
So this patch can be in upstream prior than the simpler solution to leave
us a stable base line.
I'll send out the simpler solution along with the "GPE clear" fix.
Maybe you can consider to ship it after merging this patch.
Thanks and best regards
Lv
Hi,
> From: Rafael J. Wysocki [mailto:[email protected]]
> Subject: Re: [PATCH 2/3] ACPICA: Make it possible to enable runtime GPEs earlier
>
> On Thursday, August 10, 2017 3:52:05 AM CEST Zheng, Lv wrote:
> > Hi, Rafael
> >
> > For this patch, I have a concern.
> >
> > > From: Rafael J. Wysocki [mailto:[email protected]]
> > > Subject: [PATCH 2/3] ACPICA: Make it possible to enable runtime GPEs earlier
> > >
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > Runtime GPEs have corresponding _Lxx/_Exx methods and are enabled
> > > automatically during the initialization of the ACPI subsystem through
> > > acpi_update_all_gpes() with the assumption that acpi_setup_gpe_for_wake()
> > > will be called in advance for all of the GPEs pointed to by _PRW
> > > objects in the namespace that may be affected by acpi_update_all_gpes().
> > > That is, acpi_ev_initialize_gpe_block() can only be called for a GPE
> > > block after acpi_setup_gpe_for_wake() has been called for all of the
> > > _PRW (wakeup) GPEs in it.
> > >
> > > The platform firmware on some systems, however, expects GPEs to be
> > > enabled before the enumeration of devices which is when
> > > acpi_setup_gpe_for_wake() is called and that goes against the above
> > > assumption.
> > >
> > > For this reason, introduce a new flag to be set by
> > > acpi_ev_initialize_gpe_block() when automatically enabling a GPE
> > > to indicate to acpi_setup_gpe_for_wake() that it needs to drop the
> > > reference to the GPE coming from acpi_ev_initialize_gpe_block()
> > > and modify acpi_setup_gpe_for_wake() accordingly. These changes
> > > allow acpi_setup_gpe_for_wake() and acpi_ev_initialize_gpe_block()
> > > to be invoked in any order.
> > >
> > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > ---
> > > drivers/acpi/acpica/evgpeblk.c | 2 ++
> > > drivers/acpi/acpica/evxfgpe.c | 8 ++++++++
> > > include/acpi/actypes.h | 3 ++-
> > > 3 files changed, 12 insertions(+), 1 deletion(-)
> > >
> > > Index: linux-pm/drivers/acpi/acpica/evgpeblk.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/acpi/acpica/evgpeblk.c
> > > +++ linux-pm/drivers/acpi/acpica/evgpeblk.c
> > > @@ -496,6 +496,8 @@ acpi_ev_initialize_gpe_block(struct acpi
> > > continue;
> > > }
> > >
> > > + gpe_event_info->flags |= ACPI_GPE_AUTO_ENABLED;
> > > +
> > > if (event_status & ACPI_EVENT_FLAG_STATUS_SET) {
> > > ACPI_INFO(("GPE 0x%02X active on init",
> > > gpe_number));
> > > Index: linux-pm/include/acpi/actypes.h
> > > ===================================================================
> > > --- linux-pm.orig/include/acpi/actypes.h
> > > +++ linux-pm/include/acpi/actypes.h
> > > @@ -783,7 +783,7 @@ typedef u32 acpi_event_status;
> > > * | | | | +-- 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 mechanism
> > > + * | +--------- Has been enabled automatically at init time
> > > * +------------ <Reserved>
> > > */
> > > #define ACPI_GPE_DISPATCH_NONE (u8) 0x00
> > > @@ -799,6 +799,7 @@ typedef u32 acpi_event_status;
> > > #define ACPI_GPE_XRUPT_TYPE_MASK (u8) 0x08
> > >
> > > #define ACPI_GPE_CAN_WAKE (u8) 0x10
> > > +#define ACPI_GPE_AUTO_ENABLED (u8) 0x20
> > >
> > > /*
> > > * Flags for GPE and Lock interfaces
> > > Index: linux-pm/drivers/acpi/acpica/evxfgpe.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/acpi/acpica/evxfgpe.c
> > > +++ linux-pm/drivers/acpi/acpica/evxfgpe.c
> > > @@ -435,6 +435,14 @@ acpi_setup_gpe_for_wake(acpi_handle wake
> > > */
> > > gpe_event_info->flags =
> > > (ACPI_GPE_DISPATCH_NOTIFY | ACPI_GPE_LEVEL_TRIGGERED);
> > > + } else if (gpe_event_info->flags & ACPI_GPE_AUTO_ENABLED) {
> > > + /*
> > > + * A reference to this GPE has been added during the GPE block
> > > + * initialization, so drop it now to prevent the GPE from being
> > > + * permanently enabled and clear its ACPI_GPE_AUTO_ENABLED flag.
> > > + */
> > > + (void)acpi_ev_remove_gpe_reference(gpe_event_info);
> > > + gpe_event_info->flags &= ~ACPI_GPE_AUTO_ENABLED;
> >
> > The problem is if the GPE is shared, how can we know decrement reference
> > once can sufficiently convert it into wakeup dispatcher owned GPE?
>
> Even if it is shared, the current code will not enable it if it sees
> ACPI_GPE_CAN_WAKE set.
>
> We can change that logic, but that should be a separate patch IMO and
> this is not related to the problem at hand.
OK, I see.
We can enhance that on top of these fixes.
Thanks,
Lv
On Friday, August 11, 2017 7:40:56 AM CEST Zheng, Lv wrote:
> Hi, Rafael
>
> > From: Rafael J. Wysocki [mailto:[email protected]]
> > Subject: Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> >
> > On Thursday, August 10, 2017 3:48:58 AM CEST Zheng, Lv wrote:
> > > Hi, Rafael
> > >
> > > > From: Rafael J. Wysocki [mailto:[email protected]]
> > > > Subject: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > > >
> > > > From: Rafael J. Wysocki <[email protected]>
> > > >
> > > > In some cases GPEs are already active when they are enabled by
> > > > acpi_ev_initialize_gpe_block() and whatever happens next may depend
> > > > on the result of handling the events signaled by them, so the
> > > > events should not be discarded (which is what happens currently) and
> > > > they should be handled as soon as reasonably possible.
> > > >
> > > > For this reason, modify acpi_ev_initialize_gpe_block() to
> > > > dispatch GPEs with the status flag set in-band right after
> > > > enabling them.
> > >
> > > In fact, what we need seems to be invoking acpi_ev_gpe_dispatch()
> > > right after enabling an GPE. So there are 2 conditions related:
> > > 1. GPE is enabled for the first time.
> > > 2. GPE is initialized.
> > >
> > > And we need to make sure that before acpi_update_all_gpes() is invoked,
> > > all GPE EN bits are actually disabled.
> >
> > But we don't do it today, do we?
>
> We don't do that.
>
> >
> > And still calling _dispatch() should not be incorrect even if the GPE
> > has been enabled already at this point. Worst case it just will
> > queue up the execution of _Lxx/_Exx which may or may not do anything
> > useful.
> >
> > And BTW this is all done under acpi_gbl_gpe_lock so acpi_ev_gpe_detect()
> > will block on it if run concurrently and we've checked the status, so
> > we know that the GPE *should* be dispatched, so I sort of fail to see
> > the problem.
>
> There is another problem related:
> ACPICA clears GPEs before enabling it.
> This is proven to be wrong, and we have to fix it:
> https://bugzilla.kernel.org/show_bug.cgi?id=196249
>
> without fixing this issue, in this solution, we surely need to save the
> GPE STS bit before incrementing GPE reference count, and poll it according
> to the saved STS bit. Because if we poll it after enabling, STS bit will
> be wrongly cleared.
I'm not sure if I understand you correctly, but why would we poll it?
In the $subject patch the status is checked and then
acpi_ev_add_gpe_reference() is called to add a reference to the GPE.
If this is the first reference (which will be the case in the majority
of cases), acpi_ev_enable_gpe() will be called and that will clear the
status.
Then, acpi_ev_gpe_dispatch() is called if the status was set and that
itself doesn't check the status. It disables the GPE upfront (so the
status doesn't matter from now on until the GPE is enabled again) and
clears the status unconditionally if the GPE is edge-triggered. This
means that for edge-triggered GPEs the clearing of the status by
acpi_ev_enable_gpe() doesn't matter here.
For level-triggered GPEs the status is not cleared by acpi_ev_gpe_dispatch()
itself, but _Lxx is executed (again, without checking the status) and
finally the status is cleared by acpi_ev_finish_gpe() anyway, so we
end up with cleared status no matter what (and that before re-enabling
the GPE). End even if _Lxx itself checked the status (but honestly why
would it?), then this is a level-triggered GPE, so it will re-trigger
anyway if not handled this time.
So it looks like the fact that acpi_ev_enable_gpe() clears the status before
enabling the GPE doesn't matter for this patch, although it may matter in
general, but that is a different (and somewhat related) issue.
Thanks,
Rafael
Hi, Rafael
> From: [email protected] [mailto:[email protected]] On Behalf Of Rafael J.
> Wysocki
> Subject: [PATCH 3/3] ACPI / scan: Enable GPEs before scanning the namespace
>
> From: Rafael J. Wysocki <[email protected]>
>
> On some systems the platform firmware expects GPEs to be enabled
> before the enumeration of devices and if that expectation is not
> met, the systems in question may not boot in some situations.
>
> For this reason, change the initialization ordering of the ACPI
> subsystem to make it enable GPEs before scanning the namespace
> for the first time in order to enumerate devices.
>
> Reported-by: Mika Westerberg <[email protected]>
> Suggested-by: Mika Westerberg <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/acpi/scan.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -2139,6 +2139,10 @@ int __init acpi_scan_init(void)
> acpi_get_spcr_uart_addr();
> }
>
> + acpi_gpe_apply_masked_gpes();
> + acpi_update_all_gpes();
> + acpi_ec_ecdt_start();
> +
Just for your information.
A recent internal bug reveals that acpi_ec_ecdt_start() should only be
invoked after the enumeration (acpi_ec_add()) for now.
The function contains logics that need to be altered by acpi_ec_add().
So it seems we can only do less aggressive change by moving the GPE
related 2 lines up.
Thanks and best regards
Lv
> mutex_lock(&acpi_scan_lock);
> /*
> * Enumerate devices in the ACPI namespace.
> @@ -2163,10 +2167,6 @@ int __init acpi_scan_init(void)
> }
> }
>
> - acpi_gpe_apply_masked_gpes();
> - acpi_update_all_gpes();
> - acpi_ec_ecdt_start();
> -
> acpi_scan_initialized = true;
>
> out:
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
> From: Rafael J. Wysocki [mailto:[email protected]]
> Subject: Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
>
> On Friday, August 11, 2017 7:40:56 AM CEST Zheng, Lv wrote:
> > Hi, Rafael
> >
> > > From: Rafael J. Wysocki [mailto:[email protected]]
> > > Subject: Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > >
> > > On Thursday, August 10, 2017 3:48:58 AM CEST Zheng, Lv wrote:
> > > > Hi, Rafael
> > > >
> > > > > From: Rafael J. Wysocki [mailto:[email protected]]
> > > > > Subject: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > > > >
> > > > > From: Rafael J. Wysocki <[email protected]>
> > > > >
> > > > > In some cases GPEs are already active when they are enabled by
> > > > > acpi_ev_initialize_gpe_block() and whatever happens next may depend
> > > > > on the result of handling the events signaled by them, so the
> > > > > events should not be discarded (which is what happens currently) and
> > > > > they should be handled as soon as reasonably possible.
> > > > >
> > > > > For this reason, modify acpi_ev_initialize_gpe_block() to
> > > > > dispatch GPEs with the status flag set in-band right after
> > > > > enabling them.
> > > >
> > > > In fact, what we need seems to be invoking acpi_ev_gpe_dispatch()
> > > > right after enabling an GPE. So there are 2 conditions related:
> > > > 1. GPE is enabled for the first time.
> > > > 2. GPE is initialized.
> > > >
> > > > And we need to make sure that before acpi_update_all_gpes() is invoked,
> > > > all GPE EN bits are actually disabled.
> > >
> > > But we don't do it today, do we?
> >
> > We don't do that.
> >
> > >
> > > And still calling _dispatch() should not be incorrect even if the GPE
> > > has been enabled already at this point. Worst case it just will
> > > queue up the execution of _Lxx/_Exx which may or may not do anything
> > > useful.
> > >
> > > And BTW this is all done under acpi_gbl_gpe_lock so acpi_ev_gpe_detect()
> > > will block on it if run concurrently and we've checked the status, so
> > > we know that the GPE *should* be dispatched, so I sort of fail to see
> > > the problem.
> >
> > There is another problem related:
> > ACPICA clears GPEs before enabling it.
> > This is proven to be wrong, and we have to fix it:
> > https://bugzilla.kernel.org/show_bug.cgi?id=196249
> >
> > without fixing this issue, in this solution, we surely need to save the
> > GPE STS bit before incrementing GPE reference count, and poll it according
> > to the saved STS bit. Because if we poll it after enabling, STS bit will
> > be wrongly cleared.
>
> I'm not sure if I understand you correctly, but why would we poll it?
>
> In the $subject patch the status is checked and then
> acpi_ev_add_gpe_reference() is called to add a reference to the GPE.
>
> If this is the first reference (which will be the case in the majority
> of cases), acpi_ev_enable_gpe() will be called and that will clear the
> status.
>
> Then, acpi_ev_gpe_dispatch() is called if the status was set and that
> itself doesn't check the status. It disables the GPE upfront (so the
> status doesn't matter from now on until the GPE is enabled again) and
> clears the status unconditionally if the GPE is edge-triggered. This
> means that for edge-triggered GPEs the clearing of the status by
> acpi_ev_enable_gpe() doesn't matter here.
No problem, I understood.
And was thinking thought this patch [edge GPE dispatch fix] should be
correct and good for current Linux upstream.
What I meant is:
PATCH [edge GPE clear fix] https://patchwork.kernel.org/patch/9894983/
is a fix we need for upstream as it is the only possible fix for the
issue fixed by it.
On top of that, when acpi_ev_enable_gpe() is called, GPE won't be cleared.
and then things can be done in a simpler way:
PATCH [edge GPE enable fix] https://patchwork.kernel.org/patch/9894989/
As [edge GPE clear fix] is risky, I think [edge GPE dispatch fix] is OK
for Linux upstream.
So we can have 2 processes:
1. Merge [edge GPE dispatch fix] and let [edge GPE clear fix] and
[edge GPE enable fix] released from ACPICA upstream so that:
1. We can enhance them in ACPICA upstream.
2. It will be regression safer for us to merge [edge GPE clear fix].
2. Merge [edge GPE clear fix] and [edge GPE enable fix] without
merging [edge GPE dispatch fix].
What I meant is:
It's up to you to decide which process we should take. :)
>
> For level-triggered GPEs the status is not cleared by acpi_ev_gpe_dispatch()
> itself, but _Lxx is executed (again, without checking the status) and
> finally the status is cleared by acpi_ev_finish_gpe() anyway, so we
> end up with cleared status no matter what (and that before re-enabling
> the GPE). End even if _Lxx itself checked the status (but honestly why
> would it?), then this is a level-triggered GPE, so it will re-trigger
> anyway if not handled this time.
Let's skip level-triggered GPE case.
>
> So it looks like the fact that acpi_ev_enable_gpe() clears the status before
> enabling the GPE doesn't matter for this patch, although it may matter in
> general, but that is a different (and somewhat related) issue.
I agreed.
Thanks and best regards
Lv
On Tuesday, August 15, 2017 11:59:00 AM CEST Zheng, Lv wrote:
> Hi,
>
> > From: Rafael J. Wysocki [mailto:[email protected]]
> > Subject: Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> >
> > On Friday, August 11, 2017 7:40:56 AM CEST Zheng, Lv wrote:
> > > Hi, Rafael
> > >
> > > > From: Rafael J. Wysocki [mailto:[email protected]]
> > > > Subject: Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > > >
> > > > On Thursday, August 10, 2017 3:48:58 AM CEST Zheng, Lv wrote:
> > > > > Hi, Rafael
> > > > >
> > > > > > From: Rafael J. Wysocki [mailto:[email protected]]
> > > > > > Subject: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > > > > >
> > > > > > From: Rafael J. Wysocki <[email protected]>
> > > > > >
> > > > > > In some cases GPEs are already active when they are enabled by
> > > > > > acpi_ev_initialize_gpe_block() and whatever happens next may depend
> > > > > > on the result of handling the events signaled by them, so the
> > > > > > events should not be discarded (which is what happens currently) and
> > > > > > they should be handled as soon as reasonably possible.
> > > > > >
> > > > > > For this reason, modify acpi_ev_initialize_gpe_block() to
> > > > > > dispatch GPEs with the status flag set in-band right after
> > > > > > enabling them.
> > > > >
> > > > > In fact, what we need seems to be invoking acpi_ev_gpe_dispatch()
> > > > > right after enabling an GPE. So there are 2 conditions related:
> > > > > 1. GPE is enabled for the first time.
> > > > > 2. GPE is initialized.
> > > > >
> > > > > And we need to make sure that before acpi_update_all_gpes() is invoked,
> > > > > all GPE EN bits are actually disabled.
> > > >
> > > > But we don't do it today, do we?
> > >
> > > We don't do that.
> > >
> > > >
> > > > And still calling _dispatch() should not be incorrect even if the GPE
> > > > has been enabled already at this point. Worst case it just will
> > > > queue up the execution of _Lxx/_Exx which may or may not do anything
> > > > useful.
> > > >
> > > > And BTW this is all done under acpi_gbl_gpe_lock so acpi_ev_gpe_detect()
> > > > will block on it if run concurrently and we've checked the status, so
> > > > we know that the GPE *should* be dispatched, so I sort of fail to see
> > > > the problem.
> > >
> > > There is another problem related:
> > > ACPICA clears GPEs before enabling it.
> > > This is proven to be wrong, and we have to fix it:
> > > https://bugzilla.kernel.org/show_bug.cgi?id=196249
> > >
> > > without fixing this issue, in this solution, we surely need to save the
> > > GPE STS bit before incrementing GPE reference count, and poll it according
> > > to the saved STS bit. Because if we poll it after enabling, STS bit will
> > > be wrongly cleared.
> >
> > I'm not sure if I understand you correctly, but why would we poll it?
> >
> > In the $subject patch the status is checked and then
> > acpi_ev_add_gpe_reference() is called to add a reference to the GPE.
> >
> > If this is the first reference (which will be the case in the majority
> > of cases), acpi_ev_enable_gpe() will be called and that will clear the
> > status.
> >
> > Then, acpi_ev_gpe_dispatch() is called if the status was set and that
> > itself doesn't check the status. It disables the GPE upfront (so the
> > status doesn't matter from now on until the GPE is enabled again) and
> > clears the status unconditionally if the GPE is edge-triggered. This
> > means that for edge-triggered GPEs the clearing of the status by
> > acpi_ev_enable_gpe() doesn't matter here.
>
> No problem, I understood.
> And was thinking thought this patch [edge GPE dispatch fix] should be
> correct and good for current Linux upstream.
>
> What I meant is:
> PATCH [edge GPE clear fix] https://patchwork.kernel.org/patch/9894983/
> is a fix we need for upstream as it is the only possible fix for the
> issue fixed by it.
> On top of that, when acpi_ev_enable_gpe() is called, GPE won't be cleared.
> and then things can be done in a simpler way:
> PATCH [edge GPE enable fix] https://patchwork.kernel.org/patch/9894989/
>
> As [edge GPE clear fix] is risky, I think [edge GPE dispatch fix] is OK
> for Linux upstream.
>
> So we can have 2 processes:
> 1. Merge [edge GPE dispatch fix] and let [edge GPE clear fix] and
> [edge GPE enable fix] released from ACPICA upstream so that:
> 1. We can enhance them in ACPICA upstream.
> 2. It will be regression safer for us to merge [edge GPE clear fix].
> 2. Merge [edge GPE clear fix] and [edge GPE enable fix] without
> merging [edge GPE dispatch fix].
>
> What I meant is:
> It's up to you to decide which process we should take. :)
OK
So I would go with what is already there in my tree. Please work on top of
that.
Thanks,
Rafael
On Tuesday, August 15, 2017 4:12:24 AM CEST Zheng, Lv wrote:
> Hi, Rafael
>
> > From: [email protected] [mailto:[email protected]] On Behalf Of Rafael J.
> > Wysocki
> > Subject: [PATCH 3/3] ACPI / scan: Enable GPEs before scanning the namespace
> >
> > From: Rafael J. Wysocki <[email protected]>
> >
> > On some systems the platform firmware expects GPEs to be enabled
> > before the enumeration of devices and if that expectation is not
> > met, the systems in question may not boot in some situations.
> >
> > For this reason, change the initialization ordering of the ACPI
> > subsystem to make it enable GPEs before scanning the namespace
> > for the first time in order to enumerate devices.
> >
> > Reported-by: Mika Westerberg <[email protected]>
> > Suggested-by: Mika Westerberg <[email protected]>
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > drivers/acpi/scan.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/scan.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/scan.c
> > +++ linux-pm/drivers/acpi/scan.c
> > @@ -2139,6 +2139,10 @@ int __init acpi_scan_init(void)
> > acpi_get_spcr_uart_addr();
> > }
> >
> > + acpi_gpe_apply_masked_gpes();
> > + acpi_update_all_gpes();
> > + acpi_ec_ecdt_start();
> > +
>
> Just for your information.
> A recent internal bug reveals that acpi_ec_ecdt_start() should only be
> invoked after the enumeration (acpi_ec_add()) for now.
> The function contains logics that need to be altered by acpi_ec_add().
>
> So it seems we can only do less aggressive change by moving the GPE
> related 2 lines up.
OK, done.
Please check my linux-next branch and see if that's what it should be.
Thanks,
Rafael
Hi,
> From: [email protected] [mailto:[email protected]] On Behalf Of Rafael J.
> Wysocki
> Subject: Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
>
> On Tuesday, August 15, 2017 11:59:00 AM CEST Zheng, Lv wrote:
> > Hi,
> >
> > > From: Rafael J. Wysocki [mailto:[email protected]]
> > > Subject: Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > >
> > > On Friday, August 11, 2017 7:40:56 AM CEST Zheng, Lv wrote:
> > > > Hi, Rafael
> > > >
> > > > > From: Rafael J. Wysocki [mailto:[email protected]]
> > > > > Subject: Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > > > >
> > > > > On Thursday, August 10, 2017 3:48:58 AM CEST Zheng, Lv wrote:
> > > > > > Hi, Rafael
> > > > > >
> > > > > > > From: Rafael J. Wysocki [mailto:[email protected]]
> > > > > > > Subject: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > > > > > >
> > > > > > > From: Rafael J. Wysocki <[email protected]>
> > > > > > >
> > > > > > > In some cases GPEs are already active when they are enabled by
> > > > > > > acpi_ev_initialize_gpe_block() and whatever happens next may depend
> > > > > > > on the result of handling the events signaled by them, so the
> > > > > > > events should not be discarded (which is what happens currently) and
> > > > > > > they should be handled as soon as reasonably possible.
> > > > > > >
> > > > > > > For this reason, modify acpi_ev_initialize_gpe_block() to
> > > > > > > dispatch GPEs with the status flag set in-band right after
> > > > > > > enabling them.
> > > > > >
> > > > > > In fact, what we need seems to be invoking acpi_ev_gpe_dispatch()
> > > > > > right after enabling an GPE. So there are 2 conditions related:
> > > > > > 1. GPE is enabled for the first time.
> > > > > > 2. GPE is initialized.
> > > > > >
> > > > > > And we need to make sure that before acpi_update_all_gpes() is invoked,
> > > > > > all GPE EN bits are actually disabled.
> > > > >
> > > > > But we don't do it today, do we?
> > > >
> > > > We don't do that.
> > > >
> > > > >
> > > > > And still calling _dispatch() should not be incorrect even if the GPE
> > > > > has been enabled already at this point. Worst case it just will
> > > > > queue up the execution of _Lxx/_Exx which may or may not do anything
> > > > > useful.
> > > > >
> > > > > And BTW this is all done under acpi_gbl_gpe_lock so acpi_ev_gpe_detect()
> > > > > will block on it if run concurrently and we've checked the status, so
> > > > > we know that the GPE *should* be dispatched, so I sort of fail to see
> > > > > the problem.
> > > >
> > > > There is another problem related:
> > > > ACPICA clears GPEs before enabling it.
> > > > This is proven to be wrong, and we have to fix it:
> > > > https://bugzilla.kernel.org/show_bug.cgi?id=196249
> > > >
> > > > without fixing this issue, in this solution, we surely need to save the
> > > > GPE STS bit before incrementing GPE reference count, and poll it according
> > > > to the saved STS bit. Because if we poll it after enabling, STS bit will
> > > > be wrongly cleared.
> > >
> > > I'm not sure if I understand you correctly, but why would we poll it?
> > >
> > > In the $subject patch the status is checked and then
> > > acpi_ev_add_gpe_reference() is called to add a reference to the GPE.
> > >
> > > If this is the first reference (which will be the case in the majority
> > > of cases), acpi_ev_enable_gpe() will be called and that will clear the
> > > status.
> > >
> > > Then, acpi_ev_gpe_dispatch() is called if the status was set and that
> > > itself doesn't check the status. It disables the GPE upfront (so the
> > > status doesn't matter from now on until the GPE is enabled again) and
> > > clears the status unconditionally if the GPE is edge-triggered. This
> > > means that for edge-triggered GPEs the clearing of the status by
> > > acpi_ev_enable_gpe() doesn't matter here.
> >
> > No problem, I understood.
> > And was thinking thought this patch [edge GPE dispatch fix] should be
> > correct and good for current Linux upstream.
> >
> > What I meant is:
> > PATCH [edge GPE clear fix] https://patchwork.kernel.org/patch/9894983/
> > is a fix we need for upstream as it is the only possible fix for the
> > issue fixed by it.
> > On top of that, when acpi_ev_enable_gpe() is called, GPE won't be cleared.
> > and then things can be done in a simpler way:
> > PATCH [edge GPE enable fix] https://patchwork.kernel.org/patch/9894989/
> >
> > As [edge GPE clear fix] is risky, I think [edge GPE dispatch fix] is OK
> > for Linux upstream.
> >
> > So we can have 2 processes:
> > 1. Merge [edge GPE dispatch fix] and let [edge GPE clear fix] and
> > [edge GPE enable fix] released from ACPICA upstream so that:
> > 1. We can enhance them in ACPICA upstream.
> > 2. It will be regression safer for us to merge [edge GPE clear fix].
> > 2. Merge [edge GPE clear fix] and [edge GPE enable fix] without
> > merging [edge GPE dispatch fix].
> >
> > What I meant is:
> > It's up to you to decide which process we should take. :)
>
> OK
>
> So I would go with what is already there in my tree. Please work on top of
> that.
OK.
Then I'll do that from ACPICA upstream to have more eyes on the enhancements
and more time to create the stable baseline for the [edge GPE clear fix].
Best regards,
Lv
Hi, Rafael
> From: Rafael J. Wysocki [mailto:[email protected]]
> Subject: Re: [PATCH 3/3] ACPI / scan: Enable GPEs before scanning the namespace
>
> On Tuesday, August 15, 2017 4:12:24 AM CEST Zheng, Lv wrote:
> > Hi, Rafael
> >
> > > From: [email protected] [mailto:[email protected]] On Behalf Of
> Rafael J.
> > > Wysocki
> > > Subject: [PATCH 3/3] ACPI / scan: Enable GPEs before scanning the namespace
> > >
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > On some systems the platform firmware expects GPEs to be enabled
> > > before the enumeration of devices and if that expectation is not
> > > met, the systems in question may not boot in some situations.
> > >
> > > For this reason, change the initialization ordering of the ACPI
> > > subsystem to make it enable GPEs before scanning the namespace
> > > for the first time in order to enumerate devices.
> > >
> > > Reported-by: Mika Westerberg <[email protected]>
> > > Suggested-by: Mika Westerberg <[email protected]>
> > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > ---
> > > drivers/acpi/scan.c | 8 ++++----
> > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > Index: linux-pm/drivers/acpi/scan.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/acpi/scan.c
> > > +++ linux-pm/drivers/acpi/scan.c
> > > @@ -2139,6 +2139,10 @@ int __init acpi_scan_init(void)
> > > acpi_get_spcr_uart_addr();
> > > }
> > >
> > > + acpi_gpe_apply_masked_gpes();
> > > + acpi_update_all_gpes();
> > > + acpi_ec_ecdt_start();
> > > +
> >
> > Just for your information.
> > A recent internal bug reveals that acpi_ec_ecdt_start() should only be
> > invoked after the enumeration (acpi_ec_add()) for now.
> > The function contains logics that need to be altered by acpi_ec_add().
> >
> > So it seems we can only do less aggressive change by moving the GPE
> > related 2 lines up.
>
> OK, done.
>
> Please check my linux-next branch and see if that's what it should be.
I confirmed.
And refreshed my EC regression fix on top of that with v2 tagged in the subjects.
Thanks and best regards
Lv