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

[Davidlohr Bueso <dave@xxxxxxxxxxxx>]

On Tue, 27 Aug 2024, Keith Busch wrote:

> 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 0e9b1c7b94a5a..802f7eac53115 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);
> +}

So set_bit does not imply any barriers. Does this matter in the future
when breaking up pci_rescan_remove_lock? For example, what prevents
things like:

pci_bus_add_device()			pci_stop_dev()
    pci_dev_assign_added()
	dev->priv_flags [S]
					    pci_dev_test_and_clear_added() // true
						dev->priv_flags [L]
    device_attach(&dev->dev)
					    device_release_driver(&dev->dev)

... I guess that implied barrier from that device_lock() in device_attach().
I am not familiar with this code, but overall I think any locking rework should
explain more about the ordering implications in the changes if the end result
is finer grained locking.

Thanks,
Davidlohr