Received-SPF: pass (google.com: domain of linux-kernel+bounces-139382-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20231215145207.0cf098e5@bootlin.com> <20240319152513.GA1227721@bhelgaas>
 <20240319173404.019b424a@bootlin.com>
In-Reply-To: <20240319173404.019b424a@bootlin.com>
From: Rob Herring <robh@kernel.org>
Date: Wed, 10 Apr 2024 16:41:43 -0500
Message-ID: <CAL_JsqJOgkJtOipzskWc_NzUYUE1g0VsTT3eyPbbKw=NZO_4aQ@mail.gmail.com>
Subject: Re: [PATCH v2 0/2] Attach DT nodes to existing PCI devices
To: Herve Codina <herve.codina@bootlin.com>
Cc: Bjorn Helgaas <helgaas@kernel.org>, 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>, 
	=?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= <kwilczynski@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Mar 19, 2024 at 11:34=E2=80=AFAM Herve Codina <herve.codina@bootlin=
com> wrote:
>
> Hi Bjorn,
>
> On Tue, 19 Mar 2024 10:25:13 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> > [+cc Krzysztof]
> >
> > On Fri, Dec 15, 2023 at 02:52:07PM +0100, Herve Codina wrote:
> > > On Mon, 4 Dec 2023 07:59:09 -0600
> > > Rob Herring <robh@kernel.org> wrote:
> > > > On Mon, Dec 4, 2023 at 6:43=E2=80=AFAM Herve Codina <herve.codina@b=
ootlin.com> wrote:
> > > > > On Fri, 1 Dec 2023 16:26:45 -0600
> > > > > Rob Herring <robh@kernel.org> wrote:
> > > > > > On Thu, Nov 30, 2023 at 10:57=E2=80=AFAM Herve Codina <herve.co=
dina@bootlin.com> wrote:
> > > > > > ...
> >
> > > > > > --- 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=
, 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 b=
ased on
> > > > > some already set values available in the PCI device such as its s=
truct resource
> > > > > values.
> > > > > We need to have some values set by the PCI infra in order to crea=
te 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 befor=
e
> > > > 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_n=
ode
> > > > 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 t=
he
> > > > DT node is not set before device_add().
> > >
> > > DECLARE_PCI_FIXUP_HEADER is too early as well as doing the DT node cr=
eation
> > > just before the device_add() call.
> > > Indeed, in order to fill the DT properties, resources need to be assi=
gned
> > > (needed for the 'ranges' property used for addresses translation).
> > > The resources assignment is done after the call to device_add().
> >
> > Do we need to know the actual address *value* before creating the
> > sysfs file, or is it enough to know that the file should *exist*, even
> > if the value may be changed later?
>
> I think, the problematic file is 'of_node'.
> This file is a symlink present in the device directory pointing to the
> node in a device-tree subdir.
>
> How can we create this of_node symlink without having the device-tree
> subdir available ?
>
> >
> > > Some PCI sysfs files are already created after adding the device by t=
he
> > > pci_create_sysfs_dev_files() call:
> > >   https://elixir.bootlin.com/linux/v6.6/source/drivers/pci/bus.c#L347
> > >
> > > Is it really an issue to add the of_node link to sysfs on an already
> > > present device ?
> >
> > Yes, I think this would be an issue.  We've been trying to get rid of
> > pci_create_sysfs_dev_files() altogether because there's a long history
> > of race issues related to it:
> >
> >   https://lore.kernel.org/r/1271099285.9831.13.camel@localhost/ WARNING=
: at fs/sysfs/dir.c:451 sysfs_add_one: sysfs: cannot create duplicate filen=
ame '/devices/pci0000:00/0000:00:01.0/slot'
> >   https://lore.kernel.org/r/19461.26166.427857.612983@pilspetsen.it.uu.=
se/ [2.6.35-rc1 regression] sysfs: cannot create duplicate filename ... XVR=
-600 related?
> >   https://lore.kernel.org/r/20200716110423.xtfyb3n6tn5ixedh@pali/ PCI: =
Race condition in pci_create_sysfs_dev_files
> >   https://lore.kernel.org/r/m3eebg9puj.fsf@t19.piap.pl/ PCI: Race condi=
tion in pci_create_sysfs_dev_files (can't boot)
> >   https://bugzilla.kernel.org/show_bug.cgi?id=3D215515 sysfs: cannot cr=
eate duplicate filename '.../0000:e0'
> >
> > And several previous attempts to fix them:
> >
> >   https://lore.kernel.org/r/4469eba2-188b-aab7-07d1-5c77313fc42f@gmail.=
com/ Guard pci_create_sysfs_dev_files with atomic value
> >   https://lore.kernel.org/r/20230316103036.1837869-1-alexander.stein@ew=
tq-group.com PCI/sysfs: get rid of pci_sysfs_init late_initcall
> >   https://lore.kernel.org/r/1702093576-30405-1-git-send-email-ssengar@l=
inux.microsoft.com/ PCI/sysfs: Fix race in pci sysfs creation
> >
>
> I am not sure we are facing in the same kind of issues.
> The ones you mentioned are related to some sysfs duplication.
> In the of_node case, the issue (if any) is that the symlink will be creat=
ed
> after the other device's file. Not sure that it can lead to some file
> duplication.

Again, if you notify userspace and it wants to make some decisions
based on of_node, then it has to be there when the notification
happens. As Greg says frequently, you've raced with userspace and
lost.

I imagine Bjorn won't like it, but may we need another fixup point?

Rob