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

On Tue, Jul 21, 2026 at 07:04:16PM +0100, Jonathan Cameron wrote:
> On Fri, 17 Jul 2026 14:42:46 -0700
> Alison Schofield <[email protected]> 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.

Thanks for the review Jonathan. I'm responding as I'm about to send v5,
with all this considered and fixed up.

> 
> Note on this below.  Why 1?  If it is invalid can we use a nonsense
> value like 0 so that any use of it will cause an obvious bug and
> make it easy to spot? (hopefully)

replied below


> 
> > 
> > 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 | 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);
> 
> Trivial but I'd go long and have that on one line. It's only just over 80 chars!

Done.

> 

snip

> > +
> > +		if (!committed) {
> > +			/* Ignore interleave ways until commit */
> 
> Dumb question but why is = 1 equivalent to ignore? Can we make it explicit
> either by gating at all use of interleave_ways, or using an invalid value
> to mean we don't care yet.  I like 0 as that's clearly nonsense for interleave
> ways.
> 
> > +			cxld->interleave_ways = 1;
> > +			return 0;
> > +		}

0 is no usable because cxl_decoder_acc_locked() reject ways < 1, so
every uncommitted switch decoder would fail to add. And 1 was wrong for
a different reason that Davidlohr caught on v4/patch 2 - ways is a
sysfs attribute that reports what the register field holds, so changing
that changes what usersspace sees. In v5 we leave the uncommitted read
path alone.


> > +
> > +		if (cxld->interleave_ways > CXL_HDM_DECODER0_TL_TARGETS) {
> 
> This feels a bit belt and braces.  Should we be verifying that nr_targets meets
> this constraint before setting it instead and only having the below test here?
> 

Yes, done in v5. The target count is now bounded where it is established
rather than at each use, so...
- devm_cxl_enumerate_decoders() rejects an HDM capability whose reported
  target count is outside 1..CXL_HDM_DECODER0_TL_TARGETS, once per port.
- cxl_switch_decoder_init() rejects nr_targets < 1 (Ming asked for that
  one).

init_hdm_decoder() is left with the ways > nr_targets test only.


snip

> > 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
> 
> Whilst the comment is true, perhaps better to refer to IW definition
> and 8 being max allowed for anything that isn't a end point.

Done like this:
	/* Target list capacity, i.e. max interleave ways for a non-endpoint decoder */
        #define CXL_HDM_DECODER0_TL_TARGETS 8


> 
> Maybe the spec would provide 2 registers and then only allow 6 decoders :)
> Also comment doesn't say how big registers are.  Anyhow, to me the IW
> reference is crisper.
> 
> >  #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)
> >  
>