[PATCH v2 0/3] Harden HDM decoder enumeration

[PATCH v2 0/3] Harden HDM decoder enumeration

[Alison Schofield]

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

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

Srirangan: I believe the first 2 of these also address pre-existing
issues Sashiko posted in reply to your reset series, Patch 02/11.


Alison Schofield (3):
  cxl/hdm: Reject switch decoder interleave ways that overflow targets
  cxl/hdm: Make switch decoder target parsing endian-safe
  cxl/hdm: Restore commit_end when decoder enumeration fails

 drivers/cxl/core/hdm.c  | 58 ++++++++++++++++++++++++++++++++---------
 drivers/cxl/core/port.c |  2 +-
 drivers/cxl/cxl.h       |  2 ++
 3 files changed, 48 insertions(+), 14 deletions(-)


base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
-- 
2.37.3

[PATCH v2 1/3] cxl/hdm: Reject switch decoder interleave ways that overflow targets

[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  | 26 ++++++++++++++++++++++++++
 drivers/cxl/core/port.c |  2 +-
 drivers/cxl/cxl.h       |  2 ++
 3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 0c80b76a5f9b..9f005f3193e2 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,30 @@ 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 (!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/core/port.c b/drivers/cxl/core/port.c
index 1215ee4f4035..28eccfdd75b8 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -1979,7 +1979,7 @@ static int cxl_switch_decoder_init(struct cxl_port *port,
 				   struct cxl_switch_decoder *cxlsd,
 				   int nr_targets)
 {
-	if (nr_targets > CXL_DECODER_MAX_INTERLEAVE)
+	if (nr_targets < 1 || nr_targets > CXL_DECODER_MAX_INTERLEAVE)
 		return -EINVAL;
 
 	cxlsd->nr_targets = nr_targets;
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

Re: [PATCH v2 1/3] cxl/hdm: Reject switch decoder interleave ways that overflow targets

[sashiko-bot]

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

Pre-existing issues:
- [High] Extracting target IDs from the target list register assumes little-endian byte ordering, causing incorrect target mapping on big-endian architectures.
- [High] Failure during decoder initialization leaves `port->commit_end` in a dirty state, permanently breaking subsequent device probe attempts.
--

--- Patch [1]: [PATCH v2 1/3] cxl/hdm: Reject switch decoder interleave ways that overflow targets ---
commit 4660ad9cd44e7b73b9a24a494a4fb73e7c2f2eaa
Author: Alison Schofield <alison.schofield@intel.com>

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

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.

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..9f005f3193e2f 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c

[ ... ]

> @@ -1084,6 +1086,30 @@ 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 (!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;
               ^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but will this early return leave
port->commit_end permanently modified?

Looking at init_hdm_decoder() earlier in the function, port->commit_end is
updated when the decoder is committed:

	if (committed) {
		...
		port->commit_end = cxld->id;
	}

If the initialization fails here due to exceeding hardware capacity, it returns
-ENXIO and aborts the port probe. However, port->commit_end is never reverted.

If the driver is re-probed, the next initialization attempt will falsely reject
the first decoder as 'Committed out of order'. This happens because
cxl_num_decoders_committed() uses the leaked commit_end state.

Should this error path revert port->commit_end before returning?

> +		}
> +
>  		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 extracting target IDs from the target
list register using a union byte array overlay cause incorrect target mapping
on big-endian architectures?

When init_hdm_decoder() reads hardware registers TL_LOW and TL_HIGH, it
converts them to native CPU endianness via readl() and stores them in a native
64-bit integer.

The function then extracts target IDs using target_list.target_id[i]. On
big-endian systems, byte 0 is the most significant byte of the 64-bit value,
rather than the least significant byte.

This could route CXL transactions to the wrong switch ports.

Does this logic need to be adjusted to avoid endianness issues?

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

Re: [PATCH v2 1/3] cxl/hdm: Reject switch decoder interleave ways that overflow targets

[Richard Cheng]

On Wed, Jul 15, 2026 at 02:45:47PM +0800, Alison Schofield 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>
> ---
>  drivers/cxl/core/hdm.c  | 26 ++++++++++++++++++++++++++
>  drivers/cxl/core/port.c |  2 +-
>  drivers/cxl/cxl.h       |  2 ++
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 0c80b76a5f9b..9f005f3193e2 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,30 @@ 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 (!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/core/port.c b/drivers/cxl/core/port.c
> index 1215ee4f4035..28eccfdd75b8 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1979,7 +1979,7 @@ static int cxl_switch_decoder_init(struct cxl_port *port,
>  				   struct cxl_switch_decoder *cxlsd,
>  				   int nr_targets)
>  {
> -	if (nr_targets > CXL_DECODER_MAX_INTERLEAVE)
> +	if (nr_targets < 1 || nr_targets > CXL_DECODER_MAX_INTERLEAVE)

This contradict to what the comment claims.
The comment says that Target Count is a direct cound with 1h to 8h. However,
this check uses CXL_DECODER_MAX_INTERLEAVE, which is 16.

I think since the capability field is 4 bits wide, values 9 to 15 are still
acceptable.

I'm confused by this part, it looks inconsistent, refusing enumeration above 9
doesn't make sense to me either.
Maybe some explanation to explain why ?

Best regards,
Richard Cheng.
>  		return -EINVAL;
>  
>  	cxlsd->nr_targets = nr_targets;
> 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
> 
> 

[PATCH v2 2/3] cxl/hdm: Make switch decoder target parsing endian-safe

[Alison Schofield]

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")
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 9f005f3193e2..51ab154a73fe 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,
@@ -1112,9 +1109,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

Re: [PATCH v2 2/3] cxl/hdm: Make switch decoder target parsing endian-safe

[Richard Cheng]

On Wed, Jul 15, 2026 at 02:45:48PM +0800, 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")
> 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 9f005f3193e2..51ab154a73fe 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,
> @@ -1112,9 +1109,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
> 
>

Hi Alison,

This is a good catch,

LGTM,

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

Best regards,
Richard Cheng. 

[PATCH v2 3/3] cxl/hdm: Restore commit_end when decoder enumeration fails

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

Restore commit_end to the none-committed baseline on the enumeration
failure path so a subsequent probe can enumerate decoders cleanly. The
successful path never touches commit_end, so no transient count is
exposed to concurrent readers.

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, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 51ab154a73fe..850f509432aa 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,13 +1182,13 @@ 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);
 
 	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)) {
@@ -1198,7 +1199,8 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 				dev_warn(&port->dev,
 					 "Failed to allocate decoder%d.%d\n",
 					 port->id, i);
-				return PTR_ERR(cxled);
+				rc = PTR_ERR(cxled);
+				goto err;
 			}
 			cxld = &cxled->cxld;
 		} else {
@@ -1209,7 +1211,8 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 				dev_warn(&port->dev,
 					 "Failed to allocate decoder%d.%d\n",
 					 port->id, i);
-				return PTR_ERR(cxlsd);
+				rc = PTR_ERR(cxlsd);
+				goto err;
 			}
 			cxld = &cxlsd->cxld;
 		}
@@ -1220,17 +1223,23 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
 				 "Failed to initialize decoder%d.%d\n",
 				 port->id, i);
 			put_device(&cxld->dev);
-			return rc;
+			goto err;
 		}
 		rc = add_hdm_decoder(port, cxld);
 		if (rc) {
 			dev_warn(&port->dev,
 				 "Failed to add decoder%d.%d\n", port->id, i);
-			return rc;
+			goto err;
 		}
 	}
 
 	return 0;
+
+err:
+	/* Reset to the none committed baseline for a fresh enumeration */
+	scoped_guard(rwsem_write, &cxl_rwsem.region)
+		port->commit_end = -1;
+	return rc;
 }
 
 /**
-- 
2.37.3

Re: [PATCH v2 3/3] cxl/hdm: Restore commit_end when decoder enumeration fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Mixing goto-based error handling with scope-based cleanup helpers (`scoped_guard()`) in the same function.
--

--- Patch [3]: [PATCH v2 3/3] cxl/hdm: Restore commit_end when decoder enumeration fails ---
commit fee00b887a5dc8f3c4c335d03db77d2ecce10947
Author: Alison Schofield <alison.schofield@intel.com>

cxl/hdm: Restore commit_end when decoder enumeration fails

This commit restores commit_end to the none-committed baseline on the
enumeration failure path so a subsequent probe can enumerate cleanly. It
also protects the commit_end advance against concurrent readers in the
DVSEC emulation path.

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c

[ ... ]

> @@ -1198,7 +1199,8 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>  				dev_warn(&port->dev,
>  					 "Failed to allocate decoder%d.%d\n",
>  					 port->id, i);
> -				return PTR_ERR(cxled);
> +				rc = PTR_ERR(cxled);
> +				goto err;

[ ... ]

> @@ -1220,17 +1223,23 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
>  				 "Failed to initialize decoder%d.%d\n",
>  				 port->id, i);
>  			put_device(&cxld->dev);
> -			return rc;
> +			goto err;
>  		}
>  		rc = add_hdm_decoder(port, cxld);
>  		if (rc) {
>  			dev_warn(&port->dev,
>  				 "Failed to add decoder%d.%d\n", port->id, i);
> -			return rc;
> +			goto err;
>  		}
>  	}
>  
>  	return 0;
> +
> +err:
> +	/* Reset to the none committed baseline for a fresh enumeration */
> +	scoped_guard(rwsem_write, &cxl_rwsem.region)
> +		port->commit_end = -1;
> +	return rc;
>  }

[Severity: Medium]
Does mixing goto-based error handling with scope-based cleanup helpers
(scoped_guard) in the same function violate the cleanup subsystem guidelines?

The documentation in include/linux/cleanup.h notes that usage of "goto"
and cleanup helpers should never be mixed in the same function, because it
creates confusing ownership semantics and can lead to bugs under future
modifications.

Should devm_cxl_enumerate_decoders() either be converted entirely to
scope-based cleanup, or avoid using scoped_guard() in the error path?

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

Re: [PATCH v2 3/3] cxl/hdm: Restore commit_end when decoder enumeration fails

[Richard Cheng]

On Wed, Jul 15, 2026 at 09:57:06PM +0800, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Mixing goto-based error handling with scope-based cleanup helpers (`scoped_guard()`) in the same function.
> --
> 
> --- Patch [3]: [PATCH v2 3/3] cxl/hdm: Restore commit_end when decoder enumeration fails ---
> commit fee00b887a5dc8f3c4c335d03db77d2ecce10947
> Author: Alison Schofield <alison.schofield@intel.com>
> 
> cxl/hdm: Restore commit_end when decoder enumeration fails
> 
> This commit restores commit_end to the none-committed baseline on the
> enumeration failure path so a subsequent probe can enumerate cleanly. It
> also protects the commit_end advance against concurrent readers in the
> DVSEC emulation path.
> 
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> 
> [ ... ]
> 
> > @@ -1198,7 +1199,8 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> >  				dev_warn(&port->dev,
> >  					 "Failed to allocate decoder%d.%d\n",
> >  					 port->id, i);
> > -				return PTR_ERR(cxled);
> > +				rc = PTR_ERR(cxled);
> > +				goto err;
> 
> [ ... ]
> 
> > @@ -1220,17 +1223,23 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> >  				 "Failed to initialize decoder%d.%d\n",
> >  				 port->id, i);
> >  			put_device(&cxld->dev);
> > -			return rc;
> > +			goto err;
> >  		}
> >  		rc = add_hdm_decoder(port, cxld);
> >  		if (rc) {
> >  			dev_warn(&port->dev,
> >  				 "Failed to add decoder%d.%d\n", port->id, i);
> > -			return rc;
> > +			goto err;
> >  		}
> >  	}
> >  
> >  	return 0;
> > +
> > +err:
> > +	/* Reset to the none committed baseline for a fresh enumeration */
> > +	scoped_guard(rwsem_write, &cxl_rwsem.region)
> > +		port->commit_end = -1;
> > +	return rc;
> >  }
> 
> [Severity: Medium]
> Does mixing goto-based error handling with scope-based cleanup helpers
> (scoped_guard) in the same function violate the cleanup subsystem guidelines?
> 
> The documentation in include/linux/cleanup.h notes that usage of "goto"
> and cleanup helpers should never be mixed in the same function, because it
> creates confusing ownership semantics and can lead to bugs under future
> modifications.
> 
> Should devm_cxl_enumerate_decoders() either be converted entirely to
> scope-based cleanup, or avoid using scoped_guard() in the error path?
>

Hi Alison,

I think this finding is real, at this point, decoder added by earlier loop
iterations are still registered until the caller's devres unwind runs.

So resetting commit_end here will create a window where those enable decoder
objects are still visible, but cxl_num_decoders_committed() tell us it's 0 there.

I think the commit_end reset should be ordered after the registered decoder are removed.

--Richard
 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784150131.git.alison.schofield@intel.com?part=3
>