RE: [PATCH] cxl/region: Match auto-discovered region decoders by HPA range

[Dan Williams <dan.j.williams@intel.com>]

alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Today, when the region driver attaches a region to a port, it
> selects the ports next available decoder to program.

A small nit: s/Today/Currently/

> With the addition of auto-discovered regions, a port decoder has
> already been programmed, so grabbing the next available decoder
> can be a mismatch when there is more than one region using the
> port. Match on the port HPA range for auto-discovered regions.

This patch looks correct to me, just a couple questions beloe.

It would be great if it was accompanied by a cxl_test scenario that
tested the failing case, but barring that it would be good to have some
logs from a scenario where people can notice if this fix applies to
their failure.

> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

I think this wants a "Fixes" tag:

Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")

> ---
>  
>  drivers/cxl/core/region.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e115ba382e04..8bfec7a96975 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -717,13 +717,37 @@ static int match_free_decoder(struct device *dev, void *data)
>  	return 0;
>  }
>  
> +static int match_auto_decoder(struct device *dev, void *data)
> +{
> +	struct cxl_endpoint_decoder *cxled = data;
> +	struct cxl_decoder *cxld;
> +
> +	if (!is_switch_decoder(dev))
> +		return 0;

Is this check needed? Endpoint decoders should also match by range.
Maybe make it explicit like:

    if (cxld == &cxled->cxld)
    	return 0;

...where it is obvious no further checks are needed, but I think that
also goes away with the change proposal below:


> +
> +	cxld = to_cxl_decoder(dev);
> +
> +	if (!range_contains(&cxld->hpa_range, &cxled->cxld.hpa_range))
> +		return 0;

Hmm, shouldn't it be identical and no bigger?

if (cxld->hpa_range != cxled->cxld.hpa_range)

> +
> +	if (!cxld->region)
> +		return 1;

Interesting, I am trying to think through the implications of failing
here. That would only happen if the port had been setup previously with
a different region for the same address range? How would that happen?

It feel like it should be:

	if (cxld->region) {
		dev_WARN(...)
		return 0;
	}

	return 1;

> +
> +	return 0;
> +}
> +
>  static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
> -						   struct cxl_region *cxlr)
> +						   struct cxl_region *cxlr,
> +						   struct cxl_endpoint_decoder *cxled)
>  {
>  	struct device *dev;
>  	int id = 0;
>  
> -	dev = device_find_child(&port->dev, &id, match_free_decoder);
> +	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags))
> +		dev = device_find_child(&port->dev, cxled, match_auto_decoder);
> +	else
> +		dev = device_find_child(&port->dev, &id, match_free_decoder);
> +
>  	if (!dev)
>  		return NULL;
>  	/*
> @@ -839,7 +863,8 @@ static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
>  	if (port == cxled_to_port(cxled))
>  		cxld = &cxled->cxld;
>  	else
> -		cxld = cxl_region_find_decoder(port, cxlr);
> +		cxld = cxl_region_find_decoder(port, cxlr, cxled);

It looks like the cxled is only used to convey the range. Maybe just get
that from cxlr->params->res and not add another parameter here? Of
course that would then change the suggestions above where you can not
compare 'struct range' instances directly.