Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
MIME-Version: 1.0
References: <20231130165700.685764-1-herve.codina@bootlin.com>
 <CAL_JsqJvt6FpXK+FgAwE8xN3G5Z23Ktq=SEY-K7VA7nM5XgZRg@mail.gmail.com>
 <20231204134335.3ded3d46@bootlin.com> <CAL_JsqLtCS3otZ1sfiPEWwrWB4dyNpu4e0xANWJriCEUYr+4Og@mail.gmail.com>
 <20231204163014.4da383f2@bootlin.com>
In-Reply-To: <20231204163014.4da383f2@bootlin.com>
From:   Rob Herring <robh@kernel.org>
Date:   Mon, 4 Dec 2023 17:03:21 -0600
Message-ID: <CAL_JsqJJ64513pyQggU71agTzawNWPpm6ZpWMB6e0zu-tWL8yw@mail.gmail.com>
Subject: Re: [PATCH v2 0/2] Attach DT nodes to existing PCI devices
To:     Herve Codina <herve.codina@bootlin.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Lizhi Hou <lizhi.hou@amd.com>, Max Zhen <max.zhen@amd.com>,
        Sonal Santan <sonal.santan@amd.com>,
        Stefano Stabellini <stefano.stabellini@xilinx.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        PCI <linux-pci@vger.kernel.org>,
        Allan Nielsen <allan.nielsen@microchip.com>,
        Horatiu Vultur <horatiu.vultur@microchip.com>,
        Steen Hegelund <steen.hegelund@microchip.com>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Dec 4, 2023 at 9:30=E2=80=AFAM Herve Codina <herve.codina@bootlin.c=
om> wrote:
>
> Hi Rob,
>
> On Mon, 4 Dec 2023 07:59:09 -0600
> Rob Herring <robh@kernel.org> 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 =3D of_pci_find_child_device(dev->bus->dev.of_node, de=
v->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 struc=
t resource
> > > values.
> > > We need to have some values set by the PCI infra in order to create o=
ur 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 corr=
ect
> module without any DT modalias.

Yes, you are probably right.

Rob