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

On Thu, Jul 16, 2026 at 04:49:35PM +0800, Richard Cheng wrote:
> 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/[email protected]?part=1
> > Fixes: d17d0540a0db ("cxl/core/hdm: Add CXL standard decoder enumeration to the core")
> > Signed-off-by: Alison Schofield <[email protected]>
> > ---
> >  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 ?

Thanks for the review.
Good catch. `cxl_switch_decoder_init()` is shared by root and switch decoders,
so the `< 1` check didn't belong there.

For v3 I dropped that change and moved the zero-target reject into the switch
decoder path in `init_hdm_decoder()`, alongside the other `target_count` checks.
The original `CXL_DECODER_MAX_INTERLEAVE` bound is unchanged.

-- Alison

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