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

On Mon, Jul 20, 2026 at 02:03:06PM +0800, Li Ming wrote:
> 
> 在 2026/7/18 05:42, 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/[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);
> > +
> > +		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;
> > +		}
> Current init_hdm_decoder() reads IW and IG from CTRL register whatever a
> decoder is enabled or not, just wondering any reason for that. Because my
> understanding is when CXL driver programs a decoder, IW and IG will be
> rewritten, so is it not needed to read IW and IG from the register if a
> decoder is not enabled?

Hi Ming,

Great catch! It's not only needless but stale bits could cause the
whole enumeration through eiw_to,eig_to....to return -EINVAL.

I added a patch 2 to address this (another pre existing issue).

I do see that you used the word 'read' and I wanted to note that IW and IG
are field of the ctrl register and that has to be read once to find the
committed bit. So there is not separate read to drop.  What the new patch
stops doing is decoding and validating those fields.

Take a look please. Thanks,
-- Alison