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

[Alison Schofield <alison.schofield@intel.com>]

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.



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