Attention is currently required from: Arthur Heymans, Christian Walter, Felix Held, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Patrick Rudolph, Shuo Liu, Tim Chu.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81957?usp=email )
Change subject: device_util: Handle domain device in dev_get_domain ......................................................................
Patch Set 2: Code-Review-1
(2 comments)
Patchset:
PS2: This could result in endless loops.
File src/device/device_util.c:
https://review.coreboot.org/c/coreboot/+/81957/comment/cb472196_59fa32a6 : PS2, Line 254: !is_root_device(dev) This is an important exit condition for the loop, you cannot remove it.
I know it's not obvious when coming from other projects. But coreboot's devicetree was originally designed with as few as possible NULL pointers (because NULL pointers can introduce bugs). Hence there are no upstream facing NULL pointers by design and you can't use them as exit condition when walking up. OTOH, this design makes it possible to use upstream facing pointers without the need for defensive checks (assuming a valid devicetree).
For instance, the following would be a valid and sane implementation in coreboot: ``` const struct device *dev_get_domain(const struct device *dev) { assert(dev); /* must be called with `dev != NULL` by contract */ for (; !is_root_device(dev); dev = dev->upstream->dev) { if (dev->path.type == DEVICE_PATH_DOMAIN) return dev; } return NULL; } ``` By design, there is no need for runtime NULL checks.
There was nothing wrong with PS1, though, so I suggest to simply return to that.