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

[Angel J]

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

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

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;

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

Attachment: signature.asc
Description: OpenPGP digital signature