All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/4] Harden HDM decoder enumeration
@ 2026-07-21  1:59 Alison Schofield
  2026-07-21  1:59 ` [PATCH v4 1/4] cxl/hdm: Reject switch decoder interleave ways that overflow targets Alison Schofield
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Alison Schofield @ 2026-07-21  1:59 UTC (permalink / raw)
  To: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Alison Schofield,
	Vishal Verma, Ira Weiny, Dan Williams, Li Ming
  Cc: linux-cxl, Srirangan Madhavan

Changes in v4:
A new patch was inserted as Patch2, so now set is 4 patches.
   Sashiko is likely to complain in Patch 1 for something fixed in
   Patches 2 and 3, but reordering does not makes sense.
P2: Only interpret interleave settings on committed decoders (Ming)
    This is the newly inserted patch
P4: Register reset w devm_add_action (Ming)
P4: Fix up a kernel-doc comment issue (lkp)

Changes in v3:
P1: Move zero-target reject to init_hdm_decoder() (RichardC)
P3: Reset commit_end via a devres action after decoder teardown (RichardC)

Changes in v2:
P1: Handle uncommitted decoders (Sashiko)
P1: Reject switch decoders with zero targets (Sashiko)
P1: Add in code comment clarifying target_count (Sashiko)
P1: Update commit log
P3: Reset commit_end on the failure path only (Sashiko)
P3: Update commit log
No changes in P2


This set started as Sashiko reported cleanups, and at v4, Patch 2/4
was added based on Li Ming's review and catch.

Only the first includes a link to the original Sashiko complaint, since
the others did not originally appear on Lore for me to add public links. 

Srirangan: I believe patches 1 and 3 of these also address pre-existing
issues Sashiko posted in reply to your reset series, Patch 02/11.


Alison Schofield (4):
  cxl/hdm: Reject switch decoder interleave ways that overflow targets
  cxl/hdm: Only interpret interleave settings on committed decoders
  cxl/hdm: Make switch decoder target parsing endian-safe
  cxl/hdm: Restore commit_end when decoder enumeration fails

 drivers/cxl/core/hdm.c | 98 +++++++++++++++++++++++++++++++-----------
 drivers/cxl/cxl.h      |  2 +
 2 files changed, 76 insertions(+), 24 deletions(-)


base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
-- 
2.37.3


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v4 1/4] cxl/hdm: Reject switch decoder interleave ways that overflow targets
  2026-07-21  1:59 [PATCH v4 0/4] Harden HDM decoder enumeration Alison Schofield
@ 2026-07-21  1:59 ` Alison Schofield
  2026-07-21  2:14   ` sashiko-bot
                     ` (2 more replies)
  2026-07-21  1:59 ` [PATCH v4 2/4] cxl/hdm: Only interpret interleave settings on committed decoders Alison Schofield
                   ` (3 subsequent siblings)
  4 siblings, 3 replies; 15+ messages in thread
From: Alison Schofield @ 2026-07-21  1:59 UTC (permalink / raw)
  To: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Alison Schofield,
	Vishal Verma, Ira Weiny, Dan Williams, Li Ming
  Cc: linux-cxl, Srirangan Madhavan

Switch decoder enumeration validates that the interleave ways encoding
is legal, but not that the resulting number of ways fits the available
targets. This can overrun the target arrays during enumeration.

Reject committed decoders whose interleave ways exceed either the
hardware target list capacity or the reported target count. Reject
switch decoders that report zero targets.

For uncommitted decoders, ignore the stale interleave ways value and
reset it to one until the decoder is committed.

Add a clarifying comment that target_count is a direct count, not
0-based like decoder_count.

Link: https://sashiko.dev/#/patchset/20260711180755.1779002-1-alison.schofield@intel.com?part=1
Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/hdm.c | 33 +++++++++++++++++++++++++++++++++
 drivers/cxl/cxl.h      |  2 ++
 2 files changed, 35 insertions(+)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 0c80b76a5f9b..d81df45d8005 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -76,6 +76,8 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
 
 	hdm_cap = readl(cxlhdm->regs.hdm_decoder + CXL_HDM_DECODER_CAP_OFFSET);
 	cxlhdm->decoder_count = cxl_hdm_decoder_count(hdm_cap);
+
+	/* target_count is a direct count (1h..8h), not 0-based like decoder_count */
 	cxlhdm->target_count =
 		FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, hdm_cap);
 	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap))
@@ -1084,6 +1086,37 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 		cxld->interleave_ways, cxld->interleave_granularity);
 
 	if (!cxled) {
+		struct cxl_switch_decoder *cxlsd =
+			to_cxl_switch_decoder(&cxld->dev);
+
+		if (cxlsd->nr_targets < 1) {
+			dev_err(&port->dev,
+				"decoder%d.%d: reports zero targets\n",
+				port->id, cxld->id);
+			return -ENXIO;
+		}
+
+		if (!committed) {
+			/* Ignore interleave ways until commit */
+			cxld->interleave_ways = 1;
+			return 0;
+		}
+
+		if (cxld->interleave_ways > CXL_HDM_DECODER0_TL_TARGETS) {
+			dev_err(&port->dev,
+				"decoder%d.%d: interleave ways: %d exceeds target list capacity: %d\n",
+				port->id, cxld->id, cxld->interleave_ways,
+				CXL_HDM_DECODER0_TL_TARGETS);
+			return -ENXIO;
+		}
+		if (cxld->interleave_ways > cxlsd->nr_targets) {
+			dev_err(&port->dev,
+				"decoder%d.%d: interleave ways: %d exceeds targets: %d\n",
+				port->id, cxld->id, cxld->interleave_ways,
+				cxlsd->nr_targets);
+			return -ENXIO;
+		}
+
 		lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
 		hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));
 		target_list.value = (hi << 32) + lo;
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index c0e5308e4d1b..291ada46b646 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -67,6 +67,8 @@ extern const struct nvdimm_security_ops *cxl_security_ops;
 #define   CXL_HDM_DECODER0_CTRL_HOSTONLY BIT(12)
 #define CXL_HDM_DECODER0_TL_LOW(i) (0x20 * (i) + 0x24)
 #define CXL_HDM_DECODER0_TL_HIGH(i) (0x20 * (i) + 0x28)
+/* Two registers with one target ID per byte */
+#define CXL_HDM_DECODER0_TL_TARGETS 8
 #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i)
 #define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i)
 
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 2/4] cxl/hdm: Only interpret interleave settings on committed decoders
  2026-07-21  1:59 [PATCH v4 0/4] Harden HDM decoder enumeration Alison Schofield
  2026-07-21  1:59 ` [PATCH v4 1/4] cxl/hdm: Reject switch decoder interleave ways that overflow targets Alison Schofield
@ 2026-07-21  1:59 ` Alison Schofield
  2026-07-21  6:26   ` Li Ming
  2026-07-21  1:59 ` [PATCH v4 3/4] cxl/hdm: Make switch decoder target parsing endian-safe Alison Schofield
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Alison Schofield @ 2026-07-21  1:59 UTC (permalink / raw)
  To: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Alison Schofield,
	Vishal Verma, Ira Weiny, Dan Williams, Li Ming
  Cc: linux-cxl, Srirangan Madhavan

Decoder enumeration reads the interleave ways and granularity fields
from the CTRL register unconditionally, then decodes and validates
them for every decoder. On an uncommitted decoder these fields are
stale. They hold whatever was last latched and are reprogrammed when
the decoder is committed. Interpreting them serves no purpose, and
rejecting enumeration on a reserved encoding in those stale bits can
fail an otherwise usable decoder.

Decode and validate interleave ways and granularity only when the
decoder is committed. On an uncommitted decoder the stale register
values are not interpreted. The interleave fields are set when the
decoder is programmed at commit time.


Reported-by: Li Ming <ming.li@zohomail.com>
Closes: https://lore.kernel.org/all/171d58db-3555-4a79-a9f9-8a3c2e14d91d@zohomail.com/
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/hdm.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index d81df45d8005..1f995191baf5 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -1064,21 +1064,24 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 			writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
 		}
 	}
-	rc = eiw_to_ways(FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl),
-			  &cxld->interleave_ways);
-	if (rc) {
-		dev_warn(&port->dev,
-			 "decoder%d.%d: Invalid interleave ways (ctrl: %#x)\n",
-			 port->id, cxld->id, ctrl);
-		return rc;
-	}
-	rc = eig_to_granularity(FIELD_GET(CXL_HDM_DECODER0_CTRL_IG_MASK, ctrl),
-				 &cxld->interleave_granularity);
-	if (rc) {
-		dev_warn(&port->dev,
-			 "decoder%d.%d: Invalid interleave granularity (ctrl: %#x)\n",
-			 port->id, cxld->id, ctrl);
-		return rc;
+	/* Interleave settings are only valid on a committed decoder */
+	if (committed) {
+		rc = eiw_to_ways(FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl),
+				 &cxld->interleave_ways);
+		if (rc) {
+			dev_warn(&port->dev,
+				 "decoder%d.%d: Invalid interleave ways (ctrl: %#x)\n",
+				 port->id, cxld->id, ctrl);
+			return rc;
+		}
+		rc = eig_to_granularity(FIELD_GET(CXL_HDM_DECODER0_CTRL_IG_MASK, ctrl),
+					&cxld->interleave_granularity);
+		if (rc) {
+			dev_warn(&port->dev,
+				 "decoder%d.%d: Invalid interleave granularity (ctrl: %#x)\n",
+				 port->id, cxld->id, ctrl);
+			return rc;
+		}
 	}
 
 	dev_dbg(&port->dev, "decoder%d.%d: range: %#llx-%#llx iw: %d ig: %d\n",
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 3/4] cxl/hdm: Make switch decoder target parsing endian-safe
  2026-07-21  1:59 [PATCH v4 0/4] Harden HDM decoder enumeration Alison Schofield
  2026-07-21  1:59 ` [PATCH v4 1/4] cxl/hdm: Reject switch decoder interleave ways that overflow targets Alison Schofield
  2026-07-21  1:59 ` [PATCH v4 2/4] cxl/hdm: Only interpret interleave settings on committed decoders Alison Schofield
@ 2026-07-21  1:59 ` Alison Schofield
  2026-07-21 15:09   ` Dave Jiang
  2026-07-22  1:07   ` Jonathan Cameron
  2026-07-21  1:59 ` [PATCH v4 4/4] cxl/hdm: Restore commit_end when decoder enumeration fails Alison Schofield
  2026-07-21  9:41 ` [PATCH v4 0/4] Harden HDM decoder enumeration Richard Cheng
  4 siblings, 2 replies; 15+ messages in thread
From: Alison Schofield @ 2026-07-21  1:59 UTC (permalink / raw)
  To: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Alison Schofield,
	Vishal Verma, Ira Weiny, Dan Williams, Li Ming
  Cc: linux-cxl, Srirangan Madhavan, Richard Cheng

Switch decoder target IDs are stored one per byte, starting with
target 0 in the least significant byte. The current code accesses
those bytes in memory order, which only matches register order on
little-endian hosts.

On big-endian hosts, the target IDs are reversed and the decoder is
associated with the wrong downstream ports.

Extract each target ID from the register value with an explicit shift
and mask so the target mapping is independent of host endianness.

Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
Reviewed-by: Richard Cheng <icheng@nvidia.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/hdm.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 1f995191baf5..077aface1a2a 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -976,14 +976,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 {
 	struct cxl_endpoint_decoder *cxled = NULL;
 	u64 size, base, skip, dpa_size, lo, hi;
+	u64 target_list;
 	bool committed;
 	u32 remainder;
 	int i, rc;
 	u32 ctrl;
-	union {
-		u64 value;
-		unsigned char target_id[8];
-	} target_list;
 
 	if (should_emulate_decoders(info))
 		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, dpa_base,
@@ -1122,9 +1119,9 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 
 		lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
 		hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));
-		target_list.value = (hi << 32) + lo;
+		target_list = (hi << 32) + lo;
 		for (i = 0; i < cxld->interleave_ways; i++)
-			cxld->target_map[i] = target_list.target_id[i];
+			cxld->target_map[i] = (target_list >> (i * 8)) & 0xff;
 
 		return 0;
 	}
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v4 4/4] cxl/hdm: Restore commit_end when decoder enumeration fails
  2026-07-21  1:59 [PATCH v4 0/4] Harden HDM decoder enumeration Alison Schofield
                   ` (2 preceding siblings ...)
  2026-07-21  1:59 ` [PATCH v4 3/4] cxl/hdm: Make switch decoder target parsing endian-safe Alison Schofield
@ 2026-07-21  1:59 ` Alison Schofield
  2026-07-21  6:28   ` Li Ming
                     ` (2 more replies)
  2026-07-21  9:41 ` [PATCH v4 0/4] Harden HDM decoder enumeration Richard Cheng
  4 siblings, 3 replies; 15+ messages in thread
From: Alison Schofield @ 2026-07-21  1:59 UTC (permalink / raw)
  To: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Alison Schofield,
	Vishal Verma, Ira Weiny, Dan Williams, Li Ming
  Cc: linux-cxl, Srirangan Madhavan

commit_end tracks the highest committed decoder on a port. It gets
advanced before decoder enumeration is complete, so a later failure
leaves it pointing at a decoder that was never added. The next probe
then rejects decoder0 as out of order and enumeration fails.

Reset commit_end to the none committed baseline once the decoders are
torn down, so a subsequent probe rebuilds it from scratch. Register
the reset before the decoders are added so that on unwind it runs
after every decoder has been unregistered. This prevents commit_end
from dropping below the highest committed decoder still registered and
exposing an inconsistent value to a concurrent reader.

Protect the commit_end advance in the DVSEC emulation path against
concurrent readers, matching the register-programmed path.

Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
Fixes: b777e9bec960 ("cxl/hdm: Emulate HDM decoder from DVSEC range registers")
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---
 drivers/cxl/core/hdm.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 077aface1a2a..7d4ba7fa30fa 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -955,7 +955,8 @@ static int cxl_setup_hdm_decoder_from_dvsec(
 	 * change the range registers at run time.
 	 */
 	cxld->flags |= CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
-	port->commit_end = cxld->id;
+	scoped_guard(rwsem_write, &cxl_rwsem.region)
+		port->commit_end = cxld->id;
 
 	rc = devm_cxl_dpa_reserve(cxled, *dpa_base, len, 0);
 	if (rc) {
@@ -1181,6 +1182,14 @@ static void cxl_settle_decoders(struct cxl_hdm *cxlhdm)
 		msleep(20);
 }
 
+static void cxl_reset_commit_end(void *data)
+{
+	struct cxl_port *port = data;
+
+	guard(rwsem_write)(&cxl_rwsem.region);
+	port->commit_end = -1;
+}
+
 /**
  * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
  * @cxlhdm: Structure to populate with HDM capabilities
@@ -1191,13 +1200,21 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 {
 	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
 	struct cxl_port *port = cxlhdm->port;
-	int i;
 	u64 dpa_base = 0;
+	int i, rc;
 
 	cxl_settle_decoders(cxlhdm);
 
+	/*
+	 * Reset commit_end after all decoders have been torn down so a
+	 * subsequent probe rebuilds it from scratch.
+	 */
+	rc = devm_add_action(&port->dev, cxl_reset_commit_end, port);
+	if (rc)
+		return rc;
+
 	for (i = 0; i < cxlhdm->decoder_count; i++) {
-		int rc, target_count = cxlhdm->target_count;
+		int target_count = cxlhdm->target_count;
 		struct cxl_decoder *cxld;
 
 		if (is_cxl_endpoint(port)) {
-- 
2.37.3


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/4] cxl/hdm: Reject switch decoder interleave ways that overflow targets
  2026-07-21  1:59 ` [PATCH v4 1/4] cxl/hdm: Reject switch decoder interleave ways that overflow targets Alison Schofield
@ 2026-07-21  2:14   ` sashiko-bot
  2026-07-21  6:27   ` Li Ming
  2026-07-22  0:55   ` Jonathan Cameron
  2 siblings, 0 replies; 15+ messages in thread
From: sashiko-bot @ 2026-07-21  2:14 UTC (permalink / raw)
  To: Alison Schofield; +Cc: linux-cxl

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The patch fails to reset `interleave_ways` for uncommitted endpoint decoders, contradicting its stated intent.

Pre-existing issues:
- [High] Reading the switch decoder target list via a union results in incorrect target IDs on big-endian architectures.
- [High] Unconditional parsing of interleave properties aborts initialization for uncommitted decoders with uninitialized registers.
--

--- Patch [1]: [PATCH v4 1/4] cxl/hdm: Reject switch decoder interleave ways that overflow targets ---
commit 6b69d832bd669e734938484bbf51cd067e822152
Author: Alison Schofield <alison.schofield@intel.com>

cxl/hdm: Reject switch decoder interleave ways that overflow targets

This commit validates that a switch decoder's interleave ways fit
within available targets and the hardware capacity. Uncommitted
decoders have their interleave ways ignored and reset to one,
and decoders reporting zero targets are rejected.

Link: https://sashiko.dev/#/patchset/20260711180755.1779002-1-alison.schofield@intel.com?part=1

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 0c80b76a5f9b4..d81df45d8005d 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c

[ ... ]

> @@ -1084,6 +1086,37 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  		cxld->interleave_ways, cxld->interleave_granularity);

[Severity: High]
This is a pre-existing issue, but does unconditional parsing of interleave
properties abort initialization for uncommitted decoders?

If the control register of an uncommitted decoder contains an invalid
interleave value, eiw_to_ways() or eig_to_granularity() returns -EINVAL.
This fails init_hdm_decoder() and aborts port enumeration before we can
check if the decoder is committed.

>  	if (!cxled) {
> +		struct cxl_switch_decoder *cxlsd =
> +			to_cxl_switch_decoder(&cxld->dev);
> +
> +		if (cxlsd->nr_targets < 1) {
> +			dev_err(&port->dev,
> +				"decoder%d.%d: reports zero targets\n",
> +				port->id, cxld->id);
> +			return -ENXIO;
> +		}
> +
> +		if (!committed) {
> +			/* Ignore interleave ways until commit */
> +			cxld->interleave_ways = 1;
> +			return 0;
> +		}

[Severity: Medium]
Since this reset logic is inside the if (!cxled) block, does it incorrectly
skip endpoint decoders?

For endpoint decoders, cxled is non-NULL, so the execution bypasses the
if (!cxled) block and won't reset the value. This seems to contradict the
commit message's intent to reset interleave ways for uncommitted decoders.

> +
> +		if (cxld->interleave_ways > CXL_HDM_DECODER0_TL_TARGETS) {
> +			dev_err(&port->dev,
> +				"decoder%d.%d: interleave ways: %d exceeds target list capacity: %d\n",
> +				port->id, cxld->id, cxld->interleave_ways,
> +				CXL_HDM_DECODER0_TL_TARGETS);
> +			return -ENXIO;
> +		}
> +		if (cxld->interleave_ways > cxlsd->nr_targets) {
> +			dev_err(&port->dev,
> +				"decoder%d.%d: interleave ways: %d exceeds targets: %d\n",
> +				port->id, cxld->id, cxld->interleave_ways,
> +				cxlsd->nr_targets);
> +			return -ENXIO;
> +		}
> +
>  		lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
>  		hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));
>  		target_list.value = (hi << 32) + lo;

[Severity: High]
This is a pre-existing issue, but does reading the switch decoder target
list via a union result in incorrect target IDs on big-endian architectures?

The logic combines 32-bit CPU-endian integers into a 64-bit CPU-endian value:

    target_list.value = (hi << 32) + lo;

On big-endian systems, the highest byte of this integer is stored at index 0.
A subsequent loop reading target_id[0] and assigning it to target_map[0]
would entirely reverse the CXL target map.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784595445.git.alison.schofield@intel.com?part=1

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 2/4] cxl/hdm: Only interpret interleave settings on committed decoders
  2026-07-21  1:59 ` [PATCH v4 2/4] cxl/hdm: Only interpret interleave settings on committed decoders Alison Schofield
@ 2026-07-21  6:26   ` Li Ming
  0 siblings, 0 replies; 15+ messages in thread
From: Li Ming @ 2026-07-21  6:26 UTC (permalink / raw)
  To: Alison Schofield
  Cc: linux-cxl, Srirangan Madhavan, Ira Weiny, Dan Williams,
	Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma


在 2026/7/21 09:59, Alison Schofield 写道:
> Decoder enumeration reads the interleave ways and granularity fields
> from the CTRL register unconditionally, then decodes and validates
> them for every decoder. On an uncommitted decoder these fields are
> stale. They hold whatever was last latched and are reprogrammed when
> the decoder is committed. Interpreting them serves no purpose, and
> rejecting enumeration on a reserved encoding in those stale bits can
> fail an otherwise usable decoder.
>
> Decode and validate interleave ways and granularity only when the
> decoder is committed. On an uncommitted decoder the stale register
> values are not interpreted. The interleave fields are set when the
> decoder is programmed at commit time.
>
>
> Reported-by: Li Ming <ming.li@zohomail.com>
> Closes: https://lore.kernel.org/all/171d58db-3555-4a79-a9f9-8a3c2e14d91d@zohomail.com/
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>   drivers/cxl/core/hdm.c | 33 ++++++++++++++++++---------------
>   1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index d81df45d8005..1f995191baf5 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -1064,21 +1064,24 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>   			writel(ctrl, hdm + CXL_HDM_DECODER0_CTRL_OFFSET(which));
>   		}
>   	}
> -	rc = eiw_to_ways(FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl),
> -			  &cxld->interleave_ways);
> -	if (rc) {
> -		dev_warn(&port->dev,
> -			 "decoder%d.%d: Invalid interleave ways (ctrl: %#x)\n",
> -			 port->id, cxld->id, ctrl);
> -		return rc;
> -	}
> -	rc = eig_to_granularity(FIELD_GET(CXL_HDM_DECODER0_CTRL_IG_MASK, ctrl),
> -				 &cxld->interleave_granularity);
> -	if (rc) {
> -		dev_warn(&port->dev,
> -			 "decoder%d.%d: Invalid interleave granularity (ctrl: %#x)\n",
> -			 port->id, cxld->id, ctrl);
> -		return rc;
> +	/* Interleave settings are only valid on a committed decoder */
> +	if (committed) {
> +		rc = eiw_to_ways(FIELD_GET(CXL_HDM_DECODER0_CTRL_IW_MASK, ctrl),
> +				 &cxld->interleave_ways);
> +		if (rc) {
> +			dev_warn(&port->dev,
> +				 "decoder%d.%d: Invalid interleave ways (ctrl: %#x)\n",
> +				 port->id, cxld->id, ctrl);
> +			return rc;
> +		}
> +		rc = eig_to_granularity(FIELD_GET(CXL_HDM_DECODER0_CTRL_IG_MASK, ctrl),
> +					&cxld->interleave_granularity);
> +		if (rc) {
> +			dev_warn(&port->dev,
> +				 "decoder%d.%d: Invalid interleave granularity (ctrl: %#x)\n",
> +				 port->id, cxld->id, ctrl);
> +			return rc;
> +		}

There is a "if (committed) {}" block before these changes, maybe we can 
combine them into one.

Besides, there is a piece of codes related to a committed decoder 
handling in the end of init_hdm_decoder(), maybe we should put all 
committed decoder handling codes into one "if {}" block?

>   	}
>   
>   	dev_dbg(&port->dev, "decoder%d.%d: range: %#llx-%#llx iw: %d ig: %d\n",

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/4] cxl/hdm: Reject switch decoder interleave ways that overflow targets
  2026-07-21  1:59 ` [PATCH v4 1/4] cxl/hdm: Reject switch decoder interleave ways that overflow targets Alison Schofield
  2026-07-21  2:14   ` sashiko-bot
@ 2026-07-21  6:27   ` Li Ming
  2026-07-22  0:55   ` Jonathan Cameron
  2 siblings, 0 replies; 15+ messages in thread
From: Li Ming @ 2026-07-21  6:27 UTC (permalink / raw)
  To: Alison Schofield
  Cc: linux-cxl, Srirangan Madhavan, Dan Williams, Ira Weiny,
	Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma


在 2026/7/21 09:59, Alison Schofield 写道:
> Switch decoder enumeration validates that the interleave ways encoding
> is legal, but not that the resulting number of ways fits the available
> targets. This can overrun the target arrays during enumeration.
>
> Reject committed decoders whose interleave ways exceed either the
> hardware target list capacity or the reported target count. Reject
> switch decoders that report zero targets.
>
> For uncommitted decoders, ignore the stale interleave ways value and
> reset it to one until the decoder is committed.
>
> Add a clarifying comment that target_count is a direct count, not
> 0-based like decoder_count.
>
> Link: https://sashiko.dev/#/patchset/20260711180755.1779002-1-alison.schofield@intel.com?part=1
> Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
>   drivers/cxl/core/hdm.c | 33 +++++++++++++++++++++++++++++++++
>   drivers/cxl/cxl.h      |  2 ++
>   2 files changed, 35 insertions(+)
>
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 0c80b76a5f9b..d81df45d8005 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -76,6 +76,8 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>   
>   	hdm_cap = readl(cxlhdm->regs.hdm_decoder + CXL_HDM_DECODER_CAP_OFFSET);
>   	cxlhdm->decoder_count = cxl_hdm_decoder_count(hdm_cap);
> +
> +	/* target_count is a direct count (1h..8h), not 0-based like decoder_count */
>   	cxlhdm->target_count =
>   		FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, hdm_cap);
>   	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap))
> @@ -1084,6 +1086,37 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>   		cxld->interleave_ways, cxld->interleave_granularity);
>   
>   	if (!cxled) {
> +		struct cxl_switch_decoder *cxlsd =
> +			to_cxl_switch_decoder(&cxld->dev);
> +
> +		if (cxlsd->nr_targets < 1) {
> +			dev_err(&port->dev,
> +				"decoder%d.%d: reports zero targets\n",
> +				port->id, cxld->id);
> +			return -ENXIO;
> +		}
I noticed cxl_switch_decoder_init() has a check for the value of given 
nr_targets before setting cxlsd->nr_targets, maybe we can move this 
check into cxl_switch_decoder_init()?
> +
> +		if (!committed) {
> +			/* Ignore interleave ways until commit */
> +			cxld->interleave_ways = 1;
> +			return 0;
> +		}
The following part is related to a committed decoder( including 
target_map array updating), like my comment in patch #2, maybe it is 
worth a cleanup.
> +
> +		if (cxld->interleave_ways > CXL_HDM_DECODER0_TL_TARGETS) {
> +			dev_err(&port->dev,
> +				"decoder%d.%d: interleave ways: %d exceeds target list capacity: %d\n",
> +				port->id, cxld->id, cxld->interleave_ways,
> +				CXL_HDM_DECODER0_TL_TARGETS);
> +			return -ENXIO;
> +		}
> +		if (cxld->interleave_ways > cxlsd->nr_targets) {
> +			dev_err(&port->dev,
> +				"decoder%d.%d: interleave ways: %d exceeds targets: %d\n",
> +				port->id, cxld->id, cxld->interleave_ways,
> +				cxlsd->nr_targets);
> +			return -ENXIO;
> +		}
> +
>   		lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
>   		hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));
>   		target_list.value = (hi << 32) + lo;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index c0e5308e4d1b..291ada46b646 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -67,6 +67,8 @@ extern const struct nvdimm_security_ops *cxl_security_ops;
>   #define   CXL_HDM_DECODER0_CTRL_HOSTONLY BIT(12)
>   #define CXL_HDM_DECODER0_TL_LOW(i) (0x20 * (i) + 0x24)
>   #define CXL_HDM_DECODER0_TL_HIGH(i) (0x20 * (i) + 0x28)
> +/* Two registers with one target ID per byte */
> +#define CXL_HDM_DECODER0_TL_TARGETS 8
>   #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i)
>   #define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i)
>   

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 4/4] cxl/hdm: Restore commit_end when decoder enumeration fails
  2026-07-21  1:59 ` [PATCH v4 4/4] cxl/hdm: Restore commit_end when decoder enumeration fails Alison Schofield
@ 2026-07-21  6:28   ` Li Ming
  2026-07-21 15:12   ` Dave Jiang
  2026-07-22  1:17   ` Jonathan Cameron
  2 siblings, 0 replies; 15+ messages in thread
From: Li Ming @ 2026-07-21  6:28 UTC (permalink / raw)
  To: Alison Schofield
  Cc: linux-cxl, Srirangan Madhavan, Ira Weiny, Davidlohr Bueso,
	Jonathan Cameron, Dave Jiang, Vishal Verma, Dan Williams


在 2026/7/21 09:59, Alison Schofield 写道:
> commit_end tracks the highest committed decoder on a port. It gets
> advanced before decoder enumeration is complete, so a later failure
> leaves it pointing at a decoder that was never added. The next probe
> then rejects decoder0 as out of order and enumeration fails.
>
> Reset commit_end to the none committed baseline once the decoders are
> torn down, so a subsequent probe rebuilds it from scratch. Register
> the reset before the decoders are added so that on unwind it runs
> after every decoder has been unregistered. This prevents commit_end
> from dropping below the highest committed decoder still registered and
> exposing an inconsistent value to a concurrent reader.
>
> Protect the commit_end advance in the DVSEC emulation path against
> concurrent readers, matching the register-programmed path.
>
> Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
> Fixes: b777e9bec960 ("cxl/hdm: Emulate HDM decoder from DVSEC range registers")
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Li Ming <ming.li@zohomail.com>
> ---
>   drivers/cxl/core/hdm.c | 23 ++++++++++++++++++++---
>   1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 077aface1a2a..7d4ba7fa30fa 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -955,7 +955,8 @@ static int cxl_setup_hdm_decoder_from_dvsec(
>   	 * change the range registers at run time.
>   	 */
>   	cxld->flags |= CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
> -	port->commit_end = cxld->id;
> +	scoped_guard(rwsem_write, &cxl_rwsem.region)
> +		port->commit_end = cxld->id;
>   
>   	rc = devm_cxl_dpa_reserve(cxled, *dpa_base, len, 0);
>   	if (rc) {
> @@ -1181,6 +1182,14 @@ static void cxl_settle_decoders(struct cxl_hdm *cxlhdm)
>   		msleep(20);
>   }
>   
> +static void cxl_reset_commit_end(void *data)
> +{
> +	struct cxl_port *port = data;
> +
> +	guard(rwsem_write)(&cxl_rwsem.region);
> +	port->commit_end = -1;
> +}
> +
>   /**
>    * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
>    * @cxlhdm: Structure to populate with HDM capabilities
> @@ -1191,13 +1200,21 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>   {
>   	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
>   	struct cxl_port *port = cxlhdm->port;
> -	int i;
>   	u64 dpa_base = 0;
> +	int i, rc;
>   
>   	cxl_settle_decoders(cxlhdm);
>   
> +	/*
> +	 * Reset commit_end after all decoders have been torn down so a
> +	 * subsequent probe rebuilds it from scratch.
> +	 */
> +	rc = devm_add_action(&port->dev, cxl_reset_commit_end, port);
> +	if (rc)
> +		return rc;
> +
>   	for (i = 0; i < cxlhdm->decoder_count; i++) {
> -		int rc, target_count = cxlhdm->target_count;
> +		int target_count = cxlhdm->target_count;
>   		struct cxl_decoder *cxld;
>   
>   		if (is_cxl_endpoint(port)) {

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 0/4] Harden HDM decoder enumeration
  2026-07-21  1:59 [PATCH v4 0/4] Harden HDM decoder enumeration Alison Schofield
                   ` (3 preceding siblings ...)
  2026-07-21  1:59 ` [PATCH v4 4/4] cxl/hdm: Restore commit_end when decoder enumeration fails Alison Schofield
@ 2026-07-21  9:41 ` Richard Cheng
  4 siblings, 0 replies; 15+ messages in thread
From: Richard Cheng @ 2026-07-21  9:41 UTC (permalink / raw)
  To: Alison Schofield
  Cc: Davidlohr Bueso, Jonathan Cameron, Dave Jiang, Vishal Verma,
	Ira Weiny, Dan Williams, Li Ming, linux-cxl, Srirangan Madhavan

On Mon, Jul 20, 2026 at 06:59:36PM +0800, Alison Schofield wrote:
> Changes in v4:
> A new patch was inserted as Patch2, so now set is 4 patches.
>    Sashiko is likely to complain in Patch 1 for something fixed in
>    Patches 2 and 3, but reordering does not makes sense.
> P2: Only interpret interleave settings on committed decoders (Ming)
>     This is the newly inserted patch
> P4: Register reset w devm_add_action (Ming)
> P4: Fix up a kernel-doc comment issue (lkp)
> 
> Changes in v3:
> P1: Move zero-target reject to init_hdm_decoder() (RichardC)
> P3: Reset commit_end via a devres action after decoder teardown (RichardC)
> 
> Changes in v2:
> P1: Handle uncommitted decoders (Sashiko)
> P1: Reject switch decoders with zero targets (Sashiko)
> P1: Add in code comment clarifying target_count (Sashiko)
> P1: Update commit log
> P3: Reset commit_end on the failure path only (Sashiko)
> P3: Update commit log
> No changes in P2
> 
> 
> This set started as Sashiko reported cleanups, and at v4, Patch 2/4
> was added based on Li Ming's review and catch.
> 
> Only the first includes a link to the original Sashiko complaint, since
> the others did not originally appear on Lore for me to add public links. 
> 
> Srirangan: I believe patches 1 and 3 of these also address pre-existing
> issues Sashiko posted in reply to your reset series, Patch 02/11.
> 
> 
> Alison Schofield (4):
>   cxl/hdm: Reject switch decoder interleave ways that overflow targets
>   cxl/hdm: Only interpret interleave settings on committed decoders
>   cxl/hdm: Make switch decoder target parsing endian-safe
>   cxl/hdm: Restore commit_end when decoder enumeration fails
> 
>  drivers/cxl/core/hdm.c | 98 +++++++++++++++++++++++++++++++-----------
>  drivers/cxl/cxl.h      |  2 +
>  2 files changed, 76 insertions(+), 24 deletions(-)
> 
> 
> base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
> -- 
> 2.37.3
> 
>

Reviewed-by: Richard Cheng <icheng@nvidia.com>
 

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 3/4] cxl/hdm: Make switch decoder target parsing endian-safe
  2026-07-21  1:59 ` [PATCH v4 3/4] cxl/hdm: Make switch decoder target parsing endian-safe Alison Schofield
@ 2026-07-21 15:09   ` Dave Jiang
  2026-07-22  1:07   ` Jonathan Cameron
  1 sibling, 0 replies; 15+ messages in thread
From: Dave Jiang @ 2026-07-21 15:09 UTC (permalink / raw)
  To: Alison Schofield, Davidlohr Bueso, Jonathan Cameron, Vishal Verma,
	Ira Weiny, Dan Williams, Li Ming
  Cc: linux-cxl, Srirangan Madhavan, Richard Cheng



On 7/20/26 6:59 PM, Alison Schofield wrote:
> Switch decoder target IDs are stored one per byte, starting with
> target 0 in the least significant byte. The current code accesses
> those bytes in memory order, which only matches register order on
> little-endian hosts.
> 
> On big-endian hosts, the target IDs are reversed and the decoder is
> associated with the wrong downstream ports.
> 
> Extract each target ID from the register value with an explicit shift
> and mask so the target mapping is independent of host endianness.
> 
> Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
> Reviewed-by: Richard Cheng <icheng@nvidia.com>
> Reviewed-by: Li Ming <ming.li@zohomail.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

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


> ---
>  drivers/cxl/core/hdm.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 1f995191baf5..077aface1a2a 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -976,14 +976,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  {
>  	struct cxl_endpoint_decoder *cxled = NULL;
>  	u64 size, base, skip, dpa_size, lo, hi;
> +	u64 target_list;
>  	bool committed;
>  	u32 remainder;
>  	int i, rc;
>  	u32 ctrl;
> -	union {
> -		u64 value;
> -		unsigned char target_id[8];
> -	} target_list;
>  
>  	if (should_emulate_decoders(info))
>  		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, dpa_base,
> @@ -1122,9 +1119,9 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  
>  		lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
>  		hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));
> -		target_list.value = (hi << 32) + lo;
> +		target_list = (hi << 32) + lo;
>  		for (i = 0; i < cxld->interleave_ways; i++)
> -			cxld->target_map[i] = target_list.target_id[i];
> +			cxld->target_map[i] = (target_list >> (i * 8)) & 0xff;
>  
>  		return 0;
>  	}


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 4/4] cxl/hdm: Restore commit_end when decoder enumeration fails
  2026-07-21  1:59 ` [PATCH v4 4/4] cxl/hdm: Restore commit_end when decoder enumeration fails Alison Schofield
  2026-07-21  6:28   ` Li Ming
@ 2026-07-21 15:12   ` Dave Jiang
  2026-07-22  1:17   ` Jonathan Cameron
  2 siblings, 0 replies; 15+ messages in thread
From: Dave Jiang @ 2026-07-21 15:12 UTC (permalink / raw)
  To: Alison Schofield, Davidlohr Bueso, Jonathan Cameron, Vishal Verma,
	Ira Weiny, Dan Williams, Li Ming
  Cc: linux-cxl, Srirangan Madhavan



On 7/20/26 6:59 PM, Alison Schofield wrote:
> commit_end tracks the highest committed decoder on a port. It gets
> advanced before decoder enumeration is complete, so a later failure
> leaves it pointing at a decoder that was never added. The next probe
> then rejects decoder0 as out of order and enumeration fails.
> 
> Reset commit_end to the none committed baseline once the decoders are

s/none committed/uncommitted/ ?

> torn down, so a subsequent probe rebuilds it from scratch. Register
> the reset before the decoders are added so that on unwind it runs
> after every decoder has been unregistered. This prevents commit_end
> from dropping below the highest committed decoder still registered and
> exposing an inconsistent value to a concurrent reader.
> 
> Protect the commit_end advance in the DVSEC emulation path against
> concurrent readers, matching the register-programmed path.
> 
> Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
> Fixes: b777e9bec960 ("cxl/hdm: Emulate HDM decoder from DVSEC range registers")
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

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


> ---
>  drivers/cxl/core/hdm.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 077aface1a2a..7d4ba7fa30fa 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -955,7 +955,8 @@ static int cxl_setup_hdm_decoder_from_dvsec(
>  	 * change the range registers at run time.
>  	 */
>  	cxld->flags |= CXL_DECODER_F_ENABLE | CXL_DECODER_F_LOCK;
> -	port->commit_end = cxld->id;
> +	scoped_guard(rwsem_write, &cxl_rwsem.region)
> +		port->commit_end = cxld->id;
>  
>  	rc = devm_cxl_dpa_reserve(cxled, *dpa_base, len, 0);
>  	if (rc) {
> @@ -1181,6 +1182,14 @@ static void cxl_settle_decoders(struct cxl_hdm *cxlhdm)
>  		msleep(20);
>  }
>  
> +static void cxl_reset_commit_end(void *data)
> +{
> +	struct cxl_port *port = data;
> +
> +	guard(rwsem_write)(&cxl_rwsem.region);
> +	port->commit_end = -1;
> +}
> +
>  /**
>   * devm_cxl_enumerate_decoders - add decoder objects per HDM register set
>   * @cxlhdm: Structure to populate with HDM capabilities
> @@ -1191,13 +1200,21 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>  {
>  	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
>  	struct cxl_port *port = cxlhdm->port;
> -	int i;
>  	u64 dpa_base = 0;
> +	int i, rc;
>  
>  	cxl_settle_decoders(cxlhdm);
>  
> +	/*
> +	 * Reset commit_end after all decoders have been torn down so a
> +	 * subsequent probe rebuilds it from scratch.
> +	 */
> +	rc = devm_add_action(&port->dev, cxl_reset_commit_end, port);
> +	if (rc)
> +		return rc;
> +
>  	for (i = 0; i < cxlhdm->decoder_count; i++) {
> -		int rc, target_count = cxlhdm->target_count;
> +		int target_count = cxlhdm->target_count;
>  		struct cxl_decoder *cxld;
>  
>  		if (is_cxl_endpoint(port)) {


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 1/4] cxl/hdm: Reject switch decoder interleave ways that overflow targets
  2026-07-21  1:59 ` [PATCH v4 1/4] cxl/hdm: Reject switch decoder interleave ways that overflow targets Alison Schofield
  2026-07-21  2:14   ` sashiko-bot
  2026-07-21  6:27   ` Li Ming
@ 2026-07-22  0:55   ` Jonathan Cameron
  2 siblings, 0 replies; 15+ messages in thread
From: Jonathan Cameron @ 2026-07-22  0:55 UTC (permalink / raw)
  To: Alison Schofield
  Cc: Davidlohr Bueso, Dave Jiang, Vishal Verma, Ira Weiny,
	Dan Williams, Li Ming, linux-cxl, Srirangan Madhavan

On Mon, 20 Jul 2026 18:59:37 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> Switch decoder enumeration validates that the interleave ways encoding
> is legal, but not that the resulting number of ways fits the available
> targets. This can overrun the target arrays during enumeration.
> 
> Reject committed decoders whose interleave ways exceed either the
> hardware target list capacity or the reported target count. Reject
> switch decoders that report zero targets.
> 
> For uncommitted decoders, ignore the stale interleave ways value and
> reset it to one until the decoder is committed.
> 
> Add a clarifying comment that target_count is a direct count, not
> 0-based like decoder_count.
> 
> Link: https://sashiko.dev/#/patchset/20260711180755.1779002-1-alison.schofield@intel.com?part=1
> Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

oops. I replied to v3 after you sent this out. Please take a look.

Thanks,

Jonathan

> ---
>  drivers/cxl/core/hdm.c | 33 +++++++++++++++++++++++++++++++++
>  drivers/cxl/cxl.h      |  2 ++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 0c80b76a5f9b..d81df45d8005 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -76,6 +76,8 @@ static void parse_hdm_decoder_caps(struct cxl_hdm *cxlhdm)
>  
>  	hdm_cap = readl(cxlhdm->regs.hdm_decoder + CXL_HDM_DECODER_CAP_OFFSET);
>  	cxlhdm->decoder_count = cxl_hdm_decoder_count(hdm_cap);
> +
> +	/* target_count is a direct count (1h..8h), not 0-based like decoder_count */
>  	cxlhdm->target_count =
>  		FIELD_GET(CXL_HDM_DECODER_TARGET_COUNT_MASK, hdm_cap);
>  	if (FIELD_GET(CXL_HDM_DECODER_INTERLEAVE_11_8, hdm_cap))
> @@ -1084,6 +1086,37 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  		cxld->interleave_ways, cxld->interleave_granularity);
>  
>  	if (!cxled) {
> +		struct cxl_switch_decoder *cxlsd =
> +			to_cxl_switch_decoder(&cxld->dev);
> +
> +		if (cxlsd->nr_targets < 1) {
> +			dev_err(&port->dev,
> +				"decoder%d.%d: reports zero targets\n",
> +				port->id, cxld->id);
> +			return -ENXIO;
> +		}
> +
> +		if (!committed) {
> +			/* Ignore interleave ways until commit */
> +			cxld->interleave_ways = 1;
> +			return 0;
> +		}
> +
> +		if (cxld->interleave_ways > CXL_HDM_DECODER0_TL_TARGETS) {
> +			dev_err(&port->dev,
> +				"decoder%d.%d: interleave ways: %d exceeds target list capacity: %d\n",
> +				port->id, cxld->id, cxld->interleave_ways,
> +				CXL_HDM_DECODER0_TL_TARGETS);
> +			return -ENXIO;
> +		}
> +		if (cxld->interleave_ways > cxlsd->nr_targets) {
> +			dev_err(&port->dev,
> +				"decoder%d.%d: interleave ways: %d exceeds targets: %d\n",
> +				port->id, cxld->id, cxld->interleave_ways,
> +				cxlsd->nr_targets);
> +			return -ENXIO;
> +		}
> +
>  		lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
>  		hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));
>  		target_list.value = (hi << 32) + lo;
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index c0e5308e4d1b..291ada46b646 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -67,6 +67,8 @@ extern const struct nvdimm_security_ops *cxl_security_ops;
>  #define   CXL_HDM_DECODER0_CTRL_HOSTONLY BIT(12)
>  #define CXL_HDM_DECODER0_TL_LOW(i) (0x20 * (i) + 0x24)
>  #define CXL_HDM_DECODER0_TL_HIGH(i) (0x20 * (i) + 0x28)
> +/* Two registers with one target ID per byte */
> +#define CXL_HDM_DECODER0_TL_TARGETS 8
>  #define CXL_HDM_DECODER0_SKIP_LOW(i) CXL_HDM_DECODER0_TL_LOW(i)
>  #define CXL_HDM_DECODER0_SKIP_HIGH(i) CXL_HDM_DECODER0_TL_HIGH(i)
>  


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 3/4] cxl/hdm: Make switch decoder target parsing endian-safe
  2026-07-21  1:59 ` [PATCH v4 3/4] cxl/hdm: Make switch decoder target parsing endian-safe Alison Schofield
  2026-07-21 15:09   ` Dave Jiang
@ 2026-07-22  1:07   ` Jonathan Cameron
  1 sibling, 0 replies; 15+ messages in thread
From: Jonathan Cameron @ 2026-07-22  1:07 UTC (permalink / raw)
  To: Alison Schofield
  Cc: Davidlohr Bueso, Dave Jiang, Vishal Verma, Ira Weiny,
	Dan Williams, Li Ming, linux-cxl, Srirangan Madhavan,
	Richard Cheng

On Mon, 20 Jul 2026 18:59:39 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> Switch decoder target IDs are stored one per byte, starting with
> target 0 in the least significant byte. The current code accesses
> those bytes in memory order, which only matches register order on
> little-endian hosts.
> 
> On big-endian hosts, the target IDs are reversed and the decoder is
> associated with the wrong downstream ports.
> 
> Extract each target ID from the register value with an explicit shift
> and mask so the target mapping is independent of host endianness.
> 
> Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
> Reviewed-by: Richard Cheng <icheng@nvidia.com>
> Reviewed-by: Li Ming <ming.li@zohomail.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Suggestion for explicitly handling the endian issue inline.
I'm also fine if you stick with what you have here.

Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

> ---
>  drivers/cxl/core/hdm.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 1f995191baf5..077aface1a2a 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -976,14 +976,11 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  {
>  	struct cxl_endpoint_decoder *cxled = NULL;
>  	u64 size, base, skip, dpa_size, lo, hi;
> +	u64 target_list;
>  	bool committed;
>  	u32 remainder;
>  	int i, rc;
>  	u32 ctrl;

Maybe alternative approach along the lines of:

> -	union {
> -		u64 value;
		struct {
			__le32 lo;
			__le32 hi;
		};
> -		unsigned char target_id[8];
> -	} target_list;
>  
>  	if (should_emulate_decoders(info))
>  		return cxl_setup_hdm_decoder_from_dvsec(port, cxld, dpa_base,
> @@ -1122,9 +1119,9 @@ static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  
>  		lo = readl(hdm + CXL_HDM_DECODER0_TL_LOW(which));
>  		hi = readl(hdm + CXL_HDM_DECODER0_TL_HIGH(which));

		target_list.lo = cpu_to_le32(readl());
		target_list.hi = cpu_to_le32(readl());

or just have a byte array and

		put_unaligned_le32(&target_id[0], readl());
		put_unaligned_le32(&target_id[4], readl());

> -		target_list.value = (hi << 32) + lo;
> +		target_list = (hi << 32) + lo;
>  		for (i = 0; i < cxld->interleave_ways; i++)
> -			cxld->target_map[i] = target_list.target_id[i];
> +			cxld->target_map[i] = (target_list >> (i * 8)) & 0xff;
>  
>  		return 0;
>  	}


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 4/4] cxl/hdm: Restore commit_end when decoder enumeration fails
  2026-07-21  1:59 ` [PATCH v4 4/4] cxl/hdm: Restore commit_end when decoder enumeration fails Alison Schofield
  2026-07-21  6:28   ` Li Ming
  2026-07-21 15:12   ` Dave Jiang
@ 2026-07-22  1:17   ` Jonathan Cameron
  2 siblings, 0 replies; 15+ messages in thread
From: Jonathan Cameron @ 2026-07-22  1:17 UTC (permalink / raw)
  To: Alison Schofield
  Cc: Davidlohr Bueso, Dave Jiang, Vishal Verma, Ira Weiny,
	Dan Williams, Li Ming, linux-cxl, Srirangan Madhavan

On Mon, 20 Jul 2026 18:59:40 -0700
Alison Schofield <alison.schofield@intel.com> wrote:

> commit_end tracks the highest committed decoder on a port. It gets
> advanced before decoder enumeration is complete, so a later failure
> leaves it pointing at a decoder that was never added. The next probe
> then rejects decoder0 as out of order and enumeration fails.
> 
> Reset commit_end to the none committed baseline once the decoders are
> torn down, so a subsequent probe rebuilds it from scratch. Register
> the reset before the decoders are added so that on unwind it runs
> after every decoder has been unregistered. This prevents commit_end
> from dropping below the highest committed decoder still registered and
> exposing an inconsistent value to a concurrent reader.
> 
> Protect the commit_end advance in the DVSEC emulation path against
> concurrent readers, matching the register-programmed path.
> 
> Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
> Fixes: b777e9bec960 ("cxl/hdm: Emulate HDM decoder from DVSEC range registers")
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2026-07-22  1:17 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-21  1:59 [PATCH v4 0/4] Harden HDM decoder enumeration Alison Schofield
2026-07-21  1:59 ` [PATCH v4 1/4] cxl/hdm: Reject switch decoder interleave ways that overflow targets Alison Schofield
2026-07-21  2:14   ` sashiko-bot
2026-07-21  6:27   ` Li Ming
2026-07-22  0:55   ` Jonathan Cameron
2026-07-21  1:59 ` [PATCH v4 2/4] cxl/hdm: Only interpret interleave settings on committed decoders Alison Schofield
2026-07-21  6:26   ` Li Ming
2026-07-21  1:59 ` [PATCH v4 3/4] cxl/hdm: Make switch decoder target parsing endian-safe Alison Schofield
2026-07-21 15:09   ` Dave Jiang
2026-07-22  1:07   ` Jonathan Cameron
2026-07-21  1:59 ` [PATCH v4 4/4] cxl/hdm: Restore commit_end when decoder enumeration fails Alison Schofield
2026-07-21  6:28   ` Li Ming
2026-07-21 15:12   ` Dave Jiang
2026-07-22  1:17   ` Jonathan Cameron
2026-07-21  9:41 ` [PATCH v4 0/4] Harden HDM decoder enumeration Richard Cheng

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.