Re: [PATCH v2 1/3] cxl/region: Prepare the decoder match range helper for reuse

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

On Mon, Oct 23, 2023 at 01:54:26PM -0700, Dan Williams wrote:
> alison.schofield@ wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > match_decoder_by_range() and decoder_match_range() both determine
> > if an HPA range matches a decoder. The first does it for root
> > decoders and the second one operates on switch decoders.
> > 
> > Tidy these up with clear naming and make the switch helper more
> > like the root decoder helper in style and functionality. Make it
> > take the actual range, rather than an endpoint decoder from which
> > it extracts the range.
> > 
> > Aside from aesthetics and maintainability, this is in preparation
> > for reuse.
> 
> Some minor changelog comments:
> 
> > 
> > Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> 
> Save "Fixes:" for commits that need to be backported to address an
> issue. This will be a dependency for backporting patch2, but on its own
> it does not fix anything.
> 
> > Reported-by: Dmytro Adamenko <dmytro.adamenko@intel.com>
> 
> Along the same lines this probably moves to the patch that fixes the
> report.
> 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  drivers/cxl/core/region.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)

snip

> > 
> > +static int match_switch_decoder_by_range(struct device *dev, void *data)
> >  {
> > -	struct cxl_endpoint_decoder *cxled = data;
> > +	struct range *r1, *r2 = data;
> >  	struct cxl_switch_decoder *cxlsd;
> >  
> >  	if (!is_switch_decoder(dev))
> >  		return 0;
> >  
> >  	cxlsd = to_cxl_switch_decoder(dev);
> > -	return range_contains(&cxlsd->cxld.hpa_range, &cxled->cxld.hpa_range);
> > +	r1 = &cxlsd->cxld.hpa_range;
> > +	return range_contains(r1, r2);
> 
> I know this is just maintaining the status quo, but thinking about it a
> bit deeper I think range_contains() is only correct for root decoders.
> For a given region the root decoder will contain the region, but all the
> switch decoders must be a 1:1 mapping. Linux only supports scenarios
> where all endpoint decoders map the same HPA range and identify only 1
> decoder per-switch port to map that region.
> 
> So "range_contains(r1, r2)" becomes "return *r1 == *r2".

Thanks Dan - picked up your changelog suggestions and replace this
range_contains w an 1:1 comparison.

Alison