Change in coreboot[master]: soc/amd/picasso: add NULL-pointer check to root_complex_fill_ssdt
Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42846 )
Change subject: soc/amd/picasso: add NULL-pointer check to root_complex_fill_ssdt ......................................................................
soc/amd/picasso: add NULL-pointer check to root_complex_fill_ssdt
Found-by: Coverity CID 1429980
Change-Id: Ia72b9dbe029a5da98e408a9cf16fa4a93b10917a Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/picasso/root_complex.c 1 file changed, 5 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/42846/1
diff --git a/src/soc/amd/picasso/root_complex.c b/src/soc/amd/picasso/root_complex.c index 4930a0e..af347e7 100644 --- a/src/soc/amd/picasso/root_complex.c +++ b/src/soc/amd/picasso/root_complex.c @@ -135,6 +135,11 @@ { msr_t msr;
+ if (!device) { + printk(BIOS_ERR, "Error: %s device is NULL.\n", __func__); + return; + } + acpigen_write_scope(acpi_device_scope(device));
msr = rdmsr(TOP_MEM);
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42846 )
Change subject: soc/amd/picasso: add NULL-pointer check to root_complex_fill_ssdt ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42846/1/src/soc/amd/picasso/root_co... File src/soc/amd/picasso/root_complex.c:
https://review.coreboot.org/c/coreboot/+/42846/1/src/soc/amd/picasso/root_co... PS1, Line 139: printk(BIOS_ERR, "Error: %s device is NULL.\n", __func__); Please elaborate. It’s not clear if the code continues anyway or aborts. Maybe:
Error: NULL device passed in %s. ACPI SSDT will be incomplete.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42846 )
Change subject: soc/amd/picasso: add NULL-pointer check to root_complex_fill_ssdt ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42846/1/src/soc/amd/picasso/root_co... File src/soc/amd/picasso/root_complex.c:
https://review.coreboot.org/c/coreboot/+/42846/1/src/soc/amd/picasso/root_co... PS1, Line 139: printk(BIOS_ERR, "Error: %s device is NULL.\n", __func__);
Please elaborate. It’s not clear if the code continues anyway or aborts. Maybe: […]
I'd avoid mentioning `Error` in the log message:
%s: device is NULL! ACPI SSDT will be incomplete.
https://review.coreboot.org/c/coreboot/+/42846/1/src/soc/amd/picasso/root_co... PS1, Line 163: .acpi_fill_ssdt = root_complex_fill_ssdt, Doesn't this guarantee that `device` will never be null?
Hello build bot (Jenkins), Jason Glenesk, Raul Rangel, Furquan Shaikh, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42846
to look at the new patch set (#2).
Change subject: soc/amd/picasso: add NULL-pointer check to root_complex_fill_ssdt ......................................................................
soc/amd/picasso: add NULL-pointer check to root_complex_fill_ssdt
Found-by: Coverity CID 1429980
Change-Id: Ia72b9dbe029a5da98e408a9cf16fa4a93b10917a Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/picasso/root_complex.c 1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/42846/2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42846 )
Change subject: soc/amd/picasso: add NULL-pointer check to root_complex_fill_ssdt ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42846/1/src/soc/amd/picasso/root_co... File src/soc/amd/picasso/root_complex.c:
https://review.coreboot.org/c/coreboot/+/42846/1/src/soc/amd/picasso/root_co... PS1, Line 139: printk(BIOS_ERR, "Error: %s device is NULL.\n", __func__);
I'd avoid mentioning `Error` in the log message: […]
Done
https://review.coreboot.org/c/coreboot/+/42846/1/src/soc/amd/picasso/root_co... PS1, Line 163: .acpi_fill_ssdt = root_complex_fill_ssdt,
Doesn't this guarantee that `device` will never be null?
it should. i still prefer having a bit too much error handling that lacking proper error handling and i also don't think that this will create too much additional overhead
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42846 )
Change subject: soc/amd/picasso: add NULL-pointer check to root_complex_fill_ssdt ......................................................................
Patch Set 2: Code-Review+2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42846 )
Change subject: soc/amd/picasso: add NULL-pointer check to root_complex_fill_ssdt ......................................................................
Patch Set 2: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42846 )
Change subject: soc/amd/picasso: add NULL-pointer check to root_complex_fill_ssdt ......................................................................
soc/amd/picasso: add NULL-pointer check to root_complex_fill_ssdt
Found-by: Coverity CID 1429980
Change-Id: Ia72b9dbe029a5da98e408a9cf16fa4a93b10917a Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/42846 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Raul Rangel rrangel@chromium.org --- M src/soc/amd/picasso/root_complex.c 1 file changed, 6 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Rudolph: Looks good to me, approved Raul Rangel: Looks good to me, approved
diff --git a/src/soc/amd/picasso/root_complex.c b/src/soc/amd/picasso/root_complex.c index 4930a0e..036ff00 100644 --- a/src/soc/amd/picasso/root_complex.c +++ b/src/soc/amd/picasso/root_complex.c @@ -135,6 +135,12 @@ { msr_t msr;
+ if (!device) { + printk(BIOS_ERR, "%s: device is NULL! ACPI SSDT will be incomplete.\n", + __func__); + return; + } + acpigen_write_scope(acpi_device_scope(device));
msr = rdmsr(TOP_MEM);
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42846 )
Change subject: soc/amd/picasso: add NULL-pointer check to root_complex_fill_ssdt ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42846/3/src/soc/amd/picasso/root_co... File src/soc/amd/picasso/root_complex.c:
https://review.coreboot.org/c/coreboot/+/42846/3/src/soc/amd/picasso/root_co... PS3, Line 138: if (!device) { It should have just been an assert or something. The ops which this function is a part of cannot be called with NULL. the ops are inherently bound by the __pci_driver.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42846 )
Change subject: soc/amd/picasso: add NULL-pointer check to root_complex_fill_ssdt ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42846/3/src/soc/amd/picasso/root_co... File src/soc/amd/picasso/root_complex.c:
https://review.coreboot.org/c/coreboot/+/42846/3/src/soc/amd/picasso/root_co... PS3, Line 150: XP oh no
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Felix Held (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)