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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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