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

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

[alison.schofield]

From: Alison Schofield <alison.schofield@intel.com>

Currently, when the region driver attaches a region to a port, it
selects the ports next available decoder to program.

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.

The failure appears like this with CXL DEBUG enabled:

[] cxl_core:alloc_region_ref:754: cxl region0: endpoint9: HPA order violation region0:[mem 0x14780000000-0x1478fffffff flags 0x200] vs [mem 0x880000000-0x185fffffff flags 0x200]
[] cxl_core:cxl_port_attach_region:972: cxl region0: endpoint9: failed to allocate region reference

When CXL DEBUG is not enabled, there is no failure message. The region
just never materializes. Users can suspect this issue if they know their
firmware has programmed decoders so that more than one region is using
a port. Note that the problem may appear intermittently, ie not on
every reboot.

Add a matching method for auto-discovered regions that finds a decoder
based on an HPA range. The decoder range must exactly match the region
resource parameter.

Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---

Changes in v2:
- Use cxlr->params for HPA match rather than requiring cxled (Dan)
- dev_warn() if decoder already assigned to a region (Dan)
- Add failure footprint to commit log (Dan)
- Add Fixes Tag (Dan)
- v1: https://lore.kernel.org/linux-cxl/20230804213004.1669658-1-alison.schofield@intel.com/

 drivers/cxl/core/region.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index e115ba382e04..b08aec9f0af8 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -717,13 +717,40 @@ static int match_free_decoder(struct device *dev, void *data)
 	return 0;
 }
 
+static int match_auto_decoder(struct device *dev, void *data)
+{
+	struct cxl_region_params *p = data;
+	struct cxl_decoder *cxld;
+	struct range *r;
+
+	if (!is_switch_decoder(dev))
+		return 0;
+
+	cxld = to_cxl_decoder(dev);
+	r = &cxld->hpa_range;
+
+	if (p->res && p->res->start == r->start && p->res->end == r->end) {
+		if (cxld->region) {
+			dev_WARN(dev, "decoder already attached to %s\n",
+				 dev_name(&cxld->region->dev));
+			return 0;
+		}
+		return 1;
+	}
+	return 0;
+}
+
 static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
 						   struct cxl_region *cxlr)
 {
 	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, &cxlr->params,
+					match_auto_decoder);
+	else
+		dev = device_find_child(&port->dev, &id, match_free_decoder);
 	if (!dev)
 		return NULL;
 	/*

base-commit: fe77cc2e5a6a7c85f5c6ef8a39d7694ffc7f41c9
-- 
2.37.3

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

[Dave Jiang]

On 8/21/23 18:43, alison.schofield@intel.com wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> Currently, when the region driver attaches a region to a port, it
> selects the ports next available decoder to program.
> 
> 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.
> 
> The failure appears like this with CXL DEBUG enabled:
> 
> [] cxl_core:alloc_region_ref:754: cxl region0: endpoint9: HPA order violation region0:[mem 0x14780000000-0x1478fffffff flags 0x200] vs [mem 0x880000000-0x185fffffff flags 0x200]
> [] cxl_core:cxl_port_attach_region:972: cxl region0: endpoint9: failed to allocate region reference
> 
> When CXL DEBUG is not enabled, there is no failure message. The region
> just never materializes. Users can suspect this issue if they know their
> firmware has programmed decoders so that more than one region is using
> a port. Note that the problem may appear intermittently, ie not on
> every reboot.
> 
> Add a matching method for auto-discovered regions that finds a decoder
> based on an HPA range. The decoder range must exactly match the region
> resource parameter.
> 
> Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
> 
> Changes in v2:
> - Use cxlr->params for HPA match rather than requiring cxled (Dan)
> - dev_warn() if decoder already assigned to a region (Dan)
> - Add failure footprint to commit log (Dan)
> - Add Fixes Tag (Dan)
> - v1: https://lore.kernel.org/linux-cxl/20230804213004.1669658-1-alison.schofield@intel.com/
> 
>   drivers/cxl/core/region.c | 29 ++++++++++++++++++++++++++++-
>   1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e115ba382e04..b08aec9f0af8 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -717,13 +717,40 @@ static int match_free_decoder(struct device *dev, void *data)
>   	return 0;
>   }
>   
> +static int match_auto_decoder(struct device *dev, void *data)
> +{
> +	struct cxl_region_params *p = data;
> +	struct cxl_decoder *cxld;
> +	struct range *r;
> +
> +	if (!is_switch_decoder(dev))
> +		return 0;
> +
> +	cxld = to_cxl_decoder(dev);
> +	r = &cxld->hpa_range;
> +
> +	if (p->res && p->res->start == r->start && p->res->end == r->end) {
> +		if (cxld->region) {
> +			dev_WARN(dev, "decoder already attached to %s\n",
> +				 dev_name(&cxld->region->dev));
> +			return 0;
> +		}
> +		return 1;
> +	}
> +	return 0;
> +}
> +
>   static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
>   						   struct cxl_region *cxlr)
>   {
>   	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, &cxlr->params,
> +					match_auto_decoder);
> +	else
> +		dev = device_find_child(&port->dev, &id, match_free_decoder);
>   	if (!dev)
>   		return NULL;
>   	/*
> 
> base-commit: fe77cc2e5a6a7c85f5c6ef8a39d7694ffc7f41c9

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

[Davidlohr Bueso]

On Mon, 21 Aug 2023, alison.schofield@intel.com wrote:

>From: Alison Schofield <alison.schofield@intel.com>
>
>Currently, when the region driver attaches a region to a port, it
>selects the ports next available decoder to program.
>
>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.
>
>The failure appears like this with CXL DEBUG enabled:
>
>[] cxl_core:alloc_region_ref:754: cxl region0: endpoint9: HPA order violation region0:[mem 0x14780000000-0x1478fffffff flags 0x200] vs [mem 0x880000000-0x185fffffff flags 0x200]
>[] cxl_core:cxl_port_attach_region:972: cxl region0: endpoint9: failed to allocate region reference
>
>When CXL DEBUG is not enabled, there is no failure message. The region
>just never materializes. Users can suspect this issue if they know their
>firmware has programmed decoders so that more than one region is using
>a port. Note that the problem may appear intermittently, ie not on
>every reboot.
>
>Add a matching method for auto-discovered regions that finds a decoder
>based on an HPA range. The decoder range must exactly match the region
>resource parameter.
>
>Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
>Signed-off-by: Alison Schofield <alison.schofield@intel.com>

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>

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

[Jonathan Cameron]

On Mon, 21 Aug 2023 18:43:03 -0700
alison.schofield@intel.com wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> Currently, when the region driver attaches a region to a port, it
> selects the ports next available decoder to program.
> 
> 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.
> 
> The failure appears like this with CXL DEBUG enabled:
> 
> [] cxl_core:alloc_region_ref:754: cxl region0: endpoint9: HPA order violation region0:[mem 0x14780000000-0x1478fffffff flags 0x200] vs [mem 0x880000000-0x185fffffff flags 0x200]
> [] cxl_core:cxl_port_attach_region:972: cxl region0: endpoint9: failed to allocate region reference
> 
> When CXL DEBUG is not enabled, there is no failure message. The region
> just never materializes. Users can suspect this issue if they know their
> firmware has programmed decoders so that more than one region is using
> a port. Note that the problem may appear intermittently, ie not on
> every reboot.
> 
> Add a matching method for auto-discovered regions that finds a decoder
> based on an HPA range. The decoder range must exactly match the region
> resource parameter.
> 
> Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Bikeshed time...

> ---
> 
> Changes in v2:
> - Use cxlr->params for HPA match rather than requiring cxled (Dan)
> - dev_warn() if decoder already assigned to a region (Dan)
> - Add failure footprint to commit log (Dan)
> - Add Fixes Tag (Dan)
> - v1: https://lore.kernel.org/linux-cxl/20230804213004.1669658-1-alison.schofield@intel.com/
> 
>  drivers/cxl/core/region.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e115ba382e04..b08aec9f0af8 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -717,13 +717,40 @@ static int match_free_decoder(struct device *dev, void *data)
>  	return 0;
>  }
>  
> +static int match_auto_decoder(struct device *dev, void *data)

Does more than matching a decoder and in the warning path doesn't actually
match it (not returning 1 so we carry on).

So perhaps a rename?

> +{
> +	struct cxl_region_params *p = data;
> +	struct cxl_decoder *cxld;
> +	struct range *r;
> +
> +	if (!is_switch_decoder(dev))
> +		return 0;
> +
> +	cxld = to_cxl_decoder(dev);
> +	r = &cxld->hpa_range;
> +
> +	if (p->res && p->res->start == r->start && p->res->end == r->end) {
> +		if (cxld->region) {
> +			dev_WARN(dev, "decoder already attached to %s\n",
> +				 dev_name(&cxld->region->dev));
> +			return 0;

Not obvious to me why we carry on looking for matching regions if we have
found a precise match (even if it's already attached).  
I'd expect return 1 here.  Maybe a comment on why it makes sense to keep trying?

> +		}
> +		return 1;
> +	}
> +	return 0;
> +}
> +
>  static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
>  						   struct cxl_region *cxlr)
>  {
>  	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, &cxlr->params,
> +					match_auto_decoder);
> +	else
> +		dev = device_find_child(&port->dev, &id, match_free_decoder);
>  	if (!dev)
>  		return NULL;
>  	/*
> 
> base-commit: fe77cc2e5a6a7c85f5c6ef8a39d7694ffc7f41c9

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

[Alison Schofield]

On Tue, Aug 29, 2023 at 02:21:15PM +0100, Jonathan Cameron wrote:
> On Mon, 21 Aug 2023 18:43:03 -0700
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Currently, when the region driver attaches a region to a port, it
> > selects the ports next available decoder to program.
> > 
> > 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.
> > 
> > The failure appears like this with CXL DEBUG enabled:
> > 
> > [] cxl_core:alloc_region_ref:754: cxl region0: endpoint9: HPA order violation region0:[mem 0x14780000000-0x1478fffffff flags 0x200] vs [mem 0x880000000-0x185fffffff flags 0x200]
> > [] cxl_core:cxl_port_attach_region:972: cxl region0: endpoint9: failed to allocate region reference
> > 
> > When CXL DEBUG is not enabled, there is no failure message. The region
> > just never materializes. Users can suspect this issue if they know their
> > firmware has programmed decoders so that more than one region is using
> > a port. Note that the problem may appear intermittently, ie not on
> > every reboot.
> > 
> > Add a matching method for auto-discovered regions that finds a decoder
> > based on an HPA range. The decoder range must exactly match the region
> > resource parameter.
> > 
> > Fixes: a32320b71f08 ("cxl/region: Add region autodiscovery")
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Bikeshed time...

Not bikeshed, good catch -

> 
> > ---
> > 
> > Changes in v2:
> > - Use cxlr->params for HPA match rather than requiring cxled (Dan)
> > - dev_warn() if decoder already assigned to a region (Dan)
> > - Add failure footprint to commit log (Dan)
> > - Add Fixes Tag (Dan)
> > - v1: https://lore.kernel.org/linux-cxl/20230804213004.1669658-1-alison.schofield@intel.com/
> > 
> >  drivers/cxl/core/region.c | 29 ++++++++++++++++++++++++++++-
> >  1 file changed, 28 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index e115ba382e04..b08aec9f0af8 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -717,13 +717,40 @@ static int match_free_decoder(struct device *dev, void *data)
> >  	return 0;
> >  }
> >  
> > +static int match_auto_decoder(struct device *dev, void *data)
> 
> Does more than matching a decoder and in the warning path doesn't actually
> match it (not returning 1 so we carry on).
> 
> So perhaps a rename?

I've updated to just do the match.  Checking for an attached region
was redundant as it's already done in the call path.

Thanks for the review!


> 
> > +{
> > +	struct cxl_region_params *p = data;
> > +	struct cxl_decoder *cxld;
> > +	struct range *r;
> > +
> > +	if (!is_switch_decoder(dev))
> > +		return 0;
> > +
> > +	cxld = to_cxl_decoder(dev);
> > +	r = &cxld->hpa_range;
> > +
> > +	if (p->res && p->res->start == r->start && p->res->end == r->end) {
> > +		if (cxld->region) {
> > +			dev_WARN(dev, "decoder already attached to %s\n",
> > +				 dev_name(&cxld->region->dev));
> > +			return 0;
> 
> Not obvious to me why we carry on looking for matching regions if we have
> found a precise match (even if it's already attached).  
> I'd expect return 1 here.  Maybe a comment on why it makes sense to keep trying?
> 
> > +		}
> > +		return 1;
> > +	}
> > +	return 0;
> > +}
> > +
> >  static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
> >  						   struct cxl_region *cxlr)
> >  {
> >  	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, &cxlr->params,
> > +					match_auto_decoder);
> > +	else
> > +		dev = device_find_child(&port->dev, &id, match_free_decoder);
> >  	if (!dev)
> >  		return NULL;
> >  	/*
> > 
> > base-commit: fe77cc2e5a6a7c85f5c6ef8a39d7694ffc7f41c9
>