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

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

On Tue, Oct 17, 2023 at 04:21:06PM +0000, Jim Harris wrote:
> 
> 
> > On Oct 15, 2023, at 11:02 PM, alison.schofield@intel.com 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.
> > 
> > Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> > Reported-by: Dmytro Adamenko <dmytro.adamenko@intel.com>
> > 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(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 6d63b8798c29..64206fc4d99b 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -1487,16 +1487,19 @@ static struct cxl_port *next_port(struct cxl_port *port)
> > return port->parent_dport->port;
> > }
> > 
> > -static int decoder_match_range(struct device *dev, void *data)
> > +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);
> > +}
> 
> Hi Alison,
> 
> This stray closing brace needs to be removed from this patch.

Got it, thanks!


> 
> Rest of the patch looks good, I agree the naming is much better with these
> changes.
> 
> Reviewed-by: Jim Harris <jim.harris@samsung.com>
> 
> > +
> > }
> > 
> > static void find_positions(const struct cxl_switch_decoder *cxlsd,
> > @@ -1565,7 +1568,8 @@ static int cmp_decode_pos(const void *a, const void *b)
> > goto err;
> > }
> > 
> > - dev = device_find_child(&port->dev, cxled_a, decoder_match_range);
> > + dev = device_find_child(&port->dev, &cxled_a->cxld.hpa_range,
> > + match_switch_decoder_by_range);
> > if (!dev) {
> > struct range *range = &cxled_a->cxld.hpa_range;
> > 
> > @@ -2696,7 +2700,7 @@ static int devm_cxl_add_dax_region(struct cxl_region *cxlr)
> > return rc;
> > }
> > 
> > -static int match_decoder_by_range(struct device *dev, void *data)
> > +static int match_root_decoder_by_range(struct device *dev, void *data)
> > {
> > struct range *r1, *r2 = data;
> > struct cxl_root_decoder *cxlrd;
> > @@ -2827,7 +2831,7 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
> > int rc;
> > 
> > cxlrd_dev = device_find_child(&root->dev, &cxld->hpa_range,
> > -      match_decoder_by_range);
> > +      match_root_decoder_by_range);
> > if (!cxlrd_dev) {
> > dev_err(cxlmd->dev.parent,
> > "%s:%s no CXL window for range %#llx:%#llx\n",
> > -- 
> > 2.37.3
> > 
> > 
>