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

On Sat, Jul 25, 2026 at 01:26:37AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] During region auto-discovery, a passthrough decoder's interleave granularity is not updated to its correct topological value, causing valid interleaving descendants to fail auto-discovery.

Same NO as in v1[1] (and in v2).
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.










> --
> 
> --- Patch [1]: [PATCH v3 1/2] cxl/region: Allow passthrough decoders with >16K granularity ---
> commit d213bad3e4cb8647e64aafb5eacaa82258aa533b
> Author: Alison Schofield <[email protected]>
> 
> cxl/region: Allow passthrough decoders with >16K granularity
> 
> This commit allows passthrough decoders to have computed granularities larger
> than 16K by treating the interleave granularity as a don't-care value for
> non-interleaving decoders. It updates the region configuration to skip
> validation and hardware encodability checks for these granularities, while
> keeping the computed value to seed descendant decoder setup.
> 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > --- 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,
> [ ... ]
> > +	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]
> When auto-discovering a region with a passthrough decoder (iw == 1), does this
> skip updating the decoder's interleave granularity to the computed 'ig'?
> 
> Looking at cxl_port_setup_targets(), the auto-discovery path handles
> non-interleaving decoders by bypassing the granularity mismatch check:
> 
>     if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
>             if (cxld->interleave_ways != iw ||
>                 (iw > 1 && cxld->interleave_granularity != ig) || ...
> 
> However, because cxld->interleave_granularity is never updated to 'ig' for
> auto-discovered passthrough decoders, won't it retain its stale hardware
> value (which could be a don't-care value like 0)?
> 
> If so, when descendant decoders load their parent's interleave granularity
> earlier in cxl_port_setup_targets():
> 
>     parent_rr = cxl_rr_load(parent_port, cxlr);
>     parent_cxld = parent_rr->decoder;
>     parent_ig = parent_cxld->interleave_granularity;
> 
> won't they inherit this stale hardware don't-care value? This appears to
> cause their own expected 'ig' calculation to fail validation during
> auto-discovery, resulting in the region being rejected.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1