Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40474 )
Change subject: device: Add a helper to find device behind a PCI bridge device ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40474/4/src/device/device_const.c File src/device/device_const.c:
https://review.coreboot.org/c/coreboot/+/40474/4/src/device/device_const.c@2... PS4, Line 230: bridge->path.type != DEVICE_PATH_PCI
I am not sure I understand the use case here. […]
Yes, from the top of my head IEE1394 to PCI bridges. Ok, one could model the IEE1394 part as PCI too, but as I said it's theoretical as long as there is no such case in coreboot.
https://review.coreboot.org/c/coreboot/+/40474/4/src/device/device_const.c@2... PS4, Line 233: link_list
The whole concept of multiple links for a PCI bridge is weird. I don't understand how that would really work. Also, I don't see any PCI bridges supporting multiple links(buses) under them. It is currently used only for some cpu cluster device for AMD SoCs.
In my opinion, let's not add more logic for checking multiple links here unless it is a valid scenario. (Probably, the right thing to do would be for some one to evaluate whether the whole multiple links/buses is something that needs to be redone differently. That is for a different change though.).
If you have questions/doubts about coreboot's devicetree model please take it to the mailing list. I just tried to comment here two times but Gerrit ate it all (these weird, hacky javascript input elements are not very compatible across browsers). Comments below are a little stale compared to what I wrote in the meantime...
For now, I think we can just go with the head of the linked list of links and search for device under that. I can add a comment if you think it is helpful.
Please, no comment. In my experience, comments are ignored 99% of the times they matter.
You seem to assume that the bridge will always be a standard PCI-to-PCI bridge. This is neither reflected by the function name and signature nor the commit message. I guess that's where we collide.
I wanted to suggest that we could take a `struct bus` as argument, but then realized that this is actually what you want to hide with this function. If you really want the implementation as seen here, how about a matching name?
pcidev_behind_pure_p2p_bridge()
(actually pcidev_path_behind_pure_p2p_bridge() to align with the other functions that take a `pci_devfn_t`)