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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d848f0ab-5055-4cda-8531-258c6fa7280e@intel.com>
Date: Tue, 11 Feb 2025 14:24:54 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 Li Ming <ming.li@...omail.com>
Cc: dave@...olabs.net, 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 2/11/25 10:36 AM, Jonathan Cameron wrote:
> 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...

setup_auto_region()? construct_auto_region()?

DJ

> 
> 
>> +			      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)
>