Hi Dustin, On 2026-08-05T20:31:13, Scott Moser <[email protected]> wrote: > dm: pci: fix uninitialized fdt_pci_addr fall-through in pci_get_devfn
Thanks for tracking this down. > > pci_get_devfn() reads addr.phys_hi and returns the low bits of it as the > requested devfn even on the -ENOENT branch, where ofnode_read_pci_addr() > has NOT written *addr. The C standard leaves that read undefined; in > practice the returned value depends on the compilers stack layout and on > -ftrivial-auto-var-init. That value ends up in pplat->devfn (set by > pci_uclass_child_post_bind()) and is later compared for equality in > pci_bus_find_devfn() during PCI enumeration, so an under-determined > value produces an under-determined driver-binding outcome. > > Concretely, this fires on Raspberry Pi 5 (BCM2712) with vendor U-Boot > v2026.07 when the tree is compiled with GCC -ftrivial-auto-var-init=zero. > In that build addr.phys_hi is zeroed rather than left as stack junk, so > pci_get_devfn() returns 0 for every DT-declared non-PCI child of a PCI > bus. The Pi 5 device tree includes one such child under the second root > complex -- the rp1 simple-bus node representing the on-SoC RP1 south > bridge as seen from the OS side. Under zero-init: > > * pci_uclass_child_post_bind(rp1) sets pplat->devfn = 0 > [...] > > drivers/core/util.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > Reported-by: Dustin Kirkland <[email protected]> > Fixes: b52142004fbd ("pci: Add pci_get_devfn() to extract devfn from the > fdt_pci_addr") > Signed-off-by: Dustin Kirkland <[email protected]> > Investigated-by: Claude Opus 4.7 <[email protected]> > Co-authored-by: Claude Opus 4.7 <[email protected]> This is a good find! Please drop Reported-by when the reporter is also the Signed-off-by - also U-Boot doesn't accept AI contributions, so probably best to just remove those tags. Also not that useful to mention a vendor tree - we mostly just worry about mainline here. > diff --git a/drivers/core/util.c b/drivers/core/util.c > @@ -20,10 +20,8 @@ int pci_get_devfn(struct udevice *dev) > /* Extract the devfn from fdt_pci_addr */ > ret = ofnode_read_pci_addr(dev_ofnode(dev), FDT_PCI_SPACE_CONFIG, > "reg", &addr, NULL); > - if (ret) { > - if (ret != -ENOENT) > - return -EINVAL; > - } > + if (ret) > + return -EINVAL; > > return addr.phys_hi & 0xff00; > } The fix is right, but the kerneldoc in include/dm/pci.h still says: Return: devfn in bits 15...8 if found (pci_dev_t format), or -ENODEV if not found Since this patch makes the error path observable, please either update that comment to match (-EINVAL on any lookup failure) or return -ENODEV for the ret == -ENOENT case so the documented contract holds. I'd lean towards -ENODEV for the not-present case and -EINVAL only for a malformed reg - that also reads more naturally at the callers in arch/x86/cpu/apollolake/{pmc,hostbridge}.c and intel_common/p2sb.c, which only test < 0. What do you think? Regards, Simon [1] https://lore.kernel.org/u-boot/20251113133038.GF6688@bill-the-cat/
