Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30865
Change subject: src/nb/intel/sandybridge/pcie.c: remove assert() ......................................................................
src/nb/intel/sandybridge/pcie.c: remove assert()
Use proper if() statements instead.
Change-Id: If097ae6d387de393e45d820b65d60b17026e1805 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/pcie.c 1 file changed, 3 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/30865/1
diff --git a/src/northbridge/intel/sandybridge/pcie.c b/src/northbridge/intel/sandybridge/pcie.c index bb88c7a..f942433 100644 --- a/src/northbridge/intel/sandybridge/pcie.c +++ b/src/northbridge/intel/sandybridge/pcie.c @@ -19,7 +19,6 @@ #include <device/pci.h> #include <device/pciexp.h> #include <device/pci_ids.h> -#include <assert.h>
static void pcie_disable(struct device *dev) { @@ -30,12 +29,9 @@ #if IS_ENABLED(CONFIG_HAVE_ACPI_TABLES) static const char *pcie_acpi_name(const struct device *dev) { - assert(dev); - - if (dev->path.type != DEVICE_PATH_PCI) + if (!dev || !dev->bus || dev->path.type != DEVICE_PATH_PCI) return NULL;
- assert(dev->bus); if (dev->bus->secondary == 0) switch (dev->path.pci.devfn) { case PCI_DEVFN(1, 0): @@ -49,8 +45,8 @@ };
struct device *const port = dev->bus->dev; - assert(port); - assert(port->bus); + if (!port || !port->bus) + return NULL;
if (dev->path.pci.devfn == PCI_DEVFN(0, 0) && port->bus->secondary == 0 &&
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30865 )
Change subject: src/nb/intel/sandybridge/pcie.c: remove assert() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/30865/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30865/1//COMMIT_MSG@9 PS1, Line 9: Use proper if() statements instead. Don't rephrase what your commit does. Mention why you are doing it instead. Also, because I don't understand why.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30865 )
Change subject: src/nb/intel/sandybridge/pcie.c: remove assert() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/30865/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30865/1//COMMIT_MSG@9 PS1, Line 9: Use proper if() statements instead.
Don't rephrase what your commit does. Mention why you are doing it instead. […]
AFAIK, assertions are not a mechanism for handling run-time errors, but rather to prove the code is bug-free. Not sure if this applies here, though.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30865 )
Change subject: src/nb/intel/sandybridge/pcie.c: remove assert() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/30865/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30865/1//COMMIT_MSG@9 PS1, Line 9: Use proper if() statements instead.
AFAIK, assertions are not a mechanism for handling run-time errors, but rather to prove the code is […]
They work in two ways:
1. Assertions specify a contract, e.g. in this case, don't call with `dev == NULL`. 2. When enabled at compile time, they abort program execution with an error message if the condition is false.
For the function in question, all the pointers should be set, otherwise the devicetree (structure in RAM not the file) would be inconsistent. By replacing the assertions with if's without printing a warning, you kill the possibility to detect inconsistencies with 2. Generally. Not that I'd expect somebody really to make use of the assertions here.
General rule: If an error condition can happen without the program being totally borked, always perform checks at runtime. If there'd be nothing you could do to fix things, an assertion might make sense.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30865 )
Change subject: src/nb/intel/sandybridge/pcie.c: remove assert() ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/30865/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/30865/1//COMMIT_MSG@9 PS1, Line 9: Use proper if() statements instead.
They work in two ways: […]
Ack, therefore it does not make sense to remove any assertions here.
Angel Pons has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/30865 )
Change subject: src/nb/intel/sandybridge/pcie.c: remove assert() ......................................................................
Abandoned
Check previous messages
-
Angel Pons (Code Review) -
Nico Huber (Code Review)