Re: [PATCH v4 1/2] cxl/region: Allow regions with an unencodable passthrough granularity

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

On Fri, Sep 04, 2026 at 03:36:24AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] During region auto-discovery, the driver relaxes the interleave granularity validation for non-interleaving passthrough decoders, but fails to update the decoder's software struct (`cxld->interleave_granularity`) with the computed logical value. As a result, descendant decoders inherit a stale hardware don't-care value.

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


> --
> 
> commit 581b6f11f4646ac877c9eaf9fb81a155a40889d5
> Author: Alison Schofield <alison.schofield@intel.com>
> 
> cxl/region: Allow regions with an unencodable passthrough granularity
> 
> Region creation fails when the interleave granularity accumulated below the
> root decoder exceeds 16K, the maximum encodable in the HDM Decoder Control
> IG field, even when the decoder that computes it does not interleave.
> 
> This patch keeps the computed value for passthrough decoders so it can
> seed descendant decoder setup.
> 
> Closes: https://lore.kernel.org/all/20260530002613.682E71F00893@smtp.kernel.org/
> 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 27e63e6dab7c8..89693f0181ddb 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> 
> [ ... ]
> 
> > @@ -1550,20 +1562,12 @@ 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) {
> > +	ig = parent_ig * parent_iw;
> > +	if (!granularity_is_valid(iw, ig)) {
> >  		dev_dbg(&cxlr->dev,
> >  			"%s: invalid granularity calculation (%d * %d)\n",
> >  			dev_name(&parent_port->dev), parent_ig, parent_iw);
> > -		return rc;
> > -	}
> > -
> > -	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;
> > +		return -EINVAL;
> >  	}
> >  
> >  	if (iw > 8 || iw > cxlsd->nr_targets) {
> 
> [Severity: High]
> During region auto-discovery, the driver relaxes the interleave granularity
> validation for non-interleaving passthrough decoders. However, looking at the
> CXL_REGION_F_AUTO block immediately following this diff in
> drivers/cxl/core/region.c:cxl_port_setup_targets():
> 
> 	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)) {
> 			dev_err(&cxlr->dev,
> 
> When iw == 1, does this code fail to update cxld->interleave_granularity
> with the computed logical ig?
> 
> If cxld->interleave_granularity is not updated, it seems descendant decoders
> will inherit a stale hardware don't-care value instead of the true accumulated
> granularity. Could this cause the kernel to accept physically broken topologies
> during auto-discovery, potentially leading to silent memory corruption or
> Machine Check Exceptions (MCEs) when the region is accessed?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788487020.git.alison.schofield@intel.com?part=1