Re: [PATCH v1 1/1] PCI/IOV: Add nested locking in sriov_add_vfs/sriov_del_vfs for complete serialization

[Benjamin Block]

Hey Ionut,

Incidentally I'm also working on patches related to `pci_rescan_remove_lock`
on s390. Not the same issues that you tackle here, but I also found the
unprotected calls from driver unbinds; so that's nice :).

I've put your patch on top of what I have so far, and functionality wise it
looks pretty good from my POV. I've run it with lockdep enabled with my tests
for our architecture issues (this includes the hot-unplug events), plus the
driver unbind calls, and could not trigger any lockdep splats and/or actual
deadlocks.

On Sat, Feb 14, 2026 at 09:32:37PM +0200, Ionut Nechita (Wind River) wrote:
> From: Ionut Nechita <ionut.nechita@xxxxxxxxxxxxx>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 4a659c34935e..38372ac0e2ad 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -629,19 +629,25 @@ static int sriov_add_vfs(struct pci_dev *dev, u16 num_vfs)
>  {
>  	unsigned int i;
>  	int rc;
> +	bool nested;
>  
>  	if (dev->no_vf_scan)
>  		return 0;
>  
> +	nested = !pci_lock_rescan_remove_nested();

You always call the function and negate the result (which makes sense), so I
was wondering, wouldn't it be easier to just return the result reversed
already?

>  	for (i = 0; i < num_vfs; i++) {
>  		rc = pci_iov_add_virtfn(dev, i);
>  		if (rc)
>  			goto failed;
>  	}
> +	if (!nested)
> +		pci_unlock_rescan_remove();

Hmm, you could have a sort of `pci_unlock_rescan_remove_nested()`, and pass
the return value of the `pci_lock_rescan_remove_nested()` call as argument
(probably as `const`); that way you wouldn't need to open-code this
everywhere, and I can't think of a situation where you'd want to call the
nested lock function, but don't check the result for the unlock.

> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index c8a0522e2e1f..7d3b705728fd 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -367,6 +367,7 @@ static inline void pci_remove_legacy_files(struct pci_bus *bus) { }
>  /* Lock for read/write access to pci device and bus lists */
>  extern struct rw_semaphore pci_bus_sem;
>  extern struct mutex pci_slot_mutex;
> +bool pci_lock_rescan_remove_nested(void);

This is mostly a nitpick: I'm a bit surprised this isn't next to the other
declarations in `include/linux/pci.h`. Are the any reasons for this? To
restrict access/use?

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 7711f579fa1d..5f38ed0c641a 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -3478,19 +3478,31 @@ EXPORT_SYMBOL_GPL(pci_rescan_bus);
>   * routines should always be executed under this mutex.
>   */
>  DEFINE_MUTEX(pci_rescan_remove_lock);
> +static struct task_struct *pci_rescan_remove_owner;
>  
>  void pci_lock_rescan_remove(void)
>  {
>  	mutex_lock(&pci_rescan_remove_lock);
> +	pci_rescan_remove_owner = current;
>  }
>  EXPORT_SYMBOL_GPL(pci_lock_rescan_remove);
>  
>  void pci_unlock_rescan_remove(void)
>  {
> +	pci_rescan_remove_owner = NULL;
>  	mutex_unlock(&pci_rescan_remove_lock);
>  }
>  EXPORT_SYMBOL_GPL(pci_unlock_rescan_remove);
>  
> +bool pci_lock_rescan_remove_nested(void)

Again, mostly a nitpick: we already have "nested" mutex functions, but those
don't (somewhat surprising) relate to any reentrant property, but annotate
for lockdep. Might be better to avoid the term here, to prevent further
confusions for anyone already aware of those.

> +{
> +	if (pci_rescan_remove_owner == current)
> +		return false;
> +	pci_lock_rescan_remove();
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(pci_lock_rescan_remove_nested);

In general, having reentrant locks might be an issue if the code under it
isn't aware of the possibility, but I'm not sure how we'd go about
proving/disproving this. It's hardly possible to review all the relevant
code, with how broad the protection of this particular lock spans.

And in the past the community seemed to be rather ambivalent to the concept.
Do you know other "prior art" for this in the kernel?

-- 
Best Regards, Benjamin Block        /        Linux on IBM Z Kernel Development
IBM Deutschland Research & Development GmbH    /   https://www.ibm.com/privacy
Vors. Aufs.-R.: Wolfgang Wendt         /        Geschäftsführung: David Faller
Sitz der Ges.: Ehningen     /     Registergericht: AmtsG Stuttgart, HRB 243294