Some operations need to be protected by the cxl_region_rwsem in
construct_region(). Currently, construct_region() uses down_write() and
up_write() for the cxl_region_rwsem locking, 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_auto_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_auto_region().
Signed-off-by: Li Ming <ming.li@zohomail.com>
---
v2:
- Rename __construct_region() to construct_auto_region(). (Jonathan and Dave)
---
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 320a3f218131..7a9e51aba9f4 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3216,49 +3216,31 @@ static int match_region_by_range(struct device *dev, const void *data)
return 0;
}
-/* 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_auto_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));
@@ -3281,7 +3263,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__,
@@ -3290,14 +3272,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_auto_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.1Li Ming wrote: > Some operations need to be protected by the cxl_region_rwsem in > construct_region(). Currently, construct_region() uses down_write() and > up_write() for the cxl_region_rwsem locking, 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_auto_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_auto_region(). > > Signed-off-by: Li Ming <ming.li@zohomail.com> > --- > v2: > - Rename __construct_region() to construct_auto_region(). (Jonathan and Dave) > --- > 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 320a3f218131..7a9e51aba9f4 100644 > --- a/drivers/cxl/core/region.c > +++ b/drivers/cxl/core/region.c > @@ -3216,49 +3216,31 @@ static int match_region_by_range(struct device *dev, const void *data) > return 0; > } > > -/* 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_auto_region(struct cxl_region *cxlr, ...probably would have called this __construct_region() since there is little distinction that merits adding the "auto" qualifier. Other than that, for this and the others you can add: Reviewed-by: Dan Williams <dan.j.williams@intel.com>
On 2/20/2025 9:04 AM, Dan Williams wrote: > Li Ming wrote: >> Some operations need to be protected by the cxl_region_rwsem in >> construct_region(). Currently, construct_region() uses down_write() and >> up_write() for the cxl_region_rwsem locking, 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_auto_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_auto_region(). >> >> Signed-off-by: Li Ming <ming.li@zohomail.com> >> --- >> v2: >> - Rename __construct_region() to construct_auto_region(). (Jonathan and Dave) >> --- >> 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 320a3f218131..7a9e51aba9f4 100644 >> --- a/drivers/cxl/core/region.c >> +++ b/drivers/cxl/core/region.c >> @@ -3216,49 +3216,31 @@ static int match_region_by_range(struct device *dev, const void *data) >> return 0; >> } >> >> -/* 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_auto_region(struct cxl_region *cxlr, > ...probably would have called this __construct_region() since there is > little distinction that merits adding the "auto" qualifier. Sure, will revert this renaming, continue to use __construct_region(), thanks for review. Ming > > Other than that, for this and the others you can add: > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
On Mon, Feb 17, 2025 at 10:48:28PM +0800, Li Ming wrote: > Some operations need to be protected by the cxl_region_rwsem in > construct_region(). Currently, construct_region() uses down_write() and > up_write() for the cxl_region_rwsem locking, 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_auto_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_auto_region(). > > Signed-off-by: Li Ming <ming.li@zohomail.com> Reviewed-by: Alison Schofield <alison.schofield@intel.com> snip
On 2/17/25 7:48 AM, Li Ming wrote:
> Some operations need to be protected by the cxl_region_rwsem in
> construct_region(). Currently, construct_region() uses down_write() and
> up_write() for the cxl_region_rwsem locking, 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_auto_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_auto_region().
>
> Signed-off-by: Li Ming <ming.li@zohomail.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v2:
> - Rename __construct_region() to construct_auto_region(). (Jonathan and Dave)
> ---
> 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 320a3f218131..7a9e51aba9f4 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3216,49 +3216,31 @@ static int match_region_by_range(struct device *dev, const void *data)
> return 0;
> }
>
> -/* 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_auto_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));
> @@ -3281,7 +3263,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__,
> @@ -3290,14 +3272,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_auto_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 Mon, 17 Feb 2025 22:48:28 +0800 Li Ming <ming.li@zohomail.com> wrote: > Some operations need to be protected by the cxl_region_rwsem in > construct_region(). Currently, construct_region() uses down_write() and > up_write() for the cxl_region_rwsem locking, 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_auto_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_auto_region(). > > Signed-off-by: Li Ming <ming.li@zohomail.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
© 2016 - 2025 Red Hat, Inc.