Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35985 )
Change subject: intel/skylake: Implement PCIe RP devicetree update based on LCAP ......................................................................
Patch Set 18:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... File src/soc/intel/common/block/pcie/pcie_rp.c:
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 160: *ptr = dev->sibling;
Is the idea to prev_dev->sibling = dev->sibling; and set dev = NULL;?
The former, yes, the latter, no.
But also including the first-iteration case, when we want to alter `root->children`.
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 161: dev->sibling
dev = NULL; ?
No. The next step is either leaving the function and destroying the stack frame including `dev`, or `dev = *ptr` (third part of the `for`) which would override it.
https://review.coreboot.org/c/coreboot/+/35985/18/src/soc/intel/common/block... PS18, Line 141: for (dev = *ptr; dev; dev = *ptr) { : if (dev->path.type != DEVICE_PATH_PCI) { : ptr = &dev->sibling; : continue; : } : : offset = 0; : for (group = groups; group->count; ++group) { : if (group->slot == PCI_SLOT(dev->path.pci.devfn)) : break; : offset += group->count; : } : if (!group->count) { : ptr = &dev->sibling; : continue; : } : : if (pcie_rp_update_dev(dev, offset, mapping) < 0) { : /* Unlink disabled device. */ : *ptr = dev->sibling; : dev->sibling = NULL; : } : } That reflects the idea but is flawed:
* It misses to update `root->children`. * The `prev_dev = dev,` would also set `prev_dev` to the unlinked device. So a second unlink in a row would work with the wrong `prev_dev`.
Could be fixed as follows:
... if (pcie_rp_update_dev(dev, offset, mapping) < 0) { /* Unlink disabled device. */ if (prev_dev) prev_dev->sibling = dev->sibling; else root->children = dev->sibling; dev = prev_dev; }
I'm not sure if rewinding `dev` makes things easier to read?
Since this runs before enumeration it might be possible to simply skip the unlinking as this is done elsewhere?
The problem I'm trying to solve is that inside pcie_rp_update_dev() we have no new function number for the disabled device. So if we don't unlink it, we have potentially more than one device with the same `devfn` on the bus. This would break the mixing of `devicetree.cb` nodes with new nodes during enumeration.