| Message ID | 20250418002933.406439-1-dave.jiang@intel.com |
|---|---|
| State | Accepted |
| Commit | d5f7d4ef49da3b13a872ed6a7426283994aabb5e |
| Headers | show |
| Series | cxl: Fix devm device for cxl_fwctl initialization | expand |
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; }
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> >
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"); >
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");
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(-)