Change in coreboot[master]: x86/acpi_device: Allow acpi_device_add_power_res params as optional

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38522 )

Change subject: x86/acpi_device: Allow acpi_device_add_power_res params as optional
......................................................................

x86/acpi_device: Allow acpi_device_add_power_res params as optional

Allow for making both reset_gpio && enable_gpio as optional in
the params.

BUG=b:147026979
BRANCH=none
TEST=builds

Change-Id: I8053d7a080dfed898400c0994bcea492c826fe3d
Signed-off-by: Edward O'Callaghan 
---
M src/arch/x86/acpi_device.c
1 file changed, 3 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/38522/1

diff --git a/src/arch/x86/acpi_device.c b/src/arch/x86/acpi_device.c
index d512789..b91e4cb 100644
--- a/src/arch/x86/acpi_device.c
+++ b/src/arch/x86/acpi_device.c
@@ -537,12 +537,9 @@
 void acpi_device_add_power_res(const struct acpi_power_res_params *params)
 {
    static const char *power_res_dev_states[] = { "_PR0", "_PR3" };
-	unsigned int reset_gpio = params->reset_gpio->pins[0];
-	unsigned int enable_gpio = params->enable_gpio->pins[0];
-	unsigned int stop_gpio = params->stop_gpio->pins[0];
-
-	if (!reset_gpio && !enable_gpio && !stop_gpio)
-		return;
+	unsigned int reset_gpio = params->reset_gpio ? params->reset_gpio->pins[0] : 0;
+	unsigned int enable_gpio = params->enable_gpio ? params->enable_gpio->pins[0] : 0;
+	unsigned int stop_gpio = params->stop_gpio ? params->stop_gpio->pins[0] : 0;

/* PowerResource (PRIC, 0, 0) */
    acpigen_write_power_res("PRIC", 0, 0, power_res_dev_states,

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38522 )

Change subject: x86/acpi_device: Allow acpi_device_add_power_res params as optional
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38522/1/src/arch/x86/acpi_device.c 
File src/arch/x86/acpi_device.c:

https://review.coreboot.org/c/coreboot/+/38522/1/src/arch/x86/acpi_device.c@... 
PS1, Line 544: if (!reset_gpio && !enable_gpio && !stop_gpio)
             : 		return;
             : 
Why was this check removed? Is there any advantage of adding power resource if none of the GPIOs are present? _ON and _OFF methods would all be empty.

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38522 )

Change subject: x86/acpi_device: Allow acpi_device_add_power_res params as optional
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38522/1/src/arch/x86/acpi_device.c 
File src/arch/x86/acpi_device.c:

https://review.coreboot.org/c/coreboot/+/38522/1/src/arch/x86/acpi_device.c@... 
PS1, Line 544: if (!reset_gpio && !enable_gpio && !stop_gpio)
             : 		return;
             :
> Why was this check removed? Is there any advantage of adding power resource if none of the GPIOs are […]

We only need to define a `stop_gpio` for the _ON, _OFF methods. Removing the check lossens the assumptions of acpi_device_add_power_res() and allows it to be more general. tbh it was a bit buggy before as if they were not defined the above lines would NULL deref.

Any way the checks are done below in the function in the proper places.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38522 )

Change subject: x86/acpi_device: Allow acpi_device_add_power_res params as optional
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38522/1/src/arch/x86/acpi_device.c 
File src/arch/x86/acpi_device.c:

https://review.coreboot.org/c/coreboot/+/38522/1/src/arch/x86/acpi_device.c@... 
PS1, Line 544: if (!reset_gpio && !enable_gpio && !stop_gpio)
             : 		return;
             :
> We only need to define a `stop_gpio` for the _ON, _OFF methods. […]

What the check did was: It returned early if reset gpio, enable gpio and stop gpio are all 0s since in that case there is no real power control pin. For your case, it is totally fine to just have a stop gpio without having the rest. The check would fail for !stop_gpio and continue to add power resource.

With this check removed, if a call is made to acpi_device_add_power_res() with all gpios set to 0, then empty power resource would be added to the device. It is not bad, but unnecessary.

Yes, the NULL deref above is bad, but removing the check in lines 544-545 isn't really making it more general.

[Edward O'Callaghan (Code Review)]

Hello build bot (Jenkins), Daniel Kurtz, Furquan Shaikh,

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

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

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

Change subject: x86/acpi_device: Allow acpi_device_add_power_res params as optional
......................................................................

x86/acpi_device: Allow acpi_device_add_power_res params as optional

Allow for making both reset_gpio && enable_gpio as optional in
the params by fixing a potential NULL deref and defaulting to
zero values.

BUG=b:147026979
BRANCH=none
TEST=builds

Change-Id: I8053d7a080dfed898400c0994bcea492c826fe3d
Signed-off-by: Edward O'Callaghan 
---
M src/arch/x86/acpi_device.c
1 file changed, 3 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/38522/3

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38522 )

Change subject: x86/acpi_device: Allow acpi_device_add_power_res params as optional
......................................................................

Patch Set 3:
> Patch Set 2:
>
> (1 comment)

Ah right. Sorry got you now, you meant the _ON, _OFF methods can wind up just empty. Fixed now.

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38522 )

Change subject: x86/acpi_device: Allow acpi_device_add_power_res params as optional
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38522/1/src/arch/x86/acpi_device.c 
File src/arch/x86/acpi_device.c:

https://review.coreboot.org/c/coreboot/+/38522/1/src/arch/x86/acpi_device.c@... 
PS1, Line 544: if (!reset_gpio && !enable_gpio && !stop_gpio)
             : 		return;
             :
> What the check did was: It returned early if reset gpio, enable gpio and stop gpio are all 0s since  […]

Done

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38522 )

Change subject: x86/acpi_device: Allow acpi_device_add_power_res params as optional
......................................................................

Patch Set 3: Code-Review+2

[Patrick Georgi (Code Review)]

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

Change subject: x86/acpi_device: Allow acpi_device_add_power_res params as optional
......................................................................

x86/acpi_device: Allow acpi_device_add_power_res params as optional

Allow for making both reset_gpio && enable_gpio as optional in
the params by fixing a potential NULL deref and defaulting to
zero values.

BUG=b:147026979
BRANCH=none
TEST=builds

Change-Id: I8053d7a080dfed898400c0994bcea492c826fe3d
Signed-off-by: Edward O'Callaghan 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38522
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
---
M src/arch/x86/acpi_device.c
1 file changed, 3 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved

diff --git a/src/arch/x86/acpi_device.c b/src/arch/x86/acpi_device.c
index d512789..1092c73 100644
--- a/src/arch/x86/acpi_device.c
+++ b/src/arch/x86/acpi_device.c
@@ -537,9 +537,9 @@
 void acpi_device_add_power_res(const struct acpi_power_res_params *params)
 {
    static const char *power_res_dev_states[] = { "_PR0", "_PR3" };
-	unsigned int reset_gpio = params->reset_gpio->pins[0];
-	unsigned int enable_gpio = params->enable_gpio->pins[0];
-	unsigned int stop_gpio = params->stop_gpio->pins[0];
+	unsigned int reset_gpio = params->reset_gpio ? params->reset_gpio->pins[0] : 0;
+	unsigned int enable_gpio = params->enable_gpio ? params->enable_gpio->pins[0] : 0;
+	unsigned int stop_gpio = params->stop_gpio ? params->stop_gpio->pins[0] : 0;

if (!reset_gpio && !enable_gpio && !stop_gpio)
    	return;