diff mbox series

[RFC,1/8] pci: make pci_stop_dev concurrent safe

Message ID 20240722151936.1452299-2-kbusch@meta.com
State New
Headers show
Series pci: rescan/remove locking rework | expand

Commit Message

Keith Busch July 22, 2024, 3:19 p.m. UTC 
From: Keith Busch <kbusch@kernel.org>

Use the atomic ADDED flag to safely ensure concurrent callers can't
attempt to stop the device multiple times.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/pci/bus.c    |  2 +-
 drivers/pci/pci.h    |  9 +++++++--
 drivers/pci/remove.c | 18 ++++++++----------
 3 files changed, 16 insertions(+), 13 deletions(-)

Comments

Jonathan Cameron Aug. 15, 2024, 2:17 p.m. UTC | #1 
On Mon, 22 Jul 2024 08:19:29 -0700
Keith Busch <kbusch@meta.com> wrote:

> From: Keith Busch <kbusch@kernel.org>
> 
> Use the atomic ADDED flag to safely ensure concurrent callers can't
> attempt to stop the device multiple times.

Maybe mention what concurrent paths exist where this might happen.

> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
Change looks sensible anyway. FWIW as I'm not an expert in these paths.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Keith Busch Aug. 20, 2024, 3:02 p.m. UTC | #2 
On Thu, Aug 15, 2024 at 03:17:17PM +0100, Jonathan Cameron wrote:
> On Mon, 22 Jul 2024 08:19:29 -0700
> Keith Busch <kbusch@meta.com> wrote:
> 
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > Use the atomic ADDED flag to safely ensure concurrent callers can't
> > attempt to stop the device multiple times.
> 
> Maybe mention what concurrent paths exist where this might happen.

I think everyone calling this is holding the pci_rescan_remove_lock, so
it shouldn't be possible today. This series aims to remove that lock
though, so this is more of a prep patch for that. But also, the flag is
already an atomic type, so using those properties makes sense on its own
too.
Jonathan Cameron Aug. 21, 2024, 11:01 a.m. UTC | #3 
On Tue, 20 Aug 2024 09:02:20 -0600
Keith Busch <kbusch@kernel.org> wrote:

> On Thu, Aug 15, 2024 at 03:17:17PM +0100, Jonathan Cameron wrote:
> > On Mon, 22 Jul 2024 08:19:29 -0700
> > Keith Busch <kbusch@meta.com> wrote:
> >   
> > > From: Keith Busch <kbusch@kernel.org>
> > > 
> > > Use the atomic ADDED flag to safely ensure concurrent callers can't
> > > attempt to stop the device multiple times.  
> > 
> > Maybe mention what concurrent paths exist where this might happen.  
> 
> I think everyone calling this is holding the pci_rescan_remove_lock, so
> it shouldn't be possible today. This series aims to remove that lock
> though, so this is more of a prep patch for that. But also, the flag is
> already an atomic type, so using those properties makes sense on its own
> too.

Ok. Perhaps mention that it's cleanup / a prep patch rather than a fix.
The current text scared me a bit ;)

Jonathan
diff mbox series

Patch

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 55c8536860518..e41dfece0d969 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -348,7 +348,7 @@  void pci_bus_add_device(struct pci_dev *dev)
 	if (retval < 0 && retval != -EPROBE_DEFER)
 		pci_warn(dev, "device attach failed (%d)\n", retval);
 
-	pci_dev_assign_added(dev, true);
+	pci_dev_assign_added(dev);
 
 	if (dev_of_node(&dev->dev) && pci_is_bridge(dev)) {
 		retval = of_platform_populate(dev_of_node(&dev->dev), NULL, NULL,
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 79c8398f39384..171dfd6f14e6e 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -444,9 +444,14 @@  static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
 #define PCI_DPC_RECOVERED 1
 #define PCI_DPC_RECOVERING 2
 
-static inline void pci_dev_assign_added(struct pci_dev *dev, bool added)
+static inline void pci_dev_assign_added(struct pci_dev *dev)
 {
-	assign_bit(PCI_DEV_ADDED, &dev->priv_flags, added);
+	set_bit(PCI_DEV_ADDED, &dev->priv_flags);
+}
+
+static inline bool pci_dev_test_and_clear_added(struct pci_dev *dev)
+{
+	return test_and_clear_bit(PCI_DEV_ADDED, &dev->priv_flags);
 }
 
 static inline bool pci_dev_is_added(const struct pci_dev *dev)
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 910387e5bdbf9..ec3064a115bf8 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -16,17 +16,15 @@  static void pci_free_resources(struct pci_dev *dev)
 
 static void pci_stop_dev(struct pci_dev *dev)
 {
-	pci_pme_active(dev, false);
-
-	if (pci_dev_is_added(dev)) {
-		of_platform_depopulate(&dev->dev);
-		device_release_driver(&dev->dev);
-		pci_proc_detach_device(dev);
-		pci_remove_sysfs_dev_files(dev);
-		of_pci_remove_node(dev);
+	if (!pci_dev_test_and_clear_added(dev))
+		return;
 
-		pci_dev_assign_added(dev, false);
-	}
+	pci_pme_active(dev, false);
+	of_platform_depopulate(&dev->dev);
+	device_release_driver(&dev->dev);
+	pci_proc_detach_device(dev);
+	pci_remove_sysfs_dev_files(dev);
+	of_pci_remove_node(dev);
 }
 
 static void pci_destroy_dev(struct pci_dev *dev)