From:   Alexander Stein <alexander.stein@ew.tq-group.com>
To:     Bjorn Helgaas <bhelgaas@google.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 10:33:38 +0100
Message-ID: <6131694.LvFx2qVVIh@steina-w>
Organization: TQ-Systems GmbH
In-Reply-To: <106b5618-908f-becc-6eb3-75ef136a48e4@suse.com>
References: <20230316091540.494366-1-alexander.stein@ew.tq-group.com> <106b5618-908f-becc-6eb3-75ef136a48e4@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, 10:23:54 CET schrieb Oliver Neukum:
> On 16.03.23 10:15, Alexander Stein wrote:
> > From: Korneliusz Osmenda <korneliuszo@gmail.com>
> >=20
> > On Gateworks Ventana there is a number of PCI devices and:
> >    - imx6_pcie_probe takes longer than start of late init
> >    - pci_sysfs_init sets up flag sysfs_initialized
> >    - pci_sysfs_init initializes already found devices
> >    - imx6_pcie_probe tries to reinitialize device
> >=20
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=3D215515
> >=20
> > Signed-off-by: Korneliusz Osmenda <korneliuszo@gmail.com>
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> >=20
> >   drivers/pci/pci-sysfs.c | 6 ++++++
> >   include/linux/pci.h     | 2 ++
> >   2 files changed, 8 insertions(+)
> >=20
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index dd0d9d9bc509..998e44716b6f 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -1497,6 +1497,9 @@ int __must_check pci_create_sysfs_dev_files(struct
> > pci_dev *pdev)>=20
> >   	if (!sysfs_initialized)
> >   =09
> >   		return -EACCES;
> >=20
> > +	if (atomic_cmpxchg(&pdev->sysfs_init_cnt, 0, 1) =3D=3D 1)
> > +		return 0;		/* already added */
> > +
> >=20
> >   	return pci_create_resource_files(pdev);
>=20
> This is very likely a bug. You are returning an error in the error
> case. Yet the flag stays.

Ah, you are right. This is something needed to address.

> And simply resetting it in the error case
> would be a race. There is something fishy in that design.

Admittedly=20
I would like to get rid of these two pathes for creating sysfs files in the=
=20
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=
=20
unconditionally iterates over the bus, without any locks, thus creating sys=
fs=20
files for each device added to the bus.
Any ideas?

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/