[PATCH 0/7] cxl/region: Refactor cxl_port_setup_targets()

[PATCH 0/7] cxl/region: Refactor cxl_port_setup_targets()

[Alison Schofield]

cxl_port_setup_targets() has grown to nearly 200 lines. It open codes each
step of adding an endpoint to a port's decoder: finding the distance
between peer positions, reading the parent interleave, calculating and
validating the port's interleave, setting up the decoder, and adding the
target.

This set is inspired by review feedback from Robert Richter on the series
that adds mixed-granularity region support [1]. That series needs to change
how this function calculates interleaves and Robert asked for that new code
to be moved into a helper. Rather than tidy only the new code, this set
splits up the whole function first.

Patches 1 and 2 prepare the code that moves. Both patches are suggestions
that Jonathan offered on the mixed gran series, and Patch 2 is actually 
plucked from the mixed gran series where it sat as a prep patch.

Patches 3-7 each move one step at a time into its own helper, which leaves
cxl_port_setup_targets() at 58 lines.

The refactor patches make no functional changes and aim to be mechanical
and easy to review. I tried to avoided all 'while you're at it' changes
that would muddy the mechanical split.


That base commit is from cxl/next, as of this moment.

[1] https://lore.kernel.org/linux-cxl/cover.1788475206.git.alison.schofield@intel.com/


Alison Schofield (7):
  cxl: Add interleave encoding validity helpers
  cxl/region: Name the interleave locals in cxl_port_setup_targets()
  cxl/region: Factor out cxl_port_peer_distance()
  cxl/region: Factor out cxl_port_parent_interleave()
  cxl/region: Factor out cxl_port_calc_interleave()
  cxl/region: Factor out cxl_port_setup_decoder()
  cxl/region: Factor out cxl_port_add_target()

 drivers/cxl/core/region.c | 252 +++++++++++++++++++++++---------------
 drivers/cxl/cxl.h         |  21 ++++
 2 files changed, 176 insertions(+), 97 deletions(-)


base-commit: 71392a644e88c6bb921cdf12cb2b00adec070fe4
-- 
2.37.3

[PATCH 1/7] cxl: Add interleave encoding validity helpers

[Alison Schofield]

Some callers of ways_to_eiw(), granularity_to_eig(), and
eig_to_granularity() only need to determine whether an input is valid,
but must provide storage for a converted value that is then discarded.
This obscures the purpose of the call and requires unnecessary temporary
variables.

Define and use new helpers that provide a validation-only interface for
those callers.

cxl_port_setup_targets() also encodes and then decodes its calculated
granularity solely to validate it. Calculate the granularity directly
and validate it instead, making the calculation explicit and removing
the encode/decode round trip.

No functional change.

Suggested-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/region.c | 49 +++++++++++++--------------------------
 drivers/cxl/cxl.h         | 21 +++++++++++++++++
 2 files changed, 37 insertions(+), 33 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index a048bd5c06cb..bc087ca63946 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -493,11 +493,9 @@ static int set_interleave_ways(struct cxl_region *cxlr, int val)
 	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
 	struct cxl_region_params *p = &cxlr->params;
 	int save, rc;
-	u8 iw;
 
-	rc = ways_to_eiw(val, &iw);
-	if (rc)
-		return rc;
+	if (!ways_valid(val))
+		return -EINVAL;
 
 	/*
 	 * Even for x3, x6, and x12 interleaves the region interleave must be a
@@ -566,12 +564,9 @@ static int set_interleave_granularity(struct cxl_region *cxlr, int val)
 	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
 	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
 	struct cxl_region_params *p = &cxlr->params;
-	int rc;
-	u16 ig;
 
-	rc = granularity_to_eig(val, &ig);
-	if (rc)
-		return rc;
+	if (!granularity_valid(val))
+		return -EINVAL;
 
 	/*
 	 * When the host-bridge is interleaved, disallow region granularity !=
@@ -1451,8 +1446,6 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 	struct cxl_decoder *cxld = cxl_rr->decoder;
 	struct cxl_switch_decoder *cxlsd;
 	struct cxl_port *iter = port;
-	u16 eig, peig;
-	u8 eiw, peiw;
 
 	/*
 	 * While root level decoders support x3, x6, x12, switch level
@@ -1524,48 +1517,38 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 		parent_iw = parent_cxld->interleave_ways;
 	}
 
-	rc = granularity_to_eig(parent_ig, &peig);
-	if (rc) {
+	if (!granularity_valid(parent_ig)) {
 		dev_dbg(&cxlr->dev, "%s:%s: invalid parent granularity: %d\n",
 			dev_name(parent_port->uport_dev),
 			dev_name(&parent_port->dev), parent_ig);
-		return rc;
+		return -EINVAL;
 	}
 
-	rc = ways_to_eiw(parent_iw, &peiw);
-	if (rc) {
+	if (!ways_valid(parent_iw)) {
 		dev_dbg(&cxlr->dev, "%s:%s: invalid parent interleave: %d\n",
 			dev_name(parent_port->uport_dev),
 			dev_name(&parent_port->dev), parent_iw);
-		return rc;
+		return -EINVAL;
 	}
 
 	iw = cxl_rr->nr_targets;
-	rc = ways_to_eiw(iw, &eiw);
-	if (rc) {
+	if (!ways_valid(iw)) {
 		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",
 			dev_name(port->uport_dev), dev_name(&port->dev), iw);
-		return rc;
+		return -EINVAL;
 	}
 
 	/*
 	 * Interleave granularity is a multiple of @parent_port granularity.
 	 * Multiplier is the parent port interleave ways.
 	 */
-	rc = granularity_to_eig(parent_ig * parent_iw, &eig);
-	if (rc) {
+	ig = parent_ig * parent_iw;
+
+	if (!granularity_valid(ig)) {
 		dev_dbg(&cxlr->dev,
 			"%s: invalid granularity calculation (%d * %d)\n",
 			dev_name(&parent_port->dev), parent_ig, parent_iw);
-		return rc;
-	}
-
-	rc = eig_to_granularity(eig, &ig);
-	if (rc) {
-		dev_dbg(&cxlr->dev, "%s:%s: invalid interleave: %d\n",
-			dev_name(port->uport_dev), dev_name(&port->dev),
-			256 << eig);
-		return rc;
+		return -EINVAL;
 	}
 
 	if (iw > 8 || iw > cxlsd->nr_targets) {
@@ -3131,13 +3114,13 @@ static bool cxl_is_hpa_in_chunk(u64 hpa, struct cxl_region *cxlr, int pos)
  */
 int cxl_validate_translation_params(u8 eiw, u16 eig, int pos)
 {
-	int ways, gran;
+	int ways;
 
 	if (eiw_to_ways(eiw, &ways)) {
 		pr_debug("%s: invalid eiw=%u\n", __func__, eiw);
 		return -EINVAL;
 	}
-	if (eig_to_granularity(eig, &gran)) {
+	if (!eig_valid(eig)) {
 		pr_debug("%s: invalid eig=%u\n", __func__, eig);
 		return -EINVAL;
 	}
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index f03ee0addc45..4c9173e07f1f 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -146,6 +146,27 @@ static inline int ways_to_eiw(unsigned int ways, u8 *eiw)
 	return 0;
 }
 
+static inline bool eig_valid(u16 eig)
+{
+	int granularity;
+
+	return eig_to_granularity(eig, &granularity) == 0;
+}
+
+static inline bool granularity_valid(int granularity)
+{
+	u16 eig;
+
+	return granularity_to_eig(granularity, &eig) == 0;
+}
+
+static inline bool ways_valid(unsigned int ways)
+{
+	u8 eiw;
+
+	return ways_to_eiw(ways, &eiw) == 0;
+}
+
 /* RAS Registers CXL 2.0 8.2.5.9 CXL RAS Capability Structure */
 #define CXL_RAS_UNCORRECTABLE_STATUS_OFFSET 0x0
 #define   CXL_RAS_UNCORRECTABLE_STATUS_MASK (GENMASK(16, 14) | GENMASK(11, 0))
-- 
2.37.3

[PATCH 2/7] cxl/region: Name the interleave locals in cxl_port_setup_targets()

[Alison Schofield]

cxl_port_setup_targets() derives the interleave granularity of one decoder
from the granularity and ways of the decoder above it, and takes that
decoder's own ways from the topology below it. The values from the level
above are named parent_iw and parent_ig, but the values being derived are
the bare iw and ig, so the reader has to work out which level each one
describes.

Rename iw and ig to child_iw and child_ig to match the parent_ prefix used
for the level above.

No functional change.

Suggested-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
---
 drivers/cxl/core/region.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index bc087ca63946..1a42753fda48 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1437,7 +1437,7 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 				  struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
-	int parent_iw, parent_ig, ig, iw, rc, pos = cxled->pos;
+	int parent_iw, parent_ig, child_iw, child_ig, rc, 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);
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
@@ -1531,10 +1531,10 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 		return -EINVAL;
 	}
 
-	iw = cxl_rr->nr_targets;
-	if (!ways_valid(iw)) {
+	child_iw = cxl_rr->nr_targets;
+	if (!ways_valid(child_iw)) {
 		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",
-			dev_name(port->uport_dev), dev_name(&port->dev), iw);
+			dev_name(port->uport_dev), dev_name(&port->dev), child_iw);
 		return -EINVAL;
 	}
 
@@ -1542,32 +1542,32 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 	 * Interleave granularity is a multiple of @parent_port granularity.
 	 * Multiplier is the parent port interleave ways.
 	 */
-	ig = parent_ig * parent_iw;
+	child_ig = parent_ig * parent_iw;
 
-	if (!granularity_valid(ig)) {
+	if (!granularity_valid(child_ig)) {
 		dev_dbg(&cxlr->dev,
 			"%s: invalid granularity calculation (%d * %d)\n",
 			dev_name(&parent_port->dev), parent_ig, parent_iw);
 		return -EINVAL;
 	}
 
-	if (iw > 8 || iw > cxlsd->nr_targets) {
+	if (child_iw > 8 || child_iw > cxlsd->nr_targets) {
 		dev_dbg(&cxlr->dev,
 			"%s:%s:%s: ways: %d overflows targets: %d\n",
 			dev_name(port->uport_dev), dev_name(&port->dev),
-			dev_name(&cxld->dev), iw, cxlsd->nr_targets);
+			dev_name(&cxld->dev), child_iw, cxlsd->nr_targets);
 		return -ENXIO;
 	}
 
 	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
-		if (cxld->interleave_ways != iw ||
-		    (iw > 1 && cxld->interleave_granularity != ig) ||
+		if (cxld->interleave_ways != child_iw ||
+		    (child_iw > 1 && cxld->interleave_granularity != child_ig) ||
 		    !spa_maps_hpa(p, &cxld->hpa_range) ||
 		    ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)) {
 			dev_err(&cxlr->dev,
 				"%s:%s %s expected iw: %d ig: %d %pr\n",
 				dev_name(port->uport_dev), dev_name(&port->dev),
-				__func__, iw, ig, p->res);
+				__func__, child_iw, child_ig, p->res);
 			dev_err(&cxlr->dev,
 				"%s:%s %s got iw: %d ig: %d state: %s %#llx:%#llx\n",
 				dev_name(port->uport_dev), dev_name(&port->dev),
@@ -1578,24 +1578,24 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 			return -ENXIO;
 		}
 	} else {
-		rc = check_interleave_cap(cxld, iw, ig);
+		rc = check_interleave_cap(cxld, child_iw, child_ig);
 		if (rc) {
 			dev_dbg(&cxlr->dev,
 				"%s:%s iw: %d ig: %d is not supported\n",
 				dev_name(port->uport_dev),
-				dev_name(&port->dev), iw, ig);
+				dev_name(&port->dev), child_iw, child_ig);
 			return rc;
 		}
 
-		cxld->interleave_ways = iw;
-		cxld->interleave_granularity = ig;
+		cxld->interleave_ways = child_iw;
+		cxld->interleave_granularity = child_ig;
 		cxld->hpa_range = (struct range) {
 			.start = p->res->start,
 			.end = p->res->end,
 		};
 	}
 	dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport_dev),
-		dev_name(&port->dev), iw, ig);
+		dev_name(&port->dev), child_iw, child_ig);
 add_target:
 	if (cxl_rr->nr_targets_set == cxl_rr->nr_targets) {
 		dev_dbg(&cxlr->dev,
-- 
2.37.3

[PATCH 3/7] cxl/region: Factor out cxl_port_peer_distance()

[Alison Schofield]

cxl_port_setup_targets() is nearly 200 lines long and open codes each step
of adding an endpoint to a port's decoder. One of those steps calculates
the distance between region positions that share a downstream port.

Move that calculation to a new helper, cxl_port_peer_distance().

No functional change.

Assisted-by: LLM
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/region.c | 50 +++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 1a42753fda48..80c40716c368 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1432,6 +1432,33 @@ static int check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
 	return 0;
 }
 
+static int cxl_port_peer_distance(struct cxl_port *port, struct cxl_region *cxlr)
+{
+	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
+	struct cxl_region_ref *cxl_rr_iter;
+	struct cxl_port *iter = port;
+	int distance = 1;
+
+	/*
+	 * The "distance" between peer downstream ports represents which
+	 * endpoint positions in the region interleave a given port can
+	 * host.
+	 *
+	 * For example, at the root of a hierarchy the distance is
+	 * always 1 as every index targets a different host-bridge. At
+	 * each subsequent switch level those ports map every Nth region
+	 * position where N is the width of the switch == distance.
+	 */
+	do {
+		cxl_rr_iter = cxl_rr_load(iter, cxlr);
+		distance *= cxl_rr_iter->nr_targets;
+		iter = to_cxl_port(iter->dev.parent);
+	} while (!is_cxl_root(iter));
+	distance *= cxlrd->cxlsd.cxld.interleave_ways;
+
+	return distance;
+}
+
 static int cxl_port_setup_targets(struct cxl_port *port,
 				  struct cxl_region *cxlr,
 				  struct cxl_endpoint_decoder *cxled)
@@ -1445,7 +1472,6 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 	struct cxl_region_params *p = &cxlr->params;
 	struct cxl_decoder *cxld = cxl_rr->decoder;
 	struct cxl_switch_decoder *cxlsd;
-	struct cxl_port *iter = port;
 
 	/*
 	 * While root level decoders support x3, x6, x12, switch level
@@ -1460,27 +1486,9 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 
 	cxlsd = to_cxl_switch_decoder(&cxld->dev);
 	if (cxl_rr->nr_targets_set) {
-		int i, distance = 1;
-		struct cxl_region_ref *cxl_rr_iter;
+		int distance = cxl_port_peer_distance(port, cxlr);
 
-		/*
-		 * The "distance" between peer downstream ports represents which
-		 * endpoint positions in the region interleave a given port can
-		 * host.
-		 *
-		 * For example, at the root of a hierarchy the distance is
-		 * always 1 as every index targets a different host-bridge. At
-		 * each subsequent switch level those ports map every Nth region
-		 * position where N is the width of the switch == distance.
-		 */
-		do {
-			cxl_rr_iter = cxl_rr_load(iter, cxlr);
-			distance *= cxl_rr_iter->nr_targets;
-			iter = to_cxl_port(iter->dev.parent);
-		} while (!is_cxl_root(iter));
-		distance *= cxlrd->cxlsd.cxld.interleave_ways;
-
-		for (i = 0; i < cxl_rr->nr_targets_set; i++)
+		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
 			if (ep->dport == cxlsd->target[i]) {
 				rc = check_last_peer(cxled, ep, cxl_rr,
 						     distance);
-- 
2.37.3

[PATCH 4/7] cxl/region: Factor out cxl_port_parent_interleave()

[Alison Schofield]

cxl_port_setup_targets() derives a decoder's interleave from the interleave
of the decoder above it. That parent interleave comes from either the root
decoder or a switch decoder, and each case is open coded along with the
parent validity checks.

Move the parent interleave lookup and validation to a new helper,
cxl_port_parent_interleave().

No functional change.

Assisted-by: LLM
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/region.c | 88 +++++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 36 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 80c40716c368..c1866b7b089b 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1459,45 +1459,13 @@ static int cxl_port_peer_distance(struct cxl_port *port, struct cxl_region *cxlr
 	return distance;
 }
 
-static int cxl_port_setup_targets(struct cxl_port *port,
-				  struct cxl_region *cxlr,
-				  struct cxl_endpoint_decoder *cxled)
+static int cxl_port_parent_interleave(struct cxl_port *port, struct cxl_region *cxlr,
+				      int *iw, int *ig)
 {
-	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
-	int parent_iw, parent_ig, child_iw, child_ig, rc, 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);
-	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
-	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
+	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
 	struct cxl_region_params *p = &cxlr->params;
-	struct cxl_decoder *cxld = cxl_rr->decoder;
-	struct cxl_switch_decoder *cxlsd;
-
-	/*
-	 * While root level decoders support x3, x6, x12, switch level
-	 * decoders only support powers of 2 up to x16.
-	 */
-	if (!is_power_of_2(cxl_rr->nr_targets)) {
-		dev_dbg(&cxlr->dev, "%s:%s: invalid target count %d\n",
-			dev_name(port->uport_dev), dev_name(&port->dev),
-			cxl_rr->nr_targets);
-		return -EINVAL;
-	}
-
-	cxlsd = to_cxl_switch_decoder(&cxld->dev);
-	if (cxl_rr->nr_targets_set) {
-		int distance = cxl_port_peer_distance(port, cxlr);
-
-		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
-			if (ep->dport == cxlsd->target[i]) {
-				rc = check_last_peer(cxled, ep, cxl_rr,
-						     distance);
-				if (rc)
-					return rc;
-				goto out_target_set;
-			}
-		goto add_target;
-	}
+	int parent_iw, parent_ig;
 
 	if (is_cxl_root(parent_port)) {
 		/*
@@ -1539,6 +1507,54 @@ static int cxl_port_setup_targets(struct cxl_port *port,
 		return -EINVAL;
 	}
 
+	*iw = parent_iw;
+	*ig = parent_ig;
+
+	return 0;
+}
+
+static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
+				  struct cxl_endpoint_decoder *cxled)
+{
+	int parent_iw, parent_ig, child_iw, child_ig, rc, 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);
+	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
+	struct cxl_region_params *p = &cxlr->params;
+	struct cxl_decoder *cxld = cxl_rr->decoder;
+	struct cxl_switch_decoder *cxlsd;
+
+	/*
+	 * While root level decoders support x3, x6, x12, switch level
+	 * decoders only support powers of 2 up to x16.
+	 */
+	if (!is_power_of_2(cxl_rr->nr_targets)) {
+		dev_dbg(&cxlr->dev, "%s:%s: invalid target count %d\n",
+			dev_name(port->uport_dev), dev_name(&port->dev),
+			cxl_rr->nr_targets);
+		return -EINVAL;
+	}
+
+	cxlsd = to_cxl_switch_decoder(&cxld->dev);
+	if (cxl_rr->nr_targets_set) {
+		int distance = cxl_port_peer_distance(port, cxlr);
+
+		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
+			if (ep->dport == cxlsd->target[i]) {
+				rc = check_last_peer(cxled, ep, cxl_rr,
+						     distance);
+				if (rc)
+					return rc;
+				goto out_target_set;
+			}
+		goto add_target;
+	}
+
+	rc = cxl_port_parent_interleave(port, cxlr, &parent_iw, &parent_ig);
+	if (rc)
+		return rc;
+
 	child_iw = cxl_rr->nr_targets;
 	if (!ways_valid(child_iw)) {
 		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",
-- 
2.37.3

[PATCH 5/7] cxl/region: Factor out cxl_port_calc_interleave()

[Alison Schofield]

cxl_port_setup_targets() calculates the interleave ways and granularity of
a port's decoder from the parent interleave and the number of targets below
the port, then validates the result. That calculation and its checks are
open coded between the target search and the decoder setup.

Move the calculation and its checks to a new helper,
cxl_port_calc_interleave(), so that the interleave for the port's decoder
is calculated and validated in one place.

No functional change.

Assisted-by: LLM
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/region.c | 81 ++++++++++++++++++++++++---------------
 1 file changed, 50 insertions(+), 31 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index c1866b7b089b..43f1ffbe64c0 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1513,11 +1513,58 @@ static int cxl_port_parent_interleave(struct cxl_port *port, struct cxl_region *
 	return 0;
 }
 
+static int cxl_port_calc_interleave(struct cxl_port *port, struct cxl_region *cxlr,
+				    struct cxl_region_ref *cxl_rr, int *iw, int *ig)
+{
+	struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
+	int parent_iw, parent_ig, child_iw, child_ig, rc;
+	struct cxl_decoder *cxld = cxl_rr->decoder;
+	struct cxl_switch_decoder *cxlsd;
+
+	rc = cxl_port_parent_interleave(port, cxlr, &parent_iw, &parent_ig);
+	if (rc)
+		return rc;
+
+	child_iw = cxl_rr->nr_targets;
+	if (!ways_valid(child_iw)) {
+		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",
+			dev_name(port->uport_dev), dev_name(&port->dev),
+			child_iw);
+		return -EINVAL;
+	}
+
+	/*
+	 * Interleave granularity is a multiple of @parent_port granularity.
+	 * Multiplier is the parent port interleave ways.
+	 */
+	child_ig = parent_ig * parent_iw;
+
+	if (!granularity_valid(child_ig)) {
+		dev_dbg(&cxlr->dev,
+			"%s: invalid granularity calculation (%d * %d)\n",
+			dev_name(&parent_port->dev), parent_ig, parent_iw);
+		return -EINVAL;
+	}
+
+	cxlsd = to_cxl_switch_decoder(&cxld->dev);
+	if (child_iw > 8 || child_iw > cxlsd->nr_targets) {
+		dev_dbg(&cxlr->dev,
+			"%s:%s:%s: ways: %d overflows targets: %d\n",
+			dev_name(port->uport_dev), dev_name(&port->dev),
+			dev_name(&cxld->dev), child_iw, cxlsd->nr_targets);
+		return -ENXIO;
+	}
+
+	*iw = child_iw;
+	*ig = child_ig;
+
+	return 0;
+}
+
 static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
 				  struct cxl_endpoint_decoder *cxled)
 {
-	int parent_iw, parent_ig, child_iw, child_ig, rc, pos = cxled->pos;
-	struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
+	int child_iw, child_ig, rc, pos = cxled->pos;
 	struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
@@ -1551,38 +1598,10 @@ static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr
 		goto add_target;
 	}
 
-	rc = cxl_port_parent_interleave(port, cxlr, &parent_iw, &parent_ig);
+	rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &child_iw, &child_ig);
 	if (rc)
 		return rc;
 
-	child_iw = cxl_rr->nr_targets;
-	if (!ways_valid(child_iw)) {
-		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",
-			dev_name(port->uport_dev), dev_name(&port->dev), child_iw);
-		return -EINVAL;
-	}
-
-	/*
-	 * Interleave granularity is a multiple of @parent_port granularity.
-	 * Multiplier is the parent port interleave ways.
-	 */
-	child_ig = parent_ig * parent_iw;
-
-	if (!granularity_valid(child_ig)) {
-		dev_dbg(&cxlr->dev,
-			"%s: invalid granularity calculation (%d * %d)\n",
-			dev_name(&parent_port->dev), parent_ig, parent_iw);
-		return -EINVAL;
-	}
-
-	if (child_iw > 8 || child_iw > cxlsd->nr_targets) {
-		dev_dbg(&cxlr->dev,
-			"%s:%s:%s: ways: %d overflows targets: %d\n",
-			dev_name(port->uport_dev), dev_name(&port->dev),
-			dev_name(&cxld->dev), child_iw, cxlsd->nr_targets);
-		return -ENXIO;
-	}
-
 	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
 		if (cxld->interleave_ways != child_iw ||
 		    (child_iw > 1 && cxld->interleave_granularity != child_ig) ||
-- 
2.37.3

[PATCH 6/7] cxl/region: Factor out cxl_port_setup_decoder()

[Alison Schofield]

Once cxl_port_setup_targets() has calculated the interleave for a port's
decoder, it either checks that interleave against the decoder settings
found for an auto region, or checks the decoder's interleave capabilities
and programs the decoder for a user created region. Both paths are open
coded in cxl_port_setup_targets().

Move both paths to a new helper, cxl_port_setup_decoder().

No functional change.

Assisted-by: LLM
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/region.c | 88 ++++++++++++++++++++++-----------------
 1 file changed, 50 insertions(+), 38 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 43f1ffbe64c0..4e9b0880663d 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1561,46 +1561,11 @@ static int cxl_port_calc_interleave(struct cxl_port *port, struct cxl_region *cx
 	return 0;
 }
 
-static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
-				  struct cxl_endpoint_decoder *cxled)
+static int cxl_port_setup_decoder(struct cxl_port *port, struct cxl_region *cxlr,
+				  struct cxl_decoder *cxld, int child_iw, int child_ig)
 {
-	int child_iw, child_ig, rc, pos = cxled->pos;
-	struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
-	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
-	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
 	struct cxl_region_params *p = &cxlr->params;
-	struct cxl_decoder *cxld = cxl_rr->decoder;
-	struct cxl_switch_decoder *cxlsd;
-
-	/*
-	 * While root level decoders support x3, x6, x12, switch level
-	 * decoders only support powers of 2 up to x16.
-	 */
-	if (!is_power_of_2(cxl_rr->nr_targets)) {
-		dev_dbg(&cxlr->dev, "%s:%s: invalid target count %d\n",
-			dev_name(port->uport_dev), dev_name(&port->dev),
-			cxl_rr->nr_targets);
-		return -EINVAL;
-	}
-
-	cxlsd = to_cxl_switch_decoder(&cxld->dev);
-	if (cxl_rr->nr_targets_set) {
-		int distance = cxl_port_peer_distance(port, cxlr);
-
-		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
-			if (ep->dport == cxlsd->target[i]) {
-				rc = check_last_peer(cxled, ep, cxl_rr,
-						     distance);
-				if (rc)
-					return rc;
-				goto out_target_set;
-			}
-		goto add_target;
-	}
-
-	rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &child_iw, &child_ig);
-	if (rc)
-		return rc;
+	int rc;
 
 	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
 		if (cxld->interleave_ways != child_iw ||
@@ -1639,6 +1604,53 @@ static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr
 	}
 	dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport_dev),
 		dev_name(&port->dev), child_iw, child_ig);
+
+	return 0;
+}
+
+static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
+				  struct cxl_endpoint_decoder *cxled)
+{
+	int child_iw, child_ig, rc, pos = cxled->pos;
+	struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
+	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
+	struct cxl_decoder *cxld = cxl_rr->decoder;
+	struct cxl_switch_decoder *cxlsd;
+
+	/*
+	 * While root level decoders support x3, x6, x12, switch level
+	 * decoders only support powers of 2 up to x16.
+	 */
+	if (!is_power_of_2(cxl_rr->nr_targets)) {
+		dev_dbg(&cxlr->dev, "%s:%s: invalid target count %d\n",
+			dev_name(port->uport_dev), dev_name(&port->dev),
+			cxl_rr->nr_targets);
+		return -EINVAL;
+	}
+
+	cxlsd = to_cxl_switch_decoder(&cxld->dev);
+	if (cxl_rr->nr_targets_set) {
+		int distance = cxl_port_peer_distance(port, cxlr);
+
+		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
+			if (ep->dport == cxlsd->target[i]) {
+				rc = check_last_peer(cxled, ep, cxl_rr,
+						     distance);
+				if (rc)
+					return rc;
+				goto out_target_set;
+			}
+		goto add_target;
+	}
+
+	rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &child_iw, &child_ig);
+	if (rc)
+		return rc;
+
+	rc = cxl_port_setup_decoder(port, cxlr, cxld, child_iw, child_ig);
+	if (rc)
+		return rc;
 add_target:
 	if (cxl_rr->nr_targets_set == cxl_rr->nr_targets) {
 		dev_dbg(&cxlr->dev,
-- 
2.37.3

[PATCH 7/7] cxl/region: Factor out cxl_port_add_target()

[Alison Schofield]

cxl_port_setup_targets() adds an endpoint's downstream port to a port's
decoder target list. For an auto region it checks that the decoder already
targets that downstream port at the next position. For a user created
region it programs the downstream port at that position. That step is open
coded behind an add_target label that the target search jumps to.

Move that step to a new helper, cxl_port_add_target(). With the decoder
setup and the target add now in helpers, the path for the first endpoint to
reach a port becomes an else branch of the target search, so the add_target
label is no longer needed.

No functional change.

Assisted-by: LLM
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/region.c | 82 ++++++++++++++++++++++++---------------
 1 file changed, 51 insertions(+), 31 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 4e9b0880663d..abe5fcc62e9d 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1608,15 +1608,50 @@ static int cxl_port_setup_decoder(struct cxl_port *port, struct cxl_region *cxlr
 	return 0;
 }
 
+static int cxl_port_add_target(struct cxl_port *port, struct cxl_region *cxlr,
+			       struct cxl_region_ref *cxl_rr,
+			       struct cxl_endpoint_decoder *cxled, struct cxl_ep *ep)
+{
+	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct cxl_switch_decoder *cxlsd;
+	int pos = cxled->pos;
+
+	cxlsd = to_cxl_switch_decoder(&cxl_rr->decoder->dev);
+	if (cxl_rr->nr_targets_set == cxl_rr->nr_targets) {
+		dev_dbg(&cxlr->dev,
+			"%s:%s: targets full trying to add %s:%s at %d\n",
+			dev_name(port->uport_dev), dev_name(&port->dev),
+			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), pos);
+		return -ENXIO;
+	}
+	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
+		if (cxlsd->target[cxl_rr->nr_targets_set] != ep->dport) {
+			dev_dbg(&cxlr->dev, "%s:%s: %s expected %s at %d\n",
+				dev_name(port->uport_dev), dev_name(&port->dev),
+				dev_name(&cxlsd->cxld.dev),
+				dev_name(ep->dport->dport_dev),
+				cxl_rr->nr_targets_set);
+			return -ENXIO;
+		}
+	} else {
+		cxlsd->target[cxl_rr->nr_targets_set] = ep->dport;
+		cxlsd->cxld.target_map[cxl_rr->nr_targets_set] =
+			ep->dport->port_id;
+	}
+	cxl_rr->nr_targets_set++;
+
+	return 0;
+}
+
 static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
 				  struct cxl_endpoint_decoder *cxled)
 {
-	int child_iw, child_ig, rc, pos = cxled->pos;
 	struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
 	struct cxl_decoder *cxld = cxl_rr->decoder;
 	struct cxl_switch_decoder *cxlsd;
+	int rc, pos = cxled->pos;
 
 	/*
 	 * While root level decoders support x3, x6, x12, switch level
@@ -1641,38 +1676,23 @@ static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr
 					return rc;
 				goto out_target_set;
 			}
-		goto add_target;
-	}
-
-	rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &child_iw, &child_ig);
-	if (rc)
-		return rc;
-
-	rc = cxl_port_setup_decoder(port, cxlr, cxld, child_iw, child_ig);
-	if (rc)
-		return rc;
-add_target:
-	if (cxl_rr->nr_targets_set == cxl_rr->nr_targets) {
-		dev_dbg(&cxlr->dev,
-			"%s:%s: targets full trying to add %s:%s at %d\n",
-			dev_name(port->uport_dev), dev_name(&port->dev),
-			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), pos);
-		return -ENXIO;
-	}
-	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
-		if (cxlsd->target[cxl_rr->nr_targets_set] != ep->dport) {
-			dev_dbg(&cxlr->dev, "%s:%s: %s expected %s at %d\n",
-				dev_name(port->uport_dev), dev_name(&port->dev),
-				dev_name(&cxlsd->cxld.dev),
-				dev_name(ep->dport->dport_dev),
-				cxl_rr->nr_targets_set);
-			return -ENXIO;
-		}
 	} else {
-		cxlsd->target[cxl_rr->nr_targets_set] = ep->dport;
-		cxlsd->cxld.target_map[cxl_rr->nr_targets_set] = ep->dport->port_id;
+		int child_iw, child_ig;
+
+		rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &child_iw,
+					      &child_ig);
+		if (rc)
+			return rc;
+
+		rc = cxl_port_setup_decoder(port, cxlr, cxld, child_iw,
+					    child_ig);
+		if (rc)
+			return rc;
 	}
-	cxl_rr->nr_targets_set++;
+
+	rc = cxl_port_add_target(port, cxlr, cxl_rr, cxled, ep);
+	if (rc)
+		return rc;
 out_target_set:
 	dev_dbg(&cxlr->dev, "%s:%s target[%d] = %s for %s:%s @ %d\n",
 		dev_name(port->uport_dev), dev_name(&port->dev),
-- 
2.37.3

Re: [PATCH 1/7] cxl: Add interleave encoding validity helpers

[Dave Jiang]

On 10/9/26 1:24 PM, Alison Schofield wrote:
> Some callers of ways_to_eiw(), granularity_to_eig(), and
> eig_to_granularity() only need to determine whether an input is valid,
> but must provide storage for a converted value that is then discarded.
> This obscures the purpose of the call and requires unnecessary temporary
> variables.
> 
> Define and use new helpers that provide a validation-only interface for
> those callers.
> 
> cxl_port_setup_targets() also encodes and then decodes its calculated
> granularity solely to validate it. Calculate the granularity directly
> and validate it instead, making the calculation explicit and removing
> the encode/decode round trip.
> 
> No functional change.
> 
> Suggested-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/cxl/core/region.c | 49 +++++++++++++--------------------------
>  drivers/cxl/cxl.h         | 21 +++++++++++++++++
>  2 files changed, 37 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index a048bd5c06cb..bc087ca63946 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -493,11 +493,9 @@ static int set_interleave_ways(struct cxl_region *cxlr, int val)
>  	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
>  	struct cxl_region_params *p = &cxlr->params;
>  	int save, rc;
> -	u8 iw;
>  
> -	rc = ways_to_eiw(val, &iw);
> -	if (rc)
> -		return rc;
> +	if (!ways_valid(val))
> +		return -EINVAL;
>  
>  	/*
>  	 * Even for x3, x6, and x12 interleaves the region interleave must be a
> @@ -566,12 +564,9 @@ static int set_interleave_granularity(struct cxl_region *cxlr, int val)
>  	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
>  	struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
>  	struct cxl_region_params *p = &cxlr->params;
> -	int rc;
> -	u16 ig;
>  
> -	rc = granularity_to_eig(val, &ig);
> -	if (rc)
> -		return rc;
> +	if (!granularity_valid(val))
> +		return -EINVAL;
>  
>  	/*
>  	 * When the host-bridge is interleaved, disallow region granularity !=
> @@ -1451,8 +1446,6 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  	struct cxl_decoder *cxld = cxl_rr->decoder;
>  	struct cxl_switch_decoder *cxlsd;
>  	struct cxl_port *iter = port;
> -	u16 eig, peig;
> -	u8 eiw, peiw;
>  
>  	/*
>  	 * While root level decoders support x3, x6, x12, switch level
> @@ -1524,48 +1517,38 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  		parent_iw = parent_cxld->interleave_ways;
>  	}
>  
> -	rc = granularity_to_eig(parent_ig, &peig);
> -	if (rc) {
> +	if (!granularity_valid(parent_ig)) {
>  		dev_dbg(&cxlr->dev, "%s:%s: invalid parent granularity: %d\n",
>  			dev_name(parent_port->uport_dev),
>  			dev_name(&parent_port->dev), parent_ig);
> -		return rc;
> +		return -EINVAL;
>  	}
>  
> -	rc = ways_to_eiw(parent_iw, &peiw);
> -	if (rc) {
> +	if (!ways_valid(parent_iw)) {
>  		dev_dbg(&cxlr->dev, "%s:%s: invalid parent interleave: %d\n",
>  			dev_name(parent_port->uport_dev),
>  			dev_name(&parent_port->dev), parent_iw);
> -		return rc;
> +		return -EINVAL;
>  	}
>  
>  	iw = cxl_rr->nr_targets;
> -	rc = ways_to_eiw(iw, &eiw);
> -	if (rc) {
> +	if (!ways_valid(iw)) {
>  		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",
>  			dev_name(port->uport_dev), dev_name(&port->dev), iw);
> -		return rc;
> +		return -EINVAL;
>  	}
>  
>  	/*
>  	 * Interleave granularity is a multiple of @parent_port granularity.
>  	 * Multiplier is the parent port interleave ways.
>  	 */
> -	rc = granularity_to_eig(parent_ig * parent_iw, &eig);
> -	if (rc) {
> +	ig = parent_ig * parent_iw;
> +
> +	if (!granularity_valid(ig)) {
>  		dev_dbg(&cxlr->dev,
>  			"%s: invalid granularity calculation (%d * %d)\n",
>  			dev_name(&parent_port->dev), parent_ig, parent_iw);
> -		return rc;
> -	}
> -
> -	rc = eig_to_granularity(eig, &ig);
> -	if (rc) {
> -		dev_dbg(&cxlr->dev, "%s:%s: invalid interleave: %d\n",
> -			dev_name(port->uport_dev), dev_name(&port->dev),
> -			256 << eig);
> -		return rc;
> +		return -EINVAL;
>  	}
>  
>  	if (iw > 8 || iw > cxlsd->nr_targets) {
> @@ -3131,13 +3114,13 @@ static bool cxl_is_hpa_in_chunk(u64 hpa, struct cxl_region *cxlr, int pos)
>   */
>  int cxl_validate_translation_params(u8 eiw, u16 eig, int pos)
>  {
> -	int ways, gran;
> +	int ways;
>  
>  	if (eiw_to_ways(eiw, &ways)) {
>  		pr_debug("%s: invalid eiw=%u\n", __func__, eiw);
>  		return -EINVAL;
>  	}
> -	if (eig_to_granularity(eig, &gran)) {
> +	if (!eig_valid(eig)) {
>  		pr_debug("%s: invalid eig=%u\n", __func__, eig);
>  		return -EINVAL;
>  	}
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index f03ee0addc45..4c9173e07f1f 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -146,6 +146,27 @@ static inline int ways_to_eiw(unsigned int ways, u8 *eiw)
>  	return 0;
>  }
>  
> +static inline bool eig_valid(u16 eig)
> +{
> +	int granularity;
> +
> +	return eig_to_granularity(eig, &granularity) == 0;
> +}
> +
> +static inline bool granularity_valid(int granularity)
> +{
> +	u16 eig;
> +
> +	return granularity_to_eig(granularity, &eig) == 0;
> +}
> +
> +static inline bool ways_valid(unsigned int ways)
> +{
> +	u8 eiw;
> +
> +	return ways_to_eiw(ways, &eiw) == 0;
> +}
> +
>  /* RAS Registers CXL 2.0 8.2.5.9 CXL RAS Capability Structure */
>  #define CXL_RAS_UNCORRECTABLE_STATUS_OFFSET 0x0
>  #define   CXL_RAS_UNCORRECTABLE_STATUS_MASK (GENMASK(16, 14) | GENMASK(11, 0))

Re: [PATCH 2/7] cxl/region: Name the interleave locals in cxl_port_setup_targets()

[Dave Jiang]

On 10/9/26 1:24 PM, Alison Schofield wrote:
> cxl_port_setup_targets() derives the interleave granularity of one decoder
> from the granularity and ways of the decoder above it, and takes that
> decoder's own ways from the topology below it. The values from the level
> above are named parent_iw and parent_ig, but the values being derived are
> the bare iw and ig, so the reader has to work out which level each one
> describes.
> 
> Rename iw and ig to child_iw and child_ig to match the parent_ prefix used
> for the level above.
> 
> No functional change.
> 
> Suggested-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/cxl/core/region.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index bc087ca63946..1a42753fda48 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1437,7 +1437,7 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  				  struct cxl_endpoint_decoder *cxled)
>  {
>  	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> -	int parent_iw, parent_ig, ig, iw, rc, pos = cxled->pos;
> +	int parent_iw, parent_ig, child_iw, child_ig, rc, 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);
>  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> @@ -1531,10 +1531,10 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  		return -EINVAL;
>  	}
>  
> -	iw = cxl_rr->nr_targets;
> -	if (!ways_valid(iw)) {
> +	child_iw = cxl_rr->nr_targets;
> +	if (!ways_valid(child_iw)) {
>  		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",
> -			dev_name(port->uport_dev), dev_name(&port->dev), iw);
> +			dev_name(port->uport_dev), dev_name(&port->dev), child_iw);
>  		return -EINVAL;
>  	}
>  
> @@ -1542,32 +1542,32 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  	 * Interleave granularity is a multiple of @parent_port granularity.
>  	 * Multiplier is the parent port interleave ways.
>  	 */
> -	ig = parent_ig * parent_iw;
> +	child_ig = parent_ig * parent_iw;
>  
> -	if (!granularity_valid(ig)) {
> +	if (!granularity_valid(child_ig)) {
>  		dev_dbg(&cxlr->dev,
>  			"%s: invalid granularity calculation (%d * %d)\n",
>  			dev_name(&parent_port->dev), parent_ig, parent_iw);
>  		return -EINVAL;
>  	}
>  
> -	if (iw > 8 || iw > cxlsd->nr_targets) {
> +	if (child_iw > 8 || child_iw > cxlsd->nr_targets) {
>  		dev_dbg(&cxlr->dev,
>  			"%s:%s:%s: ways: %d overflows targets: %d\n",
>  			dev_name(port->uport_dev), dev_name(&port->dev),
> -			dev_name(&cxld->dev), iw, cxlsd->nr_targets);
> +			dev_name(&cxld->dev), child_iw, cxlsd->nr_targets);
>  		return -ENXIO;
>  	}
>  
>  	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> -		if (cxld->interleave_ways != iw ||
> -		    (iw > 1 && cxld->interleave_granularity != ig) ||
> +		if (cxld->interleave_ways != child_iw ||
> +		    (child_iw > 1 && cxld->interleave_granularity != child_ig) ||
>  		    !spa_maps_hpa(p, &cxld->hpa_range) ||
>  		    ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)) {
>  			dev_err(&cxlr->dev,
>  				"%s:%s %s expected iw: %d ig: %d %pr\n",
>  				dev_name(port->uport_dev), dev_name(&port->dev),
> -				__func__, iw, ig, p->res);
> +				__func__, child_iw, child_ig, p->res);
>  			dev_err(&cxlr->dev,
>  				"%s:%s %s got iw: %d ig: %d state: %s %#llx:%#llx\n",
>  				dev_name(port->uport_dev), dev_name(&port->dev),
> @@ -1578,24 +1578,24 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  			return -ENXIO;
>  		}
>  	} else {
> -		rc = check_interleave_cap(cxld, iw, ig);
> +		rc = check_interleave_cap(cxld, child_iw, child_ig);
>  		if (rc) {
>  			dev_dbg(&cxlr->dev,
>  				"%s:%s iw: %d ig: %d is not supported\n",
>  				dev_name(port->uport_dev),
> -				dev_name(&port->dev), iw, ig);
> +				dev_name(&port->dev), child_iw, child_ig);
>  			return rc;
>  		}
>  
> -		cxld->interleave_ways = iw;
> -		cxld->interleave_granularity = ig;
> +		cxld->interleave_ways = child_iw;
> +		cxld->interleave_granularity = child_ig;
>  		cxld->hpa_range = (struct range) {
>  			.start = p->res->start,
>  			.end = p->res->end,
>  		};
>  	}
>  	dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport_dev),
> -		dev_name(&port->dev), iw, ig);
> +		dev_name(&port->dev), child_iw, child_ig);
>  add_target:
>  	if (cxl_rr->nr_targets_set == cxl_rr->nr_targets) {
>  		dev_dbg(&cxlr->dev,

Re: [PATCH 3/7] cxl/region: Factor out cxl_port_peer_distance()

[Dave Jiang]

On 10/9/26 1:24 PM, Alison Schofield wrote:
> cxl_port_setup_targets() is nearly 200 lines long and open codes each step
> of adding an endpoint to a port's decoder. One of those steps calculates
> the distance between region positions that share a downstream port.
> 
> Move that calculation to a new helper, cxl_port_peer_distance().
> 
> No functional change.
> 
> Assisted-by: LLM
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/cxl/core/region.c | 50 +++++++++++++++++++++++----------------
>  1 file changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 1a42753fda48..80c40716c368 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1432,6 +1432,33 @@ static int check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
>  	return 0;
>  }
>  
> +static int cxl_port_peer_distance(struct cxl_port *port, struct cxl_region *cxlr)
> +{
> +	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> +	struct cxl_region_ref *cxl_rr_iter;
> +	struct cxl_port *iter = port;
> +	int distance = 1;
> +
> +	/*
> +	 * The "distance" between peer downstream ports represents which
> +	 * endpoint positions in the region interleave a given port can
> +	 * host.
> +	 *
> +	 * For example, at the root of a hierarchy the distance is
> +	 * always 1 as every index targets a different host-bridge. At
> +	 * each subsequent switch level those ports map every Nth region
> +	 * position where N is the width of the switch == distance.
> +	 */
> +	do {
> +		cxl_rr_iter = cxl_rr_load(iter, cxlr);
> +		distance *= cxl_rr_iter->nr_targets;
> +		iter = to_cxl_port(iter->dev.parent);
> +	} while (!is_cxl_root(iter));
> +	distance *= cxlrd->cxlsd.cxld.interleave_ways;
> +
> +	return distance;
> +}
> +
>  static int cxl_port_setup_targets(struct cxl_port *port,
>  				  struct cxl_region *cxlr,
>  				  struct cxl_endpoint_decoder *cxled)
> @@ -1445,7 +1472,6 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  	struct cxl_region_params *p = &cxlr->params;
>  	struct cxl_decoder *cxld = cxl_rr->decoder;
>  	struct cxl_switch_decoder *cxlsd;
> -	struct cxl_port *iter = port;
>  
>  	/*
>  	 * While root level decoders support x3, x6, x12, switch level
> @@ -1460,27 +1486,9 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  
>  	cxlsd = to_cxl_switch_decoder(&cxld->dev);
>  	if (cxl_rr->nr_targets_set) {
> -		int i, distance = 1;
> -		struct cxl_region_ref *cxl_rr_iter;
> +		int distance = cxl_port_peer_distance(port, cxlr);
>  
> -		/*
> -		 * The "distance" between peer downstream ports represents which
> -		 * endpoint positions in the region interleave a given port can
> -		 * host.
> -		 *
> -		 * For example, at the root of a hierarchy the distance is
> -		 * always 1 as every index targets a different host-bridge. At
> -		 * each subsequent switch level those ports map every Nth region
> -		 * position where N is the width of the switch == distance.
> -		 */
> -		do {
> -			cxl_rr_iter = cxl_rr_load(iter, cxlr);
> -			distance *= cxl_rr_iter->nr_targets;
> -			iter = to_cxl_port(iter->dev.parent);
> -		} while (!is_cxl_root(iter));
> -		distance *= cxlrd->cxlsd.cxld.interleave_ways;
> -
> -		for (i = 0; i < cxl_rr->nr_targets_set; i++)
> +		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
>  			if (ep->dport == cxlsd->target[i]) {
>  				rc = check_last_peer(cxled, ep, cxl_rr,
>  						     distance);

Re: [PATCH 4/7] cxl/region: Factor out cxl_port_parent_interleave()

[Dave Jiang]

On 10/9/26 1:24 PM, Alison Schofield wrote:
> cxl_port_setup_targets() derives a decoder's interleave from the interleave
> of the decoder above it. That parent interleave comes from either the root
> decoder or a switch decoder, and each case is open coded along with the
> parent validity checks.
> 
> Move the parent interleave lookup and validation to a new helper,
> cxl_port_parent_interleave().
> 
> No functional change.
> 
> Assisted-by: LLM
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  drivers/cxl/core/region.c | 88 +++++++++++++++++++++++----------------
>  1 file changed, 52 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 80c40716c368..c1866b7b089b 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1459,45 +1459,13 @@ static int cxl_port_peer_distance(struct cxl_port *port, struct cxl_region *cxlr
>  	return distance;
>  }
>  
> -static int cxl_port_setup_targets(struct cxl_port *port,
> -				  struct cxl_region *cxlr,
> -				  struct cxl_endpoint_decoder *cxled)
> +static int cxl_port_parent_interleave(struct cxl_port *port, struct cxl_region *cxlr,
> +				      int *iw, int *ig)
>  {
> -	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> -	int parent_iw, parent_ig, child_iw, child_ig, rc, 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);
> -	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> -	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
> +	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
>  	struct cxl_region_params *p = &cxlr->params;
> -	struct cxl_decoder *cxld = cxl_rr->decoder;
> -	struct cxl_switch_decoder *cxlsd;
> -
> -	/*
> -	 * While root level decoders support x3, x6, x12, switch level
> -	 * decoders only support powers of 2 up to x16.
> -	 */
> -	if (!is_power_of_2(cxl_rr->nr_targets)) {
> -		dev_dbg(&cxlr->dev, "%s:%s: invalid target count %d\n",
> -			dev_name(port->uport_dev), dev_name(&port->dev),
> -			cxl_rr->nr_targets);
> -		return -EINVAL;
> -	}
> -
> -	cxlsd = to_cxl_switch_decoder(&cxld->dev);
> -	if (cxl_rr->nr_targets_set) {
> -		int distance = cxl_port_peer_distance(port, cxlr);
> -
> -		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
> -			if (ep->dport == cxlsd->target[i]) {
> -				rc = check_last_peer(cxled, ep, cxl_rr,
> -						     distance);
> -				if (rc)
> -					return rc;
> -				goto out_target_set;
> -			}
> -		goto add_target;
> -	}
> +	int parent_iw, parent_ig;
>  
>  	if (is_cxl_root(parent_port)) {
>  		/*
> @@ -1539,6 +1507,54 @@ static int cxl_port_setup_targets(struct cxl_port *port,
>  		return -EINVAL;
>  	}
>  
> +	*iw = parent_iw;
> +	*ig = parent_ig;
> +
> +	return 0;
> +}
> +
> +static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,

Do we want to just rename it to cxl_switch_port_setup_targets() since the function basically is handling switch only?

DJ

> +				  struct cxl_endpoint_decoder *cxled)
> +{
> +	int parent_iw, parent_ig, child_iw, child_ig, rc, 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);
> +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
> +	struct cxl_region_params *p = &cxlr->params;
> +	struct cxl_decoder *cxld = cxl_rr->decoder;
> +	struct cxl_switch_decoder *cxlsd;
> +
> +	/*
> +	 * While root level decoders support x3, x6, x12, switch level
> +	 * decoders only support powers of 2 up to x16.
> +	 */
> +	if (!is_power_of_2(cxl_rr->nr_targets)) {
> +		dev_dbg(&cxlr->dev, "%s:%s: invalid target count %d\n",
> +			dev_name(port->uport_dev), dev_name(&port->dev),
> +			cxl_rr->nr_targets);
> +		return -EINVAL;
> +	}
> +
> +	cxlsd = to_cxl_switch_decoder(&cxld->dev);
> +	if (cxl_rr->nr_targets_set) {
> +		int distance = cxl_port_peer_distance(port, cxlr);
> +
> +		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
> +			if (ep->dport == cxlsd->target[i]) {
> +				rc = check_last_peer(cxled, ep, cxl_rr,
> +						     distance);
> +				if (rc)
> +					return rc;
> +				goto out_target_set;
> +			}
> +		goto add_target;
> +	}
> +
> +	rc = cxl_port_parent_interleave(port, cxlr, &parent_iw, &parent_ig);
> +	if (rc)
> +		return rc;
> +
>  	child_iw = cxl_rr->nr_targets;
>  	if (!ways_valid(child_iw)) {
>  		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",

Re: [PATCH 5/7] cxl/region: Factor out cxl_port_calc_interleave()

[Dave Jiang]

On 10/9/26 1:24 PM, Alison Schofield wrote:
> cxl_port_setup_targets() calculates the interleave ways and granularity of
> a port's decoder from the parent interleave and the number of targets below
> the port, then validates the result. That calculation and its checks are
> open coded between the target search and the decoder setup.
> 
> Move the calculation and its checks to a new helper,
> cxl_port_calc_interleave(), so that the interleave for the port's decoder
> is calculated and validated in one place.
> 
> No functional change.
> 
> Assisted-by: LLM
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/cxl/core/region.c | 81 ++++++++++++++++++++++++---------------
>  1 file changed, 50 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index c1866b7b089b..43f1ffbe64c0 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1513,11 +1513,58 @@ static int cxl_port_parent_interleave(struct cxl_port *port, struct cxl_region *
>  	return 0;
>  }
>  
> +static int cxl_port_calc_interleave(struct cxl_port *port, struct cxl_region *cxlr,
> +				    struct cxl_region_ref *cxl_rr, int *iw, int *ig)
> +{
> +	struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
> +	int parent_iw, parent_ig, child_iw, child_ig, rc;
> +	struct cxl_decoder *cxld = cxl_rr->decoder;
> +	struct cxl_switch_decoder *cxlsd;
> +
> +	rc = cxl_port_parent_interleave(port, cxlr, &parent_iw, &parent_ig);
> +	if (rc)
> +		return rc;
> +
> +	child_iw = cxl_rr->nr_targets;
> +	if (!ways_valid(child_iw)) {
> +		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",
> +			dev_name(port->uport_dev), dev_name(&port->dev),
> +			child_iw);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Interleave granularity is a multiple of @parent_port granularity.
> +	 * Multiplier is the parent port interleave ways.
> +	 */
> +	child_ig = parent_ig * parent_iw;
> +
> +	if (!granularity_valid(child_ig)) {
> +		dev_dbg(&cxlr->dev,
> +			"%s: invalid granularity calculation (%d * %d)\n",
> +			dev_name(&parent_port->dev), parent_ig, parent_iw);
> +		return -EINVAL;
> +	}
> +
> +	cxlsd = to_cxl_switch_decoder(&cxld->dev);
> +	if (child_iw > 8 || child_iw > cxlsd->nr_targets) {
> +		dev_dbg(&cxlr->dev,
> +			"%s:%s:%s: ways: %d overflows targets: %d\n",
> +			dev_name(port->uport_dev), dev_name(&port->dev),
> +			dev_name(&cxld->dev), child_iw, cxlsd->nr_targets);
> +		return -ENXIO;
> +	}
> +
> +	*iw = child_iw;
> +	*ig = child_ig;
> +
> +	return 0;
> +}
> +
>  static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
>  				  struct cxl_endpoint_decoder *cxled)
>  {
> -	int parent_iw, parent_ig, child_iw, child_ig, rc, pos = cxled->pos;
> -	struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
> +	int child_iw, child_ig, rc, pos = cxled->pos;
>  	struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
>  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>  	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
> @@ -1551,38 +1598,10 @@ static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr
>  		goto add_target;
>  	}
>  
> -	rc = cxl_port_parent_interleave(port, cxlr, &parent_iw, &parent_ig);
> +	rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &child_iw, &child_ig);
>  	if (rc)
>  		return rc;
>  
> -	child_iw = cxl_rr->nr_targets;
> -	if (!ways_valid(child_iw)) {
> -		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",
> -			dev_name(port->uport_dev), dev_name(&port->dev), child_iw);
> -		return -EINVAL;
> -	}
> -
> -	/*
> -	 * Interleave granularity is a multiple of @parent_port granularity.
> -	 * Multiplier is the parent port interleave ways.
> -	 */
> -	child_ig = parent_ig * parent_iw;
> -
> -	if (!granularity_valid(child_ig)) {
> -		dev_dbg(&cxlr->dev,
> -			"%s: invalid granularity calculation (%d * %d)\n",
> -			dev_name(&parent_port->dev), parent_ig, parent_iw);
> -		return -EINVAL;
> -	}
> -
> -	if (child_iw > 8 || child_iw > cxlsd->nr_targets) {
> -		dev_dbg(&cxlr->dev,
> -			"%s:%s:%s: ways: %d overflows targets: %d\n",
> -			dev_name(port->uport_dev), dev_name(&port->dev),
> -			dev_name(&cxld->dev), child_iw, cxlsd->nr_targets);
> -		return -ENXIO;
> -	}
> -
>  	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
>  		if (cxld->interleave_ways != child_iw ||
>  		    (child_iw > 1 && cxld->interleave_granularity != child_ig) ||

Re: [PATCH 6/7] cxl/region: Factor out cxl_port_setup_decoder()

[Dave Jiang]

On 10/9/26 1:24 PM, Alison Schofield wrote:
> Once cxl_port_setup_targets() has calculated the interleave for a port's
> decoder, it either checks that interleave against the decoder settings
> found for an auto region, or checks the decoder's interleave capabilities
> and programs the decoder for a user created region. Both paths are open
> coded in cxl_port_setup_targets().
> 
> Move both paths to a new helper, cxl_port_setup_decoder().
> 
> No functional change.
> 
> Assisted-by: LLM
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/cxl/core/region.c | 88 ++++++++++++++++++++++-----------------
>  1 file changed, 50 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 43f1ffbe64c0..4e9b0880663d 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1561,46 +1561,11 @@ static int cxl_port_calc_interleave(struct cxl_port *port, struct cxl_region *cx
>  	return 0;
>  }
>  
> -static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
> -				  struct cxl_endpoint_decoder *cxled)
> +static int cxl_port_setup_decoder(struct cxl_port *port, struct cxl_region *cxlr,
> +				  struct cxl_decoder *cxld, int child_iw, int child_ig)
>  {
> -	int child_iw, child_ig, rc, pos = cxled->pos;
> -	struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
> -	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> -	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
>  	struct cxl_region_params *p = &cxlr->params;
> -	struct cxl_decoder *cxld = cxl_rr->decoder;
> -	struct cxl_switch_decoder *cxlsd;
> -
> -	/*
> -	 * While root level decoders support x3, x6, x12, switch level
> -	 * decoders only support powers of 2 up to x16.
> -	 */
> -	if (!is_power_of_2(cxl_rr->nr_targets)) {
> -		dev_dbg(&cxlr->dev, "%s:%s: invalid target count %d\n",
> -			dev_name(port->uport_dev), dev_name(&port->dev),
> -			cxl_rr->nr_targets);
> -		return -EINVAL;
> -	}
> -
> -	cxlsd = to_cxl_switch_decoder(&cxld->dev);
> -	if (cxl_rr->nr_targets_set) {
> -		int distance = cxl_port_peer_distance(port, cxlr);
> -
> -		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
> -			if (ep->dport == cxlsd->target[i]) {
> -				rc = check_last_peer(cxled, ep, cxl_rr,
> -						     distance);
> -				if (rc)
> -					return rc;
> -				goto out_target_set;
> -			}
> -		goto add_target;
> -	}
> -
> -	rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &child_iw, &child_ig);
> -	if (rc)
> -		return rc;
> +	int rc;
>  
>  	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
>  		if (cxld->interleave_ways != child_iw ||
> @@ -1639,6 +1604,53 @@ static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr
>  	}
>  	dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport_dev),
>  		dev_name(&port->dev), child_iw, child_ig);
> +
> +	return 0;
> +}
> +
> +static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
> +				  struct cxl_endpoint_decoder *cxled)
> +{
> +	int child_iw, child_ig, rc, pos = cxled->pos;
> +	struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
> +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
> +	struct cxl_decoder *cxld = cxl_rr->decoder;
> +	struct cxl_switch_decoder *cxlsd;
> +
> +	/*
> +	 * While root level decoders support x3, x6, x12, switch level
> +	 * decoders only support powers of 2 up to x16.
> +	 */
> +	if (!is_power_of_2(cxl_rr->nr_targets)) {
> +		dev_dbg(&cxlr->dev, "%s:%s: invalid target count %d\n",
> +			dev_name(port->uport_dev), dev_name(&port->dev),
> +			cxl_rr->nr_targets);
> +		return -EINVAL;
> +	}
> +
> +	cxlsd = to_cxl_switch_decoder(&cxld->dev);
> +	if (cxl_rr->nr_targets_set) {
> +		int distance = cxl_port_peer_distance(port, cxlr);
> +
> +		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
> +			if (ep->dport == cxlsd->target[i]) {
> +				rc = check_last_peer(cxled, ep, cxl_rr,
> +						     distance);
> +				if (rc)
> +					return rc;
> +				goto out_target_set;
> +			}
> +		goto add_target;
> +	}
> +
> +	rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &child_iw, &child_ig);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_port_setup_decoder(port, cxlr, cxld, child_iw, child_ig);
> +	if (rc)
> +		return rc;
>  add_target:
>  	if (cxl_rr->nr_targets_set == cxl_rr->nr_targets) {
>  		dev_dbg(&cxlr->dev,

Re: [PATCH 7/7] cxl/region: Factor out cxl_port_add_target()

[Dave Jiang]

On 10/9/26 1:24 PM, Alison Schofield wrote:
> cxl_port_setup_targets() adds an endpoint's downstream port to a port's
> decoder target list. For an auto region it checks that the decoder already
> targets that downstream port at the next position. For a user created
> region it programs the downstream port at that position. That step is open
> coded behind an add_target label that the target search jumps to.
> 
> Move that step to a new helper, cxl_port_add_target(). With the decoder
> setup and the target add now in helpers, the path for the first endpoint to
> reach a port becomes an else branch of the target search, so the add_target
> label is no longer needed.
> 
> No functional change.
> 
> Assisted-by: LLM
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>  drivers/cxl/core/region.c | 82 ++++++++++++++++++++++++---------------
>  1 file changed, 51 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 4e9b0880663d..abe5fcc62e9d 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1608,15 +1608,50 @@ static int cxl_port_setup_decoder(struct cxl_port *port, struct cxl_region *cxlr
>  	return 0;
>  }
>  
> +static int cxl_port_add_target(struct cxl_port *port, struct cxl_region *cxlr,

This is only switch port right? Maybe just pass in cxlsd instead of port and name it cxl_switch_port_add_target()?

DJ

> +			       struct cxl_region_ref *cxl_rr,
> +			       struct cxl_endpoint_decoder *cxled, struct cxl_ep *ep)
> +{
> +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +	struct cxl_switch_decoder *cxlsd;
> +	int pos = cxled->pos;
> +
> +	cxlsd = to_cxl_switch_decoder(&cxl_rr->decoder->dev);
> +	if (cxl_rr->nr_targets_set == cxl_rr->nr_targets) {
> +		dev_dbg(&cxlr->dev,
> +			"%s:%s: targets full trying to add %s:%s at %d\n",
> +			dev_name(port->uport_dev), dev_name(&port->dev),
> +			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), pos);
> +		return -ENXIO;
> +	}
> +	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> +		if (cxlsd->target[cxl_rr->nr_targets_set] != ep->dport) {
> +			dev_dbg(&cxlr->dev, "%s:%s: %s expected %s at %d\n",
> +				dev_name(port->uport_dev), dev_name(&port->dev),
> +				dev_name(&cxlsd->cxld.dev),
> +				dev_name(ep->dport->dport_dev),
> +				cxl_rr->nr_targets_set);
> +			return -ENXIO;
> +		}
> +	} else {
> +		cxlsd->target[cxl_rr->nr_targets_set] = ep->dport;
> +		cxlsd->cxld.target_map[cxl_rr->nr_targets_set] =
> +			ep->dport->port_id;
> +	}
> +	cxl_rr->nr_targets_set++;
> +
> +	return 0;
> +}
> +
>  static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
>  				  struct cxl_endpoint_decoder *cxled)
>  {
> -	int child_iw, child_ig, rc, pos = cxled->pos;
>  	struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
>  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>  	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
>  	struct cxl_decoder *cxld = cxl_rr->decoder;
>  	struct cxl_switch_decoder *cxlsd;
> +	int rc, pos = cxled->pos;
>  
>  	/*
>  	 * While root level decoders support x3, x6, x12, switch level
> @@ -1641,38 +1676,23 @@ static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr
>  					return rc;
>  				goto out_target_set;
>  			}
> -		goto add_target;
> -	}
> -
> -	rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &child_iw, &child_ig);
> -	if (rc)
> -		return rc;
> -
> -	rc = cxl_port_setup_decoder(port, cxlr, cxld, child_iw, child_ig);
> -	if (rc)
> -		return rc;
> -add_target:
> -	if (cxl_rr->nr_targets_set == cxl_rr->nr_targets) {
> -		dev_dbg(&cxlr->dev,
> -			"%s:%s: targets full trying to add %s:%s at %d\n",
> -			dev_name(port->uport_dev), dev_name(&port->dev),
> -			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), pos);
> -		return -ENXIO;
> -	}
> -	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> -		if (cxlsd->target[cxl_rr->nr_targets_set] != ep->dport) {
> -			dev_dbg(&cxlr->dev, "%s:%s: %s expected %s at %d\n",
> -				dev_name(port->uport_dev), dev_name(&port->dev),
> -				dev_name(&cxlsd->cxld.dev),
> -				dev_name(ep->dport->dport_dev),
> -				cxl_rr->nr_targets_set);
> -			return -ENXIO;
> -		}
>  	} else {
> -		cxlsd->target[cxl_rr->nr_targets_set] = ep->dport;
> -		cxlsd->cxld.target_map[cxl_rr->nr_targets_set] = ep->dport->port_id;
> +		int child_iw, child_ig;
> +
> +		rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &child_iw,
> +					      &child_ig);
> +		if (rc)
> +			return rc;
> +
> +		rc = cxl_port_setup_decoder(port, cxlr, cxld, child_iw,
> +					    child_ig);
> +		if (rc)
> +			return rc;
>  	}
> -	cxl_rr->nr_targets_set++;
> +
> +	rc = cxl_port_add_target(port, cxlr, cxl_rr, cxled, ep);
> +	if (rc)
> +		return rc;
>  out_target_set:
>  	dev_dbg(&cxlr->dev, "%s:%s target[%d] = %s for %s:%s @ %d\n",
>  		dev_name(port->uport_dev), dev_name(&port->dev),

Re: [PATCH 4/7] cxl/region: Factor out cxl_port_parent_interleave()

[Alison Schofield]

On Fri, Oct 09, 2026 at 03:39:43PM -0700, Dave Jiang wrote:
> 
> 
> On 10/9/26 1:24 PM, Alison Schofield wrote:
> > cxl_port_setup_targets() derives a decoder's interleave from the interleave
> > of the decoder above it. That parent interleave comes from either the root
> > decoder or a switch decoder, and each case is open coded along with the
> > parent validity checks.
> > 
> > Move the parent interleave lookup and validation to a new helper,
> > cxl_port_parent_interleave().
> > 
> > No functional change.
> > 
> > Assisted-by: LLM
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  drivers/cxl/core/region.c | 88 +++++++++++++++++++++++----------------
> >  1 file changed, 52 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 80c40716c368..c1866b7b089b 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1459,45 +1459,13 @@ static int cxl_port_peer_distance(struct cxl_port *port, struct cxl_region *cxlr
> >  	return distance;
> >  }
> >  
> > -static int cxl_port_setup_targets(struct cxl_port *port,
> > -				  struct cxl_region *cxlr,
> > -				  struct cxl_endpoint_decoder *cxled)
> > +static int cxl_port_parent_interleave(struct cxl_port *port, struct cxl_region *cxlr,
> > +				      int *iw, int *ig)
> >  {
> > -	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> > -	int parent_iw, parent_ig, child_iw, child_ig, rc, 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);
> > -	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > -	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
> > +	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
> >  	struct cxl_region_params *p = &cxlr->params;
> > -	struct cxl_decoder *cxld = cxl_rr->decoder;
> > -	struct cxl_switch_decoder *cxlsd;
> > -
> > -	/*
> > -	 * While root level decoders support x3, x6, x12, switch level
> > -	 * decoders only support powers of 2 up to x16.
> > -	 */
> > -	if (!is_power_of_2(cxl_rr->nr_targets)) {
> > -		dev_dbg(&cxlr->dev, "%s:%s: invalid target count %d\n",
> > -			dev_name(port->uport_dev), dev_name(&port->dev),
> > -			cxl_rr->nr_targets);
> > -		return -EINVAL;
> > -	}
> > -
> > -	cxlsd = to_cxl_switch_decoder(&cxld->dev);
> > -	if (cxl_rr->nr_targets_set) {
> > -		int distance = cxl_port_peer_distance(port, cxlr);
> > -
> > -		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
> > -			if (ep->dport == cxlsd->target[i]) {
> > -				rc = check_last_peer(cxled, ep, cxl_rr,
> > -						     distance);
> > -				if (rc)
> > -					return rc;
> > -				goto out_target_set;
> > -			}
> > -		goto add_target;
> > -	}
> > +	int parent_iw, parent_ig;
> >  
> >  	if (is_cxl_root(parent_port)) {
> >  		/*
> > @@ -1539,6 +1507,54 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> >  		return -EINVAL;
> >  	}
> >  
> > +	*iw = parent_iw;
> > +	*ig = parent_ig;
> > +
> > +	return 0;
> > +}
> > +
> > +static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
> 
> Do we want to just rename it to cxl_switch_port_setup_targets() since the function basically is handling switch only?

Yes that makes the scope clearer. I think I shall add a new rename patch
before any of the refactoring patches so the subsequent patches all use
that name.  (Rather than sneaking it in here where it is first touched.)

Thanks for the review!


> 
> DJ
> 
> > +				  struct cxl_endpoint_decoder *cxled)
> > +{
> > +	int parent_iw, parent_ig, child_iw, child_ig, rc, 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);
> > +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > +	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
> > +	struct cxl_region_params *p = &cxlr->params;
> > +	struct cxl_decoder *cxld = cxl_rr->decoder;
> > +	struct cxl_switch_decoder *cxlsd;
> > +
> > +	/*
> > +	 * While root level decoders support x3, x6, x12, switch level
> > +	 * decoders only support powers of 2 up to x16.
> > +	 */
> > +	if (!is_power_of_2(cxl_rr->nr_targets)) {
> > +		dev_dbg(&cxlr->dev, "%s:%s: invalid target count %d\n",
> > +			dev_name(port->uport_dev), dev_name(&port->dev),
> > +			cxl_rr->nr_targets);
> > +		return -EINVAL;
> > +	}
> > +
> > +	cxlsd = to_cxl_switch_decoder(&cxld->dev);
> > +	if (cxl_rr->nr_targets_set) {
> > +		int distance = cxl_port_peer_distance(port, cxlr);
> > +
> > +		for (int i = 0; i < cxl_rr->nr_targets_set; i++)
> > +			if (ep->dport == cxlsd->target[i]) {
> > +				rc = check_last_peer(cxled, ep, cxl_rr,
> > +						     distance);
> > +				if (rc)
> > +					return rc;
> > +				goto out_target_set;
> > +			}
> > +		goto add_target;
> > +	}
> > +
> > +	rc = cxl_port_parent_interleave(port, cxlr, &parent_iw, &parent_ig);
> > +	if (rc)
> > +		return rc;
> > +
> >  	child_iw = cxl_rr->nr_targets;
> >  	if (!ways_valid(child_iw)) {
> >  		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",
> 

Re: [PATCH 7/7] cxl/region: Factor out cxl_port_add_target()

[Alison Schofield]

On Fri, Oct 09, 2026 at 03:48:31PM -0700, Dave Jiang wrote:
> 
> 
> On 10/9/26 1:24 PM, Alison Schofield wrote:
> > cxl_port_setup_targets() adds an endpoint's downstream port to a port's
> > decoder target list. For an auto region it checks that the decoder already
> > targets that downstream port at the next position. For a user created
> > region it programs the downstream port at that position. That step is open
> > coded behind an add_target label that the target search jumps to.
> > 
> > Move that step to a new helper, cxl_port_add_target(). With the decoder
> > setup and the target add now in helpers, the path for the first endpoint to
> > reach a port becomes an else branch of the target search, so the add_target
> > label is no longer needed.
> > 
> > No functional change.
> > 
> > Assisted-by: LLM
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > ---
> >  drivers/cxl/core/region.c | 82 ++++++++++++++++++++++++---------------
> >  1 file changed, 51 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 4e9b0880663d..abe5fcc62e9d 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1608,15 +1608,50 @@ static int cxl_port_setup_decoder(struct cxl_port *port, struct cxl_region *cxlr
> >  	return 0;
> >  }
> >  
> > +static int cxl_port_add_target(struct cxl_port *port, struct cxl_region *cxlr,
> 
> This is only switch port right? Maybe just pass in cxlsd instead of port and name it cxl_switch_port_add_target()?

Yes, and the caller already has cxlsd. I'll pass that in directly and
do the rename. It still needs port for the dev messages, but we can get
that from cxl_rr->port.

Thanks for the review!

> 
> DJ
> 
> > +			       struct cxl_region_ref *cxl_rr,
> > +			       struct cxl_endpoint_decoder *cxled, struct cxl_ep *ep)
> > +{
> > +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> > +	struct cxl_switch_decoder *cxlsd;
> > +	int pos = cxled->pos;
> > +
> > +	cxlsd = to_cxl_switch_decoder(&cxl_rr->decoder->dev);
> > +	if (cxl_rr->nr_targets_set == cxl_rr->nr_targets) {
> > +		dev_dbg(&cxlr->dev,
> > +			"%s:%s: targets full trying to add %s:%s at %d\n",
> > +			dev_name(port->uport_dev), dev_name(&port->dev),
> > +			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), pos);
> > +		return -ENXIO;
> > +	}
> > +	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> > +		if (cxlsd->target[cxl_rr->nr_targets_set] != ep->dport) {
> > +			dev_dbg(&cxlr->dev, "%s:%s: %s expected %s at %d\n",
> > +				dev_name(port->uport_dev), dev_name(&port->dev),
> > +				dev_name(&cxlsd->cxld.dev),
> > +				dev_name(ep->dport->dport_dev),
> > +				cxl_rr->nr_targets_set);
> > +			return -ENXIO;
> > +		}
> > +	} else {
> > +		cxlsd->target[cxl_rr->nr_targets_set] = ep->dport;
> > +		cxlsd->cxld.target_map[cxl_rr->nr_targets_set] =
> > +			ep->dport->port_id;
> > +	}
> > +	cxl_rr->nr_targets_set++;
> > +
> > +	return 0;
> > +}
> > +
> >  static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
> >  				  struct cxl_endpoint_decoder *cxled)
> >  {
> > -	int child_iw, child_ig, rc, pos = cxled->pos;
> >  	struct cxl_region_ref *cxl_rr = cxl_rr_load(port, cxlr);
> >  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> >  	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
> >  	struct cxl_decoder *cxld = cxl_rr->decoder;
> >  	struct cxl_switch_decoder *cxlsd;
> > +	int rc, pos = cxled->pos;
> >  
> >  	/*
> >  	 * While root level decoders support x3, x6, x12, switch level
> > @@ -1641,38 +1676,23 @@ static int cxl_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr
> >  					return rc;
> >  				goto out_target_set;
> >  			}
> > -		goto add_target;
> > -	}
> > -
> > -	rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &child_iw, &child_ig);
> > -	if (rc)
> > -		return rc;
> > -
> > -	rc = cxl_port_setup_decoder(port, cxlr, cxld, child_iw, child_ig);
> > -	if (rc)
> > -		return rc;
> > -add_target:
> > -	if (cxl_rr->nr_targets_set == cxl_rr->nr_targets) {
> > -		dev_dbg(&cxlr->dev,
> > -			"%s:%s: targets full trying to add %s:%s at %d\n",
> > -			dev_name(port->uport_dev), dev_name(&port->dev),
> > -			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), pos);
> > -		return -ENXIO;
> > -	}
> > -	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> > -		if (cxlsd->target[cxl_rr->nr_targets_set] != ep->dport) {
> > -			dev_dbg(&cxlr->dev, "%s:%s: %s expected %s at %d\n",
> > -				dev_name(port->uport_dev), dev_name(&port->dev),
> > -				dev_name(&cxlsd->cxld.dev),
> > -				dev_name(ep->dport->dport_dev),
> > -				cxl_rr->nr_targets_set);
> > -			return -ENXIO;
> > -		}
> >  	} else {
> > -		cxlsd->target[cxl_rr->nr_targets_set] = ep->dport;
> > -		cxlsd->cxld.target_map[cxl_rr->nr_targets_set] = ep->dport->port_id;
> > +		int child_iw, child_ig;
> > +
> > +		rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &child_iw,
> > +					      &child_ig);
> > +		if (rc)
> > +			return rc;
> > +
> > +		rc = cxl_port_setup_decoder(port, cxlr, cxld, child_iw,
> > +					    child_ig);
> > +		if (rc)
> > +			return rc;
> >  	}
> > -	cxl_rr->nr_targets_set++;
> > +
> > +	rc = cxl_port_add_target(port, cxlr, cxl_rr, cxled, ep);
> > +	if (rc)
> > +		return rc;
> >  out_target_set:
> >  	dev_dbg(&cxlr->dev, "%s:%s target[%d] = %s for %s:%s @ %d\n",
> >  		dev_name(port->uport_dev), dev_name(&port->dev),
>