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

[Dan Williams <dan.j.williams@intel.com>]

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