| Message ID | 20241022224851.340648-2-kbusch@meta.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | pci cleanup/prep patches | expand |
On Tue, Oct 22, 2024 at 03:48:47PM -0700, Keith Busch wrote: > --- a/drivers/pci/remove.c > +++ b/drivers/pci/remove.c > @@ -31,18 +31,16 @@ static int pci_pwrctl_unregister(struct device *dev, void *data) > > static void pci_stop_dev(struct pci_dev *dev) > { > - pci_pme_active(dev, false); > - > - if (pci_dev_is_added(dev)) { > - device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev), > - pci_pwrctl_unregister); > - 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); > + device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev), > + pci_pwrctl_unregister); > + device_release_driver(&dev->dev); > + pci_proc_detach_device(dev); > + pci_remove_sysfs_dev_files(dev); > + of_pci_remove_node(dev); > } The above is now queued for v6.13 as commit 6d6d962a8dc2 on pci/locking. I note there's a behavioral change here: Previously "pci_pme_active(dev, false)" was called unconditionally, now only if the "added" flag has been set. The commit message doesn't explain why this change is fine, so perhaps it's inadvertent? Thanks, Lukas
On Thu, Nov 07, 2024 at 03:06:57PM +0100, Lukas Wunner wrote: > On Tue, Oct 22, 2024 at 03:48:47PM -0700, Keith Busch wrote: > > --- a/drivers/pci/remove.c > > +++ b/drivers/pci/remove.c > > @@ -31,18 +31,16 @@ static int pci_pwrctl_unregister(struct device *dev, void *data) > > > > static void pci_stop_dev(struct pci_dev *dev) > > { > > - pci_pme_active(dev, false); > > - > > - if (pci_dev_is_added(dev)) { > > - device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev), > > - pci_pwrctl_unregister); > > - 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); > > + device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev), > > + pci_pwrctl_unregister); > > + device_release_driver(&dev->dev); > > + pci_proc_detach_device(dev); > > + pci_remove_sysfs_dev_files(dev); > > + of_pci_remove_node(dev); > > } > > The above is now queued for v6.13 as commit 6d6d962a8dc2 on pci/locking. > > I note there's a behavioral change here: > > Previously "pci_pme_active(dev, false)" was called unconditionally, > now only if the "added" flag has been set. The commit message > doesn't explain why this change is fine, so perhaps it's inadvertent? Hm, not exactly intentional. It doesn't appear to accomplish anything to call it multiple times, but it also looks hamrless to do so. Looking at the history of this, it looks like it was purposefully done unconditionally with the understanding it's "safe" to do that. With that in mind, I'm happy to move it back where it was.
On Thu, Nov 07, 2024 at 08:56:26AM -0700, Keith Busch wrote: > On Thu, Nov 07, 2024 at 03:06:57PM +0100, Lukas Wunner wrote: > > On Tue, Oct 22, 2024 at 03:48:47PM -0700, Keith Busch wrote: > > > --- a/drivers/pci/remove.c > > > +++ b/drivers/pci/remove.c > > > @@ -31,18 +31,16 @@ static int pci_pwrctl_unregister(struct device *dev, void *data) > > > > > > static void pci_stop_dev(struct pci_dev *dev) > > > { > > > - pci_pme_active(dev, false); > > > - > > > - if (pci_dev_is_added(dev)) { > > > - device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev), > > > - pci_pwrctl_unregister); > > > - 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); > > > + device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev), > > > + pci_pwrctl_unregister); > > > + device_release_driver(&dev->dev); > > > + pci_proc_detach_device(dev); > > > + pci_remove_sysfs_dev_files(dev); > > > + of_pci_remove_node(dev); > > > } > > > > The above is now queued for v6.13 as commit 6d6d962a8dc2 on pci/locking. > > > > I note there's a behavioral change here: > > > > Previously "pci_pme_active(dev, false)" was called unconditionally, > > now only if the "added" flag has been set. The commit message > > doesn't explain why this change is fine, so perhaps it's inadvertent? > > Hm, not exactly intentional. It doesn't appear to accomplish anything to > call it multiple times, but it also looks hamrless to do so. Looking at > the history of this, it looks like it was purposefully done > unconditionally with the understanding it's "safe" to do that. With that > in mind, I'm happy to move it back where it was. Yes I think it would be good if you could submit a fixup that Bjorn could fold into 6d6d962a8dc2, just to minimize regression potential. Thanks, Lukas
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c index e0a2441be6d32..aec08e81abff7 100644 --- a/drivers/pci/bus.c +++ b/drivers/pci/bus.c @@ -358,7 +358,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 d89fdbf04f363..c4cceec006d0d 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -470,9 +470,16 @@ 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); + smp_mb__before_atomic(); + set_bit(PCI_DEV_ADDED, &dev->priv_flags); + smp_mb__after_atomic(); +} + +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 e4ce1145aa3ee..e0d4402ec3391 100644 --- a/drivers/pci/remove.c +++ b/drivers/pci/remove.c @@ -31,18 +31,16 @@ static int pci_pwrctl_unregister(struct device *dev, void *data) static void pci_stop_dev(struct pci_dev *dev) { - pci_pme_active(dev, false); - - if (pci_dev_is_added(dev)) { - device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev), - pci_pwrctl_unregister); - 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); + device_for_each_child(dev->dev.parent, dev_of_node(&dev->dev), + pci_pwrctl_unregister); + 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)