Change in coreboot[master]: superio/common/ssdt: Make disabled PNP devices ACPI compliant

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37692 )

Change subject: superio/common/ssdt: Make disabled PNP devices ACPI compliant
......................................................................

superio/common/ssdt: Make disabled PNP devices ACPI compliant

Always write a _HID, even for disabled PNP devices.

Fixes a BSOD on Windows 10.

Change-Id: I419a08bd6a3570fb4e1ae31bef4f9ccd6836fe1b
Signed-off-by: Patrick Rudolph 
---
M src/superio/common/ssdt.c
1 file changed, 3 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/37692/1

diff --git a/src/superio/common/ssdt.c b/src/superio/common/ssdt.c
index a919aa5..563178c 100644
--- a/src/superio/common/ssdt.c
+++ b/src/superio/common/ssdt.c
@@ -203,6 +203,9 @@
    acpigen_write_STA(dev->enabled ? 0xf : 0);

if (!dev->enabled) {
+		/* To be ACPI compliant, always write a _HID */
+		acpigen_write_name_string("_HID", ACPI_HID_PNP);
+
    	acpigen_pop_len(); /* Device */
    	acpigen_pop_len(); /* Scope */
    	return;

[Christian Walter (Code Review)]

Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37692 )

Change subject: superio/common/ssdt: Make disabled PNP devices ACPI compliant
......................................................................

Patch Set 1: Code-Review+1

[Matt DeVillier (Code Review)]

Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37692 )

Change subject: superio/common/ssdt: Make disabled PNP devices ACPI compliant
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37692/2/src/superio/common/ssdt.c 
File src/superio/common/ssdt.c:

https://review.coreboot.org/c/coreboot/+/37692/2/src/superio/common/ssdt.c@2... 
PS2, Line 206: /* To be ACPI compliant, always write a _HID */
             : 		acpigen_write_name_string("_HID", ACPI_HID_PNP);
why not just move the block from lines 228-243 above line 205?

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37692 )

Change subject: superio/common/ssdt: Make disabled PNP devices ACPI compliant
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37692/5/src/superio/common/ssdt.c 
File src/superio/common/ssdt.c:

https://review.coreboot.org/c/coreboot/+/37692/5/src/superio/common/ssdt.c@2... 
PS5, Line 207: 		/* To be ACPI compliant, always write a _HID */
Can you add the section from the specification?

[Patrick Rudolph (Code Review)]

Hello Stef van Os, Felix Held, Christian Walter, [e-mail], build bot (Jenkins), Guido Beyer @ Prodrive Technologies, Justin van Son,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/37692

to look at the new patch set (#6).

Change subject: superio/common/ssdt: Make disabled PNP devices ACPI compliant
......................................................................

superio/common/ssdt: Make disabled PNP devices ACPI compliant

Always write a _HID, even for disabled PNP devices.

Fixes a BSOD on Windows 10.

Change-Id: I419a08bd6a3570fb4e1ae31bef4f9ccd6836fe1b
Signed-off-by: Patrick Rudolph 
---
M src/superio/common/ssdt.c
1 file changed, 22 insertions(+), 22 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/37692/6

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37692 )

Change subject: superio/common/ssdt: Make disabled PNP devices ACPI compliant
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/coreboot/+/37692/2/src/superio/common/ssdt.c 
File src/superio/common/ssdt.c:

https://review.coreboot.org/c/coreboot/+/37692/2/src/superio/common/ssdt.c@2... 
PS2, Line 206: /* To be ACPI compliant, always write a _HID */
             : 		acpigen_write_name_string("_HID", ACPI_HID_PNP);
> why not just move the block from lines 228-243 above line 205?

Done

https://review.coreboot.org/c/coreboot/+/37692/5/src/superio/common/ssdt.c 
File src/superio/common/ssdt.c:

https://review.coreboot.org/c/coreboot/+/37692/5/src/superio/common/ssdt.c@2... 
PS5, Line 207: 		/* To be ACPI compliant, always write a _HID */
> Can you add the section from the specification?

Done

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37692 )

Change subject: superio/common/ssdt: Make disabled PNP devices ACPI compliant
......................................................................

Patch Set 6: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/37692/6/src/superio/common/ssdt.c 
File src/superio/common/ssdt.c:

https://review.coreboot.org/c/coreboot/+/37692/6/src/superio/common/ssdt.c@2... 
PS6, Line 206: To be 
Remove

[Patrick Rudolph (Code Review)]

Hello Stef van Os, Felix Held, Paul Menzel, Christian Walter, [e-mail], build bot (Jenkins), Guido Beyer @ Prodrive Technologies, Justin van Son,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/37692

to look at the new patch set (#7).

Change subject: superio/common/ssdt: Make disabled PNP devices ACPI compliant
......................................................................

superio/common/ssdt: Make disabled PNP devices ACPI compliant

Always write a _HID, even for disabled PNP devices.

Fixes a BSOD on Windows 10.

Change-Id: I419a08bd6a3570fb4e1ae31bef4f9ccd6836fe1b
Signed-off-by: Patrick Rudolph 
---
M src/superio/common/ssdt.c
1 file changed, 22 insertions(+), 22 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/37692/7

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37692 )

Change subject: superio/common/ssdt: Make disabled PNP devices ACPI compliant
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/37692/6/src/superio/common/ssdt.c 
File src/superio/common/ssdt.c:

https://review.coreboot.org/c/coreboot/+/37692/6/src/superio/common/ssdt.c@2... 
PS6, Line 206: To be
> Remove

Done

[Philipp Deppenwiese (Code Review)]

Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37692 )

Change subject: superio/common/ssdt: Make disabled PNP devices ACPI compliant
......................................................................

Patch Set 7: Code-Review+2

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37692 )

Change subject: superio/common/ssdt: Make disabled PNP devices ACPI compliant
......................................................................

superio/common/ssdt: Make disabled PNP devices ACPI compliant

Always write a _HID, even for disabled PNP devices.

Fixes a BSOD on Windows 10.

Change-Id: I419a08bd6a3570fb4e1ae31bef4f9ccd6836fe1b
Signed-off-by: Patrick Rudolph 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/37692
Tested-by: build bot (Jenkins) 
Reviewed-by: Philipp Deppenwiese 
---
M src/superio/common/ssdt.c
1 file changed, 22 insertions(+), 22 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Philipp Deppenwiese: Looks good to me, approved

diff --git a/src/superio/common/ssdt.c b/src/superio/common/ssdt.c
index bc5d394..41cafdf 100644
--- a/src/superio/common/ssdt.c
+++ b/src/superio/common/ssdt.c
@@ -219,28 +219,11 @@
    }
    acpigen_pop_len(); /* Method */

-	if (!dev->enabled) {
-		acpigen_pop_len(); /* Device */
-		acpigen_pop_len(); /* Scope */
-		return;
-	}
-
-	if (has_resources(dev)) {
-		/* Resources - _CRS */
-		acpigen_write_name("_CRS");
-		acpigen_write_resourcetemplate_header();
-		ldn_gen_resources(dev);
-		acpigen_write_resourcetemplate_footer();
-
-		/* Resources - _PRS */
-		acpigen_write_name("_PRS");
-		acpigen_write_resourcetemplate_header();
-		ldn_gen_resources(dev);
-		acpigen_write_resourcetemplate_footer();
-
-		/* Resources base and size for 3rd party ACPI code */
-		ldn_gen_resources_use(dev);
-	}
+	/*
+	 * The ACPI6.2 spec Chapter 6.1.5 requires to set a  _HID if no _ADR
+	 * is present. Tests on Windows 10 showed that this is also true for
+	 * disabled (_STA = 0) devices, otherwise it BSODs.
+	 */

hid = acpi_device_hid(dev);
    if (!hid) {
@@ -266,6 +249,23 @@
    }
    acpigen_pop_len(); /* Method */

+	if (dev->enabled && has_resources(dev)) {
+		/* Resources - _CRS */
+		acpigen_write_name("_CRS");
+		acpigen_write_resourcetemplate_header();
+		ldn_gen_resources(dev);
+		acpigen_write_resourcetemplate_footer();
+
+		/* Resources - _PRS */
+		acpigen_write_name("_PRS");
+		acpigen_write_resourcetemplate_header();
+		ldn_gen_resources(dev);
+		acpigen_write_resourcetemplate_footer();
+
+		/* Resources base and size for 3rd party ACPI code */
+		ldn_gen_resources_use(dev);
+	}
+
    acpigen_pop_len(); /* Device */
    acpigen_pop_len(); /* Scope */
 }