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

[Lukas Wunner <lukas@wunner.de>]

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