[PATCH] cxl: Fix devm device for cxl_fwctl initialization

[PATCH] cxl: Fix devm device for cxl_fwctl initialization

[Dave Jiang]

Testing revealed error message of for a CXL memdev that has Feature support:
[   56.690430] cxl mem0: Resources present before probing

Attach the allocation of cxl_fwctl to the parent device of cxl_memdev.
devm_add_* calls for cxl_memdev should not happen before the memdev
probe function or outside the scope of the memdev driver.

Fixes: 858ce2f56b52 ("cxl: Add FWCTL support to CXL")
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/features.c  | 4 ++--
 drivers/cxl/pci.c            | 2 +-
 include/cxl/features.h       | 5 +++--
 tools/testing/cxl/test/mem.c | 2 +-
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
index f4daefe3180e..150a1776480a 100644
--- a/drivers/cxl/core/features.c
+++ b/drivers/cxl/core/features.c
@@ -677,7 +677,7 @@ static void free_memdev_fwctl(void *_fwctl_dev)
 	fwctl_put(fwctl_dev);
 }
 
-int devm_cxl_setup_fwctl(struct cxl_memdev *cxlmd)
+int devm_cxl_setup_fwctl(struct device *host, struct cxl_memdev *cxlmd)
 {
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_features_state *cxlfs;
@@ -700,7 +700,7 @@ int devm_cxl_setup_fwctl(struct cxl_memdev *cxlmd)
 	if (rc)
 		return rc;
 
-	return devm_add_action_or_reset(&cxlmd->dev, free_memdev_fwctl,
+	return devm_add_action_or_reset(host, free_memdev_fwctl,
 					no_free_ptr(fwctl_dev));
 }
 EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_fwctl, "CXL");
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 7b14a154463c..785aa2af5eaa 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -1018,7 +1018,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
-	rc = devm_cxl_setup_fwctl(cxlmd);
+	rc = devm_cxl_setup_fwctl(&pdev->dev, cxlmd);
 	if (rc)
 		dev_dbg(&pdev->dev, "No CXL FWCTL setup\n");
 
diff --git a/include/cxl/features.h b/include/cxl/features.h
index a3bb34694c06..5f7f842765a5 100644
--- a/include/cxl/features.h
+++ b/include/cxl/features.h
@@ -66,7 +66,7 @@ struct cxl_memdev;
 #ifdef CONFIG_CXL_FEATURES
 inline struct cxl_features_state *to_cxlfs(struct cxl_dev_state *cxlds);
 int devm_cxl_setup_features(struct cxl_dev_state *cxlds);
-int devm_cxl_setup_fwctl(struct cxl_memdev *cxlmd);
+int devm_cxl_setup_fwctl(struct device *host, struct cxl_memdev *cxlmd);
 #else
 static inline struct cxl_features_state *to_cxlfs(struct cxl_dev_state *cxlds)
 {
@@ -78,7 +78,8 @@ static inline int devm_cxl_setup_features(struct cxl_dev_state *cxlds)
 	return -EOPNOTSUPP;
 }
 
-static inline int devm_cxl_setup_fwctl(struct cxl_memdev *cxlmd)
+static inline int devm_cxl_setup_fwctl(struct device *host,
+				       struct cxl_memdev *cxlmd)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index f2957a3e36fe..bf9caa908f89 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -1780,7 +1780,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 	if (rc)
 		return rc;
 
-	rc = devm_cxl_setup_fwctl(cxlmd);
+	rc = devm_cxl_setup_fwctl(&pdev->dev, cxlmd);
 	if (rc)
 		dev_dbg(dev, "No CXL FWCTL setup\n");
 
-- 
2.49.0

Re: [PATCH] cxl: Fix devm device for cxl_fwctl initialization

[Dan Williams]

Dave Jiang wrote:
> Testing revealed error message of for a CXL memdev that has Feature support:

s/revealed error message of for/revealed the following error message for/

> [   56.690430] cxl mem0: Resources present before probing
> 
> Attach the allocation of cxl_fwctl to the parent device of cxl_memdev.
> devm_add_* calls for cxl_memdev should not happen before the memdev
> probe function or outside the scope of the memdev driver.

Always nice to include a "how missed" explanation in addition to a how
found. Something like:

"cxl_test missed this bug because cxl_test always arranges for the
cxl_mem driver to be loaded before cxl_mock_mem runs. So the driver core
always finds the devres list idle in that case."

> Fixes: 858ce2f56b52 ("cxl: Add FWCTL support to CXL")
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

With above, looks good:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>

I tested this with the patch below that catches this bug and tries to
catch the issue generically across the kernel. However, it results in
too many false positives so I do not see an easy path to ship this
sanity checking upstream:

-- 8< --
diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index d8a733ea5e1a..f96a5bd37452 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -231,6 +231,16 @@ void devres_free(void *res)
 }
 EXPORT_SYMBOL_GPL(devres_free);
 
+void devres_add_unlocked(struct device *dev, void *res)
+{
+	struct devres *dr = container_of(res, struct devres, data);
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev->devres_lock, flags);
+	add_dr(dev, &dr->node);
+	spin_unlock_irqrestore(&dev->devres_lock, flags);
+}
+
 /**
  * devres_add - Register device resource
  * @dev: Device to add resource to
@@ -242,12 +252,8 @@ EXPORT_SYMBOL_GPL(devres_free);
  */
 void devres_add(struct device *dev, void *res)
 {
-	struct devres *dr = container_of(res, struct devres, data);
-	unsigned long flags;
-
-	spin_lock_irqsave(&dev->devres_lock, flags);
-	add_dr(dev, &dr->node);
-	spin_unlock_irqrestore(&dev->devres_lock, flags);
+	lockdep_assert_held(&dev->mutex);
+	devres_add_unlocked(dev, res);
 }
 EXPORT_SYMBOL_GPL(devres_add);
 
diff --git a/include/linux/device/devres.h b/include/linux/device/devres.h
index 9b49f9915850..7ad9f7fea46c 100644
--- a/include/linux/device/devres.h
+++ b/include/linux/device/devres.h
@@ -30,6 +30,7 @@ void devres_for_each_res(struct device *dev, dr_release_t release,
 			 void *data);
 void devres_free(void *res);
 void devres_add(struct device *dev, void *res);
+void devres_add_unlocked(struct device *dev, void *res);
 void *devres_find(struct device *dev, dr_release_t release, dr_match_t match, void *match_data);
 void *devres_get(struct device *dev, void *new_res, dr_match_t match, void *match_data);
 void *devres_remove(struct device *dev, dr_release_t release, dr_match_t match, void *match_data);
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index 5c8d43cdb0a3..2bdf625916fe 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -338,7 +338,7 @@ int msi_setup_device_data(struct device *dev)
 
 	mutex_init(&md->mutex);
 	dev->msi.data = md;
-	devres_add(dev, md);
+	devres_add_unlocked(dev, md);
 	return 0;
 }
 

Re: [PATCH] cxl: Fix devm device for cxl_fwctl initialization

[Alison Schofield]

On Thu, Apr 17, 2025 at 05:29:33PM -0700, Dave Jiang wrote:
> Testing revealed error message of for a CXL memdev that has Feature support:
> [   56.690430] cxl mem0: Resources present before probing
> 
> Attach the allocation of cxl_fwctl to the parent device of cxl_memdev.
> devm_add_* calls for cxl_memdev should not happen before the memdev
> probe function or outside the scope of the memdev driver.
> 
> Fixes: 858ce2f56b52 ("cxl: Add FWCTL support to CXL")
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

> 

Re: [PATCH] cxl: Fix devm device for cxl_fwctl initialization

[Dave Jiang]

On 4/17/25 5:29 PM, Dave Jiang wrote:
> Testing revealed error message of for a CXL memdev that has Feature support:
> [   56.690430] cxl mem0: Resources present before probing
> 
> Attach the allocation of cxl_fwctl to the parent device of cxl_memdev.
> devm_add_* calls for cxl_memdev should not happen before the memdev
> probe function or outside the scope of the memdev driver.
> 
> Fixes: 858ce2f56b52 ("cxl: Add FWCTL support to CXL")
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>

Applied to cxl/fixes

> ---
>  drivers/cxl/core/features.c  | 4 ++--
>  drivers/cxl/pci.c            | 2 +-
>  include/cxl/features.h       | 5 +++--
>  tools/testing/cxl/test/mem.c | 2 +-
>  4 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> index f4daefe3180e..150a1776480a 100644
> --- a/drivers/cxl/core/features.c
> +++ b/drivers/cxl/core/features.c
> @@ -677,7 +677,7 @@ static void free_memdev_fwctl(void *_fwctl_dev)
>  	fwctl_put(fwctl_dev);
>  }
>  
> -int devm_cxl_setup_fwctl(struct cxl_memdev *cxlmd)
> +int devm_cxl_setup_fwctl(struct device *host, struct cxl_memdev *cxlmd)
>  {
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_features_state *cxlfs;
> @@ -700,7 +700,7 @@ int devm_cxl_setup_fwctl(struct cxl_memdev *cxlmd)
>  	if (rc)
>  		return rc;
>  
> -	return devm_add_action_or_reset(&cxlmd->dev, free_memdev_fwctl,
> +	return devm_add_action_or_reset(host, free_memdev_fwctl,
>  					no_free_ptr(fwctl_dev));
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_cxl_setup_fwctl, "CXL");
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 7b14a154463c..785aa2af5eaa 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -1018,7 +1018,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> -	rc = devm_cxl_setup_fwctl(cxlmd);
> +	rc = devm_cxl_setup_fwctl(&pdev->dev, cxlmd);
>  	if (rc)
>  		dev_dbg(&pdev->dev, "No CXL FWCTL setup\n");
>  
> diff --git a/include/cxl/features.h b/include/cxl/features.h
> index a3bb34694c06..5f7f842765a5 100644
> --- a/include/cxl/features.h
> +++ b/include/cxl/features.h
> @@ -66,7 +66,7 @@ struct cxl_memdev;
>  #ifdef CONFIG_CXL_FEATURES
>  inline struct cxl_features_state *to_cxlfs(struct cxl_dev_state *cxlds);
>  int devm_cxl_setup_features(struct cxl_dev_state *cxlds);
> -int devm_cxl_setup_fwctl(struct cxl_memdev *cxlmd);
> +int devm_cxl_setup_fwctl(struct device *host, struct cxl_memdev *cxlmd);
>  #else
>  static inline struct cxl_features_state *to_cxlfs(struct cxl_dev_state *cxlds)
>  {
> @@ -78,7 +78,8 @@ static inline int devm_cxl_setup_features(struct cxl_dev_state *cxlds)
>  	return -EOPNOTSUPP;
>  }
>  
> -static inline int devm_cxl_setup_fwctl(struct cxl_memdev *cxlmd)
> +static inline int devm_cxl_setup_fwctl(struct device *host,
> +				       struct cxl_memdev *cxlmd)
>  {
>  	return -EOPNOTSUPP;
>  }
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index f2957a3e36fe..bf9caa908f89 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -1780,7 +1780,7 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
>  	if (rc)
>  		return rc;
>  
> -	rc = devm_cxl_setup_fwctl(cxlmd);
> +	rc = devm_cxl_setup_fwctl(&pdev->dev, cxlmd);
>  	if (rc)
>  		dev_dbg(dev, "No CXL FWCTL setup\n");
>