Re: [PATCHv3 1/5] pci: make pci_stop_dev concurrent safe

[Keith Busch <kbusch@kernel.org>]

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.