linux-kernel - Re: [PATCH v1 7/7] cxl/region: Drop goto pattern of construct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250211173606.00004d33@huawei.com>
Date: Tue, 11 Feb 2025 17:36:06 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Li Ming <ming.li@...omail.com>
CC: <dave@...olabs.net>, <dave.jiang@...el.com>, <alison.schofield@...el.com>,
	<vishal.l.verma@...el.com>, <ira.weiny@...el.com>,
	<dan.j.williams@...el.com>, <linux-cxl@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 7/7] cxl/region: Drop goto pattern of
 construct_region()

On Tue, 11 Feb 2025 15:57:27 +0800
Li Ming <ming.li@...omail.com> wrote:

> Some operations need to be procted by the cxl_region_rwsem in construct
> region(). Currently, construct_region() uses down_write() and up_write()
> for the cxl_region_rwsem, so there is a goto pattern after down_write()
> invoked to release cxl_region_rwsem.
> 
> construct region() can be optimized to remove the goto pattern. The
> changes are creating a new function called __construct_region() which
> will include all checking and operations protected by the
> cxl_region_rwsem, and using guard(rwsem_write) to replace down_write()
> and up_write() in __construct_region().
> 
> Signed-off-by: Li Ming <ming.li@...omail.com>
> ---
>  drivers/cxl/core/region.c | 71 +++++++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 36f3771818d3..170278bdcedc 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3217,49 +3217,31 @@ static int match_region_by_range(struct device *dev, const void *data)
>  	return rc;
>  }
>  
> -/* Establish an empty region covering the given HPA range */
> -static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> -					   struct cxl_endpoint_decoder *cxled)
> +static int __construct_region(struct cxl_region *cxlr,

This is only factoring out part, so I'm not sure the naming makes sense.
I don't have a better name however as this is doing various different things...
setup_region() doesn't feel right either...


> +			      struct cxl_root_decoder *cxlrd,
> +			      struct cxl_endpoint_decoder *cxled)
>  {
>  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> -	struct cxl_port *port = cxlrd_to_port(cxlrd);
>  	struct range *hpa = &cxled->cxld.hpa_range;
>  	struct cxl_region_params *p;
> -	struct cxl_region *cxlr;
>  	struct resource *res;
>  	int rc;
>  
> -	do {
> -		cxlr = __create_region(cxlrd, cxled->mode,
> -				       atomic_read(&cxlrd->region_id));
> -	} while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY);
> -
> -	if (IS_ERR(cxlr)) {
> -		dev_err(cxlmd->dev.parent,
> -			"%s:%s: %s failed assign region: %ld\n",
> -			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> -			__func__, PTR_ERR(cxlr));
> -		return cxlr;
> -	}
> -
> -	down_write(&cxl_region_rwsem);
> +	guard(rwsem_write)(&cxl_region_rwsem);
>  	p = &cxlr->params;
>  	if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) {
>  		dev_err(cxlmd->dev.parent,
>  			"%s:%s: %s autodiscovery interrupted\n",
>  			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
>  			__func__);
> -		rc = -EBUSY;
> -		goto err;
> +		return -EBUSY;
>  	}
>  
>  	set_bit(CXL_REGION_F_AUTO, &cxlr->flags);
>  
>  	res = kmalloc(sizeof(*res), GFP_KERNEL);
> -	if (!res) {
> -		rc = -ENOMEM;
> -		goto err;
> -	}
> +	if (!res)
> +		return -ENOMEM;
>  
>  	*res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
>  				    dev_name(&cxlr->dev));
> @@ -3282,7 +3264,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>  
>  	rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_target_group());
>  	if (rc)
> -		goto err;
> +		return rc;
>  
>  	dev_dbg(cxlmd->dev.parent, "%s:%s: %s %s res: %pr iw: %d ig: %d\n",
>  		dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), __func__,
> @@ -3291,14 +3273,39 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>  
>  	/* ...to match put_device() in cxl_add_to_region() */
>  	get_device(&cxlr->dev);
> -	up_write(&cxl_region_rwsem);
>  
> -	return cxlr;
> +	return 0;
> +}
>  
> -err:
> -	up_write(&cxl_region_rwsem);
> -	devm_release_action(port->uport_dev, unregister_region, cxlr);
> -	return ERR_PTR(rc);
> +/* Establish an empty region covering the given HPA range */
> +static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> +					   struct cxl_endpoint_decoder *cxled)
> +{
> +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +	struct cxl_port *port = cxlrd_to_port(cxlrd);
> +	struct cxl_region *cxlr;
> +	int rc;
> +
> +	do {
> +		cxlr = __create_region(cxlrd, cxled->mode,
> +				       atomic_read(&cxlrd->region_id));
> +	} while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY);
> +
> +	if (IS_ERR(cxlr)) {
> +		dev_err(cxlmd->dev.parent,
> +			"%s:%s: %s failed assign region: %ld\n",
> +			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
> +			__func__, PTR_ERR(cxlr));
> +		return cxlr;
> +	}
> +
> +	rc = __construct_region(cxlr, cxlrd, cxled);
> +	if (rc) {
> +		devm_release_action(port->uport_dev, unregister_region, cxlr);
> +		return ERR_PTR(rc);
> +	}
> +
> +	return cxlr;
>  }
>  
>  int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)