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
