Re: [REGRESSION] PCI: Dynamic OF node creation hangs on invalid bridge configuration

[Andrea della Porta]

Hi Angel,

On 23:01 Fri 11 Sep     , Angel J wrote:
> Hi Andrea, Hervé, Bjorn, Thorsten,
> 
> Thanks for the guidance, it was very useful. I went back through the tests
> and got some more insight into the hang.
> 
> Andrea wrote:
> > Angel, could you please provide the following output:
> >
> > lspci -nn
> > setpci -s 00:00.0 HEADER_TYPE
> >
> > from a running (i.e. with CONFIG_PCI_DYNAMIC_OF_NODES=n) system?
> 
> On Linux 6.12.109, with CONFIG_PCI_DYNAMIC_OF_NODES disabled, the relevant
> output is:
> 
>   $ lspci -nn
>   00:00.0 PCI bridge [0604]: Intel Corporation Device [8086:4c43] (rev 01)
> 
>   $ setpci -s 00:00.0 HEADER_TYPE
>   01
> 
>   $ setpci -s 00:00.0 PRIMARY_BUS SECONDARY_BUS SUBORDINATE_BUS
>   ff
>   ff
>   ff

So it seems that a device which is reported as a PCI bridge does not manage any
bus underneath.

> 
> The device is bound to icl_uncore. I get the same values on the patched
> kernel with CONFIG_PCI_DYNAMIC_OF_NODES=y.
> 
> Hervé wrote:
> > Is this PCI logs reported with PCI_DYNAMIC_OF_NODES=y or PCI_DYNAMIC_OF_NODES=n
> > or always whatever the PCI_DYNAMIC_OF_NODES Kconfig value ?
> 
> The messages appear on successful boots with either setting:
> 
>   pci 0000:00:00.0: [8086:4c43] type 01 class 0x060400 conventional PCI bridge
>   pci 0000:00:00.0: bridge configuration invalid ([bus ff-ff]), reconfiguring
> 
> I confirmed this on 6.12.107 with the option disabled and 6.12.108 with it
> enabled. They also appear on the patched 6.18.44 kernel.
> 
> Bjorn wrote:
> > I don't think 49d63971f963 ("misc: rp1: RaspberryPi RP1 misc driver")
> > is a likely culprit by itself because there's just nothing there that
> > looks like it would relate to a Dell XPS 8940.
> 
> That change exposed the problem by enabling PCI_DYNAMIC_OF_NODES in my
> configuration. I bisected again with the option enabled throughout and
> found an earlier boundary:
> 
>   3dc8adeeefa0  PCI: of_property: Constify parameter in of_pci_get_addr_flags()
>   1f340724419e  PCI: of: Create device tree PCI host bridge node
> 
> The first is the direct parent of the second. I boot-tested both without
> any guard, changing only the source commit; their generated kernel
> configurations are identical.
> 
> All of these tests have CONFIG_PCI_DYNAMIC_OF_NODES=y:
> 
>   Source         Change             Result
>   3dc8adeeefa0    None               Boots
>   1f340724419e    None               Hangs
>   1f340724419e    Subordinate guard  Boots
>   6.18.44        None               Hangs
>   6.18.44        Subordinate guard  Boots
> 
> Hervé wrote:
> > Maybe the test done at [1] should be improved to detect those wrong bridges.
> > and skip the of_pci_make_dev_node() call when a wrong bridge is detected.
> 
> I added logging to the first bad commit to check the bridge scan. It shows
> that 00:00.0 has no subordinate bus after either pass, despite satisfying
> pci_is_bridge():
> 
>   pci 0000:00:00.0: PCI OF debug: scan pass 0, buses ff/ff/ff
>   pci 0000:00:00.0: PCI OF debug: scan pass 0 done, subordinate bus absent
>   pci 0000:00:00.0: PCI OF debug: scan pass 1, buses ff/ff/ff
>   pci 0000:00:00.0: PCI OF debug: scan pass 1 done, subordinate bus absent

This is confirmed by the BIOS/fw not filling the bus range and by the kernel
failing to reallocate the bus since the range registers are probably read-only.
Apparently this is not a bridge, it should be at most a host controller.

> 
> of_pci_prop_bus_range() dereferences pdev->subordinate without checking it.
> of_pci_prop_intr_map() also uses that pointer. Before 1f340724419e,
> of_pci_make_dev_node() returns because the parent OF node is missing on
> this ACPI system. That commit creates the parent node, allowing property
> generation to reach the unchecked access.
> 
> My earlier report overstated the device_type="pci" result. The minimal-node
> test that hung added device_type, bus-range and interrupt-map together;
> I haven't confirmed a hang with device_type alone.
> 
> I tested this guard in of_pci_make_dev_node(), before node creation:
> 
>   if (pci_is_bridge(pdev) && !pdev->subordinate)
>           return;

Not sure, maybe can it be considered a hw bug? If this is the case, we can maybe
add a quirk for this device. Could you please test adding a quirk as PCI_FIXUP_HEADER
in which you downgrade the class from 0604 to 0600 and see it works even without
your proposed check in of_pci_make_dev_node()?

If this works, maybe we can just turn the body of your conditional into just an error
log plus fast exit, because a bridge must have valid subordinate.

Many thanks,
Andrea

> Both the first bad commit and 6.18.44 boot with it. The 6.18.44 test uses
> only the guard, without diagnostic logging. The host OF node and the nodes
> for bridges 00:01.0 and 00:1c.0 are still created; 00:00.0 is skipped.
> 
> This points to the NULL subordinate pointer as the cause of the hang,
> although I still don't have a crash trace from an unguarded boot. I'll
> send the patch as a reply to this email. Is of_pci_make_dev_node() the
> right place for this check?
> 
> #regzbot introduced: 1f340724419eda8ab07a20edcaf5ec8f70134231
> 
> Thanks,
> Angel J