[PATCH] cxl: Fix unregister_region() callback parameter assignment

[PATCH] cxl: Fix unregister_region() callback parameter assignment

[Dave Jiang]

In devm_cxl_add_region(), devm_add_action_or_reset() is called by passing
in unregister_region() with data ptr of 'cxlr'. However, in
unregister_region(), the passed in parameter is incorrectly assumed to be
a 'struct device' rather than the 'cxlr' pointer. The code has been
working because 'struct device' is the first member of 'struct
cxl_region'. Issue found by inspection. Fix the assignment so that cxlr is
pointing directly to the passed in parameter.

Fixes: 23a22cd1c98b ("cxl/region: Allocate HPA capacity to regions")
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/region.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 56e575c79bb4..a61703c9d72a 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2083,13 +2083,13 @@ static struct cxl_region *to_cxl_region(struct device *dev)
 	return container_of(dev, struct cxl_region, dev);
 }
 
-static void unregister_region(void *dev)
+static void unregister_region(void *_cxlr)
 {
-	struct cxl_region *cxlr = to_cxl_region(dev);
+	struct cxl_region *cxlr = _cxlr;
 	struct cxl_region_params *p = &cxlr->params;
 	int i;
 
-	device_del(dev);
+	device_del(&cxlr->dev);
 
 	/*
 	 * Now that region sysfs is shutdown, the parameter block is now
@@ -2100,7 +2100,7 @@ static void unregister_region(void *dev)
 		detach_target(cxlr, i);
 
 	cxl_region_iomem_release(cxlr);
-	put_device(dev);
+	put_device(&cxlr->dev);
 }
 
 static struct lock_class_key cxl_region_key;

Re: [PATCH] cxl: Fix unregister_region() callback parameter assignment

[Ira Weiny]

Dave Jiang wrote:
> In devm_cxl_add_region(), devm_add_action_or_reset() is called by passing
> in unregister_region() with data ptr of 'cxlr'. However, in
> unregister_region(), the passed in parameter is incorrectly assumed to be
> a 'struct device' rather than the 'cxlr' pointer. The code has been
> working because 'struct device' is the first member of 'struct
> cxl_region'. Issue found by inspection. Fix the assignment so that cxlr is
> pointing directly to the passed in parameter.
> 
> Fixes: 23a22cd1c98b ("cxl/region: Allocate HPA capacity to regions")

Technically I think the essence of the bug existed in 

Fixes: 779dd20cfb56 ("cxl/region: Add region creation support") 

...because the region pointer was used as a device there too.

Other than that:

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/region.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

[snip]

Re: [PATCH] cxl: Fix unregister_region() callback parameter assignment

[Dan Williams]

Ira Weiny wrote:
> Dave Jiang wrote:
> > In devm_cxl_add_region(), devm_add_action_or_reset() is called by passing
> > in unregister_region() with data ptr of 'cxlr'. However, in
> > unregister_region(), the passed in parameter is incorrectly assumed to be
> > a 'struct device' rather than the 'cxlr' pointer. The code has been
> > working because 'struct device' is the first member of 'struct
> > cxl_region'. Issue found by inspection. Fix the assignment so that cxlr is
> > pointing directly to the passed in parameter.
> > 
> > Fixes: 23a22cd1c98b ("cxl/region: Allocate HPA capacity to regions")
> 
> Technically I think the essence of the bug existed in 
> 
> Fixes: 779dd20cfb56 ("cxl/region: Add region creation support") 

Agree, that's the correct Fixes: line, but I am also debating dropping
Fixes because unless the definition of 'struct cxl_region' is changed
there is no problem in practice. So a Fixes line will generate
unncessary backport thrash. I'll just change it to be mentioned as the
originator of thinko.

Re: [PATCH] cxl: Fix unregister_region() callback parameter assignment

[Ira Weiny]

Dan Williams wrote:
> Ira Weiny wrote:
> > Dave Jiang wrote:
> > > In devm_cxl_add_region(), devm_add_action_or_reset() is called by passing
> > > in unregister_region() with data ptr of 'cxlr'. However, in
> > > unregister_region(), the passed in parameter is incorrectly assumed to be
> > > a 'struct device' rather than the 'cxlr' pointer. The code has been
> > > working because 'struct device' is the first member of 'struct
> > > cxl_region'. Issue found by inspection. Fix the assignment so that cxlr is
> > > pointing directly to the passed in parameter.
> > > 
> > > Fixes: 23a22cd1c98b ("cxl/region: Allocate HPA capacity to regions")
> > 
> > Technically I think the essence of the bug existed in 
> > 
> > Fixes: 779dd20cfb56 ("cxl/region: Add region creation support") 
> 
> Agree, that's the correct Fixes: line, but I am also debating dropping
> Fixes because unless the definition of 'struct cxl_region' is changed
> there is no problem in practice. So a Fixes line will generate
> unncessary backport thrash. I'll just change it to be mentioned as the
> originator of thinko.

Yea that makes a lot of sense.

Ira