| Message ID | 20240722151936.1452299-2-kbusch@meta.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | pci: rescan/remove locking rework | expand |
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>
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.
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 --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)