A region is always bound to a root decoder. The region's associated
root decoder is often needed. Add it to struct cxl_region.
This simplifies code by removing dynamic lookups and removing the root
decoder argument from the function argument list where possible.
Patch is a prerequisite to implement address translation which uses
struct cxl_region to store all relevant region and interleaving
parameters.
Signed-off-by: Robert Richter <rrichter@amd.com>
---
drivers/cxl/core/region.c | 37 +++++++++++++++++++------------------
drivers/cxl/cxl.h | 2 ++
2 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 29d3809ab2bb..2c37c060d983 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -489,9 +489,9 @@ static ssize_t interleave_ways_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t len)
{
- struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent);
- struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
struct cxl_region *cxlr = to_cxl_region(dev);
+ struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
+ struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
struct cxl_region_params *p = &cxlr->params;
unsigned int val, save;
int rc;
@@ -552,9 +552,9 @@ static ssize_t interleave_granularity_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t len)
{
- struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent);
- struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
struct cxl_region *cxlr = to_cxl_region(dev);
+ struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
+ struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
struct cxl_region_params *p = &cxlr->params;
int rc, val;
u16 ig;
@@ -628,7 +628,7 @@ static DEVICE_ATTR_RO(mode);
static int alloc_hpa(struct cxl_region *cxlr, resource_size_t size)
{
- struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
+ struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
struct cxl_region_params *p = &cxlr->params;
struct resource *res;
u64 remainder = 0;
@@ -1321,7 +1321,7 @@ static int cxl_port_setup_targets(struct cxl_port *port,
struct cxl_region *cxlr,
struct cxl_endpoint_decoder *cxled)
{
- struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
+ struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
int parent_iw, parent_ig, ig, iw, rc, inc = 0, pos = cxled->pos;
struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
@@ -1678,10 +1678,10 @@ static int cxl_region_validate_position(struct cxl_region *cxlr,
}
static int cxl_region_attach_position(struct cxl_region *cxlr,
- struct cxl_root_decoder *cxlrd,
struct cxl_endpoint_decoder *cxled,
const struct cxl_dport *dport, int pos)
{
+ struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
struct cxl_switch_decoder *cxlsd = &cxlrd->cxlsd;
struct cxl_decoder *cxld = &cxlsd->cxld;
@@ -1918,7 +1918,7 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
static int cxl_region_attach(struct cxl_region *cxlr,
struct cxl_endpoint_decoder *cxled, int pos)
{
- struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
+ struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
struct cxl_dev_state *cxlds = cxlmd->cxlds;
struct cxl_region_params *p = &cxlr->params;
@@ -2023,8 +2023,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
ep_port = cxled_to_port(cxled);
dport = cxl_find_dport_by_dev(root_port,
ep_port->host_bridge);
- rc = cxl_region_attach_position(cxlr, cxlrd, cxled,
- dport, i);
+ rc = cxl_region_attach_position(cxlr, cxled, dport, i);
if (rc)
return rc;
}
@@ -2047,7 +2046,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
if (rc)
return rc;
- rc = cxl_region_attach_position(cxlr, cxlrd, cxled, dport, pos);
+ rc = cxl_region_attach_position(cxlr, cxled, dport, pos);
if (rc)
return rc;
@@ -2343,8 +2342,8 @@ static const struct attribute_group *region_groups[] = {
static void cxl_region_release(struct device *dev)
{
- struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent);
struct cxl_region *cxlr = to_cxl_region(dev);
+ struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
int id = atomic_read(&cxlrd->region_id);
/*
@@ -2427,10 +2426,12 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i
* region id allocations
*/
get_device(dev->parent);
+ cxlr->cxlrd = cxlrd;
+ cxlr->id = id;
+
device_set_pm_not_required(dev);
dev->bus = &cxl_bus_type;
dev->type = &cxl_region_type;
- cxlr->id = id;
return cxlr;
}
@@ -2931,7 +2932,7 @@ static bool has_spa_to_hpa(struct cxl_root_decoder *cxlrd)
u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
u64 dpa)
{
- struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
+ struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
u64 dpa_offset, hpa_offset, bits_upper, mask_upper, hpa;
struct cxl_region_params *p = &cxlr->params;
struct cxl_endpoint_decoder *cxled = NULL;
@@ -3007,7 +3008,7 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
struct dpa_result *result)
{
struct cxl_region_params *p = &cxlr->params;
- struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
+ struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
struct cxl_endpoint_decoder *cxled;
u64 hpa, hpa_offset, dpa_offset;
u64 bits_upper, bits_lower;
@@ -3401,7 +3402,7 @@ static int match_region_by_range(struct device *dev, const void *data)
static int cxl_extended_linear_cache_resize(struct cxl_region *cxlr,
struct resource *res)
{
- struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
+ struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
struct cxl_region_params *p = &cxlr->params;
resource_size_t size = resource_size(res);
resource_size_t cache_size, start;
@@ -3437,9 +3438,9 @@ static int cxl_extended_linear_cache_resize(struct cxl_region *cxlr,
}
static int __construct_region(struct cxl_region *cxlr,
- struct cxl_root_decoder *cxlrd,
struct cxl_endpoint_decoder *cxled)
{
+ struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
struct range *hpa = &cxled->cxld.hpa_range;
struct cxl_region_params *p;
@@ -3531,7 +3532,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
return cxlr;
}
- rc = __construct_region(cxlr, cxlrd, cxled);
+ rc = __construct_region(cxlr, cxled);
if (rc) {
devm_release_action(port->uport_dev, unregister_region, cxlr);
return ERR_PTR(rc);
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 4fe3df06f57a..350ccd6949b3 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -517,6 +517,7 @@ enum cxl_partition_mode {
* struct cxl_region - CXL region
* @dev: This region's device
* @id: This region's id. Id is globally unique across all regions
+ * @cxlrd: Region's root decoder
* @mode: Operational mode of the mapped capacity
* @type: Endpoint decoder target type
* @cxl_nvb: nvdimm bridge for coordinating @cxlr_pmem setup / shutdown
@@ -530,6 +531,7 @@ enum cxl_partition_mode {
struct cxl_region {
struct device dev;
int id;
+ struct cxl_root_decoder *cxlrd;
enum cxl_partition_mode mode;
enum cxl_decoder_type type;
struct cxl_nvdimm_bridge *cxl_nvb;
--
2.39.5On Fri, Sep 12, 2025 at 04:45:03PM +0200, Robert Richter wrote: > A region is always bound to a root decoder. The region's associated > root decoder is often needed. Add it to struct cxl_region. > > This simplifies code by removing dynamic lookups and removing the root > decoder argument from the function argument list where possible. > > Patch is a prerequisite to implement address translation which uses > struct cxl_region to store all relevant region and interleaving > parameters. Where is the follow on patch that makes this a prerequisite?
On 23.09.25 14:40:31, Alison Schofield wrote: > On Fri, Sep 12, 2025 at 04:45:03PM +0200, Robert Richter wrote: > > A region is always bound to a root decoder. The region's associated > > root decoder is often needed. Add it to struct cxl_region. > > > > This simplifies code by removing dynamic lookups and removing the root > > decoder argument from the function argument list where possible. > > > > Patch is a prerequisite to implement address translation which uses > > struct cxl_region to store all relevant region and interleaving > > parameters. > > Where is the follow on patch that makes this a prerequisite? I changed this to simplify and rework the argument list of __construct_region(cxlr, cxled). Later, this changes further to use a context argument: __construct_region(cxlr, ctx). The patch additionally simplifies the arg list of cxl_region_attach_position() and it removes the use of to_cxl_root_decoder() which always reconstructs and checks the pointer. The pointer never changes and is frequently used. Also, code becomes more readable as this amphazises the binding between both objects. Overall, I don't see a reason to use to_cxl_root_decoder() as an alternative to avoid the the pointer, which would generally be possible. I will update the description accordingly. Thanks, -Robert
On Fri, Sep 12, 2025 at 04:45:03PM +0200, Robert Richter wrote: > A region is always bound to a root decoder. The region's associated > root decoder is often needed. Add it to struct cxl_region. > > This simplifies code by removing dynamic lookups and removing the root > decoder argument from the function argument list where possible. > > Patch is a prerequisite to implement address translation which uses > struct cxl_region to store all relevant region and interleaving > parameters. > > Signed-off-by: Robert Richter <rrichter@amd.com> Nice Reviewed-by: Gregory Price <gourry@gourry.net>
On 9/12/25 7:45 AM, Robert Richter wrote:
> A region is always bound to a root decoder. The region's associated
> root decoder is often needed. Add it to struct cxl_region.
>
> This simplifies code by removing dynamic lookups and removing the root
> decoder argument from the function argument list where possible.
>
> Patch is a prerequisite to implement address translation which uses
> struct cxl_region to store all relevant region and interleaving
> parameters.
>
> Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> drivers/cxl/core/region.c | 37 +++++++++++++++++++------------------
> drivers/cxl/cxl.h | 2 ++
> 2 files changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 29d3809ab2bb..2c37c060d983 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -489,9 +489,9 @@ static ssize_t interleave_ways_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t len)
> {
> - struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent);
> - struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
> struct cxl_region *cxlr = to_cxl_region(dev);
> + struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> + struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
> struct cxl_region_params *p = &cxlr->params;
> unsigned int val, save;
> int rc;
> @@ -552,9 +552,9 @@ static ssize_t interleave_granularity_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t len)
> {
> - struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent);
> - struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
> struct cxl_region *cxlr = to_cxl_region(dev);
> + struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> + struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
> struct cxl_region_params *p = &cxlr->params;
> int rc, val;
> u16 ig;
> @@ -628,7 +628,7 @@ static DEVICE_ATTR_RO(mode);
>
> static int alloc_hpa(struct cxl_region *cxlr, resource_size_t size)
> {
> - struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> + struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> struct cxl_region_params *p = &cxlr->params;
> struct resource *res;
> u64 remainder = 0;
> @@ -1321,7 +1321,7 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> struct cxl_region *cxlr,
> struct cxl_endpoint_decoder *cxled)
> {
> - struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> + struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> int parent_iw, parent_ig, ig, iw, rc, inc = 0, pos = cxled->pos;
> struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
> struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
> @@ -1678,10 +1678,10 @@ static int cxl_region_validate_position(struct cxl_region *cxlr,
> }
>
> static int cxl_region_attach_position(struct cxl_region *cxlr,
> - struct cxl_root_decoder *cxlrd,
> struct cxl_endpoint_decoder *cxled,
> const struct cxl_dport *dport, int pos)
> {
> + struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> struct cxl_switch_decoder *cxlsd = &cxlrd->cxlsd;
> struct cxl_decoder *cxld = &cxlsd->cxld;
> @@ -1918,7 +1918,7 @@ static int cxl_region_sort_targets(struct cxl_region *cxlr)
> static int cxl_region_attach(struct cxl_region *cxlr,
> struct cxl_endpoint_decoder *cxled, int pos)
> {
> - struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> + struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> struct cxl_dev_state *cxlds = cxlmd->cxlds;
> struct cxl_region_params *p = &cxlr->params;
> @@ -2023,8 +2023,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> ep_port = cxled_to_port(cxled);
> dport = cxl_find_dport_by_dev(root_port,
> ep_port->host_bridge);
> - rc = cxl_region_attach_position(cxlr, cxlrd, cxled,
> - dport, i);
> + rc = cxl_region_attach_position(cxlr, cxled, dport, i);
> if (rc)
> return rc;
> }
> @@ -2047,7 +2046,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
> if (rc)
> return rc;
>
> - rc = cxl_region_attach_position(cxlr, cxlrd, cxled, dport, pos);
> + rc = cxl_region_attach_position(cxlr, cxled, dport, pos);
> if (rc)
> return rc;
>
> @@ -2343,8 +2342,8 @@ static const struct attribute_group *region_groups[] = {
>
> static void cxl_region_release(struct device *dev)
> {
> - struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(dev->parent);
> struct cxl_region *cxlr = to_cxl_region(dev);
> + struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> int id = atomic_read(&cxlrd->region_id);
>
> /*
> @@ -2427,10 +2426,12 @@ static struct cxl_region *cxl_region_alloc(struct cxl_root_decoder *cxlrd, int i
> * region id allocations
> */
> get_device(dev->parent);
> + cxlr->cxlrd = cxlrd;
> + cxlr->id = id;
> +
> device_set_pm_not_required(dev);
> dev->bus = &cxl_bus_type;
> dev->type = &cxl_region_type;
> - cxlr->id = id;
>
> return cxlr;
> }
> @@ -2931,7 +2932,7 @@ static bool has_spa_to_hpa(struct cxl_root_decoder *cxlrd)
> u64 cxl_dpa_to_hpa(struct cxl_region *cxlr, const struct cxl_memdev *cxlmd,
> u64 dpa)
> {
> - struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> + struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> u64 dpa_offset, hpa_offset, bits_upper, mask_upper, hpa;
> struct cxl_region_params *p = &cxlr->params;
> struct cxl_endpoint_decoder *cxled = NULL;
> @@ -3007,7 +3008,7 @@ static int region_offset_to_dpa_result(struct cxl_region *cxlr, u64 offset,
> struct dpa_result *result)
> {
> struct cxl_region_params *p = &cxlr->params;
> - struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> + struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> struct cxl_endpoint_decoder *cxled;
> u64 hpa, hpa_offset, dpa_offset;
> u64 bits_upper, bits_lower;
> @@ -3401,7 +3402,7 @@ static int match_region_by_range(struct device *dev, const void *data)
> static int cxl_extended_linear_cache_resize(struct cxl_region *cxlr,
> struct resource *res)
> {
> - struct cxl_root_decoder *cxlrd = to_cxl_root_decoder(cxlr->dev.parent);
> + struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> struct cxl_region_params *p = &cxlr->params;
> resource_size_t size = resource_size(res);
> resource_size_t cache_size, start;
> @@ -3437,9 +3438,9 @@ static int cxl_extended_linear_cache_resize(struct cxl_region *cxlr,
> }
>
> static int __construct_region(struct cxl_region *cxlr,
> - struct cxl_root_decoder *cxlrd,
> struct cxl_endpoint_decoder *cxled)
> {
> + struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> struct range *hpa = &cxled->cxld.hpa_range;
> struct cxl_region_params *p;
> @@ -3531,7 +3532,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
> return cxlr;
> }
>
> - rc = __construct_region(cxlr, cxlrd, cxled);
> + rc = __construct_region(cxlr, cxled);
> if (rc) {
> devm_release_action(port->uport_dev, unregister_region, cxlr);
> return ERR_PTR(rc);
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 4fe3df06f57a..350ccd6949b3 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -517,6 +517,7 @@ enum cxl_partition_mode {
> * struct cxl_region - CXL region
> * @dev: This region's device
> * @id: This region's id. Id is globally unique across all regions
> + * @cxlrd: Region's root decoder
> * @mode: Operational mode of the mapped capacity
> * @type: Endpoint decoder target type
> * @cxl_nvb: nvdimm bridge for coordinating @cxlr_pmem setup / shutdown
> @@ -530,6 +531,7 @@ enum cxl_partition_mode {
> struct cxl_region {
> struct device dev;
> int id;
> + struct cxl_root_decoder *cxlrd;
> enum cxl_partition_mode mode;
> enum cxl_decoder_type type;
> struct cxl_nvdimm_bridge *cxl_nvb;
On Fri, 12 Sep 2025 08:52:06 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > On 9/12/25 7:45 AM, Robert Richter wrote: > > A region is always bound to a root decoder. The region's associated > > root decoder is often needed. Add it to struct cxl_region. > > > > This simplifies code by removing dynamic lookups and removing the root > > decoder argument from the function argument list where possible. > > > > Patch is a prerequisite to implement address translation which uses > > struct cxl_region to store all relevant region and interleaving > > parameters. > > > > Signed-off-by: Robert Richter <rrichter@amd.com> > > Reviewed-by: Dave Jiang <dave.jiang@intel.com> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
© 2016 - 2025 Red Hat, Inc.