Re: [PATCH] PCI: Add lock and reference count in pci_get_domain_bus_and_slot()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Wed, 1 Oct 2025, Kaushlendra Kumar wrote:

> Add proper locking and reference counting to pci_get_domain_bus_and_slot

Add () to any function names.

> during PCI device enumeration. The function now holds pci_bus_sem during
> device iteration and properly increments the device reference count
> before returning.

These two sentences duplicate each other. Could you state things just once 
with the more precise explanation that is in the 2nd sentence.

Also, I'd prefer to see the problem described in a paragraph preceeding 
this solution paragraph/sentence.

Fixes tag?

> Signed-off-by: Kaushlendra Kumar <kaushlendra.kumar@intel.com>
> ---
>  drivers/pci/search.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/search.c b/drivers/pci/search.c
> index 53840634fbfc..dc49d3db69a4 100644
> --- a/drivers/pci/search.c
> +++ b/drivers/pci/search.c
> @@ -230,12 +230,17 @@ struct pci_dev *pci_get_domain_bus_and_slot(int domain, unsigned int bus,
>  {
>  	struct pci_dev *dev = NULL;
>  
> +	down_read(&pci_bus_sem);

How about using

guard(rwsem_read)(&pci_bus_sem);

?

I think that would look a lot cleaner overall.

>  	for_each_pci_dev(dev) {
>  		if (pci_domain_nr(dev->bus) == domain &&
>  		    (dev->bus->number == bus && dev->devfn == devfn))
> -			return dev;
> +			goto out;
>  	}
> -	return NULL;
> +	dev = NULL;
> + out:
> +	pci_dev_get(dev);

So now you're incrementing the reference count but there's no code in this 
patch to counterdecrement it? Are all callers already decrementing it 
(which leads to underflows w/o this patch)? This needs to be explained in 
the changelog text.

> +	up_read(&pci_bus_sem);
> +	return dev;
>  }
>  EXPORT_SYMBOL(pci_get_domain_bus_and_slot);

-- 
 i.