Skip to contentSkip to conversation

Possible mis-backport of 4abb951b in 4.19.35 ("ACPICA: AML interpreter: add region addresses...")

DISCUSSION Paul Gortmaker 5 participants 10 messages May 5, 2019 – May 17, 2019

Subsystems (3)
Stable branch ACPI ACPI component architecture (acpica)
lore
Share & tools
lore

Paul Gortmaker

author
#0 lore raw

I noticed 4.19.35 got a backport of mainline 4abb951b, but it appears to be a duplicate backport that landed in the wrong function. We can see this in the stable-queue repo:

stable-queue$ find . -name '*acpica-aml-interpreter-add-region-addr*' |grep 4.19 ./releases/4.19.6/acpica-aml-interpreter-add-region-addresses-in-global-list-during-initialization.patch ./releases/4.19.3/revert-acpica-aml-interpreter-add-region-addresses-in.patch ./releases/4.19.35/acpica-aml-interpreter-add-region-addresses-in-global-list-during-initialization.patch ./releases/4.19.2/acpica-aml-interpreter-add-region-addresses-in-global-list-during-initialization.patch

So it was added to 4.19.2, reverted in .3, re-added in .6, and then finally patched into a similar looking but wrong function in .35

If we diff the .6 and .35 versions, we see the function difference:

-@@ -417,6 +417,10 @@ acpi_ds_eval_region_operands(struct acpi +@@ -523,6 +523,10 @@ acpi_ds_eval_table_region_operands(struc

I don't know what the history is/was around the 2/3/6 churn, but the re-addition in 4.19.35 to a different function sure looks wrong.

The commit adds a call "status = acpi_ut_add_address_range(..." and if we check mainline, there is only one in that file, but in 4.19.35+ there now are two calls - since the two functions had similar context and comments, it isn't hard to see how patch could/would apply it a 2nd time in the wrong place.

I didn't check if any of the other currently maintained linux-stable versions also had this possible issue.

Paul.

maintainer
#1 lore raw
Quoted 21 lines · Paul Gortmaker

I noticed 4.19.35 got a backport of mainline 4abb951b, but it appears to be a duplicate backport that landed in the wrong function. We can see this in the stable-queue repo:

stable-queue$ find . -name '*acpica-aml-interpreter-add-region-addr*' |grep 4.19 ./releases/4.19.6/acpica-aml-interpreter-add-region-addresses-in-global-list-during-initialization.patch ./releases/4.19.3/revert-acpica-aml-interpreter-add-region-addresses-in.patch ./releases/4.19.35/acpica-aml-interpreter-add-region-addresses-in-global-list-during-initialization.patch ./releases/4.19.2/acpica-aml-interpreter-add-region-addresses-in-global-list-during-initialization.patch

So it was added to 4.19.2, reverted in .3, re-added in .6, and then finally patched into a similar looking but wrong function in .35

If we diff the .6 and .35 versions, we see the function difference:

-@@ -417,6 +417,10 @@ acpi_ds_eval_region_operands(struct acpi +@@ -523,6 +523,10 @@ acpi_ds_eval_table_region_operands(struc

I don't know what the history is/was around the 2/3/6 churn, but the re-addition in 4.19.35 to a different function sure looks wrong.

Quoted 9 lines

The commit adds a call "status = acpi_ut_add_address_range(..." and if we check mainline, there is only one in that file, but in 4.19.35+ there now are two calls - since the two functions had similar context and comments, it isn't hard to see how patch could/would apply it a 2nd time in the wrong place.

I didn't check if any of the other currently maintained linux-stable versions also had this possible issue.

Ugh, Rafael, did I mess this up again? Can you check to see if I need to fix this somehow?

thanks,

greg k-h

#2 lore raw
Quoted 11 lines

-----Original Message-----
From: Greg Kroah-Hartman [mailto:[email protected]]
Sent: Monday, May 6, 2019 1:42 AM
To: Paul Gortmaker <[email protected]>; Wysocki, Rafael J <[email protected]>
Cc: [email protected]; Schmauss, Erik <[email protected]>
Subject: Re: Possible mis-backport of 4abb951b in 4.19.35 ("ACPICA: AML
interpreter: add region addresses...")

Quoted 37 lines
Quoted 35 lines · Paul Gortmaker

I noticed 4.19.35 got a backport of mainline 4abb951b, but it appears to be a duplicate backport that landed in the wrong function. We can see this in the stable-queue repo:

stable-queue$ find . -name '*acpica-aml-interpreter-add-region-addr*' |grep 4.19 ./releases/4.19.6/acpica-aml-interpreter-add-region-addresses-in-globa l-list-during-initialization.patch ./releases/4.19.3/revert-acpica-aml-interpreter-add-region-addresses-i n.patch ./releases/4.19.35/acpica-aml-interpreter-add-region-addresses-in-glob al-list-during-initialization.patch ./releases/4.19.2/acpica-aml-interpreter-add-region-addresses-in-globa l-list-during-initialization.patch

So it was added to 4.19.2, reverted in .3, re-added in .6, and then finally patched into a similar looking but wrong function in .35

If we diff the .6 and .35 versions, we see the function difference:

-@@ -417,6 +417,10 @@ acpi_ds_eval_region_operands(struct acpi +@@ -523,6 +523,10 @@ acpi_ds_eval_table_region_operands(struc

I don't know what the history is/was around the 2/3/6 churn, but the re-addition in 4.19.35 to a different function sure looks wrong.

The commit adds a call "status = acpi_ut_add_address_range(..." and if we check mainline, there is only one in that file, but in 4.19.35+ there now are two calls - since the two functions had similar context and comments, it isn't hard to see how patch could/would apply it a 2nd time in the wrong place.

I didn't check if any of the other currently maintained linux-stable versions also had this possible issue.

Hi Greg,

Quoted 2 lines

Ugh, Rafael, did I mess this up again? Can you check to see if I need to fix this somehow?

It should be called in acpi_ds_eval_region_operands rather than acpi_ds_eval_table_region_operands. Please remove the call from acpi_ds_eval_table_region_operands.

Thanks,
Erik

Quoted 4 lines

thanks,

greg k-h

maintainer
#3 lore raw
Quoted 48 lines · Erik
Quoted 11 lines

-----Original Message-----
From: Greg Kroah-Hartman [mailto:[email protected]]
Sent: Monday, May 6, 2019 1:42 AM
To: Paul Gortmaker <[email protected]>; Wysocki, Rafael J <[email protected]>
Cc: [email protected]; Schmauss, Erik <[email protected]>
Subject: Re: Possible mis-backport of 4abb951b in 4.19.35 ("ACPICA: AML
interpreter: add region addresses...")

Quoted 37 lines
Quoted 35 lines · Paul Gortmaker

I noticed 4.19.35 got a backport of mainline 4abb951b, but it appears to be a duplicate backport that landed in the wrong function. We can see this in the stable-queue repo:

stable-queue$ find . -name '*acpica-aml-interpreter-add-region-addr*' |grep 4.19 ./releases/4.19.6/acpica-aml-interpreter-add-region-addresses-in-globa l-list-during-initialization.patch ./releases/4.19.3/revert-acpica-aml-interpreter-add-region-addresses-i n.patch ./releases/4.19.35/acpica-aml-interpreter-add-region-addresses-in-glob al-list-during-initialization.patch ./releases/4.19.2/acpica-aml-interpreter-add-region-addresses-in-globa l-list-during-initialization.patch

So it was added to 4.19.2, reverted in .3, re-added in .6, and then finally patched into a similar looking but wrong function in .35

If we diff the .6 and .35 versions, we see the function difference:

-@@ -417,6 +417,10 @@ acpi_ds_eval_region_operands(struct acpi +@@ -523,6 +523,10 @@ acpi_ds_eval_table_region_operands(struc

I don't know what the history is/was around the 2/3/6 churn, but the re-addition in 4.19.35 to a different function sure looks wrong.

The commit adds a call "status = acpi_ut_add_address_range(..." and if we check mainline, there is only one in that file, but in 4.19.35+ there now are two calls - since the two functions had similar context and comments, it isn't hard to see how patch could/would apply it a 2nd time in the wrong place.

I didn't check if any of the other currently maintained linux-stable versions also had this possible issue.

Quoted 7 lines

Hi Greg,

Quoted 2 lines

Ugh, Rafael, did I mess this up again? Can you check to see if I need to fix this somehow?

It should be called in acpi_ds_eval_region_operands rather than acpi_ds_eval_table_region_operands. Please remove the call from acpi_ds_eval_table_region_operands.

Great, can someone please send me a patch for this so that I don't get it wrong myself?

thanks,

greg k-h

#4 lore raw
Quoted 47 lines · Greg Kroah-Hartman
Quoted 46 lines · Erik
Quoted 46 lines

-----Original Message-----
From: Greg Kroah-Hartman [mailto:[email protected]]
Sent: Monday, May 6, 2019 1:42 AM
To: Paul Gortmaker <[email protected]>; Wysocki, Rafael J <[email protected]>
Cc: [email protected]; Schmauss, Erik <[email protected]>
Subject: Re: Possible mis-backport of 4abb951b in 4.19.35 ("ACPICA: AML
interpreter: add region addresses...")

Quoted 35 lines · Paul Gortmaker

I noticed 4.19.35 got a backport of mainline 4abb951b, but it appears to be a duplicate backport that landed in the wrong function. We can see this in the stable-queue repo:

stable-queue$ find . -name '*acpica-aml-interpreter-add-region-addr*' |grep 4.19 ./releases/4.19.6/acpica-aml-interpreter-add-region-addresses-in-globa l-list-during-initialization.patch ./releases/4.19.3/revert-acpica-aml-interpreter-add-region-addresses-i n.patch ./releases/4.19.35/acpica-aml-interpreter-add-region-addresses-in-glob al-list-during-initialization.patch ./releases/4.19.2/acpica-aml-interpreter-add-region-addresses-in-globa l-list-during-initialization.patch

So it was added to 4.19.2, reverted in .3, re-added in .6, and then finally patched into a similar looking but wrong function in .35

If we diff the .6 and .35 versions, we see the function difference:

-@@ -417,6 +417,10 @@ acpi_ds_eval_region_operands(struct acpi +@@ -523,6 +523,10 @@ acpi_ds_eval_table_region_operands(struc

I don't know what the history is/was around the 2/3/6 churn, but the re-addition in 4.19.35 to a different function sure looks wrong.

The commit adds a call "status = acpi_ut_add_address_range(..." and if we check mainline, there is only one in that file, but in 4.19.35+ there now are two calls - since the two functions had similar context and comments, it isn't hard to see how patch could/would apply it a 2nd time in the wrong place.

I didn't check if any of the other currently maintained linux-stable versions also had this possible issue.

Quoted 8 lines
Quoted 6 lines

Hi Greg,

Quoted 2 lines

Ugh, Rafael, did I mess this up again? Can you check to see if I need to fix this somehow?

It should be called in acpi_ds_eval_region_operands rather than acpi_ds_eval_table_region_operands. Please remove the call from acpi_ds_eval_table_region_operands.

Great, can someone please send me a patch for this so that I don't get it wrong myself?

Erik, can you please cut a patch for that against 4.19.35 and send it to Greg?

#5 lore raw
Quoted 67 lines

-----Original Message-----
From: Wysocki, Rafael J
Sent: Wednesday, May 15, 2019 1:57 PM
To: Schmauss, Erik <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>; Paul Gortmaker <[email protected]>; [email protected]
Subject: Re: Possible mis-backport of 4abb951b in 4.19.35 ("ACPICA: AML
interpreter: add region addresses...")

Quoted 55 lines · Greg Kroah-Hartman
Quoted 54 lines · Erik
Quoted 11 lines

-----Original Message-----
From: Greg Kroah-Hartman [mailto:[email protected]]
Sent: Monday, May 6, 2019 1:42 AM
To: Paul Gortmaker <[email protected]>; Wysocki, Rafael J <[email protected]>
Cc: [email protected]; Schmauss, Erik <[email protected]>
Subject: Re: Possible mis-backport of 4abb951b in 4.19.35 ("ACPICA:
AML
interpreter: add region addresses...")

Quoted 39 lines
Quoted 38 lines · Paul Gortmaker

I noticed 4.19.35 got a backport of mainline 4abb951b, but it appears to be a duplicate backport that landed in the wrong function. We can see this in the stable-queue repo:

stable-queue$ find . -name '*acpica-aml-interpreter-add-region-addr*' |grep 4.19 ./releases/4.19.6/acpica-aml-interpreter-add-region-addresses-in-gl oba
l-list-during-initialization.patch ./releases/4.19.3/revert-acpica-aml-interpreter-add-region-addresse s-i
n.patch ./releases/4.19.35/acpica-aml-interpreter-add-region-addresses-in-g lob al-list-during-initialization.patch ./releases/4.19.2/acpica-aml-interpreter-add-region-addresses-in-gl oba
l-list-during-initialization.patch

So it was added to 4.19.2, reverted in .3, re-added in .6, and then finally patched into a similar looking but wrong function in .35

If we diff the .6 and .35 versions, we see the function difference:

-@@ -417,6 +417,10 @@ acpi_ds_eval_region_operands(struct acpi +@@ -523,6 +523,10 @@ acpi_ds_eval_table_region_operands(struc

I don't know what the history is/was around the 2/3/6 churn, but the re-addition in 4.19.35 to a different function sure looks wrong.

The commit adds a call "status = acpi_ut_add_address_range(..." and if we check mainline, there is only one in that file, but in 4.19.35+ there now are two calls - since the two functions had similar context and comments, it isn't hard to see how patch could/would apply it a 2nd time in the wrong place.

I didn't check if any of the other currently maintained linux-stable versions also had this possible issue.

Hi Greg,

Quoted 2 lines

Ugh, Rafael, did I mess this up again? Can you check to see if I need to fix this somehow?

Quoted 8 lines
Quoted 1 line
Quoted 1 line

It should be called in acpi_ds_eval_region_operands rather than

acpi_ds_eval_table_region_operands.

Quoted 3 lines
Quoted 1 line

Please remove the call from acpi_ds_eval_table_region_operands.

Great, can someone please send me a patch for this so that I don't get it wrong myself?

Erik, can you please cut a patch for that against 4.19.35 and send it to Greg?

I'm not sure what the process is for this case but here's the patch... Let me know if you need me to send it some other way...

From a738f1c452c0762d3c0a1b1a9a12c78bd97b0a23 Mon Sep 17 00:00:00 2001
From: Erik Schmauss <[email protected]>
Date: Wed, 15 May 2019 17:25:31 -0700
Subject: [PATCH] Revert "ACPICA: AML interpreter: add region addresses in
global list during initialization"

This reverts commit f8053df634d40c733f26ca49c2c3835002e61b77 that was unintentionally included as a part of the stable branch.

Reported-by Paul Gortmaker <[email protected]>
Signed-off-by Erik Schmauss <[email protected]>
---
 drivers/acpi/acpica/dsopcode.c | 4 ----
 1 file changed, 4 deletions(-)
drivers/acpi/acpica/dsopcode.c +0 -4
index 2f4641e5ecde..78f9de260d5f 100644
@@ -523,10 +523,6 @@ acpi_ds_eval_table_region_operands(struct acpi_walk_state *walk_state,
                          ACPI_FORMAT_UINT64(obj_desc->region.address),
                          obj_desc->region.length));
 
-       status = acpi_ut_add_address_range(obj_desc->region.space_id,
-                                          obj_desc->region.address,
-                                          obj_desc->region.length, node);
-
        /* Now the address and length are valid for this opregion */
 
        obj_desc->region.flags |= AOPOBJ_DATA_VALID;

--
2.17.2

#6 lore raw
Quoted 106 lines · Erik
Quoted 63 lines

-----Original Message-----
From: Wysocki, Rafael J
Sent: Wednesday, May 15, 2019 1:57 PM
To: Schmauss, Erik <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>; Paul Gortmaker <[email protected]>; [email protected]
Subject: Re: Possible mis-backport of 4abb951b in 4.19.35 ("ACPICA: AML
interpreter: add region addresses...")

Quoted 52 lines · Greg Kroah-Hartman
Quoted 51 lines · Erik
Quoted 10 lines

-----Original Message-----
From: Greg Kroah-Hartman [mailto:[email protected]]
Sent: Monday, May 6, 2019 1:42 AM
To: Paul Gortmaker <[email protected]>; Wysocki, Rafael J <[email protected]>
Cc: [email protected]; Schmauss, Erik <[email protected]>
Subject: Re: Possible mis-backport of 4abb951b in 4.19.35 ("ACPICA:
AML
interpreter: add region addresses...")

Quoted 39 lines
Quoted 38 lines · Paul Gortmaker

I noticed 4.19.35 got a backport of mainline 4abb951b, but it appears to be a duplicate backport that landed in the wrong function. We can see this in the stable-queue repo:

stable-queue$ find . -name '*acpica-aml-interpreter-add-region-addr*' |grep 4.19 ./releases/4.19.6/acpica-aml-interpreter-add-region-addresses-in-gl oba
l-list-during-initialization.patch ./releases/4.19.3/revert-acpica-aml-interpreter-add-region-addresse s-i
n.patch ./releases/4.19.35/acpica-aml-interpreter-add-region-addresses-in-g lob al-list-during-initialization.patch ./releases/4.19.2/acpica-aml-interpreter-add-region-addresses-in-gl oba
l-list-during-initialization.patch

So it was added to 4.19.2, reverted in .3, re-added in .6, and then finally patched into a similar looking but wrong function in .35

If we diff the .6 and .35 versions, we see the function difference:

-@@ -417,6 +417,10 @@ acpi_ds_eval_region_operands(struct acpi +@@ -523,6 +523,10 @@ acpi_ds_eval_table_region_operands(struc

I don't know what the history is/was around the 2/3/6 churn, but the re-addition in 4.19.35 to a different function sure looks wrong.

The commit adds a call "status = acpi_ut_add_address_range(..." and if we check mainline, there is only one in that file, but in 4.19.35+ there now are two calls - since the two functions had similar context and comments, it isn't hard to see how patch could/would apply it a 2nd time in the wrong place.

I didn't check if any of the other currently maintained linux-stable versions also had this possible issue.

Hi Greg,

Quoted 9 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines

Ugh, Rafael, did I mess this up again? Can you check to see if I need to fix this somehow?

It should be called in acpi_ds_eval_region_operands rather than

acpi_ds_eval_table_region_operands.

Quoted 3 lines
Quoted 1 line

Please remove the call from acpi_ds_eval_table_region_operands.

Great, can someone please send me a patch for this so that I don't get it wrong myself?

Erik, can you please cut a patch for that against 4.19.35 and send it to Greg?

I'm not sure what the process is for this case but here's the patch... Let me know if you need me to send it some other way...

From a738f1c452c0762d3c0a1b1a9a12c78bd97b0a23 Mon Sep 17 00:00:00 2001
From: Erik Schmauss <[email protected]>
Date: Wed, 15 May 2019 17:25:31 -0700
Subject: [PATCH] Revert "ACPICA: AML interpreter: add region addresses in
global list during initialization"

This reverts commit f8053df634d40c733f26ca49c2c3835002e61b77 that was unintentionally included as a part of the stable branch.

Reported-by Paul Gortmaker <[email protected]>
Signed-off-by Erik Schmauss <[email protected]>
---
  drivers/acpi/acpica/dsopcode.c | 4 ----
  1 file changed, 4 deletions(-)
drivers/acpi/acpica/dsopcode.c +0 -5
index 2f4641e5ecde..78f9de260d5f 100644
@@ -523,10 +523,6 @@ acpi_ds_eval_table_region_operands(struct acpi_walk_state *walk_state,
                           ACPI_FORMAT_UINT64(obj_desc->region.address),
                           obj_desc->region.length));
 
-       status = acpi_ut_add_address_range(obj_desc->region.space_id,
-                                          obj_desc->region.address,
-                                          obj_desc->region.length, node);
-
         /* Now the address and length are valid for this opregion */
 
         obj_desc->region.flags |= AOPOBJ_DATA_VALID;
--
Quoted 1 line

2.17.2

Thanks Erik, LGTM.

#7 lore raw
Quoted 131 lines

-----Original Message-----
From: Wysocki, Rafael J
Sent: Thursday, May 16, 2019 8:36 AM
To: Schmauss, Erik <[email protected]>; Greg Kroah-Hartman <[email protected]>
Cc: Paul Gortmaker <[email protected]>; [email protected]
Subject: Re: Possible mis-backport of 4abb951b in 4.19.35 ("ACPICA: AML
interpreter: add region addresses...")

Quoted 11 lines · Erik
Quoted 11 lines

-----Original Message-----
From: Wysocki, Rafael J
Sent: Wednesday, May 15, 2019 1:57 PM
To: Schmauss, Erik <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>; Paul Gortmaker <[email protected]>; [email protected]
Subject: Re: Possible mis-backport of 4abb951b in 4.19.35 ("ACPICA:
AML
interpreter: add region addresses...")

Quoted 6 lines
Quoted 6 lines · Greg Kroah-Hartman
Quoted 5 lines · Erik
Quoted 4 lines
Quoted 4 lines

-----Original Message-----
From: Greg Kroah-Hartman [mailto:[email protected]]
Sent: Monday, May 6, 2019 1:42 AM
To: Paul Gortmaker <[email protected]>; Wysocki,

Rafael

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines

J <[email protected]>
Cc: [email protected]; Schmauss, Erik <[email protected]>
Subject: Re: Possible mis-backport of 4abb951b in 4.19.35 ("ACPICA:
AML
interpreter: add region addresses...")

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines · Paul Gortmaker
Quoted 5 lines

I noticed 4.19.35 got a backport of mainline 4abb951b, but it appears to be a duplicate backport that landed in the wrong function. We can see this in the stable-queue repo:

stable-queue$ find . -name '*acpica-aml-interpreter-add-region-

addr*'

Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines

|grep 4.19 ./releases/4.19.6/acpica-aml-interpreter-add-region-addresses-in- gl
oba
l-list-during-initialization.patch ./releases/4.19.3/revert-acpica-aml-interpreter-add-region-addres se
s-i
n.patch ./releases/4.19.35/acpica-aml-interpreter-add-region-addresses-in -g lob al-list-during-initialization.patch ./releases/4.19.2/acpica-aml-interpreter-add-region-addresses-in- gl
oba
l-list-during-initialization.patch

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

So it was added to 4.19.2, reverted in .3, re-added in .6, and then finally patched into a similar looking but wrong function in .35

If we diff the .6 and .35 versions, we see the function difference:

-@@ -417,6 +417,10 @@ acpi_ds_eval_region_operands(struct acpi +@@ -523,6 +523,10 @@

acpi_ds_eval_table_region_operands(struc

Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 10 lines
Quoted 10 lines

I don't know what the history is/was around the 2/3/6 churn, but the re-addition in 4.19.35 to a different function sure looks wrong.

The commit adds a call "status = acpi_ut_add_address_range(..." and if we check mainline, there is only one in that file, but in 4.19.35+ there now are two calls - since the two functions had similar context and comments, it isn't hard to see how patch could/would apply it a 2nd time in the wrong place.

Quoted 3 lines
Quoted 3 lines

I didn't check if any of the other currently maintained linux-stable versions also had this possible issue.

Hi Greg,

Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines

Ugh, Rafael, did I mess this up again? Can you check to see if I need to fix this somehow?

It should be called in acpi_ds_eval_region_operands rather than

acpi_ds_eval_table_region_operands.

Quoted 3 lines
Quoted 1 line

Please remove the call from acpi_ds_eval_table_region_operands.

Great, can someone please send me a patch for this so that I don't get it wrong myself?

Erik, can you please cut a patch for that against 4.19.35 and send it to

Greg?

Quoted 9 lines
Quoted 1 line

I'm not sure what the process is for this case but here's the patch... Let me know if you need me to send it some other way...

From a738f1c452c0762d3c0a1b1a9a12c78bd97b0a23 Mon Sep 17 00:00:00
2001
From: Erik Schmauss <[email protected]>
Date: Wed, 15 May 2019 17:25:31 -0700
Subject: [PATCH] Revert "ACPICA: AML interpreter: add region addresses

in

drivers/acpi/acpica/dsopcode.c acpi_ds_eval_table_region_operands(struct

Quoted 17 lines

global list during initialization"

This reverts commit f8053df634d40c733f26ca49c2c3835002e61b77 that was unintentionally included as a part of the stable branch.

Reported-by Paul Gortmaker <[email protected]>
Signed-off-by Erik Schmauss <[email protected]>
---
  drivers/acpi/acpica/dsopcode.c | 4 ----
  1 file changed, 4 deletions(-)
(quoted diff) +0 -0

b/drivers/acpi/acpica/dsopcode.c index 2f4641e5ecde..78f9de260d5f 100644

drivers/acpi/acpica/dsopcode.c +0 -0
@@ -523,10 +523,6 @@ acpi_ds_eval_table_region_operands(struct

acpi_walk_state *walk_state,

Quoted 9 lines
drivers/acpi/acpica/dsopcode.c +0 -4
                           ACPI_FORMAT_UINT64(obj_desc->region.address),
                           obj_desc->region.length));
 
-       status = acpi_ut_add_address_range(obj_desc->region.space_id,
-                                          obj_desc->region.address,
-                                          obj_desc->region.length, node);
-
         /* Now the address and length are valid for this opregion */
Quoted 6 lines
Quoted 3 lines
drivers/acpi/acpica/dsopcode.c +0 -1
         obj_desc->region.flags |= AOPOBJ_DATA_VALID;
--

2.17.2

Thanks Erik, LGTM.

Thanks for your patience with these patches!

Erik

maintainer
#8 lore raw
In reply to Schmauss, Erik
Quoted 79 lines · Erik
Quoted 67 lines

-----Original Message-----
From: Wysocki, Rafael J
Sent: Wednesday, May 15, 2019 1:57 PM
To: Schmauss, Erik <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>; Paul Gortmaker <[email protected]>; [email protected]
Subject: Re: Possible mis-backport of 4abb951b in 4.19.35 ("ACPICA: AML
interpreter: add region addresses...")

Quoted 55 lines · Greg Kroah-Hartman
Quoted 54 lines · Erik
Quoted 11 lines

-----Original Message-----
From: Greg Kroah-Hartman [mailto:[email protected]]
Sent: Monday, May 6, 2019 1:42 AM
To: Paul Gortmaker <[email protected]>; Wysocki, Rafael J <[email protected]>
Cc: [email protected]; Schmauss, Erik <[email protected]>
Subject: Re: Possible mis-backport of 4abb951b in 4.19.35 ("ACPICA:
AML
interpreter: add region addresses...")

Quoted 39 lines
Quoted 38 lines · Paul Gortmaker

I noticed 4.19.35 got a backport of mainline 4abb951b, but it appears to be a duplicate backport that landed in the wrong function. We can see this in the stable-queue repo:

stable-queue$ find . -name '*acpica-aml-interpreter-add-region-addr*' |grep 4.19 ./releases/4.19.6/acpica-aml-interpreter-add-region-addresses-in-gl oba
l-list-during-initialization.patch ./releases/4.19.3/revert-acpica-aml-interpreter-add-region-addresse s-i
n.patch ./releases/4.19.35/acpica-aml-interpreter-add-region-addresses-in-g lob al-list-during-initialization.patch ./releases/4.19.2/acpica-aml-interpreter-add-region-addresses-in-gl oba
l-list-during-initialization.patch

So it was added to 4.19.2, reverted in .3, re-added in .6, and then finally patched into a similar looking but wrong function in .35

If we diff the .6 and .35 versions, we see the function difference:

-@@ -417,6 +417,10 @@ acpi_ds_eval_region_operands(struct acpi +@@ -523,6 +523,10 @@ acpi_ds_eval_table_region_operands(struc

I don't know what the history is/was around the 2/3/6 churn, but the re-addition in 4.19.35 to a different function sure looks wrong.

The commit adds a call "status = acpi_ut_add_address_range(..." and if we check mainline, there is only one in that file, but in 4.19.35+ there now are two calls - since the two functions had similar context and comments, it isn't hard to see how patch could/would apply it a 2nd time in the wrong place.

I didn't check if any of the other currently maintained linux-stable versions also had this possible issue.

Hi Greg,

Quoted 2 lines

Ugh, Rafael, did I mess this up again? Can you check to see if I need to fix this somehow?

Quoted 8 lines
Quoted 1 line
Quoted 1 line

It should be called in acpi_ds_eval_region_operands rather than

acpi_ds_eval_table_region_operands.

Quoted 3 lines
Quoted 1 line

Please remove the call from acpi_ds_eval_table_region_operands.

Great, can someone please send me a patch for this so that I don't get it wrong myself?

Erik, can you please cut a patch for that against 4.19.35 and send it to Greg?

I'm not sure what the process is for this case but here's the patch... Let me know if you need me to send it some other way...

Quoted 8 lines

From a738f1c452c0762d3c0a1b1a9a12c78bd97b0a23 Mon Sep 17 00:00:00 2001
From: Erik Schmauss <[email protected]>
Date: Wed, 15 May 2019 17:25:31 -0700
Subject: [PATCH] Revert "ACPICA: AML interpreter: add region addresses in global list during initialization"

This reverts commit f8053df634d40c733f26ca49c2c3835002e61b77 that was unintentionally included as a part of the stable branch.

So to clarify, we're now just going to revert it? It's not needed in -stable?

--
Thanks,
Sasha

#9 lore raw
Quoted 108 lines

-----Original Message-----
From: Sasha Levin [mailto:[email protected]]
Sent: Thursday, May 16, 2019 5:52 PM
To: Schmauss, Erik <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>; Paul Gortmaker <[email protected]>; [email protected]; Wysocki, Rafael J <[email protected]>
Subject: Re: Possible mis-backport of 4abb951b in 4.19.35 ("ACPICA: AML
interpreter: add region addresses...")

Quoted 26 lines · Erik
Quoted 26 lines

-----Original Message-----
From: Wysocki, Rafael J
Sent: Wednesday, May 15, 2019 1:57 PM
To: Schmauss, Erik <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>; Paul Gortmaker <[email protected]>; [email protected]
Subject: Re: Possible mis-backport of 4abb951b in 4.19.35 ("ACPICA:
AML
interpreter: add region addresses...")

Quoted 13 lines · Greg Kroah-Hartman
Quoted 12 lines · Erik
Quoted 12 lines

-----Original Message-----
From: Greg Kroah-Hartman [mailto:[email protected]]
Sent: Monday, May 6, 2019 1:42 AM
To: Paul Gortmaker <[email protected]>; Wysocki, Rafael J <[email protected]>
Cc: [email protected]; Schmauss, Erik <[email protected]>
Subject: Re: Possible mis-backport of 4abb951b in 4.19.35 ("ACPICA:
AML
interpreter: add region addresses...")

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines · Paul Gortmaker
Quoted 5 lines

I noticed 4.19.35 got a backport of mainline 4abb951b, but it appears to be a duplicate backport that landed in the wrong function. We can see this in the stable-queue repo:

stable-queue$ find . -name '*acpica-aml-interpreter-add-region-

addr*'

Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines
Quoted 16 lines

|grep 4.19 ./releases/4.19.6/acpica-aml-interpreter-add-region-addresses-in -gl
oba
l-list-during-initialization.patch ./releases/4.19.3/revert-acpica-aml-interpreter-add-region-addre sse
s-i
n.patch ./releases/4.19.35/acpica-aml-interpreter-add-region-addresses-i n-g lob al-list-during-initialization.patch ./releases/4.19.2/acpica-aml-interpreter-add-region-addresses-in -gl
oba
l-list-during-initialization.patch

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

So it was added to 4.19.2, reverted in .3, re-added in .6, and then finally patched into a similar looking but wrong function in .35

If we diff the .6 and .35 versions, we see the function difference:

-@@ -417,6 +417,10 @@ acpi_ds_eval_region_operands(struct acpi +@@ -523,6 +523,10 @@

acpi_ds_eval_table_region_operands(struc

Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
Quoted 17 lines
Quoted 10 lines
Quoted 10 lines

I don't know what the history is/was around the 2/3/6 churn, but the re-addition in 4.19.35 to a different function sure looks wrong.

The commit adds a call "status = acpi_ut_add_address_range(..." and if we check mainline, there is only one in that file, but in 4.19.35+ there now are two calls - since the two functions had similar context and comments, it isn't hard to see how patch could/would apply it a 2nd time in the wrong place.

Quoted 3 lines
Quoted 3 lines

I didn't check if any of the other currently maintained linux-stable versions also had this possible issue.

Hi Greg,

Quoted 2 lines

Ugh, Rafael, did I mess this up again? Can you check to see if I need to fix this somehow?

Quoted 7 lines
Quoted 7 lines
Quoted 1 line
Quoted 1 line

It should be called in acpi_ds_eval_region_operands rather than

acpi_ds_eval_table_region_operands.

Quoted 3 lines
Quoted 1 line

Please remove the call from acpi_ds_eval_table_region_operands.

Great, can someone please send me a patch for this so that I don't get it wrong myself?

Erik, can you please cut a patch for that against 4.19.35 and send it to

Greg?

Quoted 6 lines
Quoted 1 line

I'm not sure what the process is for this case but here's the patch... Let me know if you need me to send it some other way...

From a738f1c452c0762d3c0a1b1a9a12c78bd97b0a23 Mon Sep 17 00:00:00

2001

Quoted 5 lines

From: Erik Schmauss <[email protected]>
Date: Wed, 15 May 2019 17:25:31 -0700
Subject: [PATCH] Revert "ACPICA: AML interpreter: add region addresses in global list during initialization"

Quoted 4 lines
Quoted 2 lines

This reverts commit f8053df634d40c733f26ca49c2c3835002e61b77 that was unintentionally included as a part of the stable branch.

So to clarify, we're now just going to revert it? It's not needed in -stable?

Yes, we're going to revert a commit that got added by accident to the 4.19.y branch.
It may not apply to all stable branches because the reverted commit might not exist in all stable branches.

I hope this helps,

Erik

Quoted 4 lines

--
Thanks,
Sasha