[PATCH] cxl/region: Convert cxl_pmem_region_alloc to scope-based resource management

[PATCH] cxl/region: Convert cxl_pmem_region_alloc to scope-based resource management

[Dan Williams]

A recent bugfix to cxl_pmem_region_alloc() to fix an
error-unwind-memleak [1], highlighted a use case for scope-based resource
management.

Delete the goto for releasing @cxl_region_rwsem, and return error codes
directly from error condition paths.

The caller, devm_cxl_add_pmem_region(), is no longer given @cxlr_pmem
directly it must retrieve it from @cxlr->cxlr_pmem. This retrieval from
@cxlr was already in place for @cxlr->cxl_nvb, and converting
cxl_pmem_region_alloc() to return an int makes it less awkward to handle
no_free_ptr().

Cc: Li Zhijian <lizhijian@fujitsu.com>
Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Closes: http://lore.kernel.org/r/20240430174540.000039ce@Huawei.com
Link: http://lore.kernel.org/r/20240428030748.318985-1-lizhijian@fujitsu.com
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Hey Dave, this applies on top of Li's fix.

 drivers/cxl/core/region.c |   43 +++++++++++++++++--------------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 812b2948b6c6..e0577d99c4db 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2681,26 +2681,21 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port)
 
 static struct lock_class_key cxl_pmem_region_key;
 
-static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
+static int cxl_pmem_region_alloc(struct cxl_region *cxlr)
 {
 	struct cxl_region_params *p = &cxlr->params;
 	struct cxl_nvdimm_bridge *cxl_nvb;
-	struct cxl_pmem_region *cxlr_pmem;
 	struct device *dev;
 	int i;
 
-	down_read(&cxl_region_rwsem);
-	if (p->state != CXL_CONFIG_COMMIT) {
-		cxlr_pmem = ERR_PTR(-ENXIO);
-		goto out;
-	}
+	guard(rwsem_read)(&cxl_region_rwsem);
+	if (p->state != CXL_CONFIG_COMMIT)
+		return -ENXIO;
 
-	cxlr_pmem = kzalloc(struct_size(cxlr_pmem, mapping, p->nr_targets),
-			    GFP_KERNEL);
-	if (!cxlr_pmem) {
-		cxlr_pmem = ERR_PTR(-ENOMEM);
-		goto out;
-	}
+	struct cxl_pmem_region *cxlr_pmem __free(kfree) = kzalloc(
+		struct_size(cxlr_pmem, mapping, p->nr_targets), GFP_KERNEL);
+	if (!cxlr_pmem)
+		return -ENOMEM;
 
 	cxlr_pmem->hpa_range.start = p->res->start;
 	cxlr_pmem->hpa_range.end = p->res->end;
@@ -2718,11 +2713,8 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
 		 */
 		if (i == 0) {
 			cxl_nvb = cxl_find_nvdimm_bridge(cxlmd);
-			if (!cxl_nvb) {
-				kfree(cxlr_pmem);
-				cxlr_pmem = ERR_PTR(-ENODEV);
-				goto out;
-			}
+			if (!cxl_nvb)
+				return -ENODEV;
 			cxlr->cxl_nvb = cxl_nvb;
 		}
 		m->cxlmd = cxlmd;
@@ -2733,18 +2725,16 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
 	}
 
 	dev = &cxlr_pmem->dev;
-	cxlr_pmem->cxlr = cxlr;
-	cxlr->cxlr_pmem = cxlr_pmem;
 	device_initialize(dev);
 	lockdep_set_class(&dev->mutex, &cxl_pmem_region_key);
 	device_set_pm_not_required(dev);
 	dev->parent = &cxlr->dev;
 	dev->bus = &cxl_bus_type;
 	dev->type = &cxl_pmem_region_type;
-out:
-	up_read(&cxl_region_rwsem);
+	cxlr_pmem->cxlr = cxlr;
+	cxlr->cxlr_pmem = no_free_ptr(cxlr_pmem);
 
-	return cxlr_pmem;
+	return 0;
 }
 
 static void cxl_dax_region_release(struct device *dev)
@@ -2861,9 +2851,10 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
 	struct device *dev;
 	int rc;
 
-	cxlr_pmem = cxl_pmem_region_alloc(cxlr);
-	if (IS_ERR(cxlr_pmem))
-		return PTR_ERR(cxlr_pmem);
+	rc = cxl_pmem_region_alloc(cxlr);
+	if (rc)
+		return rc;
+	cxlr_pmem = cxlr->cxlr_pmem;
 	cxl_nvb = cxlr->cxl_nvb;
 
 	dev = &cxlr_pmem->dev;

Re: [PATCH] cxl/region: Convert cxl_pmem_region_alloc to scope-based resource management

[fan]

On Tue, Apr 30, 2024 at 02:59:00PM -0700, Dan Williams wrote:
> A recent bugfix to cxl_pmem_region_alloc() to fix an
> error-unwind-memleak [1], highlighted a use case for scope-based resource
> management.
> 
> Delete the goto for releasing @cxl_region_rwsem, and return error codes
> directly from error condition paths.
> 
> The caller, devm_cxl_add_pmem_region(), is no longer given @cxlr_pmem
> directly it must retrieve it from @cxlr->cxlr_pmem. This retrieval from
> @cxlr was already in place for @cxlr->cxl_nvb, and converting
> cxl_pmem_region_alloc() to return an int makes it less awkward to handle
> no_free_ptr().
> 
> Cc: Li Zhijian <lizhijian@fujitsu.com>
> Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Closes: http://lore.kernel.org/r/20240430174540.000039ce@Huawei.com
> Link: http://lore.kernel.org/r/20240428030748.318985-1-lizhijian@fujitsu.com
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> Hey Dave, this applies on top of Li's fix.
> 
>  drivers/cxl/core/region.c |   43 +++++++++++++++++--------------------------
>  1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 812b2948b6c6..e0577d99c4db 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2681,26 +2681,21 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port)
>  
>  static struct lock_class_key cxl_pmem_region_key;
>  
> -static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
> +static int cxl_pmem_region_alloc(struct cxl_region *cxlr)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
>  	struct cxl_nvdimm_bridge *cxl_nvb;
> -	struct cxl_pmem_region *cxlr_pmem;
>  	struct device *dev;
>  	int i;
>  
> -	down_read(&cxl_region_rwsem);
> -	if (p->state != CXL_CONFIG_COMMIT) {
> -		cxlr_pmem = ERR_PTR(-ENXIO);
> -		goto out;
> -	}
> +	guard(rwsem_read)(&cxl_region_rwsem);
> +	if (p->state != CXL_CONFIG_COMMIT)
> +		return -ENXIO;
>  
> -	cxlr_pmem = kzalloc(struct_size(cxlr_pmem, mapping, p->nr_targets),
> -			    GFP_KERNEL);
> -	if (!cxlr_pmem) {
> -		cxlr_pmem = ERR_PTR(-ENOMEM);
> -		goto out;
> -	}
> +	struct cxl_pmem_region *cxlr_pmem __free(kfree) = kzalloc(
> +		struct_size(cxlr_pmem, mapping, p->nr_targets), GFP_KERNEL);
> +	if (!cxlr_pmem)
> +		return -ENOMEM;
>  
>  	cxlr_pmem->hpa_range.start = p->res->start;
>  	cxlr_pmem->hpa_range.end = p->res->end;
> @@ -2718,11 +2713,8 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
>  		 */
>  		if (i == 0) {
>  			cxl_nvb = cxl_find_nvdimm_bridge(cxlmd);
> -			if (!cxl_nvb) {
> -				kfree(cxlr_pmem);
> -				cxlr_pmem = ERR_PTR(-ENODEV);
> -				goto out;
> -			}
> +			if (!cxl_nvb)
> +				return -ENODEV;
>  			cxlr->cxl_nvb = cxl_nvb;
>  		}
>  		m->cxlmd = cxlmd;
> @@ -2733,18 +2725,16 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
>  	}
>  
>  	dev = &cxlr_pmem->dev;
> -	cxlr_pmem->cxlr = cxlr;
> -	cxlr->cxlr_pmem = cxlr_pmem;
>  	device_initialize(dev);
>  	lockdep_set_class(&dev->mutex, &cxl_pmem_region_key);
>  	device_set_pm_not_required(dev);
>  	dev->parent = &cxlr->dev;
>  	dev->bus = &cxl_bus_type;
>  	dev->type = &cxl_pmem_region_type;
> -out:
> -	up_read(&cxl_region_rwsem);
> +	cxlr_pmem->cxlr = cxlr;
> +	cxlr->cxlr_pmem = no_free_ptr(cxlr_pmem);
>  
> -	return cxlr_pmem;
> +	return 0;
>  }
>  
>  static void cxl_dax_region_release(struct device *dev)
> @@ -2861,9 +2851,10 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
>  	struct device *dev;
>  	int rc;
>  
> -	cxlr_pmem = cxl_pmem_region_alloc(cxlr);
> -	if (IS_ERR(cxlr_pmem))
> -		return PTR_ERR(cxlr_pmem);
> +	rc = cxl_pmem_region_alloc(cxlr);
> +	if (rc)
> +		return rc;
> +	cxlr_pmem = cxlr->cxlr_pmem;
>  	cxl_nvb = cxlr->cxl_nvb;
>  
>  	dev = &cxlr_pmem->dev;
> 

LGTM. 

Fan

Re: [PATCH] cxl/region: Convert cxl_pmem_region_alloc to scope-based resource management

[Jonathan Cameron]

On Tue, 30 Apr 2024 14:59:00 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> A recent bugfix to cxl_pmem_region_alloc() to fix an
> error-unwind-memleak [1], highlighted a use case for scope-based resource
> management.
> 
> Delete the goto for releasing @cxl_region_rwsem, and return error codes
> directly from error condition paths.
> 
> The caller, devm_cxl_add_pmem_region(), is no longer given @cxlr_pmem
> directly it must retrieve it from @cxlr->cxlr_pmem. This retrieval from
> @cxlr was already in place for @cxlr->cxl_nvb, and converting
> cxl_pmem_region_alloc() to return an int makes it less awkward to handle
> no_free_ptr().
> 
> Cc: Li Zhijian <lizhijian@fujitsu.com>
> Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Closes: http://lore.kernel.org/r/20240430174540.000039ce@Huawei.com
> Link: http://lore.kernel.org/r/20240428030748.318985-1-lizhijian@fujitsu.com
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Trivial comments inline  Up to Dave or you on whether you want to act on them.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> Hey Dave, this applies on top of Li's fix.
> 
>  drivers/cxl/core/region.c |   43 +++++++++++++++++--------------------------
>  1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 812b2948b6c6..e0577d99c4db 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2681,26 +2681,21 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port)
>  
>  static struct lock_class_key cxl_pmem_region_key;
>  
> -static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
> +static int cxl_pmem_region_alloc(struct cxl_region *cxlr)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
>  	struct cxl_nvdimm_bridge *cxl_nvb;
> -	struct cxl_pmem_region *cxlr_pmem;
>  	struct device *dev;
>  	int i;
>  
> -	down_read(&cxl_region_rwsem);
> -	if (p->state != CXL_CONFIG_COMMIT) {
> -		cxlr_pmem = ERR_PTR(-ENXIO);
> -		goto out;
> -	}
> +	guard(rwsem_read)(&cxl_region_rwsem);
> +	if (p->state != CXL_CONFIG_COMMIT)
> +		return -ENXIO;
>  
> -	cxlr_pmem = kzalloc(struct_size(cxlr_pmem, mapping, p->nr_targets),
> -			    GFP_KERNEL);
> -	if (!cxlr_pmem) {
> -		cxlr_pmem = ERR_PTR(-ENOMEM);
> -		goto out;
> -	}
> +	struct cxl_pmem_region *cxlr_pmem __free(kfree) = kzalloc(
> +		struct_size(cxlr_pmem, mapping, p->nr_targets), GFP_KERNEL);

That is rather ugly.  Maybe bring the kzalloc down onto the second line and
wrap the GFP_KERNEL (or just go over 80 chars)?

> +	if (!cxlr_pmem)
> +		return -ENOMEM;
>  
>  	cxlr_pmem->hpa_range.start = p->res->start;
>  	cxlr_pmem->hpa_range.end = p->res->end;
> @@ -2718,11 +2713,8 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
>  		 */
>  		if (i == 0) {
>  			cxl_nvb = cxl_find_nvdimm_bridge(cxlmd);
> -			if (!cxl_nvb) {
> -				kfree(cxlr_pmem);
> -				cxlr_pmem = ERR_PTR(-ENODEV);
> -				goto out;
> -			}
> +			if (!cxl_nvb)
> +				return -ENODEV;
>  			cxlr->cxl_nvb = cxl_nvb;
>  		}
>  		m->cxlmd = cxlmd;
> @@ -2733,18 +2725,16 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
>  	}
>  
>  	dev = &cxlr_pmem->dev;
> -	cxlr_pmem->cxlr = cxlr;
> -	cxlr->cxlr_pmem = cxlr_pmem;
>  	device_initialize(dev);
>  	lockdep_set_class(&dev->mutex, &cxl_pmem_region_key);
>  	device_set_pm_not_required(dev);
>  	dev->parent = &cxlr->dev;
>  	dev->bus = &cxl_bus_type;
>  	dev->type = &cxl_pmem_region_type;
> -out:
> -	up_read(&cxl_region_rwsem);
> +	cxlr_pmem->cxlr = cxlr;
> +	cxlr->cxlr_pmem = no_free_ptr(cxlr_pmem);
>  
> -	return cxlr_pmem;
> +	return 0;
>  }
>  
>  static void cxl_dax_region_release(struct device *dev)
> @@ -2861,9 +2851,10 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
>  	struct device *dev;
>  	int rc;
>  
> -	cxlr_pmem = cxl_pmem_region_alloc(cxlr);
> -	if (IS_ERR(cxlr_pmem))
> -		return PTR_ERR(cxlr_pmem);
> +	rc = cxl_pmem_region_alloc(cxlr);
> +	if (rc)
> +		return rc;
> +	cxlr_pmem = cxlr->cxlr_pmem;

Is the local variable worth retaining? It's only used twice.

The fact it's a clxr->cxlr* also seems rather like it could be named
in a more compact fashion inside the cxl_region.  Would pmemr
work for instance?

>  	cxl_nvb = cxlr->cxl_nvb;
>  
>  	dev = &cxlr_pmem->dev;
> 
> 

Re: [PATCH] cxl/region: Convert cxl_pmem_region_alloc to scope-based resource management

[Dan Williams]

Jonathan Cameron wrote:
> On Tue, 30 Apr 2024 14:59:00 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > A recent bugfix to cxl_pmem_region_alloc() to fix an
> > error-unwind-memleak [1], highlighted a use case for scope-based resource
> > management.
> > 
> > Delete the goto for releasing @cxl_region_rwsem, and return error codes
> > directly from error condition paths.
> > 
> > The caller, devm_cxl_add_pmem_region(), is no longer given @cxlr_pmem
> > directly it must retrieve it from @cxlr->cxlr_pmem. This retrieval from
> > @cxlr was already in place for @cxlr->cxl_nvb, and converting
> > cxl_pmem_region_alloc() to return an int makes it less awkward to handle
> > no_free_ptr().
> > 
> > Cc: Li Zhijian <lizhijian@fujitsu.com>
> > Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> > Closes: http://lore.kernel.org/r/20240430174540.000039ce@Huawei.com
> > Link: http://lore.kernel.org/r/20240428030748.318985-1-lizhijian@fujitsu.com
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> Trivial comments inline  Up to Dave or you on whether you want to act on them.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
[..]
> > -	cxlr_pmem = kzalloc(struct_size(cxlr_pmem, mapping, p->nr_targets),
> > -			    GFP_KERNEL);
> > -	if (!cxlr_pmem) {
> > -		cxlr_pmem = ERR_PTR(-ENOMEM);
> > -		goto out;
> > -	}
> > +	struct cxl_pmem_region *cxlr_pmem __free(kfree) = kzalloc(
> > +		struct_size(cxlr_pmem, mapping, p->nr_targets), GFP_KERNEL);
> 
> That is rather ugly.  Maybe bring the kzalloc down onto the second line and
> wrap the GFP_KERNEL (or just go over 80 chars)?

I have made a conscious decision to stop caring about manually making 80
column collisions look nicer or selectively breaking the 80 column limit
rule. For this drivers/cxl/ corner of the kernel, whatever the
.clang-format template allows, works for me.

So if you can propose a .clang-format change that makes things prettier
in this case I would entertain it, but otherwise, life's too short.

Along those lines it would be nice if usages of __free caused the
automatic line break to happen after the "=" sign rather than later at
the arguments to kzalloc() like the above. However, I don't know the
template incantations to achieve that.

[..]
> > @@ -2861,9 +2851,10 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
> >  	struct device *dev;
> >  	int rc;
> >  
> > -	cxlr_pmem = cxl_pmem_region_alloc(cxlr);
> > -	if (IS_ERR(cxlr_pmem))
> > -		return PTR_ERR(cxlr_pmem);
> > +	rc = cxl_pmem_region_alloc(cxlr);
> > +	if (rc)
> > +		return rc;
> > +	cxlr_pmem = cxlr->cxlr_pmem;
> 
> Is the local variable worth retaining? It's only used twice.
> 
> The fact it's a clxr->cxlr* also seems rather like it could be named
> in a more compact fashion inside the cxl_region.  Would pmemr
> work for instance?

I am ambivalent about a cxlr_pmem rename. It would need to be a separate
patch to handle the 46 instances of cxlr_pmem.

Re: [PATCH] cxl/region: Convert cxl_pmem_region_alloc to scope-based resource management

[Zhijian Li (Fujitsu)]

On 01/05/2024 05:59, Dan Williams wrote:
> A recent bugfix to cxl_pmem_region_alloc() to fix an
> error-unwind-memleak [1], highlighted a use case for scope-based resource
> management.
> 
> Delete the goto for releasing @cxl_region_rwsem, and return error codes
> directly from error condition paths.
> 
> The caller, devm_cxl_add_pmem_region(), is no longer given @cxlr_pmem
> directly it must retrieve it from @cxlr->cxlr_pmem. This retrieval from
> @cxlr was already in place for @cxlr->cxl_nvb, and converting
> cxl_pmem_region_alloc() to return an int makes it less awkward to handle
> no_free_ptr().
> 
> Cc: Li Zhijian <lizhijian@fujitsu.com>
> Reported-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Closes: http://lore.kernel.org/r/20240430174540.000039ce@Huawei.com
> Link: http://lore.kernel.org/r/20240428030748.318985-1-lizhijian@fujitsu.com
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Thanks, LGTM

Reviewed-by: Li Zhijian <lizhijian@fujitsu.com>
and
Tested-by: Li Zhijian <lizhijian@fujitsu.com>



> ---
> Hey Dave, this applies on top of Li's fix.
> 
>   drivers/cxl/core/region.c |   43 +++++++++++++++++--------------------------
>   1 file changed, 17 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 812b2948b6c6..e0577d99c4db 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2681,26 +2681,21 @@ int cxl_get_poison_by_endpoint(struct cxl_port *port)
>   
>   static struct lock_class_key cxl_pmem_region_key;
>   
> -static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
> +static int cxl_pmem_region_alloc(struct cxl_region *cxlr)
>   {
>   	struct cxl_region_params *p = &cxlr->params;
>   	struct cxl_nvdimm_bridge *cxl_nvb;
> -	struct cxl_pmem_region *cxlr_pmem;
>   	struct device *dev;
>   	int i;
>   
> -	down_read(&cxl_region_rwsem);
> -	if (p->state != CXL_CONFIG_COMMIT) {
> -		cxlr_pmem = ERR_PTR(-ENXIO);
> -		goto out;
> -	}
> +	guard(rwsem_read)(&cxl_region_rwsem);
> +	if (p->state != CXL_CONFIG_COMMIT)
> +		return -ENXIO;
>   
> -	cxlr_pmem = kzalloc(struct_size(cxlr_pmem, mapping, p->nr_targets),
> -			    GFP_KERNEL);
> -	if (!cxlr_pmem) {
> -		cxlr_pmem = ERR_PTR(-ENOMEM);
> -		goto out;
> -	}
> +	struct cxl_pmem_region *cxlr_pmem __free(kfree) = kzalloc(
> +		struct_size(cxlr_pmem, mapping, p->nr_targets), GFP_KERNEL);
> +	if (!cxlr_pmem)
> +		return -ENOMEM;
>   
>   	cxlr_pmem->hpa_range.start = p->res->start;
>   	cxlr_pmem->hpa_range.end = p->res->end;
> @@ -2718,11 +2713,8 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
>   		 */
>   		if (i == 0) {
>   			cxl_nvb = cxl_find_nvdimm_bridge(cxlmd);
> -			if (!cxl_nvb) {
> -				kfree(cxlr_pmem);
> -				cxlr_pmem = ERR_PTR(-ENODEV);
> -				goto out;
> -			}
> +			if (!cxl_nvb)
> +				return -ENODEV;
>   			cxlr->cxl_nvb = cxl_nvb;
>   		}
>   		m->cxlmd = cxlmd;
> @@ -2733,18 +2725,16 @@ static struct cxl_pmem_region *cxl_pmem_region_alloc(struct cxl_region *cxlr)
>   	}
>   
>   	dev = &cxlr_pmem->dev;
> -	cxlr_pmem->cxlr = cxlr;
> -	cxlr->cxlr_pmem = cxlr_pmem;
>   	device_initialize(dev);
>   	lockdep_set_class(&dev->mutex, &cxl_pmem_region_key);
>   	device_set_pm_not_required(dev);
>   	dev->parent = &cxlr->dev;
>   	dev->bus = &cxl_bus_type;
>   	dev->type = &cxl_pmem_region_type;
> -out:
> -	up_read(&cxl_region_rwsem);
> +	cxlr_pmem->cxlr = cxlr;
> +	cxlr->cxlr_pmem = no_free_ptr(cxlr_pmem);
>   
> -	return cxlr_pmem;
> +	return 0;
>   }
>   
>   static void cxl_dax_region_release(struct device *dev)
> @@ -2861,9 +2851,10 @@ static int devm_cxl_add_pmem_region(struct cxl_region *cxlr)
>   	struct device *dev;
>   	int rc;
>   
> -	cxlr_pmem = cxl_pmem_region_alloc(cxlr);
> -	if (IS_ERR(cxlr_pmem))
> -		return PTR_ERR(cxlr_pmem);
> +	rc = cxl_pmem_region_alloc(cxlr);
> +	if (rc)
> +		return rc;
> +	cxlr_pmem = cxlr->cxlr_pmem;
>   	cxl_nvb = cxlr->cxl_nvb;
>   
>   	dev = &cxlr_pmem->dev;
>