Nico Huber has posted comments on this change. ( https://review.coreboot.org/29078 )
Change subject: add newer pci infrastructure ......................................................................
Patch Set 2:
(15 comments)
Just a kick-off, this will take time. Didn't look ahead yet, so I might still have comments on the semantics later. The course looks good.
https://review.coreboot.org/#/c/29078/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/29078/2//COMMIT_MSG@9 PS2, Line 9: This code does not negatively affect existing pci support and existing : pci devices, This reads like "This can't hurt, please wave through.", please remove/rephrase.
https://review.coreboot.org/#/c/29078/2//COMMIT_MSG@16 PS2, Line 16: but means that support for other operating systems still : needs to be cobbled together. Why not fall back to the existing dev-mem code on other OS'?
https://review.coreboot.org/#/c/29078/2/pcidev.c File pcidev.c:
https://review.coreboot.org/#/c/29078/2/pcidev.c@354 PS2, Line 354: */ What is with these empty comments? Please remove (or fill).
https://review.coreboot.org/#/c/29078/2/pcidev.c@356 PS2, Line 356: void * This makes C's type checking even weaker. Why not declare the parameter as `struct flashrom_pci_device *`? You could still pass a `void *`; just not any pointer.
https://review.coreboot.org/#/c/29078/2/pcidev.c@361 PS2, Line 361: cleanup `clean up`
https://review.coreboot.org/#/c/29078/2/pcidev.c@367 PS2, Line 367: device->sysfs_path = NULL; What for? we release it before it could be dereferenced again.
https://review.coreboot.org/#/c/29078/2/pcidev.c@404 PS2, Line 404: return NULL; leaking `pcidev_bdf`
https://review.coreboot.org/#/c/29078/2/pcidev.c@410 PS2, Line 410: for (dev = pacc->devices; dev; dev = dev->next) Please always use braces if multiple lines follow.
https://review.coreboot.org/#/c/29078/2/pcidev.c@411 PS2, Line 411: if (pci_filter_match(&filter, dev)) { Please turn into
if (!pci_filter_match(&filter, dev)) continue;
to save the extra indentation level.
https://review.coreboot.org/#/c/29078/2/pcidev.c@412 PS2, Line 412: no space before semicolon
https://review.coreboot.org/#/c/29078/2/pcidev.c@424 PS2, Line 424: if (name) Please use a conditional expression or just:
if (!name) name = "<unknown pciids>";
which should also fix the strdup() call below.
https://review.coreboot.org/#/c/29078/2/pcidev.c@452 PS2, Line 452: card s/card/device/ ? it doesn't have to be a "card"
https://review.coreboot.org/#/c/29078/2/pcidev.c@465 PS2, Line 465: - 1, `- 1` is not necessary
https://review.coreboot.org/#/c/29078/2/pcidev.c@467 PS2, Line 467: 0 found_dev->domain ?
https://review.coreboot.org/#/c/29078/2/programmer.h File programmer.h:
https://review.coreboot.org/#/c/29078/2/programmer.h@852 PS2, Line 852: #if NEED_PCI == 1 Why guard? are there conflicting alternative definitions?
-
Nico Huber (Code Review)