Re: [PATCH v6 1/1] PCI/IOV: Make pci_lock_rescan_remove() reentrant and protect sriov_add_vfs/sriov_del_vfs

[Benjamin Block]

Hey Ionut,

On Fri, Mar 06, 2026 at 10:21:08AM +0200, Ionut Nechita (Wind River) wrote:
--8<--
> Fixes: 18f9e9d150fc ("PCI/IOV: Factor out sriov_add_vfs()")
> Cc: stable@xxxxxxxxxxxxxxx
> Suggested-by: Lukas Wunner <lukas@xxxxxxxxx>
> Tested-by: Dragos Tatulea <dtatulea@xxxxxxxxxx>
> Reviewed-by: Benjamin Block <bblock@xxxxxxxxxxxxx>
> Tested-by: Benjamin Block <bblock@xxxxxxxxxxxxx>

I'm not sure whether you are aware of this, but generally, once you make
substantially changes to a patch, it is common practice to remove the previous
Tested-by and/or Reviewed-by tags again:
https://docs.kernel.org/process/submitting-patches.html#reviewer-s-statement-of-oversight
(3. paragraph).

It's a bit of a judgement call what level of change is necessary to be
"substantial", but anyway, I thought I'd mention it :)

> Signed-off-by: Ionut Nechita <ionut_n2001@xxxxxxxxx>
> Signed-off-by: Ionut Nechita <ionut.nechita@xxxxxxxxxxxxx>
> ---
--8<--
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index bccc7a4bdd794..c7efb8e1010d3 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -3509,16 +3509,27 @@ 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;
> +static unsigned int pci_rescan_remove_count;
>  
>  void pci_lock_rescan_remove(void)
>  {
> +	if (pci_rescan_remove_owner == current) {
> +		pci_rescan_remove_count++;
> +		return;
> +	}
>  	mutex_lock(&pci_rescan_remove_lock);
> +	pci_rescan_remove_owner = current;
> +	pci_rescan_remove_count = 1;
>  }
>  EXPORT_SYMBOL_GPL(pci_lock_rescan_remove);
>  
>  void pci_unlock_rescan_remove(void)
>  {
> -	mutex_unlock(&pci_rescan_remove_lock);
> +	if (--pci_rescan_remove_count == 0) {
> +		pci_rescan_remove_owner = NULL;
> +		mutex_unlock(&pci_rescan_remove_lock);
> +	}
>  }
>  EXPORT_SYMBOL_GPL(pci_unlock_rescan_remove);

I'm not sure Lukas meant to revert to a local cache for the task_struct, just
that the counter variable itself doesn't need to be an atomic.

How the counting is done itself is probably very much a taste question. I've
run tests in my test lab with this change:

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6686cee98afc..28c0384424db 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -3510,27 +3510,23 @@ EXPORT_SYMBOL_GPL(pci_rescan_bus);
  */
 DEFINE_MUTEX(pci_rescan_remove_lock);
 EXPORT_SYMBOL_GPL(pci_rescan_remove_lock);
-static struct task_struct *pci_rescan_remove_owner;
-static unsigned int pci_rescan_remove_count;
+static size_t pci_rescan_remove_reentrant_count = 0;
 
 void pci_lock_rescan_remove(void)
 {
-	if (pci_rescan_remove_owner == current) {
-		pci_rescan_remove_count++;
-		return;
-	}
-	mutex_lock(&pci_rescan_remove_lock);
-	pci_rescan_remove_owner = current;
-	pci_rescan_remove_count = 1;
+	if (mutex_get_owner(&pci_rescan_remove_lock) == (unsigned long)current)
+		pci_rescan_remove_reentrant_count++;
+	else
+		mutex_lock(&pci_rescan_remove_lock);
 }
 EXPORT_SYMBOL_GPL(pci_lock_rescan_remove);
 
 void pci_unlock_rescan_remove(void)
 {
-	if (--pci_rescan_remove_count == 0) {
-		pci_rescan_remove_owner = NULL;
+	if (pci_rescan_remove_reentrant_count > 0)
+		pci_rescan_remove_reentrant_count--;
+	else
 		mutex_unlock(&pci_rescan_remove_lock);
-	}
 }
 EXPORT_SYMBOL_GPL(pci_unlock_rescan_remove);
 
But like I said, the counting comes down to about the same as with your patch
(as far as I can tell), just that it also uses the mutex_get_owner() call.

-- 
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