On Mon, Dec 4, 2023 at 9:30 AM Herve Codina <herve.codina@xxxxxxxxxxx> wrote: > > Hi Rob, > > On Mon, 4 Dec 2023 07:59:09 -0600 > Rob Herring <robh@xxxxxxxxxx> wrote: > > [...] > > > > > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c > > > > index 9c2137dae429..46b252bbe500 100644 > > > > --- a/drivers/pci/bus.c > > > > +++ b/drivers/pci/bus.c > > > > @@ -342,8 +342,6 @@ void pci_bus_add_device(struct pci_dev *dev) > > > > */ > > > > pcibios_bus_add_device(dev); > > > > pci_fixup_device(pci_fixup_final, dev); > > > > - if (pci_is_bridge(dev)) > > > > - of_pci_make_dev_node(dev); > > > > pci_create_sysfs_dev_files(dev); > > > > pci_proc_attach_device(dev); > > > > pci_bridge_d3_update(dev); > > > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > > > > index 51e3dd0ea5ab..e15eaf0127fc 100644 > > > > --- a/drivers/pci/of.c > > > > +++ b/drivers/pci/of.c > > > > @@ -31,6 +31,8 @@ int pci_set_of_node(struct pci_dev *dev) > > > > return 0; > > > > > > > > node = of_pci_find_child_device(dev->bus->dev.of_node, dev->devfn); > > > > + if (!node && pci_is_bridge(dev)) > > > > + of_pci_make_dev_node(dev); > > > > if (!node) > > > > return 0; > > > > > > Maybe it is too early. > > > of_pci_make_dev_node() creates a node and fills some properties based on > > > some already set values available in the PCI device such as its struct resource > > > values. > > > We need to have some values set by the PCI infra in order to create our DT node > > > with correct values. > > > > Indeed, that's probably the issue I'm having. In that case, > > DECLARE_PCI_FIXUP_HEADER should work. That's later, but still before > > device_add(). > > > > I think modifying sysfs after device_add() is going to race with > > userspace. Userspace is notified of a new device, and then the of_node > > link may or may not be there when it reads sysfs. Also, not sure if > > we'll need DT modaliases with PCI devices, but they won't work if the > > DT node is not set before device_add(). > > Ok, we can try using DECLARE_PCI_FIXUP_HEADER. > On your side, is moving from DECLARE_PCI_FIXUP_EARLY to DECLARE_PCI_FIXUP_HEADER > fix your QEMU unittest ? No... And testing the bridge part crashes. That's because there's a dependency on the bridge->subordinate to write out bus-range and interrupt-map. I think the fix there is we should just not write those properties. The bus range isn't needed because the kernel does its own assignments. For interrupt-map, it is only needed if "interrupts" is present in the child devices. If not present, then the standard PCI swizzling is used. Alternatively, I think the interrupt mapping could be simplified to just implement the standard swizzling at each level which isn't dependent on any of the devices on the bus. I gave that a go where each interrupt-map just points to the parent bridge, but ran into an issue that the bridge nodes don't have a phandle. That should be fixable, but I'd rather go with the first option. I suppose that depends on how the interrupts downstream of the PCI device need to get resolved. It could be that the PCI device serves as the interrupt controller and can resolve the parent interrupt on its own (which may be needed for ACPI host anyways). > We have to note that between the pci_fixup_device(pci_fixup_header, dev) call > and the device_add() call, the call to pci_set_msi_domain() is present. > MSIs are not supported currently but in the future ... MSI's aren't ever described in PCI nodes. Only the host bridge. So I don't think we should have problems there. > Related to DT modaliases, I don't think they are needed. > All drivers related to PCI device should be declared as pci_driver. > Correct me if I am wrong but I think that the core PCI will load the correct > module without any DT modalias. Yes, you are probably right. Rob
