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

["Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com>]

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