[PATCH] ACPICA: Events: Execute some _REG methods in early boot

[PATCH] ACPICA: Events: Execute some _REG methods in early boot

[Chris Bainbridge]

The regression caused _REG methods to not be run in early boot for all
space IDs, but a removed comment stated that _REG methods should be
executed for IDs like embedded_controller. Before the regression this
was the case:

[    0.230469] Executing  Method       \_SB.PCI0.LPCB.EC._REG
[    0.230531] Initializing Region       \GNVS
[    0.230607] Initializing Region       \_SB.PCI0.LPCB.EC.ECOR
[    0.231043] Initializing Region       \_SB.PCI0.IGPU.IGDM

After the regression the initialisation is not done and ODEBUG warnings
are shown at boot and/or shutdown:

[    6.676570] WARNING: CPU: 0 PID: 3317 at lib/debugobjects.c:263 debug_print_object+0x85/0xa0()
[    6.676576] ODEBUG: assert_init not available (active state 0) object type: timer_list hint: stub_timer+0x0/0x20
[    6.676578] Modules linked in:
[    6.676582] CPU: 0 PID: 3317 Comm: ccpd Not tainted 4.5.0-rc6 #509
[    6.676584] Hardware name: Apple Inc. MacBookPro10,2/Mac-AFD8A9D944EA4843, BIOS MBP102.88Z.0106.B0A.1509130955 09/13/2015
[    6.676586]  0000000000000000 ffff880256543db0 ffffffff814802b5 ffff880256543df8
[    6.676590]  ffffffff81f40dbd ffff880256543de8 ffffffff810bd0ec ffff880256543e90
[    6.676594]  ffffffff822532c0 ffffffff81f40e63 ffffffff83138c88 0000000001fdf040
[    6.676598] Call Trace:
[    6.676603]  [<ffffffff814802b5>] dump_stack+0x67/0x92
[    6.676608]  [<ffffffff810bd0ec>] warn_slowpath_common+0x7c/0xb0
[    6.676611]  [<ffffffff810bd167>] warn_slowpath_fmt+0x47/0x50
[    6.676614]  [<ffffffff8149d7a5>] debug_print_object+0x85/0xa0
[    6.676616]  [<ffffffff8111f440>] ? cascade+0x70/0x70
[    6.676620]  [<ffffffff8149e398>] debug_object_assert_init+0xf8/0x130
[    6.676624]  [<ffffffff811038cd>] ? trace_hardirqs_on+0xd/0x10
[    6.676626]  [<ffffffff8111fcff>] del_timer+0x1f/0x70
[    6.676631]  [<ffffffff811852f1>] laptop_sync_completion+0x61/0x100
[    6.676633]  [<ffffffff81185290>] ? laptop_io_completion+0x30/0x30
[    6.676637]  [<ffffffff81212c9f>] sys_sync+0x7f/0x90
[    6.676641]  [<ffffffff81ad0d97>] entry_SYSCALL_64_fastpath+0x12/0x6f

Fixes: efaed9be998b ("ACPICA: Events: Enhance acpi_ev_execute_reg_method() to ensure no _REG evaluations can happen during OS early boot stages")
Link: https://lkml.org/lkml/2016/2/3/273
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=112911
Signed-off-by: Chris Bainbridge <chris.bainbridge@gmail.com>
---
 drivers/acpi/acpica/evregion.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c
index 47092b4d633c..d0b02ef0effa 100644
--- a/drivers/acpi/acpica/evregion.c
+++ b/drivers/acpi/acpica/evregion.c
@@ -590,6 +590,7 @@ acpi_ev_execute_reg_method(union acpi_operand_object *region_obj, u32 function)
 	union acpi_operand_object *args[3];
 	union acpi_operand_object *region_obj2;
 	acpi_status status;
+	bool sp;
 
 	ACPI_FUNCTION_TRACE(ev_execute_reg_method);
 
@@ -598,9 +599,28 @@ acpi_ev_execute_reg_method(union acpi_operand_object *region_obj, u32 function)
 		return_ACPI_STATUS(AE_NOT_EXIST);
 	}
 
+	/*
+	 * For the default space_IDs, (the IDs for which there are default region handlers
+	 * installed) Only execute the _REG methods if the global initialization _REG
+	 * methods have already been run (via acpi_initialize_objects). In other words,
+	 * we will defer the execution of the _REG methods for these space_IDs until
+	 * execution of acpi_initialize_objects. This is done because we need the handlers
+	 * for the default spaces (mem/io/pci/table) to be installed before we can run
+	 * any control methods (or _REG methods). There is known BIOS code that depends
+	 * on this.
+	 *
+	 * For all other space_IDs, we can safely execute the _REG methods immediately.
+	 * This means that for IDs like embedded_controller, this function should be called
+	 * only after acpi_enable_subsystem has been called.
+	 */
+
+	sp = (region_obj->region.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY ||
+	      region_obj->region.space_id == ACPI_ADR_SPACE_SYSTEM_IO ||
+	      region_obj->region.space_id == ACPI_ADR_SPACE_PCI_CONFIG ||
+	      region_obj->region.space_id == ACPI_ADR_SPACE_DATA_TABLE);
 	if (region_obj2->extra.method_REG == NULL ||
 	    region_obj->region.handler == NULL ||
-	    !acpi_gbl_reg_methods_enabled) {
+	    (sp && !acpi_gbl_reg_methods_enabled)) {
 		return_ACPI_STATUS(AE_OK);
 	}
 
-- 
2.1.4

RE: [PATCH] ACPICA: Events: Execute some _REG methods in early boot

[Zheng, Lv]

Hi,

First of all, why don't you respond on the kernel bugzilla?
Posting a fix here directly without communication doesn't look like a constructive help but more like a destructive attack to me.

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> Subject: [PATCH] ACPICA: Events: Execute some _REG methods in early boot
> 
> The regression caused _REG methods to not be run in early boot for all
> space IDs, but a removed comment stated that _REG methods should be
> executed for IDs like embedded_controller. Before the regression this
> was the case:
> 
> [    0.230469] Executing  Method       \_SB.PCI0.LPCB.EC._REG
> [    0.230531] Initializing Region       \GNVS
> [    0.230607] Initializing Region       \_SB.PCI0.LPCB.EC.ECOR
> [    0.231043] Initializing Region       \_SB.PCI0.IGPU.IGDM
[Lv Zheng] 
As I said in the previous reply, this is the known issue and can be fixed by applying the whole series.
Especially this commit:
https://patchwork.kernel.org/patch/8241421/
That's why I asked you to test again by applying the whole series.
And that's why after having not seen your response for so long time, we prepared a test branch and was waiting for your response.

You need to post acpidump there to have the issue root caused so that more accurate fix can be generated.

> 
> After the regression the initialisation is not done and ODEBUG warnings
> are shown at boot and/or shutdown:
> 
> [    6.676570] WARNING: CPU: 0 PID: 3317 at lib/debugobjects.c:263
> debug_print_object+0x85/0xa0()
> [    6.676576] ODEBUG: assert_init not available (active state 0) object type:
> timer_list hint: stub_timer+0x0/0x20
> [    6.676578] Modules linked in:
> [    6.676582] CPU: 0 PID: 3317 Comm: ccpd Not tainted 4.5.0-rc6 #509
> [    6.676584] Hardware name: Apple Inc. MacBookPro10,2/Mac-
> AFD8A9D944EA4843, BIOS MBP102.88Z.0106.B0A.1509130955 09/13/2015
> [    6.676586]  0000000000000000 ffff880256543db0 ffffffff814802b5
> ffff880256543df8
> [    6.676590]  ffffffff81f40dbd ffff880256543de8 ffffffff810bd0ec
> ffff880256543e90
> [    6.676594]  ffffffff822532c0 ffffffff81f40e63 ffffffff83138c88
> 0000000001fdf040
> [    6.676598] Call Trace:
> [    6.676603]  [<ffffffff814802b5>] dump_stack+0x67/0x92
> [    6.676608]  [<ffffffff810bd0ec>] warn_slowpath_common+0x7c/0xb0
> [    6.676611]  [<ffffffff810bd167>] warn_slowpath_fmt+0x47/0x50
> [    6.676614]  [<ffffffff8149d7a5>] debug_print_object+0x85/0xa0
> [    6.676616]  [<ffffffff8111f440>] ? cascade+0x70/0x70
> [    6.676620]  [<ffffffff8149e398>] debug_object_assert_init+0xf8/0x130
> [    6.676624]  [<ffffffff811038cd>] ? trace_hardirqs_on+0xd/0x10
> [    6.676626]  [<ffffffff8111fcff>] del_timer+0x1f/0x70
> [    6.676631]  [<ffffffff811852f1>] laptop_sync_completion+0x61/0x100
> [    6.676633]  [<ffffffff81185290>] ? laptop_io_completion+0x30/0x30
> [    6.676637]  [<ffffffff81212c9f>] sys_sync+0x7f/0x90
> [    6.676641]  [<ffffffff81ad0d97>] entry_SYSCALL_64_fastpath+0x12/0x6f
> 
> Fixes: efaed9be998b ("ACPICA: Events: Enhance acpi_ev_execute_reg_method()
> to ensure no _REG evaluations can happen during OS early boot stages")
> Link: https://lkml.org/lkml/2016/2/3/273
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=112911
> Signed-off-by: Chris Bainbridge <chris.bainbridge@gmail.com>
> ---
>  drivers/acpi/acpica/evregion.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c
> index 47092b4d633c..d0b02ef0effa 100644
> --- a/drivers/acpi/acpica/evregion.c
> +++ b/drivers/acpi/acpica/evregion.c
> @@ -590,6 +590,7 @@ acpi_ev_execute_reg_method(union
> acpi_operand_object *region_obj, u32 function)
>  	union acpi_operand_object *args[3];
>  	union acpi_operand_object *region_obj2;
>  	acpi_status status;
> +	bool sp;
> 
>  	ACPI_FUNCTION_TRACE(ev_execute_reg_method);
> 
> @@ -598,9 +599,28 @@ acpi_ev_execute_reg_method(union
> acpi_operand_object *region_obj, u32 function)
>  		return_ACPI_STATUS(AE_NOT_EXIST);
>  	}
> 
> +	/*
> +	 * For the default space_IDs, (the IDs for which there are default region
> handlers
> +	 * installed) Only execute the _REG methods if the global initialization
> _REG
> +	 * methods have already been run (via acpi_initialize_objects). In other
> words,
> +	 * we will defer the execution of the _REG methods for these
> space_IDs until
> +	 * execution of acpi_initialize_objects. This is done because we need
> the handlers
> +	 * for the default spaces (mem/io/pci/table) to be installed before we
> can run
> +	 * any control methods (or _REG methods). There is known BIOS code
> that depends
> +	 * on this.
> +	 *
> +	 * For all other space_IDs, we can safely execute the _REG methods
> immediately.
> +	 * This means that for IDs like embedded_controller, this function
> should be called
> +	 * only after acpi_enable_subsystem has been called.
> +	 */
> +
> +	sp = (region_obj->region.space_id ==
> ACPI_ADR_SPACE_SYSTEM_MEMORY ||
> +	      region_obj->region.space_id == ACPI_ADR_SPACE_SYSTEM_IO ||
> +	      region_obj->region.space_id == ACPI_ADR_SPACE_PCI_CONFIG ||
> +	      region_obj->region.space_id == ACPI_ADR_SPACE_DATA_TABLE);
>  	if (region_obj2->extra.method_REG == NULL ||
>  	    region_obj->region.handler == NULL ||
> -	    !acpi_gbl_reg_methods_enabled) {
> +	    (sp && !acpi_gbl_reg_methods_enabled)) {
>  		return_ACPI_STATUS(AE_OK);
>  	}
> 
[Lv Zheng] 
The above fix looks hackish to me.
IMO, if you want to stop regressions that are triggered by this commit.
A simpler and proper way would be to move acpi_gbl_reg_method_enabled = TRUE to the end of acpi_load_tables().
So that when the order of table loading and ECDT probing is corrected, the correct logic can still apply.

I don't have ECDT platforms in hand to confirm.
Can you help to just give it a try?

Thanks and best regards
-Lv

> --
> 2.1.4
> 
> --
> 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

Re: [PATCH] ACPICA: Events: Execute some _REG methods in early boot

[Chris Bainbridge]

On Mon, Mar 07, 2016 at 06:36:05AM +0000, Zheng, Lv wrote:
> Hi,
> 
> First of all, why don't you respond on the kernel bugzilla?
> Posting a fix here directly without communication doesn't look like a constructive help but more like a destructive attack to me.

Sorry if it appears to be an attack. It wasn't meant that way. I was
under the impression that email is the preferred means of communication
for kernel development. I'm not sure whether other developers actively
monitor bugzilla reports (some do, but bug trackers are often graveyards
for bug reports and it's easy for communications to be missed).

> As I said in the previous reply, this is the known issue and can be fixed by applying the whole series.
> Especially this commit:
> https://patchwork.kernel.org/patch/8241421/
> That's why I asked you to test again by applying the whole series.
> And that's why after having not seen your response for so long time, we prepared a test branch and was waiting for your response.

I already replied 9 days ago: https://lkml.org/lkml/2016/2/27/164
The suggested patch did not fix the issue and the patch series did not
apply cleanly.

(btw I'm not paid for this work so I tend to handle it in batches when I
have spare time, which is why you may see replies delayed to weekends etc.)

> You need to post acpidump there to have the issue root caused so that more accurate fix can be generated.

I already sent an acpidump for this system to you and Robert (email 3rd Feb).

> The above fix looks hackish to me.
> IMO, if you want to stop regressions that are triggered by this commit.
> A simpler and proper way would be to move acpi_gbl_reg_method_enabled = TRUE to the end of acpi_load_tables().
> So that when the order of table loading and ECDT probing is corrected, the correct logic can still apply.
> 
> I don't have ECDT platforms in hand to confirm.
> Can you help to just give it a try?

Yes the fix may be hackish, but it served the purpose of correctly
identifying the bug. Your suggestion works fine, for reference the
tested patch was:


diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c
index 278666e39563..9136d7250022 100644
--- a/drivers/acpi/acpica/tbxfload.c
+++ b/drivers/acpi/acpica/tbxfload.c
@@ -83,6 +83,7 @@ acpi_status __init acpi_load_tables(void)
 				"While loading namespace from ACPI tables"));
 	}
 
+	acpi_gbl_reg_methods_enabled = TRUE;
 	return_ACPI_STATUS(status);
 }
 
diff --git a/drivers/acpi/acpica/utxfinit.c b/drivers/acpi/acpica/utxfinit.c
index 721b87cce908..2c0491038068 100644
--- a/drivers/acpi/acpica/utxfinit.c
+++ b/drivers/acpi/acpica/utxfinit.c
@@ -267,7 +267,6 @@ acpi_status __init acpi_initialize_objects(u32 flags)
 	 * initialized, even if they contain executable AML (see the call to
 	 * acpi_ns_initialize_objects below).
 	 */
-	acpi_gbl_reg_methods_enabled = TRUE;
 	if (!(flags & ACPI_NO_ADDRESS_SPACE_INIT)) {
 		ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
 				  "[Init] Executing _REG OpRegion methods\n"));


With this patch applied the ODEBUG errors do not occur.

Thanks.

RE: [PATCH] ACPICA: Events: Execute some _REG methods in early boot

[Zheng, Lv]

Hi, Chris

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> Subject: Re: [PATCH] ACPICA: Events: Execute some _REG methods in early
> boot
> 
> On Mon, Mar 07, 2016 at 06:36:05AM +0000, Zheng, Lv wrote:
> > Hi,
> >
> > First of all, why don't you respond on the kernel bugzilla?
> > Posting a fix here directly without communication doesn't look like a
> constructive help but more like a destructive attack to me.
> 
> Sorry if it appears to be an attack. It wasn't meant that way. I was
> under the impression that email is the preferred means of communication
> for kernel development. I'm not sure whether other developers actively
> monitor bugzilla reports (some do, but bug trackers are often graveyards
> for bug reports and it's easy for communications to be missed).
> 
[Lv Zheng] 
Apologies. It's my fault.
I didn't see your last reply.

> > As I said in the previous reply, this is the known issue and can be fixed by
> applying the whole series.
> > Especially this commit:
> > https://patchwork.kernel.org/patch/8241421/
> > That's why I asked you to test again by applying the whole series.
> > And that's why after having not seen your response for so long time, we
> prepared a test branch and was waiting for your response.
> 
> I already replied 9 days ago: https://lkml.org/lkml/2016/2/27/164
> The suggested patch did not fix the issue and the patch series did not
> apply cleanly.
> 
> (btw I'm not paid for this work so I tend to handle it in batches when I
> have spare time, which is why you may see replies delayed to weekends etc.)
> 
[Lv Zheng] 
The problem is the reply didn't arrive the proper mailbox here.

> > You need to post acpidump there to have the issue root caused so that more
> accurate fix can be generated.
> 
> I already sent an acpidump for this system to you and Robert (email 3rd Feb).
> 
> > The above fix looks hackish to me.
> > IMO, if you want to stop regressions that are triggered by this commit.
> > A simpler and proper way would be to move acpi_gbl_reg_method_enabled =
> TRUE to the end of acpi_load_tables().
> > So that when the order of table loading and ECDT probing is corrected, the
> correct logic can still apply.
> >
> > I don't have ECDT platforms in hand to confirm.
> > Can you help to just give it a try?
> 
> Yes the fix may be hackish, but it served the purpose of correctly
> identifying the bug. Your suggestion works fine, for reference the
> tested patch was:
> 
> 
> diff --git a/drivers/acpi/acpica/tbxfload.c b/drivers/acpi/acpica/tbxfload.c
> index 278666e39563..9136d7250022 100644
> --- a/drivers/acpi/acpica/tbxfload.c
> +++ b/drivers/acpi/acpica/tbxfload.c
> @@ -83,6 +83,7 @@ acpi_status __init acpi_load_tables(void)
>  				"While loading namespace from ACPI tables"));
>  	}
> 
> +	acpi_gbl_reg_methods_enabled = TRUE;
>  	return_ACPI_STATUS(status);
>  }
> 
> diff --git a/drivers/acpi/acpica/utxfinit.c b/drivers/acpi/acpica/utxfinit.c
> index 721b87cce908..2c0491038068 100644
> --- a/drivers/acpi/acpica/utxfinit.c
> +++ b/drivers/acpi/acpica/utxfinit.c
> @@ -267,7 +267,6 @@ acpi_status __init acpi_initialize_objects(u32 flags)
>  	 * initialized, even if they contain executable AML (see the call to
>  	 * acpi_ns_initialize_objects below).
>  	 */
> -	acpi_gbl_reg_methods_enabled = TRUE;
>  	if (!(flags & ACPI_NO_ADDRESS_SPACE_INIT)) {
>  		ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
>  				  "[Init] Executing _REG OpRegion
> methods\n"));
> 
> 
> With this patch applied the ODEBUG errors do not occur.
[Lv Zheng] 
Great!
This line doesn't differ too much in ACPICA upstream, while it matters much in Linux code base.

I should rebase the other patches along with this on top of ACPICA Feb release.
But as I don't have real ECDT platforms, I can just test the fixes by faking ECDT using initrd table override mechanism.
I think it's better to have you confirm the whole series again.
I saw you started to reply on the Bugzilla entry, I'll ping you there after rebasing the patchset.
> 
> Thanks.
[Lv Zheng] 
Thanks and best regards
-Lv