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@zohomail.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,
+ 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)
--
2.34.1On Tue, 11 Feb 2025 15:57:27 +0800
Li Ming <ming.li@zohomail.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@zohomail.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)
On 2/11/25 10:36 AM, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 15:57:27 +0800
> Li Ming <ming.li@zohomail.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@zohomail.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)
>
On 2/12/2025 5:24 AM, Dave Jiang wrote: > > On 2/11/25 10:36 AM, Jonathan Cameron wrote: >> On Tue, 11 Feb 2025 15:57:27 +0800 >> Li Ming <ming.li@zohomail.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@zohomail.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 Will use construct_auto_region() in V2 if no a better name, thanks for that. Ming
© 2016 - 2025 Red Hat, Inc.