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

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

On Fri, Jul 31, 2026 at 03:41:54PM +0800, Richard Cheng wrote:
> On Fri, Jul 24, 2026 at 06:08:54PM +0800, Alison Schofield wrote:

snip

> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 1e211542b6b6..5a443551288c 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1521,12 +1521,20 @@ static int cxl_port_setup_targets(struct cxl_port *port,
> >  		parent_iw = parent_cxld->interleave_ways;
> >  	}
> >  
> > -	rc = granularity_to_eig(parent_ig, &peig);
> > -	if (rc) {
> > -		dev_dbg(&cxlr->dev, "%s:%s: invalid parent granularity: %d\n",
> > -			dev_name(parent_port->uport_dev),
> > -			dev_name(&parent_port->dev), parent_ig);
> > -		return rc;
> > +	/*
> > +	 * A non-interleaving parent does not encode its granularity, so its
> > +	 * stored value may exceed the maximum encodable and need not be
> > +	 * validated here.
> > +	 */
> > +	if (parent_iw > 1) {
> > +		rc = granularity_to_eig(parent_ig, &peig);
> > +		if (rc) {
> > +			dev_dbg(&cxlr->dev,
> > +				"%s:%s: invalid parent granularity: %d\n",
> > +				dev_name(parent_port->uport_dev),
> > +				dev_name(&parent_port->dev), parent_ig);
> > +			return rc;
> > +		}
> >  	}
> >  
> >  	rc = ways_to_eiw(parent_iw, &peiw);
> > @@ -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;
> > +		}
> >  	}
> >
> 
> Hi Alison,
> 
> Looks sane to me, just that this check pattern appears twice and peig
> and eig are both write-only afterwards.
> Could they be formed into a helper or macro ?
> 
> Something like the following might do ?
> """
> static bool granularity_is_encodable(int ways, int granularity)
> {
>     u16 eig;
>     if (ways <= 1)
>         return true;
> 
>     return granularity_to_eig(granularity, &eig) == 0;
> }
> 
> if (!granularity_is_encodable(parent_iw, parent_ig) {
>     dev_dbg();
>     return -EINVAL;
> }
> """
> 
> Reviewed-by: Richard Cheng <icheng@nvidia.com>


Thanks Richard!
I took the helper but wrote it to return an errno rather than a bool
because it seemed that a bool would answer true for any gran at all
when ways is 1.

Went w the style of the other check_*() helpers in region.c that return
errno.

Please take a look in v4.

-- Alison


> 
> Best regards,
> Richard Cheng.
> 
> >  	if (iw > 8 || iw > cxlsd->nr_targets) {
> > -- 
> > 2.37.3
> > 
> >