From:   Alexander Stein <alexander.stein@ew.tq-group.com>
To:     Bjorn Helgaas <bhelgaas@google.com>,
        Oliver Neukum <oneukum@suse.com>,
        Oliver Neukum <oneukum@suse.com>
Cc:     Korneliusz Osmenda <korneliuszo@gmail.com>,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/1] Guard pci_create_sysfs_dev_files with atomic value
Date:   Thu, 16 Mar 2023 12:58:53 +0100
Message-ID: <4888964.44csPzL39Z@steina-w>
Organization: TQ-Systems GmbH
In-Reply-To: <612dfdd2-7de1-12a7-c47c-7569c3466224@suse.com>
References: <20230316091540.494366-1-alexander.stein@ew.tq-group.com> <6131694.LvFx2qVVIh@steina-w> <612dfdd2-7de1-12a7-c47c-7569c3466224@suse.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Precedence: bulk

Hi Oliver,

Am Donnerstag, 16. M=E4rz 2023, 12:17:32 CET schrieb Oliver Neukum:
> On 16.03.23 10:33, Alexander Stein wrote:
> > Hi Oliver,
>=20
> Hi,
>=20
> > Admittedly
> > I would like to get rid of these two pathes for creating sysfs files in
> > the
> > first place, but I do not know the pci subsystem very well.
> > IMHO for_each_pci_dev(pdev) in pci_sysfs_init is part of the problem as=
 it
> > unconditionally iterates over the bus, without any locks, thus creating
> > sysfs files for each device added to the bus.
> > Any ideas?
>=20
> First of all, this existing code is a mess.
>=20
> If I understand you have the issue that your driver adds a bridge
> in dw_pcie_host_init() and the generic code in pci_create_sysfs_dev_files=
()
> populates the directory before or while your driver does so and
> the devices are effectively discovered twice.

Yep, that's my observation as well.

> It seems to me that you must not add a bridge before
> pci_create_sysfs_dev_files() has finished. Now you could add a wait_queue
> and a flag and wait for it to finish. But that is not very elegant.

Do we need the pci_sysfs_init initcall at all? Or to put it in other words,=
=20
what does this initcall solve?
See my different approach eliminating this race at all.

> From which initcall is your driver probed?

The callstack looks like this:
> imx6_pcie_probe from platform_probe+0x5c/0xb8
> platform_probe from call_driver_probe+0x24/0x118
> call_driver_probe from really_probe+0xc4/0x31c
> really_probe from __driver_probe_device+0x8c/0x120
> __driver_probe_device from driver_probe_device+0x30/0xc0
> driver_probe_device from __driver_attach_async_helper+0x50/0xd8
> __driver_attach_async_helper from async_run_entry_fn+0x30/0x144
> async_run_entry_fn from process_one_work+0x1c4/0x3d0
> process_one_work from worker_thread+0x50/0x41c
> worker_thread from kthread+0xec/0x104
> kthread from ret_from_fork+0x14/0x2c

So technically the device is not probed from within a initcall but a kthrea=
d.=20
It is set to be probed asynchronous in imx6_pcie_driver.

This async call is scheduled in __driver_attach, from this callstack:
> __driver_attach from bus_for_each_dev+0x74/0xc8
> bus_for_each_dev from bus_add_driver+0xf0/0x1f4
> bus_add_driver from driver_register+0x7c/0x118
> driver_register from do_one_initcall+0x4c/0x180
> do_one_initcall from do_initcalls+0xe0/0x114
> do_initcalls from kernel_init_freeable+0xd8/0x100
> kernel_init_freeable from kernel_init+0x18/0x12c
> kernel_init from ret_from_fork+0x14/0x2c

Best regards,
Alexander
=2D-=20
TQ-Systems GmbH | M=FChlstra=DFe 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht M=FCnchen, HRB 105018
Gesch=E4ftsf=FChrer: Detlef Schneider, R=FCdiger Stahl, Stefan Schneider
http://www.tq-group.com/