[3/3] ACPI / scan: Enable GPEs before scanning the namespace

Message ID 6344941.kJ2o2XerZ0@aspire.rjw.lan
State Not Applicable
Headers show

Commit Message

Rafael J. Wysocki Aug. 9, 2017, 10:34 p.m.
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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 <mika.westerberg@linux.intel.com>
Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Lv Zheng Aug. 10, 2017, 1:54 a.m. | #1
Hi, Rafael

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> Wysocki
> Subject: [PATCH 3/3] ACPI / scan: Enable GPEs before scanning the namespace
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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 <lv.zheng@intel.com>

Thanks and best regards
Lv

> 
> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Wunner Aug. 10, 2017, 5:10 a.m. | #2
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
Lv Zheng Aug. 10, 2017, 7:45 a.m. | #3
Hi,

> From: Lukas Wunner [mailto:lukas@wunner.de]
> 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
Mika Westerberg Aug. 10, 2017, 8:23 a.m. | #4
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... ;-)

;-)
Lv Zheng Aug. 15, 2017, 2:12 a.m. | #5
Hi, Rafael

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> Wysocki
> Subject: [PATCH 3/3] ACPI / scan: Enable GPEs before scanning the namespace
> 
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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 <mika.westerberg@linux.intel.com>
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Aug. 15, 2017, 12:22 p.m. | #6
On Tuesday, August 15, 2017 4:12:24 AM CEST Zheng, Lv wrote:
> Hi, Rafael
> 
> > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> > Wysocki
> > Subject: [PATCH 3/3] ACPI / scan: Enable GPEs before scanning the namespace
> > 
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > 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 <mika.westerberg@linux.intel.com>
> > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  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
Lv Zheng Aug. 17, 2017, 2:25 a.m. | #7
Hi, Rafael

> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> 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: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of
> Rafael J.
> > > Wysocki
> > > Subject: [PATCH 3/3] ACPI / scan: Enable GPEs before scanning the namespace
> > >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > 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 <mika.westerberg@linux.intel.com>
> > > Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > ---
> > >  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

Patch

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: