cxl/region: Refactor cxl_port_setup_targets()

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 <[email protected]>

Signed-off-by Alison Schofield <[email protected]>

Reviewed-by Dave Jiang <[email protected]>

Reviewed-by Jonathan Cameron <[email protected]>

---
  drivers/cxl/core/region.c | 49 +++++++++++++--------------------------
  drivers/cxl/cxl.h         | 21 +++++++++++++++++
  2 files changed, 37 insertions(+), 33 deletions(-)

drivers/cxl/core/region.c +16 -33

index a048bd5c06cb..bc087ca63946 100644
@@ -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;
  	}

drivers/cxl/cxl.h +21 -0

index d96210b8f147..8f0a79e7b8ab 100644
@@ -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))

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 <[email protected]>

Signed-off-by Alison Schofield <[email protected]>

Reviewed-by Dave Jiang <[email protected]>

Reviewed-by Jonathan Cameron <[email protected]>

---
 drivers/cxl/core/region.c | 49 +++++++++++++--------------------------
 drivers/cxl/cxl.h         | 21 +++++++++++++++++
 2 files changed, 37 insertions(+), 33 deletions(-)

drivers/cxl/core/region.c +16 -33

index a048bd5c06cb..bc087ca63946 100644
@@ -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;
 	}

drivers/cxl/cxl.h +21 -0

index d96210b8f147..8f0a79e7b8ab 100644
@@ -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))

Signature · 1 line

2.37.3

cxl_port_setup_targets() configures the switch decoder targets for a region. It runs for each switch port, including host bridges, between the root and an endpoint.

Rename the function to cxl_switch_port_setup_targets() to make its scope explicit.

No functional change.

Suggested-by Dave Jiang <[email protected]>

Assisted-by LLM

Signed-off-by Alison Schofield <[email protected]>

Reviewed-by Jonathan Cameron <[email protected]>

---
  drivers/cxl/core/region.c | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

drivers/cxl/core/region.c +4 -4

index bc087ca63946..9b0e2f195d1f 100644
@@ -1432,9 +1432,9 @@ static int check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
  	return 0;
  }
 
-static int cxl_port_setup_targets(struct cxl_port *port,
-				  struct cxl_region *cxlr,
-				  struct cxl_endpoint_decoder *cxled)
+static int cxl_switch_port_setup_targets(struct cxl_port *port,
+					 struct cxl_region *cxlr,
+					 struct cxl_endpoint_decoder *cxled)
  {
  	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
  	int parent_iw, parent_ig, ig, iw, rc, pos = cxled->pos;

@@ -1716,7 +1716,7 @@ static int cxl_region_setup_targets(struct cxl_region *cxlr)
  		 */
  		for (ep = cxl_ep_load(iter, cxlmd); iter;
  		     iter = ep->next, ep = cxl_ep_load(iter, cxlmd)) {
-			rc = cxl_port_setup_targets(iter, cxlr, cxled);
+			rc = cxl_switch_port_setup_targets(iter, cxlr, cxled);
  			if (rc) {
  				cxl_region_teardown_targets(cxlr);
  				return rc;

cxl_port_setup_targets() configures the switch decoder targets for a region. It runs for each switch port, including host bridges, between the root and an endpoint.

Rename the function to cxl_switch_port_setup_targets() to make its scope explicit.

No functional change.

Suggested-by Dave Jiang <[email protected]>

Assisted-by LLM

Signed-off-by Alison Schofield <[email protected]>

Reviewed-by Jonathan Cameron <[email protected]>

---
 drivers/cxl/core/region.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/cxl/core/region.c +4 -4

index bc087ca63946..9b0e2f195d1f 100644
@@ -1432,9 +1432,9 @@ static int check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
 	return 0;
 }
 
-static int cxl_port_setup_targets(struct cxl_port *port,
-				  struct cxl_region *cxlr,
-				  struct cxl_endpoint_decoder *cxled)
+static int cxl_switch_port_setup_targets(struct cxl_port *port,
+					 struct cxl_region *cxlr,
+					 struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
 	int parent_iw, parent_ig, ig, iw, rc, pos = cxled->pos;

@@ -1716,7 +1716,7 @@ static int cxl_region_setup_targets(struct cxl_region *cxlr)
 		 */
 		for (ep = cxl_ep_load(iter, cxlmd); iter;
 		     iter = ep->next, ep = cxl_ep_load(iter, cxlmd)) {
-			rc = cxl_port_setup_targets(iter, cxlr, cxled);
+			rc = cxl_switch_port_setup_targets(iter, cxlr, cxled);
 			if (rc) {
 				cxl_region_teardown_targets(cxlr);
 				return rc;

Signature · 1 line

2.37.3

cxl_switch_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 <[email protected]>

Reviewed-by Dave Jiang <[email protected]>

Reviewed-by Jonathan Cameron <[email protected]>

---
  drivers/cxl/core/region.c | 50 +++++++++++++++++++++++----------------
  1 file changed, 29 insertions(+), 21 deletions(-)

drivers/cxl/core/region.c +29 -21

index 9b0e2f195d1f..ea281ce4ec49 100644
@@ -1432,6 +1432,33 @@ static int check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
  	return 0;
  }
 
+/*
+ * 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.
+ */
+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;
+
+	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_switch_port_setup_targets(struct cxl_port *port,
  					 struct cxl_region *cxlr,
  					 struct cxl_endpoint_decoder *cxled)

@@ -1445,7 +1472,6 @@ static int cxl_switch_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_switch_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);

cxl_switch_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 <[email protected]>

Reviewed-by Dave Jiang <[email protected]>

Reviewed-by Jonathan Cameron <[email protected]>

---
 drivers/cxl/core/region.c | 50 +++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 21 deletions(-)

drivers/cxl/core/region.c +29 -21

index 9b0e2f195d1f..ea281ce4ec49 100644
@@ -1432,6 +1432,33 @@ static int check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
 	return 0;
 }
 
+/*
+ * 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.
+ */
+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;
+
+	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_switch_port_setup_targets(struct cxl_port *port,
 					 struct cxl_region *cxlr,
 					 struct cxl_endpoint_decoder *cxled)

@@ -1445,7 +1472,6 @@ static int cxl_switch_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_switch_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);

Signature · 1 line

2.37.3

Quoted 5 lines · Alison Schofield

cxl_switch_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.

Quoted 8 lines

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

No functional change.

Assisted-by LLM

Signed-off-by Alison Schofield <[email protected]>

Reviewed-by Dave Jiang <[email protected]>

Reviewed-by Jonathan Cameron <[email protected]>

Reviewed-by Richard Cheng <[email protected]>

This patch inspired me that there might be some space for optimization when facing different topology, while keeping this one as a generic arithmetic.

I'll try to draft a RFC patch series these days.

Best regards,
Richard Cheng.

Quoted 83 lines

---
 drivers/cxl/core/region.c | 50 +++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 21 deletions(-)

drivers/cxl/core/region.c +29 -21

index 9b0e2f195d1f..ea281ce4ec49 100644
@@ -1432,6 +1432,33 @@ static int check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
 	return 0;
 }
 
+/*
+ * 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.
+ */
+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;
+
+	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_switch_port_setup_targets(struct cxl_port *port,
 					 struct cxl_region *cxlr,
 					 struct cxl_endpoint_decoder *cxled)

@@ -1445,7 +1472,6 @@ static int cxl_switch_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_switch_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);

Signature · 1 line

2.37.3

Quoted 17 lines · On Wed, 14 Oct 2026 17:02:35 +0800 Richard Cheng <[email protected]> wrote:

Quoted 5 lines · Alison Schofield

cxl_switch_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.

Quoted 8 lines

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

No functional change.

Assisted-by LLM

Signed-off-by Alison Schofield <[email protected]>

Reviewed-by Dave Jiang <[email protected]>

Reviewed-by Jonathan Cameron <[email protected]>

Reviewed-by Richard Cheng <[email protected]>

Quoted 4 lines

This patch inspired me that there might be some space for optimization when facing different topology, while keeping this one as a generic arithmetic.

I'll try to draft a RFC patch series these days.

The patch Richard sent probably just wants rolling in here or slotting in immediately after.

Quoted 89 lines

Best regards,
Richard Cheng.

Quoted 83 lines

---
 drivers/cxl/core/region.c | 50 +++++++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 21 deletions(-)

drivers/cxl/core/region.c +29 -21

index 9b0e2f195d1f..ea281ce4ec49 100644
@@ -1432,6 +1432,33 @@ static int check_interleave_cap(struct cxl_decoder *cxld, int iw, int ig)
 	return 0;
 }
 
+/*
+ * 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.
+ */
+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;
+
+	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_switch_port_setup_targets(struct cxl_port *port,
 					 struct cxl_region *cxlr,
 					 struct cxl_endpoint_decoder *cxled)

@@ -1445,7 +1472,6 @@ static int cxl_switch_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_switch_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);

Signature · 1 line

2.37.3

cxl_switch_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 <[email protected]>

---
  drivers/cxl/core/region.c | 88 +++++++++++++++++++++++----------------
  1 file changed, 52 insertions(+), 36 deletions(-)

drivers/cxl/core/region.c +52 -36

index ea281ce4ec49..c14ac041c4ac 100644
@@ -1459,45 +1459,13 @@ static int cxl_port_peer_distance(struct cxl_port *port, struct cxl_region *cxlr
  	return distance;
  }
 
-static int cxl_switch_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 *out_iw, int *out_ig)
  {
-	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
-	int parent_iw, parent_ig, ig, iw, 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_switch_port_setup_targets(struct cxl_port *port,
  		return -EINVAL;
  	}
 
+	*out_iw = parent_iw;
+	*out_ig = parent_ig;
+
+	return 0;
+}
+
+static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
+					 struct cxl_endpoint_decoder *cxled)
+{
+	int parent_iw, parent_ig, ig, iw, 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;
+
  	iw = cxl_rr->nr_targets;
  	if (!ways_valid(iw)) {
  		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",

cxl_switch_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 <[email protected]>

---
 drivers/cxl/core/region.c | 88 +++++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 36 deletions(-)

drivers/cxl/core/region.c +2 -7

index ea281ce4ec49..c14ac041c4ac 100644
@@ -1459,45 +1459,13 @@ static int cxl_port_peer_distance(struct cxl_port *port, struct cxl_region *cxlr
 	return distance;
 }
 
-static int cxl_switch_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 *out_iw, int *out_ig)
 {
-	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
-	int parent_iw, parent_ig, ig, iw, 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);

drivers/cxl/core/region.c +49 -21

-	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_switch_port_setup_targets(struct cxl_port *port,
 		return -EINVAL;
 	}
 
+	*out_iw = parent_iw;
+	*out_ig = parent_ig;
+
+	return 0;
+}
+
+static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
+					 struct cxl_endpoint_decoder *cxled)
+{
+	int parent_iw, parent_ig, ig, iw, 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;
+
 	iw = cxl_rr->nr_targets;
 	if (!ways_valid(iw)) {
 		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",

Signature · 1 line

2.37.3

Quoted 35 lines · Alison Schofield

cxl_switch_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 <[email protected]>

---
 drivers/cxl/core/region.c | 88 +++++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 36 deletions(-)

drivers/cxl/core/region.c +2 -7

index ea281ce4ec49..c14ac041c4ac 100644
@@ -1459,45 +1459,13 @@ static int cxl_port_peer_distance(struct cxl_port *port, struct cxl_region *cxlr
 	return distance;
 }
 
-static int cxl_switch_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 *out_iw, int *out_ig)
 {
-	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
-	int parent_iw, parent_ig, ig, iw, 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);

drivers/cxl/core/region.c static int cxl_port_peer_distance(struct cxl_port *port, struct cxl_region *cxlr · line 1479

Quoted 10 lines

drivers/cxl/core/region.c +1 -8

-	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.
-	 */

Hi Alison,

Can we make the comment more explicit ?
For 3/6/12-way roots, the helper returns a normalized out_iw of 1/2/4 rather than the parent's actual IW.

Maybe we should document that return value convention ?
Simply browse through the helper, I would think it just retrieves the parent's setting.

Not a blocker though.

Reviewed-by Richard Cheng <[email protected]>

Best regards,
Richard Cheng.

Quoted 83 lines

drivers/cxl/core/region.c +49 -21

-	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_switch_port_setup_targets(struct cxl_port *port,
 		return -EINVAL;
 	}
 
+	*out_iw = parent_iw;
+	*out_ig = parent_ig;
+
+	return 0;
+}
+
+static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
+					 struct cxl_endpoint_decoder *cxled)
+{
+	int parent_iw, parent_ig, ig, iw, 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;
+
 	iw = cxl_rr->nr_targets;
 	if (!ways_valid(iw)) {
 		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",

Signature · 1 line

2.37.3

Quoted 49 lines · Richard Cheng

Quoted 35 lines · Alison Schofield

cxl_switch_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 <[email protected]>

---
 drivers/cxl/core/region.c | 88 +++++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 36 deletions(-)

drivers/cxl/core/region.c +2 -7

index ea281ce4ec49..c14ac041c4ac 100644
@@ -1459,45 +1459,13 @@ static int cxl_port_peer_distance(struct cxl_port *port, struct cxl_region *cxlr
 	return distance;
 }
 
-static int cxl_switch_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 *out_iw, int *out_ig)
 {
-	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
-	int parent_iw, parent_ig, ig, iw, 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);

drivers/cxl/core/region.c static int cxl_port_peer_distance(struct cxl_port *port, struct cxl_region *cxlr · line 1479

Quoted 10 lines

drivers/cxl/core/region.c +1 -8

-	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.
-	 */

Hi Alison,

Quoted 7 lines

Can we make the comment more explicit ?
For 3/6/12-way roots, the helper returns a normalized out_iw of 1/2/4 rather than the parent's actual IW.

Maybe we should document that return value convention ?
Simply browse through the helper, I would think it just retrieves the parent's setting.

Thanks for the review and good point. The normalizations happens in the root decoder branch of cxl_port_parent_interleave. This diff doesn't show that because it moved unchanged. Here's what I'm planning for v4:
/*
* For purposes of address bit routing, use power-of-2 math for
* switch ports. For an x3, x6, or x12 root this returns parent
* ways of 1, 2, or 4 rather than the root decoder's ways. */

	if (!is_power_of_2(parent_iw))
		parent_iw /= 3;

Quoted 92 lines

Not a blocker though.

Reviewed-by Richard Cheng <[email protected]>

Best regards,
Richard Cheng.

Quoted 83 lines

drivers/cxl/core/region.c +49 -21

-	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_switch_port_setup_targets(struct cxl_port *port,
 		return -EINVAL;
 	}
 
+	*out_iw = parent_iw;
+	*out_ig = parent_ig;
+
+	return 0;
+}
+
+static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
+					 struct cxl_endpoint_decoder *cxled)
+{
+	int parent_iw, parent_ig, ig, iw, 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;
+
 	iw = cxl_rr->nr_targets;
 	if (!ways_valid(iw)) {
 		dev_dbg(&cxlr->dev, "%s:%s: invalid port interleave: %d\n",

Signature · 1 line

2.37.3

cxl_switch_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 <[email protected]>

cxl_switch_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 <[email protected]>

Reviewed-by Dave Jiang <[email protected]>

---
  drivers/cxl/core/region.c | 81 ++++++++++++++++++++++++---------------
  1 file changed, 50 insertions(+), 31 deletions(-)

drivers/cxl/core/region.c +50 -31

index c14ac041c4ac..ee9e615a9a4c 100644
@@ -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 *out_iw,
+				    int *out_ig)
+{
+	struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
+	struct cxl_decoder *cxld = cxl_rr->decoder;
+	int parent_iw, parent_ig, iw, ig, rc;
+	struct cxl_switch_decoder *cxlsd;
+
+	rc = cxl_port_parent_interleave(port, cxlr, &parent_iw, &parent_ig);
+	if (rc)
+		return rc;
+
+	iw = cxl_rr->nr_targets;
+	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 -EINVAL;
+	}
+
+	/*
+	 * Interleave granularity is a multiple of @parent_port granularity.
+	 * Multiplier is the parent port interleave ways.
+	 */
+	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 -EINVAL;
+	}
+
+	cxlsd = to_cxl_switch_decoder(&cxld->dev);
+	if (iw > 8 || 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);
+		return -ENXIO;
+	}
+
+	*out_iw = iw;
+	*out_ig = ig;
+
+	return 0;
+}
+
  static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
  					 struct cxl_endpoint_decoder *cxled)
  {
-	int parent_iw, parent_ig, ig, iw, rc, pos = cxled->pos;
-	struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
+	int iw, 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_switch_port_setup_targets(struct cxl_port *port, struct cxl_regio
  		goto add_target;
  	}
 
-	rc = cxl_port_parent_interleave(port, cxlr, &parent_iw, &parent_ig);
+	rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &iw, &ig);
  	if (rc)
  		return rc;
 
-	iw = cxl_rr->nr_targets;
-	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 -EINVAL;
-	}
-
-	/*
-	 * Interleave granularity is a multiple of @parent_port granularity.
-	 * Multiplier is the parent port interleave ways.
-	 */
-	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 -EINVAL;
-	}
-
-	if (iw > 8 || 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);
-		return -ENXIO;
-	}
-
  	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
  		if (cxld->interleave_ways != iw ||
  		    (iw > 1 && cxld->interleave_granularity != ig) ||

cxl_switch_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 <[email protected]>

Reviewed-by Dave Jiang <[email protected]>

---
 drivers/cxl/core/region.c | 81 ++++++++++++++++++++++++---------------
 1 file changed, 50 insertions(+), 31 deletions(-)

drivers/cxl/core/region.c +50 -31

index c14ac041c4ac..ee9e615a9a4c 100644
@@ -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 *out_iw,
+				    int *out_ig)
+{
+	struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
+	struct cxl_decoder *cxld = cxl_rr->decoder;
+	int parent_iw, parent_ig, iw, ig, rc;
+	struct cxl_switch_decoder *cxlsd;
+
+	rc = cxl_port_parent_interleave(port, cxlr, &parent_iw, &parent_ig);
+	if (rc)
+		return rc;
+
+	iw = cxl_rr->nr_targets;
+	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 -EINVAL;
+	}
+
+	/*
+	 * Interleave granularity is a multiple of @parent_port granularity.
+	 * Multiplier is the parent port interleave ways.
+	 */
+	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 -EINVAL;
+	}
+
+	cxlsd = to_cxl_switch_decoder(&cxld->dev);
+	if (iw > 8 || 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);
+		return -ENXIO;
+	}
+
+	*out_iw = iw;
+	*out_ig = ig;
+
+	return 0;
+}
+
 static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
 					 struct cxl_endpoint_decoder *cxled)
 {
-	int parent_iw, parent_ig, ig, iw, rc, pos = cxled->pos;
-	struct cxl_port *parent_port = to_cxl_port(port->dev.parent);
+	int iw, 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_switch_port_setup_targets(struct cxl_port *port, struct cxl_regio
 		goto add_target;
 	}
 
-	rc = cxl_port_parent_interleave(port, cxlr, &parent_iw, &parent_ig);
+	rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &iw, &ig);
 	if (rc)
 		return rc;
 
-	iw = cxl_rr->nr_targets;
-	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 -EINVAL;
-	}
-
-	/*
-	 * Interleave granularity is a multiple of @parent_port granularity.
-	 * Multiplier is the parent port interleave ways.
-	 */
-	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 -EINVAL;
-	}
-
-	if (iw > 8 || 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);
-		return -ENXIO;
-	}
-
 	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
 		if (cxld->interleave_ways != iw ||
 		    (iw > 1 && cxld->interleave_granularity != ig) ||

Signature · 1 line

2.37.3

cxl_switch_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 <[email protected]>

Reviewed-by Dave Jiang <[email protected]>

Once cxl_switch_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_switch_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 <[email protected]>

Reviewed-by Dave Jiang <[email protected]>

---
  drivers/cxl/core/region.c | 88 ++++++++++++++++++++++-----------------
  1 file changed, 50 insertions(+), 38 deletions(-)

drivers/cxl/core/region.c +50 -38

index ee9e615a9a4c..229372123271 100644
@@ -1561,46 +1561,11 @@ static int cxl_port_calc_interleave(struct cxl_port *port, struct cxl_region *cx
  	return 0;
  }
 
-static int cxl_switch_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 iw, int ig)
  {
-	int iw, 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, &iw, &ig);
-	if (rc)
-		return rc;
+	int rc;
 
  	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
  		if (cxld->interleave_ways != iw ||

@@ -1639,6 +1604,53 @@ static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_regio
  	}
  	dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport_dev),
  		dev_name(&port->dev), iw, ig);
+
+	return 0;
+}
+
+static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
+					 struct cxl_endpoint_decoder *cxled)
+{
+	int iw, 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, &iw, &ig);
+	if (rc)
+		return rc;
+
+	rc = cxl_port_setup_decoder(port, cxlr, cxld, iw, ig);
+	if (rc)
+		return rc;
  add_target:
  	if (cxl_rr->nr_targets_set == cxl_rr->nr_targets) {
  		dev_dbg(&cxlr->dev,

Quoted 10 lines · Alison Schofield

Once cxl_switch_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_switch_port_setup_targets().

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

No functional change.

Quoted 3 lines

Assisted-by LLM

Signed-off-by Alison Schofield <[email protected]>

Reviewed-by Dave Jiang <[email protected]>

Reviewed-by Li Ming <[email protected]>

See my comment in patch #7.

Quoted 111 lines

---
  drivers/cxl/core/region.c | 88 ++++++++++++++++++++++-----------------
  1 file changed, 50 insertions(+), 38 deletions(-)

drivers/cxl/core/region.c +50 -38

index ee9e615a9a4c..229372123271 100644
@@ -1561,46 +1561,11 @@ static int cxl_port_calc_interleave(struct cxl_port *port, struct cxl_region *cx
  	return 0;
  }
-static int cxl_switch_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 iw, int ig)
  {
-	int iw, 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, &iw, &ig);
-	if (rc)
-		return rc;
+	int rc;
  	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
  		if (cxld->interleave_ways != iw ||

@@ -1639,6 +1604,53 @@ static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_regio
  	}
  	dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport_dev),
  		dev_name(&port->dev), iw, ig);
+
+	return 0;
+}
+
+static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
+					 struct cxl_endpoint_decoder *cxled)
+{
+	int iw, 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, &iw, &ig);
+	if (rc)
+		return rc;
+
+	rc = cxl_port_setup_decoder(port, cxlr, cxld, iw, ig);
+	if (rc)
+		return rc;
  add_target:
  	if (cxl_rr->nr_targets_set == cxl_rr->nr_targets) {
  		dev_dbg(&cxlr->dev,

Once cxl_switch_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_switch_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 <[email protected]>

Reviewed-by Dave Jiang <[email protected]>

drivers/cxl/core/region.c +20 -0

 	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
 		if (cxld->interleave_ways != iw ||

@@ -1639,6 +1604,53 @@ static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_regio
 	}
 	dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport_dev),
 		dev_name(&port->dev), iw, ig);
+
+	return 0;
+}
+
+static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
+					 struct cxl_endpoint_decoder *cxled)
+{
+	int iw, 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",

drivers/cxl/core/region.c +17 -0

+			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, &iw, &ig);
+	if (rc)
+		return rc;
+
+	rc = cxl_port_setup_decoder(port, cxlr, cxld, iw, ig);
+	if (rc)
+		return rc;
 add_target:
 	if (cxl_rr->nr_targets_set == cxl_rr->nr_targets) {
 		dev_dbg(&cxlr->dev,

Quoted 6 lines · On Mon, 12 Oct 2026 21:26:02 -0700 Alison Schofield…

Once cxl_switch_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_switch_port_setup_targets().

Quoted 7 lines

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

No functional change.

Assisted-by LLM

Signed-off-by Alison Schofield <[email protected]>

Reviewed-by Dave Jiang <[email protected]>

Maybe some further opportunities in here to improve readability (using Ming and Richard's suggestions)

Quoted 26 lines

drivers/cxl/core/region.c +20 -0

 	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
 		if (cxld->interleave_ways != iw ||

@@ -1639,6 +1604,53 @@ static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_regio
 	}
 	dev_dbg(&cxlr->dev, "%s:%s iw: %d ig: %d\n", dev_name(port->uport_dev),
 		dev_name(&port->dev), iw, ig);
+
+	return 0;
+}
+
+static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
+					 struct cxl_endpoint_decoder *cxled)
+{
+	int iw, 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",

drivers/cxl/core/region.c static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_regio · line 1636

Quoted 10 lines

drivers/cxl/core/region.c +10 -0

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

I wonder if it is worth taking opportunity to flip things:

		for (int i = 0; i < cxl_rr_nr_targets_set; i++) {
			if (ep->dport != cxlsd->target[i])
				continue;

			distance = cxl_port_peer_distance(port, cxlr);
			return check_last_peer(cxled, ep, cxl_rr,
					       cxl_port_peer_distance(port, cxlr));
			// Ming's comment and Richard's patch rolled in
			// The local variable ended up ugly hence inline unlike
			// Richard's suggestion.
		}
		goto add_target;

Quoted 20 lines

drivers/cxl/core/region.c +17 -0

+			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, &iw, &ig);
+	if (rc)
+		return rc;
+
+	rc = cxl_port_setup_decoder(port, cxlr, cxld, iw, ig);
+	if (rc)
+		return rc;
 add_target:
 	if (cxl_rr->nr_targets_set == cxl_rr->nr_targets) {
 		dev_dbg(&cxlr->dev,

cxl_switch_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_switch_port_add_target(). The helper takes the switch decoder whose target list it updates, rather than the port, which makes its scope explicit. 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 <[email protected]>

---
  drivers/cxl/core/region.c | 80 ++++++++++++++++++++++++---------------
  1 file changed, 49 insertions(+), 31 deletions(-)

drivers/cxl/core/region.c +42 -30

index 229372123271..50f4830c225d 100644
@@ -1608,15 +1608,50 @@ static int cxl_port_setup_decoder(struct cxl_port *port, struct cxl_region *cxlr
  	return 0;
  }
 
+static int cxl_switch_port_add_target(struct cxl_switch_decoder *cxlsd,
+				      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_port *port = cxl_rr->port;
+	int pos = cxled->pos;
+
+	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_switch_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
  					 struct cxl_endpoint_decoder *cxled)
  {
-	int iw, 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,21 @@ static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_regio
  					return rc;
  				goto out_target_set;
  			}
-		goto add_target;
-	}
-
-	rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &iw, &ig);
-	if (rc)
-		return rc;
-
-	rc = cxl_port_setup_decoder(port, cxlr, cxld, iw, 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 iw, ig;
+
+		rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &iw, &ig);
+		if (rc)
+			return rc;
+

drivers/cxl/core/region.c +0 -0

  	dev_dbg(&cxlr->dev, "%s:%s target[%d] = %s for %s:%s @ %d\n",
  		dev_name(port->uport_dev), dev_name(&port->dev),

Quoted 13 lines · Alison Schofield

cxl_switch_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_switch_port_add_target(). The helper takes the switch decoder whose target list it updates, rather than the port, which makes its scope explicit. 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.

Quoted 4 lines

No functional change.

Assisted-by LLM

Signed-off-by Alison Schofield <[email protected]>

Reviewed-by Li Ming <[email protected]>

Just one minor comment below.

drivers/cxl/core/region.c static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_regio · line 1694

Quoted 5 lines

drivers/cxl/core/region.c +4 -0

+
+	rc = cxl_switch_port_add_target(cxlsd, cxlr, cxl_rr, cxled, ep);
+	if (rc)
+		return rc;
  out_target_set:

Do we really need this "out_target_set"? Seems like it can be removed too. Once a new target is set up, this function outputs the following log, if this function finds a registered target, I guess we don't need to output the following log again?

Quoted 2 lines

dev_dbg(&cxlr->dev, "%s:%s target[%d] = %s for %s:%s @ %d\n",
dev_name(port->uport_dev), dev_name(&port->dev),

cxl_switch_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_switch_port_add_target(). The helper takes the switch decoder whose target list it updates, rather than the port, which makes its scope explicit. 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 <[email protected]>

---
 drivers/cxl/core/region.c | 80 ++++++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 31 deletions(-)

drivers/cxl/core/region.c +49 -31

index 229372123271..50f4830c225d 100644
@@ -1608,15 +1608,50 @@ static int cxl_port_setup_decoder(struct cxl_port *port, struct cxl_region *cxlr
 	return 0;
 }
 
+static int cxl_switch_port_add_target(struct cxl_switch_decoder *cxlsd,
+				      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_port *port = cxl_rr->port;
+	int pos = cxled->pos;
+
+	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_switch_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
 					 struct cxl_endpoint_decoder *cxled)
 {
-	int iw, 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,21 @@ static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_regio
 					return rc;
 				goto out_target_set;
 			}
-		goto add_target;
-	}
-
-	rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &iw, &ig);
-	if (rc)
-		return rc;
-
-	rc = cxl_port_setup_decoder(port, cxlr, cxld, iw, 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 iw, ig;
+
+		rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &iw, &ig);
+		if (rc)
+			return rc;
+
+		rc = cxl_port_setup_decoder(port, cxlr, cxld, iw, ig);
+		if (rc)
+			return rc;
 	}
-	cxl_rr->nr_targets_set++;
+
+	rc = cxl_switch_port_add_target(cxlsd, 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),

Signature · 1 line

2.37.3

cxl_switch_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_switch_port_add_target(). The helper takes the switch decoder whose target list it updates, rather than the port, which makes its scope explicit. 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 <[email protected]>

---
 drivers/cxl/core/region.c | 80 ++++++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 31 deletions(-)

drivers/cxl/core/region.c +36 -30

index 229372123271..50f4830c225d 100644
@@ -1608,15 +1608,50 @@ static int cxl_port_setup_decoder(struct cxl_port *port, struct cxl_region *cxlr
 	return 0;
 }
 
+static int cxl_switch_port_add_target(struct cxl_switch_decoder *cxlsd,
+				      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_port *port = cxl_rr->port;
+	int pos = cxled->pos;
+
+	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_switch_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
 					 struct cxl_endpoint_decoder *cxled)
 {
-	int iw, 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,21 @@ static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_regio
 					return rc;
 				goto out_target_set;
 			}
-		goto add_target;
-	}
-
-	rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &iw, &ig);
-	if (rc)
-		return rc;
-
-	rc = cxl_port_setup_decoder(port, cxlr, cxld, iw, 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;

drivers/cxl/core/region.c +4 -1

-	cxl_rr->nr_targets_set++;
+
+	rc = cxl_switch_port_add_target(cxlsd, 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),

Quoted 6 lines · On Mon, 12 Oct 2026 21:26:03 -0700 Alison Schofield…

cxl_switch_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.

Quoted 6 lines

Move that step to a new helper, cxl_switch_port_add_target(). The helper takes the switch decoder whose target list it updates, rather than the port, which makes its scope explicit. 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.

That else doesn't really have much to do with the helper I think? It is an unrelated change wrt to the helper added in this patch.

Maybe I am missing something but to me looks like you should split that bit out.

Otherwise, LGTM

Quoted 106 lines

No functional change.

Assisted-by LLM

Signed-off-by Alison Schofield <[email protected]>

---
 drivers/cxl/core/region.c | 80 ++++++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 31 deletions(-)

drivers/cxl/core/region.c +42 -30

index 229372123271..50f4830c225d 100644
@@ -1608,15 +1608,50 @@ static int cxl_port_setup_decoder(struct cxl_port *port, struct cxl_region *cxlr
 	return 0;
 }
 
+static int cxl_switch_port_add_target(struct cxl_switch_decoder *cxlsd,
+				      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_port *port = cxl_rr->port;
+	int pos = cxled->pos;
+
+	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_switch_port_setup_targets(struct cxl_port *port, struct cxl_region *cxlr,
 					 struct cxl_endpoint_decoder *cxled)
 {
-	int iw, 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,21 @@ static int cxl_switch_port_setup_targets(struct cxl_port *port, struct cxl_regio
 					return rc;
 				goto out_target_set;
 			}
-		goto add_target;
-	}
-
-	rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &iw, &ig);
-	if (rc)
-		return rc;
-
-	rc = cxl_port_setup_decoder(port, cxlr, cxld, iw, 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 iw, ig;
+
+		rc = cxl_port_calc_interleave(port, cxlr, cxl_rr, &iw, &ig);
+		if (rc)
+			return rc;
+