* [PATCH 1/3] PCI/sysfs: sort headers alphabetically
2023-04-27 14:28 [PATCH 0/3] PCI: Fix race condition upon sysfs init Alexander Stein
@ 2023-04-27 14:28 ` Alexander Stein
2023-04-27 14:29 ` [PATCH 2/3] PCI/sysfs: create private functions for pci_create_legacy_files/pci_create_sysfs_dev_files Alexander Stein
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Alexander Stein @ 2023-04-27 14:28 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Alexander Stein, linux-pci, linux-kernel, Korneliusz Osmenda,
Oliver Neukum
The includes were not sorted alphabetically. Sorts them properly.
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
drivers/pci/pci-sysfs.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index ab32a91f287b..289c1c17b41f 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -13,22 +13,22 @@
*/
-#include <linux/kernel.h>
-#include <linux/sched.h>
-#include <linux/pci.h>
-#include <linux/stat.h>
+#include <linux/aperture.h>
+#include <linux/capability.h>
#include <linux/export.h>
-#include <linux/topology.h>
-#include <linux/mm.h>
#include <linux/fs.h>
-#include <linux/capability.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/msi.h>
+#include <linux/of.h>
+#include <linux/pci.h>
+#include <linux/pm_runtime.h>
+#include <linux/sched.h>
#include <linux/security.h>
#include <linux/slab.h>
+#include <linux/stat.h>
+#include <linux/topology.h>
#include <linux/vgaarb.h>
-#include <linux/pm_runtime.h>
-#include <linux/msi.h>
-#include <linux/of.h>
-#include <linux/aperture.h>
#include "pci.h"
static int sysfs_initialized; /* = 0 */
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 2/3] PCI/sysfs: create private functions for pci_create_legacy_files/pci_create_sysfs_dev_files
2023-04-27 14:28 [PATCH 0/3] PCI: Fix race condition upon sysfs init Alexander Stein
2023-04-27 14:28 ` [PATCH 1/3] PCI/sysfs: sort headers alphabetically Alexander Stein
@ 2023-04-27 14:29 ` Alexander Stein
2023-04-27 20:38 ` kernel test robot
2023-05-07 9:05 ` kernel test robot
2023-04-27 14:29 ` [PATCH 3/3] PCI/sysfs: Fix sysfs init race condition Alexander Stein
2023-04-27 16:14 ` [PATCH 0/3] PCI: Fix race condition upon sysfs init Bjorn Helgaas
3 siblings, 2 replies; 7+ messages in thread
From: Alexander Stein @ 2023-04-27 14:29 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Alexander Stein, linux-pci, linux-kernel, Korneliusz Osmenda,
Oliver Neukum
The only difference is they don't have the check against sysfs_initialized.
This is a preparation for the sysfs init race condition fix.
No functional change intended.
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
drivers/pci/pci-sysfs.c | 44 ++++++++++++++++++++++++++---------------
1 file changed, 28 insertions(+), 16 deletions(-)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 289c1c17b41f..7d4733773633 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -938,24 +938,10 @@ void __weak pci_adjust_legacy_attr(struct pci_bus *b,
{
}
-/**
- * pci_create_legacy_files - create legacy I/O port and memory files
- * @b: bus to create files under
- *
- * Some platforms allow access to legacy I/O port and ISA memory space on
- * a per-bus basis. This routine creates the files and ties them into
- * their associated read, write and mmap files from pci-sysfs.c
- *
- * On error unwind, but don't propagate the error to the caller
- * as it is ok to set up the PCI bus without these files.
- */
-void pci_create_legacy_files(struct pci_bus *b)
+static void __pci_create_legacy_files(struct pci_bus *b)
{
int error;
- if (!sysfs_initialized)
- return;
-
b->legacy_io = kcalloc(2, sizeof(struct bin_attribute),
GFP_ATOMIC);
if (!b->legacy_io)
@@ -998,6 +984,25 @@ void pci_create_legacy_files(struct pci_bus *b)
dev_warn(&b->dev, "could not create legacy I/O port and ISA memory resources in sysfs\n");
}
+/**
+ * pci_create_legacy_files - create legacy I/O port and memory files
+ * @b: bus to create files under
+ *
+ * Some platforms allow access to legacy I/O port and ISA memory space on
+ * a per-bus basis. This routine creates the files and ties them into
+ * their associated read, write and mmap files from pci-sysfs.c
+ *
+ * On error unwind, but don't propagate the error to the caller
+ * as it is ok to set up the PCI bus without these files.
+ */
+void pci_create_legacy_files(struct pci_bus *b)
+{
+ if (!sysfs_initialized)
+ return;
+
+ __pci_create_legacy_files(b);
+}
+
void pci_remove_legacy_files(struct pci_bus *b)
{
if (b->legacy_io) {
@@ -1006,6 +1011,8 @@ void pci_remove_legacy_files(struct pci_bus *b)
kfree(b->legacy_io); /* both are allocated here */
}
}
+#else
+static void __pci_create_legacy_files(struct pci_bus *b) {}
#endif /* HAVE_PCI_LEGACY */
#if defined(HAVE_PCI_MMAP) || defined(ARCH_GENERIC_PCI_MMAP_RESOURCE)
@@ -1492,12 +1499,17 @@ static const struct attribute_group pci_dev_resource_resize_group = {
.is_visible = resource_resize_is_visible,
};
+int __must_check __pci_create_sysfs_dev_files(struct pci_dev *pdev)
+{
+ return pci_create_resource_files(pdev);
+}
+
int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
{
if (!sysfs_initialized)
return -EACCES;
- return pci_create_resource_files(pdev);
+ return __pci_create_sysfs_dev_files(pdev);
}
/**
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/3] PCI/sysfs: create private functions for pci_create_legacy_files/pci_create_sysfs_dev_files
2023-04-27 14:29 ` [PATCH 2/3] PCI/sysfs: create private functions for pci_create_legacy_files/pci_create_sysfs_dev_files Alexander Stein
@ 2023-04-27 20:38 ` kernel test robot
2023-05-07 9:05 ` kernel test robot
1 sibling, 0 replies; 7+ messages in thread
From: kernel test robot @ 2023-04-27 20:38 UTC (permalink / raw)
To: Alexander Stein, Bjorn Helgaas
Cc: oe-kbuild-all, Alexander Stein, linux-pci, linux-kernel,
Korneliusz Osmenda, Oliver Neukum
Hi Alexander,
kernel test robot noticed the following build warnings:
[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus linus/master v6.3 next-20230427]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Alexander-Stein/PCI-sysfs-sort-headers-alphabetically/20230427-223059
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link: https://lore.kernel.org/r/20230427142901.3570536-3-alexander.stein%40ew.tq-group.com
patch subject: [PATCH 2/3] PCI/sysfs: create private functions for pci_create_legacy_files/pci_create_sysfs_dev_files
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20230428/202304280450.17v5JK13-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/cbc778730116b49890b5c41ffc9fc664b566e3c4
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Alexander-Stein/PCI-sysfs-sort-headers-alphabetically/20230427-223059
git checkout cbc778730116b49890b5c41ffc9fc664b566e3c4
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/pci/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304280450.17v5JK13-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/pci/pci-sysfs.c:1256:12: warning: no previous prototype for 'pci_create_resource_files' [-Wmissing-prototypes]
1256 | int __weak pci_create_resource_files(struct pci_dev *dev) { return 0; }
| ^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/pci/pci-sysfs.c:1257:13: warning: no previous prototype for 'pci_remove_resource_files' [-Wmissing-prototypes]
1257 | void __weak pci_remove_resource_files(struct pci_dev *dev) { return; }
| ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/pci/pci-sysfs.c:1502:18: warning: no previous prototype for '__pci_create_sysfs_dev_files' [-Wmissing-prototypes]
1502 | int __must_check __pci_create_sysfs_dev_files(struct pci_dev *pdev)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/pci/pci-sysfs.c:1015:13: warning: '__pci_create_legacy_files' defined but not used [-Wunused-function]
1015 | static void __pci_create_legacy_files(struct pci_bus *b) {}
| ^~~~~~~~~~~~~~~~~~~~~~~~~
vim +/__pci_create_sysfs_dev_files +1502 drivers/pci/pci-sysfs.c
1501
> 1502 int __must_check __pci_create_sysfs_dev_files(struct pci_dev *pdev)
1503 {
1504 return pci_create_resource_files(pdev);
1505 }
1506
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 2/3] PCI/sysfs: create private functions for pci_create_legacy_files/pci_create_sysfs_dev_files
2023-04-27 14:29 ` [PATCH 2/3] PCI/sysfs: create private functions for pci_create_legacy_files/pci_create_sysfs_dev_files Alexander Stein
2023-04-27 20:38 ` kernel test robot
@ 2023-05-07 9:05 ` kernel test robot
1 sibling, 0 replies; 7+ messages in thread
From: kernel test robot @ 2023-05-07 9:05 UTC (permalink / raw)
To: Alexander Stein, Bjorn Helgaas
Cc: llvm, oe-kbuild-all, Alexander Stein, linux-pci, linux-kernel,
Korneliusz Osmenda, Oliver Neukum
Hi Alexander,
kernel test robot noticed the following build warnings:
[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus linus/master v6.3 next-20230505]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Alexander-Stein/PCI-sysfs-sort-headers-alphabetically/20230427-223059
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link: https://lore.kernel.org/r/20230427142901.3570536-3-alexander.stein%40ew.tq-group.com
patch subject: [PATCH 2/3] PCI/sysfs: create private functions for pci_create_legacy_files/pci_create_sysfs_dev_files
config: mips-maltaup_defconfig (https://download.01.org/0day-ci/archive/20230507/202305071649.zjynmg3E-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project b0fb98227c90adf2536c9ad644a74d5e92961111)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install mips cross compiling tool for clang build
# apt-get install binutils-mipsel-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/cbc778730116b49890b5c41ffc9fc664b566e3c4
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Alexander-Stein/PCI-sysfs-sort-headers-alphabetically/20230427-223059
git checkout cbc778730116b49890b5c41ffc9fc664b566e3c4
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash drivers/pci/ mm/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202305071649.zjynmg3E-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/pci/pci-sysfs.c:1502:18: warning: no previous prototype for function '__pci_create_sysfs_dev_files' [-Wmissing-prototypes]
int __must_check __pci_create_sysfs_dev_files(struct pci_dev *pdev)
^
drivers/pci/pci-sysfs.c:1502:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int __must_check __pci_create_sysfs_dev_files(struct pci_dev *pdev)
^
static
drivers/pci/pci-sysfs.c:1015:13: warning: unused function '__pci_create_legacy_files' [-Wunused-function]
static void __pci_create_legacy_files(struct pci_bus *b) {}
^
2 warnings generated.
vim +/__pci_create_sysfs_dev_files +1502 drivers/pci/pci-sysfs.c
1501
> 1502 int __must_check __pci_create_sysfs_dev_files(struct pci_dev *pdev)
1503 {
1504 return pci_create_resource_files(pdev);
1505 }
1506
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/3] PCI/sysfs: Fix sysfs init race condition
2023-04-27 14:28 [PATCH 0/3] PCI: Fix race condition upon sysfs init Alexander Stein
2023-04-27 14:28 ` [PATCH 1/3] PCI/sysfs: sort headers alphabetically Alexander Stein
2023-04-27 14:29 ` [PATCH 2/3] PCI/sysfs: create private functions for pci_create_legacy_files/pci_create_sysfs_dev_files Alexander Stein
@ 2023-04-27 14:29 ` Alexander Stein
2023-04-27 16:14 ` [PATCH 0/3] PCI: Fix race condition upon sysfs init Bjorn Helgaas
3 siblings, 0 replies; 7+ messages in thread
From: Alexander Stein @ 2023-04-27 14:29 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Alexander Stein, linux-pci, linux-kernel, Korneliusz Osmenda,
Oliver Neukum
sysfs attribute files for PCIe devices (pci_create_sysfs_dev_files) can be
created by two paths:
1. pci_sysfs_init()
2. pci_bus_add_device() (drivers/pci/bus.c)
There is a race during startup where an asynchronous PCIe host probe races
against the pci_sysfs_init() late_initcall. In this case the PCIe devices
are already added to the bus, for_each_pci_dev() will see them, but
pci_bus_add_device() has not yet finished, so both code paths try to add
the sysfs attributes.
Fix this by waiting on a workqueue until sysfs has been initialized.
pci_sysfs_init() needs the internal function without the check that
sysfs_initialized has been set to 1.
__pci_create_sysfs_dev_files still needs to remove resource files,
which might have been created during pci_sysfs_init initcall.
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
drivers/pci/pci-sysfs.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 7d4733773633..3067d55f981c 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -29,9 +29,11 @@
#include <linux/stat.h>
#include <linux/topology.h>
#include <linux/vgaarb.h>
+#include <linux/wait.h>
#include "pci.h"
static int sysfs_initialized; /* = 0 */
+static DECLARE_WAIT_QUEUE_HEAD(sysfs_wq);
/* show configuration fields */
#define pci_config_attr(field, format_string) \
@@ -997,8 +999,7 @@ static void __pci_create_legacy_files(struct pci_bus *b)
*/
void pci_create_legacy_files(struct pci_bus *b)
{
- if (!sysfs_initialized)
- return;
+ wait_event(sysfs_wq, sysfs_initialized);
__pci_create_legacy_files(b);
}
@@ -1501,13 +1502,18 @@ static const struct attribute_group pci_dev_resource_resize_group = {
int __must_check __pci_create_sysfs_dev_files(struct pci_dev *pdev)
{
+ /*
+ * sysfs attributes might already be created by pci_sysfs_init(),
+ * delete them here just in case
+ */
+ pci_remove_resource_files(pdev);
return pci_create_resource_files(pdev);
}
int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
{
- if (!sysfs_initialized)
- return -EACCES;
+ /* Wait until sysfs has been initialized */
+ wait_event(sysfs_wq, sysfs_initialized);
return __pci_create_sysfs_dev_files(pdev);
}
@@ -1520,8 +1526,8 @@ int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
*/
void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
{
- if (!sysfs_initialized)
- return;
+ /* Wait until sysfs has been initialized */
+ wait_event(sysfs_wq, sysfs_initialized);
pci_remove_resource_files(pdev);
}
@@ -1532,9 +1538,8 @@ static int __init pci_sysfs_init(void)
struct pci_bus *pbus = NULL;
int retval;
- sysfs_initialized = 1;
for_each_pci_dev(pdev) {
- retval = pci_create_sysfs_dev_files(pdev);
+ retval = __pci_create_sysfs_dev_files(pdev);
if (retval) {
pci_dev_put(pdev);
return retval;
@@ -1542,7 +1547,9 @@ static int __init pci_sysfs_init(void)
}
while ((pbus = pci_find_next_bus(pbus)))
- pci_create_legacy_files(pbus);
+ __pci_create_legacy_files(pbus);
+ sysfs_initialized = 1;
+ wake_up_all(&sysfs_wq);
return 0;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 0/3] PCI: Fix race condition upon sysfs init
2023-04-27 14:28 [PATCH 0/3] PCI: Fix race condition upon sysfs init Alexander Stein
` (2 preceding siblings ...)
2023-04-27 14:29 ` [PATCH 3/3] PCI/sysfs: Fix sysfs init race condition Alexander Stein
@ 2023-04-27 16:14 ` Bjorn Helgaas
3 siblings, 0 replies; 7+ messages in thread
From: Bjorn Helgaas @ 2023-04-27 16:14 UTC (permalink / raw)
To: Alexander Stein
Cc: Bjorn Helgaas, linux-pci, linux-kernel, Korneliusz Osmenda,
Oliver Neukum
On Thu, Apr 27, 2023 at 04:28:58PM +0200, Alexander Stein wrote:
> Hi everyone,
>
> this series is a totally different approach for fixing the sysfs init race
> condition. The initial problem is stated at [1]. Previous proposals were
> rejected ([2] and [3]). Here is what's happening
>
>
> CPU 0 CPU 1
>
> imx6_pcie_probe()
> dw_pcie_host_init()
> pci_host_probe()
> pci_scan_root_bus_bridge()
> pci_scan_child_bus_extend()
> pci_scan_slot()
> pci_scan_single_device()
> pci_device_add()
> pci_sysfs_init() device_add()
> sysfs_initialized = 1; bus_add_device()
> for_each_pci_dev() ...
> pci_create_sysfs_dev_files()
> pci_bus_add_devices()
> pci_bus_add_device()
> pci_create_sysfs_dev_files()
>
> Eventually calling pci_create_sysfs_dev_files() twice on the same pci_dev.
> It's a very tight window, deeper PCIe trees increase that window during
> host probe. Asynchronous PCIe host probe is a necessity
> (PROBE_PREFER_ASYNCHRONOUS).
>
> The first two patches are preparations for the last one actually fixing
> the race. As functions like pci_create_sysfs_dev_files() are called from
> externtal and internal to pci-sysfs, an internal version without checking
> for sysfs_initialized is required.
> For the fix a wait queue is introduced where all callers from external
> callsites (regarding pci-sysfs.c) are waiting until pci_sysfs_init
> initcall has finished and woken up all waiters.
>
> A subtlety is that within __pci_create_sysfs_dev_files the resource files
> (created by pci_sysfs_init) need to be removed, so they can be created
> again from pci_host_probe call.
I'll look at this in more detail, but if there's any way at all that
we could get rid of pci_sysfs_init() completely and do this with
static attributes or some other existing sysfs infrastructure, I would
STRONGLY prefer it because that infrastructure has already solved this
problem.
Maybe that's impossible and we really need to make a one-off solution
just for PCI, but ... I haven't been convinced yet.
> Links:
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=215515
> [2] https://lore.kernel.org/linux-pci/20230316091540.494366-1-alexander.stein@ew.tq-group.com/
> [3] https://lore.kernel.org/linux-pci/20230316103036.1837869-1-alexander.stein@ew.tq-group.com/
>
> Alexander Stein (3):
> PCI/sysfs: sort headers alphabetically
> PCI/sysfs: create private functions for
> pci_create_legacy_files/pci_create_sysfs_dev_files
> PCI/sysfs: Fix sysfs init race condition
>
> drivers/pci/pci-sysfs.c | 87 +++++++++++++++++++++++++----------------
> 1 file changed, 53 insertions(+), 34 deletions(-)
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread