Re: [PATCH 1/9] dmaengine: idxd: Fix lockdep warnings when calling idxd_device_config()

[Dave Jiang <dave.jiang@intel.com>]

On 8/4/25 6:27 PM, Vinicius Costa Gomes wrote:
> idxd_device_config() should only be called with idxd->dev_lock held.
> Hold the lock to the calls that were missing.
> 
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>

Patch looks fine. What about doing something like this:

---

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index 5cf419fe6b46..06c182ec3c04 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -1103,11 +1103,15 @@ static int idxd_wqs_setup(struct idxd_device *idxd)
 	return 0;
 }
 
-int idxd_device_config(struct idxd_device *idxd)
+int idxd_device_config_locked(struct idxd_device *idxd)
 {
 	int rc;
 
 	lockdep_assert_held(&idxd->dev_lock);
+
+	if (!test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags))
+		return -EPERM;
+
 	rc = idxd_wqs_setup(idxd);
 	if (rc < 0)
 		return rc;
@@ -1129,6 +1133,12 @@ int idxd_device_config(struct idxd_device *idxd)
 	return 0;
 }
 
+int idxd_device_config(struct idxd_device *idxd)
+{
+	guard(spinlock)(&idxd->dev_lock);
+	return idxd_device_config_locked(idxd);
+}
+
 static int idxd_wq_load_config(struct idxd_wq *wq)
 {
 	struct idxd_device *idxd = wq->idxd;
@@ -1434,11 +1444,7 @@ int idxd_drv_enable_wq(struct idxd_wq *wq)
 		}
 	}
 
-	rc = 0;
-	spin_lock(&idxd->dev_lock);
-	if (test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags))
-		rc = idxd_device_config(idxd);
-	spin_unlock(&idxd->dev_lock);
+	rc = idxd_device_config(idxd);
 	if (rc < 0) {
 		dev_dbg(dev, "Writing wq %d config failed: %d\n", wq->id, rc);
 		goto err;
@@ -1521,7 +1527,7 @@ EXPORT_SYMBOL_NS_GPL(idxd_drv_disable_wq, "IDXD");
 int idxd_device_drv_probe(struct idxd_dev *idxd_dev)
 {
 	struct idxd_device *idxd = idxd_dev_to_idxd(idxd_dev);
-	int rc = 0;
+	int rc;
 
 	/*
 	 * Device should be in disabled state for the idxd_drv to load. If it's in
@@ -1534,10 +1540,7 @@ int idxd_device_drv_probe(struct idxd_dev *idxd_dev)
 	}
 
 	/* Device configuration */
-	spin_lock(&idxd->dev_lock);
-	if (test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags))
-		rc = idxd_device_config(idxd);
-	spin_unlock(&idxd->dev_lock);
+	rc = idxd_device_config(idxd);
 	if (rc < 0)
 		return -ENXIO;
 
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 74e6695881e6..f15bc2281c6b 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -760,6 +760,7 @@ int idxd_device_disable(struct idxd_device *idxd);
 void idxd_device_reset(struct idxd_device *idxd);
 void idxd_device_clear_state(struct idxd_device *idxd);
 int idxd_device_config(struct idxd_device *idxd);
+int idxd_device_config_locked(struct idxd_device *idxd);
 void idxd_device_drain_pasid(struct idxd_device *idxd, int pasid);
 int idxd_device_load_config(struct idxd_device *idxd);
 int idxd_device_request_int_handle(struct idxd_device *idxd, int idx, int *handle,
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 80355d03004d..193b9282e30f 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -1091,12 +1091,10 @@ static void idxd_reset_done(struct pci_dev *pdev)
 	idxd_device_config_restore(idxd, idxd->idxd_saved);
 
 	/* Re-configure IDXD device if allowed. */
-	if (test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags)) {
-		rc = idxd_device_config(idxd);
-		if (rc < 0) {
-			dev_err(dev, "HALT: %s config fails\n", idxd_name);
-			goto out;
-		}
+	rc = idxd_device_config(idxd);
+	if (rc < 0) {
+		dev_err(dev, "HALT: %s config fails\n", idxd_name);
+		goto out;
 	}
 
 	/* Bind IDXD device to driver. */


> ---
>  drivers/dma/idxd/init.c | 2 ++
>  drivers/dma/idxd/irq.c  | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index 35bdefd3728bb851beb0f235fae7c6d71bd59239..d828d352ab008127e5e442e7072c9d5df0f2c6cf 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -1091,7 +1091,9 @@ static void idxd_reset_done(struct pci_dev *pdev)
>  
>  	/* Re-configure IDXD device if allowed. */
>  	if (test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags)) {
> +		spin_lock(&idxd->dev_lock);
>  		rc = idxd_device_config(idxd);
> +		spin_unlock(&idxd->dev_lock);
>  		if (rc < 0) {
>  			dev_err(dev, "HALT: %s config fails\n", idxd_name);
>  			goto out;
> diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
> index 1107db3ce0a3a65246bd0d9b1f96e99c9fa3def6..74059fe43fafeb930f58db21d3824f62b095b968 100644
> --- a/drivers/dma/idxd/irq.c
> +++ b/drivers/dma/idxd/irq.c
> @@ -36,7 +36,9 @@ static void idxd_device_reinit(struct work_struct *work)
>  	int rc, i;
>  
>  	idxd_device_reset(idxd);
> +	spin_lock(&idxd->dev_lock);
>  	rc = idxd_device_config(idxd);
> +	spin_unlock(&idxd->dev_lock);
>  	if (rc < 0)
>  		goto out;
>  
>