Re: [PATCH v4 2/4] cxl/hdm: Only interpret interleave settings on committed decoders

On Tue, Jul 21, 2026 at 02:26:57PM +0800, Li Ming wrote:
> 
> 在 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 <[email protected]>
> > Closes: https://lore.kernel.org/all/[email protected]/
> > Signed-off-by: Alison Schofield <[email protected]>
> > ---
> >   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?

Restructuring not needed now that I've dropped the patch in v5. (but
good ideas ;))

Your v3 ask led me to add this patch, but in hindsight, I should have
just answered your question and not jumped to a code change. The driver
didn't need the values but userspace does, so they are left untouched.

The hardening the series keeps is in the bounds instead: P1 rejects a
decoder whose interleave ways exceed its target count, and a new P4
bounds the walks of cxlsd->target[].

-- Alison