Before adding an endpoint to a region, the endpoint is initialized
first. Move that part to a new function cxl_endpoint_initialize().
The function is in preparation of adding more parameters that need to
be determined in a setup.

The split also helps better separating the code. After initialization
the addition of an endpoint may fail with an error code and all the
data would need to be reverted to not leave the endpoint in an
undefined state. With separate functions the init part can succeed
even if the endpoint cannot be added.

Function naming follows the style of device_register() etc. Thus,
rename function cxl_add_to_region() to cxl_endpoint_register().

Signed-off-by: Robert Richter <rrichter@amd.com>
---
 drivers/cxl/core/region.c | 36 ++++++++++++++++++++++++++++--------
 drivers/cxl/cxl.h         |  5 +++--
 drivers/cxl/port.c        |  9 +++++----
 3 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index d5dcc94df0a5..5132c689b1f2 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3340,7 +3340,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 		dev_name(&cxlr->dev), p->res, p->interleave_ways,
 		p->interleave_granularity);
 
-	/* ...to match put_device() in cxl_add_to_region() */
+	/* ...to match put_device() in cxl_endpoint_add() */
 	get_device(&cxlr->dev);
 	up_write(&cxl_region_rwsem);
 
@@ -3352,19 +3352,28 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 	return ERR_PTR(rc);
 }
 
-int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
+static int cxl_endpoint_initialize(struct cxl_endpoint_decoder *cxled)
 {
-	struct range *hpa = &cxled->cxld.hpa_range;
 	struct cxl_root_decoder *cxlrd;
-	struct cxl_region_params *p;
-	struct cxl_region *cxlr;
-	bool attach = false;
-	int rc;
 
 	cxlrd = cxl_find_root_decoder(cxled);
 	if (!cxlrd)
 		return -ENXIO;
 
+	cxled->cxlrd = cxlrd;
+
+	return 0;
+}
+
+static int cxl_endpoint_add(struct cxl_endpoint_decoder *cxled)
+{
+	struct range *hpa = &cxled->cxld.hpa_range;
+	struct cxl_root_decoder *cxlrd = cxled->cxlrd;
+	struct cxl_region_params *p;
+	struct cxl_region *cxlr;
+	bool attach = false;
+	int rc;
+
 	/*
 	 * Ensure that if multiple threads race to construct_region() for @hpa
 	 * one does the construction and the others add to that.
@@ -3401,7 +3410,18 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
 
 	return rc;
 }
-EXPORT_SYMBOL_NS_GPL(cxl_add_to_region, "CXL");
+
+int cxl_endpoint_register(struct cxl_endpoint_decoder *cxled)
+{
+	int rc;
+
+	rc = cxl_endpoint_initialize(cxled);
+	if (rc)
+		return rc;
+
+	return cxl_endpoint_add(cxled);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_endpoint_register, "CXL");
 
 static int is_system_ram(struct resource *res, void *arg)
 {
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 5c1a55181e0f..b3989dc58ed1 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -417,6 +417,7 @@ enum cxl_decoder_state {
  */
 struct cxl_endpoint_decoder {
 	struct cxl_decoder cxld;
+	struct cxl_root_decoder *cxlrd;
 	struct resource *dpa_res;
 	resource_size_t skip;
 	enum cxl_decoder_mode mode;
@@ -872,7 +873,7 @@ struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_port *port);
 #ifdef CONFIG_CXL_REGION
 bool is_cxl_pmem_region(struct device *dev);
 struct cxl_pmem_region *to_cxl_pmem_region(struct device *dev);
-int cxl_add_to_region(struct cxl_endpoint_decoder *cxled);
+int cxl_endpoint_register(struct cxl_endpoint_decoder *cxled);
 struct cxl_dax_region *to_cxl_dax_region(struct device *dev);
 #else
 static inline bool is_cxl_pmem_region(struct device *dev)
@@ -883,7 +884,7 @@ static inline struct cxl_pmem_region *to_cxl_pmem_region(struct device *dev)
 {
 	return NULL;
 }
-static inline int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
+static inline int cxl_endpoint_register(struct cxl_endpoint_decoder *cxled)
 {
 	return 0;
 }
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 74587a403e3d..6eb82a118bd5 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -46,13 +46,14 @@ static int discover_region(struct device *dev, void *unused)
 		return 0;
 
 	/*
-	 * Region enumeration is opportunistic, if this add-event fails,
+	 * Region enumeration is opportunistic, ignore errors and
 	 * continue to the next endpoint decoder.
 	 */
-	rc = cxl_add_to_region(cxled);
+	rc = cxl_endpoint_register(cxled);
 	if (rc)
-		dev_dbg(dev, "failed to add to region: %#llx-%#llx\n",
-			cxled->cxld.hpa_range.start, cxled->cxld.hpa_range.end);
+		dev_warn(cxled->cxld.dev.parent,
+			"failed to register %s: %d\n",
+			dev_name(&cxled->cxld.dev), rc);
 
 	return 0;
 }
-- 
2.39.5

On 1/7/2025 10:09 PM, Robert Richter wrote:
> Before adding an endpoint to a region, the endpoint is initialized
> first. Move that part to a new function cxl_endpoint_initialize().
> The function is in preparation of adding more parameters that need to
> be determined in a setup.
>
> The split also helps better separating the code. After initialization
> the addition of an endpoint may fail with an error code and all the
> data would need to be reverted to not leave the endpoint in an
> undefined state. With separate functions the init part can succeed
> even if the endpoint cannot be added.
>
> Function naming follows the style of device_register() etc. Thus,
> rename function cxl_add_to_region() to cxl_endpoint_register().
>
> Signed-off-by: Robert Richter <rrichter@amd.com>
> ---
>  drivers/cxl/core/region.c | 36 ++++++++++++++++++++++++++++--------
>  drivers/cxl/cxl.h         |  5 +++--
>  drivers/cxl/port.c        |  9 +++++----
>  3 files changed, 36 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index d5dcc94df0a5..5132c689b1f2 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3340,7 +3340,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>  		dev_name(&cxlr->dev), p->res, p->interleave_ways,
>  		p->interleave_granularity);
>  
> -	/* ...to match put_device() in cxl_add_to_region() */
> +	/* ...to match put_device() in cxl_endpoint_add() */
>  	get_device(&cxlr->dev);
>  	up_write(&cxl_region_rwsem);
>  
> @@ -3352,19 +3352,28 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>  	return ERR_PTR(rc);
>  }
>  
> -int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
> +static int cxl_endpoint_initialize(struct cxl_endpoint_decoder *cxled)
>  {
> -	struct range *hpa = &cxled->cxld.hpa_range;
>  	struct cxl_root_decoder *cxlrd;
> -	struct cxl_region_params *p;
> -	struct cxl_region *cxlr;
> -	bool attach = false;
> -	int rc;
>  
>  	cxlrd = cxl_find_root_decoder(cxled);
>  	if (!cxlrd)
>  		return -ENXIO;
>  
> +	cxled->cxlrd = cxlrd;
> +
> +	return 0;
> +}
> +
> +static int cxl_endpoint_add(struct cxl_endpoint_decoder *cxled)
> +{
> +	struct range *hpa = &cxled->cxld.hpa_range;
> +	struct cxl_root_decoder *cxlrd = cxled->cxlrd;
> +	struct cxl_region_params *p;
> +	struct cxl_region *cxlr;
> +	bool attach = false;
> +	int rc;
> +
>  	/*
>  	 * Ensure that if multiple threads race to construct_region() for @hpa
>  	 * one does the construction and the others add to that.
> @@ -3401,7 +3410,18 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
>  
>  	return rc;
>  }
> -EXPORT_SYMBOL_NS_GPL(cxl_add_to_region, "CXL");
> +
> +int cxl_endpoint_register(struct cxl_endpoint_decoder *cxled)
> +{
> +	int rc;
> +
> +	rc = cxl_endpoint_initialize(cxled);
> +	if (rc)
> +		return rc;
> +
> +	return cxl_endpoint_add(cxled);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_endpoint_register, "CXL");

Hi Robert,


cxl_endpoint_initialize(), cxl_endpoint_add(), cxl_endpoint_register() feels like some functions related to an endpoint, but I think they are for an endpoint decoder enabling, maybe rename them to cxl_endpoint_decoder_initialize()/add()/register()?


Ming

Ming,

On 09.01.25 09:08:32, Li Ming wrote:
> On 1/7/2025 10:09 PM, Robert Richter wrote:

> > +int cxl_endpoint_register(struct cxl_endpoint_decoder *cxled)
> > +{
> > +	int rc;
> > +
> > +	rc = cxl_endpoint_initialize(cxled);
> > +	if (rc)
> > +		return rc;
> > +
> > +	return cxl_endpoint_add(cxled);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_endpoint_register, "CXL");
> 
> Hi Robert,

> cxl_endpoint_initialize(), cxl_endpoint_add(),
> cxl_endpoint_register() feels like some functions related to an
> endpoint, but I think they are for an endpoint decoder enabling,
> maybe rename them to
> cxl_endpoint_decoder_initialize()/add()/register()?

Yes, this handles the endpoint decoder. I noticed that too but kept
the short naming. Will rename it. This aligns then with other existing
cxl_endpoint_decoder_*() functions.

Thanks for review,

-Robert

On Tue, Jan 07, 2025 at 03:09:54PM +0100, Robert Richter wrote:
> Before adding an endpoint to a region, the endpoint is initialized
> first. Move that part to a new function cxl_endpoint_initialize().
> The function is in preparation of adding more parameters that need to
> be determined in a setup.
> 
> The split also helps better separating the code. After initialization
> the addition of an endpoint may fail with an error code and all the
> data would need to be reverted to not leave the endpoint in an
> undefined state. With separate functions the init part can succeed
> even if the endpoint cannot be added.
> 
> Function naming follows the style of device_register() etc. Thus,
> rename function cxl_add_to_region() to cxl_endpoint_register().
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>

Little bit difficult to read mixing style and functionality, but I like
this update and I understand why. One inline question

Reviewed-by: Gregory Price <gourry@gourry.net>

> ---
>  drivers/cxl/core/region.c | 36 ++++++++++++++++++++++++++++--------
>  drivers/cxl/cxl.h         |  5 +++--
>  drivers/cxl/port.c        |  9 +++++----
>  3 files changed, 36 insertions(+), 14 deletions(-)
> 
... snip ...
> +	rc = cxl_endpoint_register(cxled);
>  	if (rc)
> -		dev_dbg(dev, "failed to add to region: %#llx-%#llx\n",
> -			cxled->cxld.hpa_range.start, cxled->cxld.hpa_range.end);
> +		dev_warn(cxled->cxld.dev.parent,
> +			"failed to register %s: %d\n",
> +			dev_name(&cxled->cxld.dev), rc);

Is it worth differentiating obvious failures here for a better warning?
I'm fine either way.

~Gregory

On Tue, Jan 07, 2025 at 01:29:03PM -0500, Gregory Price wrote:
> On Tue, Jan 07, 2025 at 03:09:54PM +0100, Robert Richter wrote:

> ... snip ...
> > +	rc = cxl_endpoint_register(cxled);
> >  	if (rc)
> > -		dev_dbg(dev, "failed to add to region: %#llx-%#llx\n",
> > -			cxled->cxld.hpa_range.start, cxled->cxld.hpa_range.end);
> > +		dev_warn(cxled->cxld.dev.parent,
> > +			"failed to register %s: %d\n",
> > +			dev_name(&cxled->cxld.dev), rc);
> 
> Is it worth differentiating obvious failures here for a better warning?
> I'm fine either way.

If an endpoint cannot be registered, this will likly cause a region
probe failure too. I raised the log level to make this visible for
non-dbg logging. I have also removed access to cxled->cxld.hpa_range
as this is implemenation specific to cxl_endpoint_register(). There
are other debug messages to determine the details of the failure here.

-Robert