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

On Tue, Jul 14, 2026 at 03:56:18PM +0800, Richard Cheng wrote:
> 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?

Yes, and fixed that up in v3.

> For latter case, AUTO still need to restore the computed logical IG before
> walking descendants.

I don't find it needed for the unencodabe (>16K) case. Every descendant
is itself passthru and never validates granularity. And with the encode
fix, the encodabe case is no longer clobbered, so there is nothing
state to restore, and nothing left to test for.

Please take a look at v3.

Thanks for the review!

-- Alison


> 
> 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
> >