Change in coreboot[master]: superio/common: Validate devicetree

[Patrick Rudolph (Code Review)]

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

Change subject: superio/common: Validate devicetree
......................................................................

superio/common: Validate devicetree

As the SSDT generator for LDNs expects a "parent" PNP device
for proper ACPI code generation, validate that it is present.

Make sure the devicetree looks as expected and print a BUG message
if that's not the case.

Tested on HP Z220:
No BUG message was printed.

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/38863/1

diff --git a/src/superio/common/ssdt.c b/src/superio/common/ssdt.c
index 7aa24ea..945d0a6 100644
--- a/src/superio/common/ssdt.c
+++ b/src/superio/common/ssdt.c
@@ -168,6 +168,21 @@
    const u8 vldn = (dev->path.pnp.device >> 8) & 0x7;
    const char *hid;

+	/* Validate devicetree settings */
+	if (dev && dev->bus && dev->bus->dev) {
+		bool bug = false;
+		if (dev->bus->dev->path.type != DEVICE_PATH_PNP) {
+			bug = true;
+			printk(BIOS_CRIT, "BUG: Parent of device %s is not a PNP device\n",
+				dev_path(dev));
+		} else if (dev->bus->dev->path.pnp.port != dev->path.pnp.port) {
+			bug = true;
+			printk(BIOS_CRIT, "BUG: Parent of device %s has wrong I/O port\n",
+				dev_path(dev));
+		}
+		if (bug)
+			printk(BIOS_CRIT, "BUG: Check your devicetree!\n");
+	}
    if (!scope || !name) {
    	printk(BIOS_ERR, "%s: Missing ACPI path/scope\n", dev_path(dev));
    	return;

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38863 )

Change subject: superio/common: Validate devicetree
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/38863/2/src/superio/common/ssdt.c@1... 
PS2, Line 167: const u8 ldn = dev->path.pnp.device & 0xff;
             : 	const u8 vldn = (dev->path.pnp.device >> 8) & 0x7;
here *dev already gets dereferenced, the check that dev isn't 0 probably should be before and not after this

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38863 )

Change subject: superio/common: Validate devicetree
......................................................................

Patch Set 2: Code-Review+1

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/38863/2/src/superio/common/ssdt.c@1... 
PS2, Line 167: const u8 ldn = dev->path.pnp.device & 0xff;
             : 	const u8 vldn = (dev->path.pnp.device >> 8) & 0x7;
> here *dev already gets dereferenced, the check that dev isn't 0 probably should be before and not af […]

And I guess that running the SSDT generator would not be possible, so an early return would be needed I guess?

https://review.coreboot.org/c/coreboot/+/38863/2/src/superio/common/ssdt.c@1... 
PS2, Line 184: BIOS_CRIT
If it's a critical bug, shouldn't we return instead of trying to generate the SSDT?

[Patrick Rudolph (Code Review)]

Hello Felix Held, Angel Pons, build bot (Jenkins),

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

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

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

Change subject: superio/common: Validate devicetree
......................................................................

superio/common: Validate devicetree

As the SSDT generator for LDNs expects a "parent" PNP device
for proper ACPI code generation, validate that it is present.

Make sure the devicetree looks as expected and print a BUG message
if that's not the case.

Tested on HP Z220:
No BUG message was printed.

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

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/63/38863/3

[Patrick Rudolph (Code Review)]

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

Change subject: superio/common: Validate devicetree
......................................................................

Patch Set 3:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/38863/2/src/superio/common/ssdt.c@1... 
PS2, Line 167: const u8 ldn = dev->path.pnp.device & 0xff;
             : 	const u8 vldn = (dev->path.pnp.device >> 8) & 0x7;
> And I guess that running the SSDT generator would not be possible, so an early return would be neede […]

Done

https://review.coreboot.org/c/coreboot/+/38863/2/src/superio/common/ssdt.c@1... 
PS2, Line 184: BIOS_CRIT
> If it's a critical bug, shouldn't we return instead of trying to generate the SSDT?

Done

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38863 )

Change subject: superio/common: Validate devicetree
......................................................................

Patch Set 3: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38863 )

Change subject: superio/common: Validate devicetree
......................................................................

Patch Set 3: Code-Review+2

[Felix Held (Code Review)]

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

Change subject: superio/common: Validate devicetree
......................................................................

superio/common: Validate devicetree

As the SSDT generator for LDNs expects a "parent" PNP device
for proper ACPI code generation, validate that it is present.

Make sure the devicetree looks as expected and print a BUG message
if that's not the case.

Tested on HP Z220:
No BUG message was printed.

Change-Id: I6cbcba8ac86a2a837e23055fdd7e529f9b3277a2
Signed-off-by: Patrick Rudolph 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38863
Tested-by: build bot (Jenkins) 
Reviewed-by: Felix Held 
Reviewed-by: Angel Pons 
---
M src/superio/common/ssdt.c
1 file changed, 21 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Felix Held: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/superio/common/ssdt.c b/src/superio/common/ssdt.c
index 7aa24ea..e31660f 100644
--- a/src/superio/common/ssdt.c
+++ b/src/superio/common/ssdt.c
@@ -162,12 +162,33 @@

void superio_common_fill_ssdt_generator(struct device *dev)
 {
+	if (!dev || !dev->bus || !dev->bus->dev) {
+		printk(BIOS_CRIT, "BUG: Invalid argument in %s!\n", __func__);
+		return;
+	}
+
    const char *scope = acpi_device_scope(dev);
    const char *name = acpi_device_name(dev);
    const u8 ldn = dev->path.pnp.device & 0xff;
    const u8 vldn = (dev->path.pnp.device >> 8) & 0x7;
    const char *hid;

+	/* Validate devicetree settings */
+	bool bug = false;
+	if (dev->bus->dev->path.type != DEVICE_PATH_PNP) {
+		bug = true;
+		printk(BIOS_CRIT, "BUG: Parent of device %s is not a PNP device\n",
+			dev_path(dev));
+	} else if (dev->bus->dev->path.pnp.port != dev->path.pnp.port) {
+		bug = true;
+		printk(BIOS_CRIT, "BUG: Parent of device %s has wrong I/O port\n",
+			dev_path(dev));
+	}
+	if (bug) {
+		printk(BIOS_CRIT, "BUG: Check your devicetree!\n");
+		return;
+	}
+
    if (!scope || !name) {
    	printk(BIOS_ERR, "%s: Missing ACPI path/scope\n", dev_path(dev));
    	return;

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38863 )

Change subject: superio/common: Validate devicetree
......................................................................

Patch Set 4:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/964
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/963
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/962

Please note: This test is under development and might not be accurate at all!