Re: [PATCH v2 1/2] cxl/region: Allow passthrough decoders with >16K granularity

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 14 Jul 2026 15:56:18 +0800
Richard Cheng <icheng@nvidia.com> wrote:

> On Sat, Jul 11, 2026 at 07:09:47PM +0800, Alison Schofield wrote:
> > On Sat, Jul 11, 2026 at 08:15:07PM +0000, sashiko-bot@kernel.org wrote:  
> > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > - [High] During auto-discovery (CXL_REGION_F_AUTO), passthrough decoders fail to update their `interleave_granularity` software state to the computed value, causing downstream decoders to inherit incorrect granularity and fail validation upon reboot.  
> > 
> > Same NO as in v1[1]  and I don't see a way to graciously alert you to this
> > in the code. Gratuitously updating the field is not the right thing to
> > do.
> > 
> > 
> > [1] https://lore.kernel.org/linux-cxl/alKP8OP4DNT2nJnu@aschofie-mobl2.lan/#r
> > No. A downstream decoder can't inherit an unecodable value into a failing
> > check. A >16K granularity only appears below a wide interleave, and
> > everything below that inherits an equal-or-larger value that an interleaving
> > decoder couldn't encode. So every descendant is necessarily passthrough and
> > never validates granularity. There is no path where updating the F_AUTO
> > value would prevent a failure because the failure cannot happen.
> >  
> 
> Hi Alison,
> 
> I'm thinking of 1 scenario where a regression might still happen,
> independent of whether an interleaving decoder can appear below a >16k
> passthrough decoder.
> 
> The new cxld_set_interleave() behavior writes EIG=0 for every IW=1 decoder,
> including cases where logical granularity is encodable.
> 
> For example,
> root: IW=2, IG=4k
> passthrough: IW=1, logical IG=8K
> downstream: IW=2, IG=8K
> 
> During manual setup, the passthrough decoder gets a logical IG of 8K.
> With your change, committing it writes EIG=0. On rediscovery, that is
> read back as 256 byte.
> 
> The AUTO path accepts the passthrough decoder because it skips the IG
> comparison for IW=1, but it does not restore.
> 
> The downstream decoder will inherits 256 bytes instead of 8K and its 8K config
> fails validation.

I was looking back having seen some text on this in v4 that confused me.

Isn't a failure on this combination a bug anyway (that we should fix)? 
The downstream decoder shouldn't inherit a value of granularity from a
pass through decoder (as it's meaningless) it should be getting the
appropriate values from next level up in the tree (or further if
there are more passthrough decoders up there).

Jonathan

> 
> Could cxld_set_interleave() keep the real IG encoding when it's encodable and
> use EIG=0 only when an IW=1 value isn't encodable?
> For latter case, AUTO still need to restore the computed logical IG before
> walking descendants.
> 
> What do you think ?
> 
> Best regards,
> Richard Cheng.
> > 
> >   
> > > --
> > > 
> > > --- Patch [1]: [PATCH v2 1/2] cxl/region: Allow passthrough decoders with >16K granularity ---
> > > commit 9e2526342188a97e338bfc63d454c253f1e2b3ef
> > > Author: Alison Schofield <alison.schofield@intel.com>
> > > 
> > > cxl/region: Allow passthrough decoders with >16K granularity
> > > 
> > > This commit allows passthrough decoders beneath wide parent interleaves
> > > to have a granularity exceeding 16K by programming a don't-care value
> > > in hardware for non-interleaving decoders.
> > >   
> > > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > > > index 0c80b76a5f9b4..d164662668c19 100644
> > > > --- a/drivers/cxl/core/hdm.c
> > > > +++ b/drivers/cxl/core/hdm.c
> > > > @@ -691,9 +691,16 @@ static void cxld_set_interleave(struct cxl_decoder *cxld, u32 *ctrl)
> > > >  	if (WARN_ONCE(ways_to_eiw(cxld->interleave_ways, &eiw),
> > > >  		      "invalid interleave_ways: %d\n", cxld->interleave_ways))
> > > >  		return;
> > > > -	if (WARN_ONCE(granularity_to_eig(cxld->interleave_granularity, &eig),
> > > > -		      "invalid interleave_granularity: %d\n",
> > > > -		      cxld->interleave_granularity))
> > > > +
> > > > +	/*
> > > > +	 * A non-interleaving decoder ignores the IG field. Encode a
> > > > +	 * don't-care value instead of validating the stored granularity.
> > > > +	 */
> > > > +	if (cxld->interleave_ways == 1)
> > > > +		eig = 0;
> > > > +	else if (WARN_ONCE(granularity_to_eig(cxld->interleave_granularity, &eig),
> > > > +			   "invalid interleave_granularity: %d\n",
> > > > +			   cxld->interleave_granularity))
> > > >  		return;
> > > >  
> > > >  	u32p_replace_bits(ctrl, eig, CXL_HDM_DECODER0_CTRL_IG_MASK);  
> > > 
> > > [ ... ]
> > >   
> > > > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > > > index 1e211542b6b64..5a443551288c4 100644
> > > > --- a/drivers/cxl/core/region.c
> > > > +++ b/drivers/cxl/core/region.c  
> > > 
> > > [ ... ]
> > >   
> > > > @@ -1549,20 +1557,21 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> > > >  	 * Interleave granularity is a multiple of @parent_port granularity.
> > > >  	 * Multiplier is the parent port interleave ways.
> > > >  	 */
> > > > -	rc = granularity_to_eig(parent_ig * parent_iw, &eig);
> > > > -	if (rc) {
> > > > -		dev_dbg(&cxlr->dev,
> > > > -			"%s: invalid granularity calculation (%d * %d)\n",
> > > > -			dev_name(&parent_port->dev), parent_ig, parent_iw);
> > > > -		return rc;
> > > > -	}
> > > > +	ig = parent_ig * parent_iw;
> > > >  
> > > > -	rc = eig_to_granularity(eig, &ig);
> > > > -	if (rc) {
> > > > -		dev_dbg(&cxlr->dev, "%s:%s: invalid interleave: %d\n",
> > > > -			dev_name(port->uport_dev), dev_name(&port->dev),
> > > > -			256 << eig);
> > > > -		return rc;
> > > > +	/*
> > > > +	 * Keep the computed granularity for descendant setup. Only
> > > > +	 * interleaving decoders require an encodable granularity.
> > > > +	 */
> > > > +	if (iw > 1) {
> > > > +		rc = granularity_to_eig(ig, &eig);
> > > > +		if (rc) {
> > > > +			dev_dbg(&cxlr->dev,
> > > > +				"%s: invalid granularity calculation (%d * %d)\n",
> > > > +				dev_name(&parent_port->dev), parent_ig,
> > > > +				parent_iw);
> > > > +			return rc;
> > > > +		}
> > > >  	}
> > > >  
> > > >  	if (iw > 8 || iw > cxlsd->nr_targets) {  
> > > 
> > > [Severity: High]
> > > Does this code break region auto-discovery on reboot for topologies
> > > containing a passthrough decoder?
> > > 
> > > When cxld_set_interleave() configures a passthrough decoder, it now sets
> > > eig = 0 (a don't-care value) in the hardware.
> > > 
> > > Upon reboot, reading this hardware state will initialize the decoder's
> > > software interleave_granularity to a stale value.
> > > 
> > > During auto-discovery in cxl_port_setup_targets(), the CXL_REGION_F_AUTO
> > > path skips validating the granularity for passthrough decoders (iw == 1),
> > > but it appears to be missing an update to correct the software state:
> > > 
> > > 	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> > > 		if (cxld->interleave_ways != iw ||
> > > 		    (iw > 1 && cxld->interleave_granularity != ig) ||
> > > 		    !spa_maps_hpa(p, &cxld->hpa_range) ||
> > > 		    ((cxld->flags & CXL_DECODER_F_ENABLE) == 0)) {
> > > 			...
> > > 			return -ENXIO;
> > > 		}
> > > 		/* missing update: cxld->interleave_granularity = ig; */
> > > 	}
> > > 
> > > If cxld->interleave_granularity is not updated to the computed ig here, won't
> > > any interleaving descendants read the stale parent_ig, compute a mismatched
> > > expected granularity, and fail validation?
> > > 
> > > -- 
> > > Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783795720.git.alison.schofield@intel.com?part=1  
> >