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

On Sat, Jul 11, 2026 at 07:09:47PM +0800, Alison Schofield wrote:
> On Sat, Jul 11, 2026 at 08:15:07PM +0000, [email protected] 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/[email protected]/#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.

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 <[email protected]>
> > 
> > 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/[email protected]?part=1
>