Change in coreboot[master]: soc/skylake: prevent null pointer dereferences

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35173 )

Change subject: soc/skylake: prevent null pointer dereferences
......................................................................

soc/skylake: prevent null pointer dereferences

Change-Id: Ide10223e7fc37a6c4bfa408234ef3efe1846236a
Signed-off-by: Angel Pons 
---
M src/soc/intel/skylake/chip.c
M src/soc/intel/skylake/chip_fsp20.c
2 files changed, 19 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/35173/1

diff --git a/src/soc/intel/skylake/chip.c b/src/soc/intel/skylake/chip.c
index a7d5872..812572a 100644
--- a/src/soc/intel/skylake/chip.c
+++ b/src/soc/intel/skylake/chip.c
@@ -153,10 +153,7 @@

/* Enable ISH if device is on */
    dev = pcidev_path_on_root(PCH_DEVFN_ISH);
-	if (dev)
-		params->IshEnable = dev->enabled;
-	else
-		params->IshEnable = 0;
+	params->IshEnable = (dev) ? dev->enabled : 0;

params->EnableAzalia = config->EnableAzalia;
    params->IoBufferOwnership = config->IoBufferOwnership;
@@ -210,23 +207,24 @@
     * do the changes and then lock it back in coreboot
     *
     */
-	if (config->HeciEnabled == 0)
-		params->PsfUnlock = 1;
-	else
-		params->PsfUnlock = 0;
+	params->PsfUnlock = !config->HeciEnabled;

for (i = 0; i < ARRAY_SIZE(config->domain_vr_config); i++)
    	fill_vr_domain_config(params, i, &config->domain_vr_config[i]);

/* Show SPI controller if enabled in devicetree.cb */
    dev = pcidev_path_on_root(PCH_DEVFN_SPI);
-	params->ShowSpiController = dev->enabled;
+	params->ShowSpiController = (dev) ? dev->enabled : 0;

/* Enable xDCI controller if enabled in devicetree and allowed */
    dev = pcidev_path_on_root(PCH_DEVFN_USBOTG);
-	if (!xdci_can_enable())
-		dev->enabled = 0;
-	params->XdciEnable = dev->enabled;
+	if (dev) {
+		if (!xdci_can_enable())
+			dev->enabled = 0;
+		params->XdciEnable = dev->enabled;
+	} else {
+		params->XdciEnable = 0;
+	}

params->SendVrMbxCmd = config->SendVrMbxCmd;

diff --git a/src/soc/intel/skylake/chip_fsp20.c b/src/soc/intel/skylake/chip_fsp20.c
index 064f71e..d594bbb 100644
--- a/src/soc/intel/skylake/chip_fsp20.c
+++ b/src/soc/intel/skylake/chip_fsp20.c
@@ -354,10 +354,7 @@

/* If ISH is enabled, enable ISH elements */
    dev = pcidev_path_on_root(PCH_DEVFN_ISH);
-	if (dev)
-		params->PchIshEnable = dev->enabled;
-	else
-		params->PchIshEnable = 0;
+	params->PchIshEnable = (dev) ? dev->enabled : 0;

params->PchHdaEnable = config->EnableAzalia;
    params->PchHdaIoBufferOwnership = config->IoBufferOwnership;
@@ -433,13 +430,17 @@

/* Show SPI controller if enabled in devicetree.cb */
    dev = pcidev_path_on_root(PCH_DEVFN_SPI);
-	params->ShowSpiController = dev->enabled;
+	params->ShowSpiController = (dev) ? dev->enabled : 0;

/* Enable xDCI controller if enabled in devicetree and allowed */
    dev = pcidev_path_on_root(PCH_DEVFN_USBOTG);
-	if (!xdci_can_enable())
-		dev->enabled = 0;
-	params->XdciEnable = dev->enabled;
+	if (dev) {
+		if (!xdci_can_enable())
+			dev->enabled = 0;
+		params->XdciEnable = dev->enabled;
+	} else {
+		params->XdciEnable = 0;
+	}

/*
     * Send VR specific mailbox commands:

[Name of user not set (Code Review)]

Name of user not set #1002476 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35173 )

Change subject: soc/skylake: prevent null pointer dereferences
......................................................................

Patch Set 1:

(4 comments)

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip.... 
File src/soc/intel/skylake/chip.c:

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip.... 
PS1, Line 156: (dev)
Without parenthesis?

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip.... 
PS1, Line 217: (dev)
Without parenthesis?

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip_... 
File src/soc/intel/skylake/chip_fsp20.c:

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip_... 
PS1, Line 357: (dev)
Without parenthesis?

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip_... 
PS1, Line 433: (dev)
Without parenthesis?

[Angel Pons (Code Review)]

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

Change subject: soc/skylake: prevent null pointer dereferences
......................................................................

Patch Set 1:

(4 comments)

Note: Since the four comments are about the same thing, marked three of them as resolved so as to keep the conversation on only one of them. If there's anything to change, I will update all four ocurrences, though.

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip.... 
File src/soc/intel/skylake/chip.c:

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip.... 
PS1, Line 156: (dev)
> Without parenthesis?

I personally prefer to leave the parentheses around the logical expression, as I find it easier to understand. Not sure if our code style says anything about it, though.

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip.... 
PS1, Line 217: (dev)
> Without parenthesis?

Ack

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip_... 
File src/soc/intel/skylake/chip_fsp20.c:

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip_... 
PS1, Line 357: (dev)
> Without parenthesis?

Ack

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip_... 
PS1, Line 433: (dev)
> Without parenthesis?

Ack

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35173 )

Change subject: soc/skylake: prevent null pointer dereferences
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip.... 
File src/soc/intel/skylake/chip.c:

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip.... 
PS1, Line 156: (dev)
> I personally prefer to leave the parentheses around the logical expression, as I find it easier to u […]

IMHO, these parentheses look ugly and unbalanced. i.e. why isn't it

(dev) ? (dev->enabled) : (0);

(not that I would prefer that)

[Angel Pons (Code Review)]

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

Change subject: soc/skylake: prevent null pointer dereferences
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip.... 
File src/soc/intel/skylake/chip.c:

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip.... 
PS1, Line 156: (dev)
> I personally prefer to leave the parentheses around the logical expression, as I find it easier to u […]

Just saw Patrick Georgi's comment on CB:35168 about the same thing, so I'll update these.

[Angel Pons (Code Review)]

Hello Patrick Rudolph, Jacob Garber, Maxim Polyakov, Matt DeVillier, build bot (Jenkins), Nico Huber, Patrick Georgi,

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

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

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

Change subject: soc/skylake: prevent null pointer dereferences
......................................................................

soc/skylake: prevent null pointer dereferences

Change-Id: Ide10223e7fc37a6c4bfa408234ef3efe1846236a
Signed-off-by: Angel Pons 
---
M src/soc/intel/skylake/chip.c
M src/soc/intel/skylake/chip_fsp20.c
2 files changed, 19 insertions(+), 20 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/35173/2

[Angel Pons (Code Review)]

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

Change subject: soc/skylake: prevent null pointer dereferences
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip.... 
File src/soc/intel/skylake/chip.c:

https://review.coreboot.org/c/coreboot/+/35173/1/src/soc/intel/skylake/chip.... 
PS1, Line 156: (dev)
> Just saw Patrick Georgi's comment on CB:35168 about the same thing, so I'll update these.

Done

[Patrick Georgi (Code Review)]

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

Change subject: soc/skylake: prevent null pointer dereferences
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35173 )

Change subject: soc/skylake: prevent null pointer dereferences
......................................................................

soc/skylake: prevent null pointer dereferences

Change-Id: Ide10223e7fc37a6c4bfa408234ef3efe1846236a
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35173
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Georgi 
---
M src/soc/intel/skylake/chip.c
M src/soc/intel/skylake/chip_fsp20.c
2 files changed, 19 insertions(+), 20 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved

diff --git a/src/soc/intel/skylake/chip.c b/src/soc/intel/skylake/chip.c
index a7d5872..212c244 100644
--- a/src/soc/intel/skylake/chip.c
+++ b/src/soc/intel/skylake/chip.c
@@ -153,10 +153,7 @@

/* Enable ISH if device is on */
    dev = pcidev_path_on_root(PCH_DEVFN_ISH);
-	if (dev)
-		params->IshEnable = dev->enabled;
-	else
-		params->IshEnable = 0;
+	params->IshEnable = dev ? dev->enabled : 0;

params->EnableAzalia = config->EnableAzalia;
    params->IoBufferOwnership = config->IoBufferOwnership;
@@ -210,23 +207,24 @@
     * do the changes and then lock it back in coreboot
     *
     */
-	if (config->HeciEnabled == 0)
-		params->PsfUnlock = 1;
-	else
-		params->PsfUnlock = 0;
+	params->PsfUnlock = !config->HeciEnabled;

for (i = 0; i < ARRAY_SIZE(config->domain_vr_config); i++)
    	fill_vr_domain_config(params, i, &config->domain_vr_config[i]);

/* Show SPI controller if enabled in devicetree.cb */
    dev = pcidev_path_on_root(PCH_DEVFN_SPI);
-	params->ShowSpiController = dev->enabled;
+	params->ShowSpiController = dev ? dev->enabled : 0;

/* Enable xDCI controller if enabled in devicetree and allowed */
    dev = pcidev_path_on_root(PCH_DEVFN_USBOTG);
-	if (!xdci_can_enable())
-		dev->enabled = 0;
-	params->XdciEnable = dev->enabled;
+	if (dev) {
+		if (!xdci_can_enable())
+			dev->enabled = 0;
+		params->XdciEnable = dev->enabled;
+	} else {
+		params->XdciEnable = 0;
+	}

params->SendVrMbxCmd = config->SendVrMbxCmd;

diff --git a/src/soc/intel/skylake/chip_fsp20.c b/src/soc/intel/skylake/chip_fsp20.c
index 85d3edf..e9c37d6 100644
--- a/src/soc/intel/skylake/chip_fsp20.c
+++ b/src/soc/intel/skylake/chip_fsp20.c
@@ -354,10 +354,7 @@

/* If ISH is enabled, enable ISH elements */
    dev = pcidev_path_on_root(PCH_DEVFN_ISH);
-	if (dev)
-		params->PchIshEnable = dev->enabled;
-	else
-		params->PchIshEnable = 0;
+	params->PchIshEnable = dev ? dev->enabled : 0;

params->PchHdaEnable = config->EnableAzalia;
    params->PchHdaIoBufferOwnership = config->IoBufferOwnership;
@@ -433,13 +430,17 @@

/* Show SPI controller if enabled in devicetree.cb */
    dev = pcidev_path_on_root(PCH_DEVFN_SPI);
-	params->ShowSpiController = dev->enabled;
+	params->ShowSpiController = dev ? dev->enabled : 0;

/* Enable xDCI controller if enabled in devicetree and allowed */
    dev = pcidev_path_on_root(PCH_DEVFN_USBOTG);
-	if (!xdci_can_enable())
-		dev->enabled = 0;
-	params->XdciEnable = dev->enabled;
+	if (dev) {
+		if (!xdci_can_enable())
+			dev->enabled = 0;
+		params->XdciEnable = dev->enabled;
+	} else {
+		params->XdciEnable = 0;
+	}

/* Enable or disable Gaussian Mixture Model in devicetree */
    dev = pcidev_path_on_root(SA_DEVFN_GMM);