[PATCH v3 00/18] cxl: Address translation support, part 1: Cleanups and refactoring

[PATCH v3 00/18] cxl: Address translation support, part 1: Cleanups and refactoring

[Robert Richter]

This series is the first part of adding support for CXL address
translation. It contains cleanup and code refactoring in preparation
of the actual implementation that will be sent in part 2. Cleanup and
code refactoring have been split in a separate series to reduce the
number of patches of the series. Even without address translation on
top this rework improves esp. the region code, cleans it up,
simplifies it and adds debugging messages to better analyze region
creation failures:

Content of patches:

 * Patches 1: Remove else after return.

 * Patches 2-4: Cleanups and comments around cxl_hdm_decode_init().

 * Patches 5, 6, 13: Adding and modifying helper functions.

 * Patches 7-12, 14: Refactoring of endpoint decoder setup.

 * Patches 15-18: Adding and modifying debug messages.

Most of the patches were part of my first submission of v1 [1], some
of them were already reviewed.

[1] https://lore.kernel.org/linux-cxl/20250107141015.3367194-1-rrichter@amd.com/

v3:
 * added tags to SOB chain,
 * fixed NULL pointer dereference in cxl_find_root_decoder() (Alison),
 * updated subject line of patches that add kernel messages and
   included example log messages (Alison),

V2:
 * rebased onto cxl/next,
 * added tags to SOB chain,
 * move patches with cleanups and refactoring into this separate
   series (Dave),
 * added patch "cxl/acpi: Unify CFMWS memory log messages with SRAT
   messages" to improve CFMWS log messages,
 * renamed endpoint decoder functions to cxl_endpoint_decoder_*() (Li),
 * reworded patch description that moves find_cxl_root() and reworks
   cxl_find_root_decoder() (Terry),
 * small changes to cxl_find_root_decoder()/
   cxl_endpoint_decoder_initialize() (Jonanthan),
 * updated comment in cxl_port_find_switch_decoder() (Ben),
 * cxl_endpoint_decoder_initialize(): Simplify variable declaration
   (Jonathan, Ben),
 * cxl_find_decoder_early(): Added comment on function usage (Gregory),
 * reordered patches and reworded some of the subject for a better
   structure.

Robert Richter (18):
  cxl: Remove else after return
  cxl/pci: Moving code in cxl_hdm_decode_init()
  cxl/pci: cxl_hdm_decode_init: Move comment
  cxl/pci: Add comments to cxl_hdm_decode_init()
  cxl: Introduce parent_port_of() helper
  cxl/region: Rename function to cxl_find_decoder_early()
  cxl/region: Avoid duplicate call of cxl_find_decoder_early()
  cxl/region: Move find_cxl_root() to cxl_add_to_region()
  cxl/region: Factor out code to find the root decoder
  cxl/region: Factor out code to find a root decoder's region
  cxl/region: Split region registration into an initialization and
    adding part
  cxl/region: Use iterator to find the root port in
    cxl_find_root_decoder()
  cxl/region: Add function to find a port's switch decoder by range
  cxl/region: Unfold cxl_find_root_decoder() into
    cxl_endpoint_decoder_initialize()
  cxl/region: Add a dev_warn() on registration failure
  cxl/region: Add a dev_err() on missing target list entries
  cxl: Add a dev_dbg() when a decoder was added to a port
  cxl/acpi: Unify CFMWS memory log messages with SRAT messages

 drivers/cxl/acpi.c        |  12 ++-
 drivers/cxl/core/cdat.c   |   2 +-
 drivers/cxl/core/hdm.c    |   3 +-
 drivers/cxl/core/pci.c    |  44 +++++----
 drivers/cxl/core/port.c   |  15 +--
 drivers/cxl/core/region.c | 190 +++++++++++++++++++++++++-------------
 drivers/cxl/cxl.h         |   9 +-
 drivers/cxl/port.c        |  22 ++---
 8 files changed, 187 insertions(+), 110 deletions(-)


base-commit: 5585e342e8d38cc598279bdb87f235f8b954dd5a
-- 
2.39.5

[PATCH v3 01/18] cxl: Remove else after return

[Robert Richter]

Remove unnecessary 'else' after return. Improves readability of code.
It is easier to place comments. Check and fix all occurrences under
drivers/cxl/.

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/cdat.c   | 2 +-
 drivers/cxl/core/pci.c    | 3 ++-
 drivers/cxl/core/region.c | 4 +++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
index bfba7f5019cf..0a09add46d5c 100644
--- a/drivers/cxl/core/cdat.c
+++ b/drivers/cxl/core/cdat.c
@@ -28,7 +28,7 @@ static u32 cdat_normalize(u16 entry, u64 base, u8 type)
 	 */
 	if (entry == 0xffff || !entry)
 		return 0;
-	else if (base > (UINT_MAX / (entry)))
+	if (base > (UINT_MAX / (entry)))
 		return 0;
 
 	/*
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index a5c65f79db18..2ec8c97ab160 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -415,7 +415,8 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	 */
 	if (global_ctrl & CXL_HDM_DECODER_ENABLE || (!hdm && info->mem_enabled))
 		return devm_cxl_enable_mem(&port->dev, cxlds);
-	else if (!hdm)
+
+	if (!hdm)
 		return -ENODEV;
 
 	root = to_cxl_port(port->dev.parent);
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 84ce625b8591..5d252dfae138 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1926,7 +1926,9 @@ static int cxl_region_attach(struct cxl_region *cxlr,
 	if (p->state > CXL_CONFIG_INTERLEAVE_ACTIVE) {
 		dev_dbg(&cxlr->dev, "region already active\n");
 		return -EBUSY;
-	} else if (p->state < CXL_CONFIG_INTERLEAVE_ACTIVE) {
+	}
+
+	if (p->state < CXL_CONFIG_INTERLEAVE_ACTIVE) {
 		dev_dbg(&cxlr->dev, "interleave config missing\n");
 		return -ENXIO;
 	}
-- 
2.39.5

[PATCH v3 02/18] cxl/pci: Moving code in cxl_hdm_decode_init()

[Robert Richter]

Commit 3f9e07531778 ("cxl/pci: simplify the check of mem_enabled in
cxl_hdm_decode_init()") changed the code flow in this function. The
root port is determined before a check to leave the function. Since
the root port is not used by the check it can be moved to run the
check first. This improves code readability and avoids unnesessary
code execution.

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/pci.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 2ec8c97ab160..f8e22bc278c3 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -419,14 +419,6 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	if (!hdm)
 		return -ENODEV;
 
-	root = to_cxl_port(port->dev.parent);
-	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
-		root = to_cxl_port(root->dev.parent);
-	if (!is_cxl_root(root)) {
-		dev_err(dev, "Failed to acquire root port for HDM enable\n");
-		return -ENODEV;
-	}
-
 	if (!info->mem_enabled) {
 		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
 		if (rc)
@@ -435,6 +427,14 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 		return devm_cxl_enable_mem(&port->dev, cxlds);
 	}
 
+	root = to_cxl_port(port->dev.parent);
+	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
+		root = to_cxl_port(root->dev.parent);
+	if (!is_cxl_root(root)) {
+		dev_err(dev, "Failed to acquire root port for HDM enable\n");
+		return -ENODEV;
+	}
+
 	for (i = 0, allowed = 0; i < info->ranges; i++) {
 		struct device *cxld_dev;
 
-- 
2.39.5

Re: [PATCH v3 02/18] cxl/pci: Moving code in cxl_hdm_decode_init()

[Jonathan Cameron]

On Tue, 11 Feb 2025 10:53:32 +0100
Robert Richter <rrichter@amd.com> wrote:

> Commit 3f9e07531778 ("cxl/pci: simplify the check of mem_enabled in
> cxl_hdm_decode_init()") changed the code flow in this function. The
> root port is determined before a check to leave the function. Since
> the root port is not used by the check it can be moved to run the
> check first. This improves code readability and avoids unnesessary
> code execution.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> Tested-by: Gregory Price <gourry@gourry.net>
Seems reasonable.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v3 02/18] cxl/pci: Moving code in cxl_hdm_decode_init()

[Dave Jiang]

On 2/11/25 2:53 AM, Robert Richter wrote:
> Commit 3f9e07531778 ("cxl/pci: simplify the check of mem_enabled in
> cxl_hdm_decode_init()") changed the code flow in this function. The
> root port is determined before a check to leave the function. Since
> the root port is not used by the check it can be moved to run the
> check first. This improves code readability and avoids unnesessary
> code execution.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> Tested-by: Gregory Price <gourry@gourry.net>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/pci.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 2ec8c97ab160..f8e22bc278c3 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -419,14 +419,6 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>  	if (!hdm)
>  		return -ENODEV;
>  
> -	root = to_cxl_port(port->dev.parent);
> -	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
> -		root = to_cxl_port(root->dev.parent);
> -	if (!is_cxl_root(root)) {
> -		dev_err(dev, "Failed to acquire root port for HDM enable\n");
> -		return -ENODEV;
> -	}
> -
>  	if (!info->mem_enabled) {
>  		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
>  		if (rc)
> @@ -435,6 +427,14 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>  		return devm_cxl_enable_mem(&port->dev, cxlds);
>  	}
>  
> +	root = to_cxl_port(port->dev.parent);
> +	while (!is_cxl_root(root) && is_cxl_port(root->dev.parent))
> +		root = to_cxl_port(root->dev.parent);
> +	if (!is_cxl_root(root)) {
> +		dev_err(dev, "Failed to acquire root port for HDM enable\n");
> +		return -ENODEV;
> +	}
> +
>  	for (i = 0, allowed = 0; i < info->ranges; i++) {
>  		struct device *cxld_dev;
>  

[PATCH v3 03/18] cxl/pci: cxl_hdm_decode_init: Move comment

[Robert Richter]

The comment applies to the check, move it there.

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/pci.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index f8e22bc278c3..c49efc419285 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -419,6 +419,15 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	if (!hdm)
 		return -ENODEV;
 
+	/*
+	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
+	 * [High,Low] when HDM operation is enabled the range register values
+	 * are ignored by the device, but the spec also recommends matching the
+	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
+	 * are expected even though Linux does not require or maintain that
+	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
+	 * Decoder Capability Enable.
+	 */
 	if (!info->mem_enabled) {
 		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
 		if (rc)
@@ -454,15 +463,6 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 		return -ENXIO;
 	}
 
-	/*
-	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
-	 * [High,Low] when HDM operation is enabled the range register values
-	 * are ignored by the device, but the spec also recommends matching the
-	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
-	 * are expected even though Linux does not require or maintain that
-	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
-	 * Decoder Capability Enable.
-	 */
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, "CXL");
-- 
2.39.5

Re: [PATCH v3 03/18] cxl/pci: cxl_hdm_decode_init: Move comment

[Jonathan Cameron]

On Tue, 11 Feb 2025 10:53:33 +0100
Robert Richter <rrichter@amd.com> wrote:

> The comment applies to the check, move it there.

I think I disagree. It was in the right place as far as I can tell.
It is an odd place for comment, but it's kind of describing
why it is not an error to get down there.

> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
> ---
>  drivers/cxl/core/pci.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index f8e22bc278c3..c49efc419285 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -419,6 +419,15 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>  	if (!hdm)
>  		return -ENODEV;
>  
> +	/*
> +	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
> +	 * [High,Low] when HDM operation is enabled the range register values
> +	 * are ignored by the device, but the spec also recommends matching the
> +	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
> +	 * are expected even though Linux does not require or maintain that
> +	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
> +	 * Decoder Capability Enable.

This check is about mem_enabled. Would be fine to add another comment here to
say.

	/*
	 * If mem_enabled is not set prior configuration is irrelevant and we
	 * can do what we like so enable HDM decoders and ignore DVSEC registers.
	 */
> +	 */
>  	if (!info->mem_enabled) {
>  		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
>  		if (rc)
> @@ -454,15 +463,6 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>  		return -ENXIO;
>  	}
>  
> -	/*
> -	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
> -	 * [High,Low] when HDM operation is enabled the range register values
> -	 * are ignored by the device, but the spec also recommends matching the
> -	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
> -	 * are expected even though Linux does not require or maintain that
> -	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
> -	 * Decoder Capability Enable.
> -	 */

This is the path the comment is talking about because only if we get to this
return path are we 'skipping' the HDM decoder capability and not returning
an error.  The path representing an HDM decoder equipped device that
was configured by a BIOS that decided to use the DVSEC registers.

I'm not sure why we care about how the hdm decoders are programmed inthis
case though.

I'm confused :(

>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, "CXL");

Re: [PATCH v3 03/18] cxl/pci: cxl_hdm_decode_init: Move comment

[Robert Richter]

On 12.02.25 18:09:10, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 10:53:33 +0100
> Robert Richter <rrichter@amd.com> wrote:
> 
> > The comment applies to the check, move it there.
> 
> I think I disagree. It was in the right place as far as I can tell.
> It is an odd place for comment, but it's kind of describing
> why it is not an error to get down there.

Ah, that was not obvious to the reader. :-)

> 
> > 
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > Reviewed-by: Gregory Price <gourry@gourry.net>
> > Tested-by: Gregory Price <gourry@gourry.net>
> > ---
> >  drivers/cxl/core/pci.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index f8e22bc278c3..c49efc419285 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -419,6 +419,15 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
> >  	if (!hdm)
> >  		return -ENODEV;
> >  
> > +	/*
> > +	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
> > +	 * [High,Low] when HDM operation is enabled the range register values
> > +	 * are ignored by the device, but the spec also recommends matching the
> > +	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
> > +	 * are expected even though Linux does not require or maintain that
> > +	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
> > +	 * Decoder Capability Enable.
> 
> This check is about mem_enabled. Would be fine to add another comment here to
> say.

The next patch extends the comment for more clarification (I hope so).

> 
> 	/*
> 	 * If mem_enabled is not set prior configuration is irrelevant and we
> 	 * can do what we like so enable HDM decoders and ignore DVSEC registers.
> 	 */
> > +	 */
> >  	if (!info->mem_enabled) {
> >  		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
> >  		if (rc)
> > @@ -454,15 +463,6 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
> >  		return -ENXIO;
> >  	}
> >  
> > -	/*
> > -	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
> > -	 * [High,Low] when HDM operation is enabled the range register values
> > -	 * are ignored by the device, but the spec also recommends matching the
> > -	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
> > -	 * are expected even though Linux does not require or maintain that
> > -	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
> > -	 * Decoder Capability Enable.
> > -	 */
> 
> This is the path the comment is talking about because only if we get to this
> return path are we 'skipping' the HDM decoder capability and not returning
> an error.  The path representing an HDM decoder equipped device that
> was configured by a BIOS that decided to use the DVSEC registers.
> 
> I'm not sure why we care about how the hdm decoders are programmed inthis
> case though.
> 
> I'm confused :(

There is an HDM, but it is disabled (CXL_HDM_DECODER_ENABLE is
cleared). If the DVSEC range regs do not have valid values
(!info->mem_enabled, firmware indicates it is not used), just go and
enable the HDM.

We try to use the hdm decoders here to be able to use them for a
non-auto setup. Else, decoder emulation is used
(should_emulate_decoders()) and decoders are locked
(CXL_DECODER_F_LOCK will be set).

Maybe take a look at the whole change with added comments including
patch 4/18?

I hope to not add confusion here. :-)

-Robert

> 
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, "CXL");
> 

Re: [PATCH v3 03/18] cxl/pci: cxl_hdm_decode_init: Move comment

[Jonathan Cameron]

On Thu, 13 Feb 2025 01:35:29 +0100
Robert Richter <rrichter@amd.com> wrote:

> On 12.02.25 18:09:10, Jonathan Cameron wrote:
> > On Tue, 11 Feb 2025 10:53:33 +0100
> > Robert Richter <rrichter@amd.com> wrote:
> >   
> > > The comment applies to the check, move it there.  
> > 
> > I think I disagree. It was in the right place as far as I can tell.
> > It is an odd place for comment, but it's kind of describing
> > why it is not an error to get down there.  
> 
> Ah, that was not obvious to the reader. :-)
> 
> >   
> > > 
> > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > > Reviewed-by: Gregory Price <gourry@gourry.net>
> > > Tested-by: Gregory Price <gourry@gourry.net>
> > > ---
> > >  drivers/cxl/core/pci.c | 18 +++++++++---------
> > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > > index f8e22bc278c3..c49efc419285 100644
> > > --- a/drivers/cxl/core/pci.c
> > > +++ b/drivers/cxl/core/pci.c
> > > @@ -419,6 +419,15 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
> > >  	if (!hdm)
> > >  		return -ENODEV;
> > >  
> > > +	/*
> > > +	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
> > > +	 * [High,Low] when HDM operation is enabled the range register values
> > > +	 * are ignored by the device, but the spec also recommends matching the
> > > +	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
> > > +	 * are expected even though Linux does not require or maintain that
> > > +	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
> > > +	 * Decoder Capability Enable.  
> > 
> > This check is about mem_enabled. Would be fine to add another comment here to
> > say.  
> 
> The next patch extends the comment for more clarification (I hope so).

Not to me.  It says 'else' when referring to what happens in the if.

> 
> > 
> > 	/*
> > 	 * If mem_enabled is not set prior configuration is irrelevant and we
> > 	 * can do what we like so enable HDM decoders and ignore DVSEC registers.
> > 	 */  
> > > +	 */
> > >  	if (!info->mem_enabled) {
> > >  		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
> > >  		if (rc)
> > > @@ -454,15 +463,6 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
> > >  		return -ENXIO;
> > >  	}
> > >  
> > > -	/*
> > > -	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
> > > -	 * [High,Low] when HDM operation is enabled the range register values
> > > -	 * are ignored by the device, but the spec also recommends matching the
> > > -	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
> > > -	 * are expected even though Linux does not require or maintain that
> > > -	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
> > > -	 * Decoder Capability Enable.
> > > -	 */  
> > 
> > This is the path the comment is talking about because only if we get to this
> > return path are we 'skipping' the HDM decoder capability and not returning
> > an error.  The path representing an HDM decoder equipped device that
> > was configured by a BIOS that decided to use the DVSEC registers.
> > 
> > I'm not sure why we care about how the hdm decoders are programmed inthis
> > case though.
> > 
> > I'm confused :(  
> 
> There is an HDM, but it is disabled (CXL_HDM_DECODER_ENABLE is
> cleared). If the DVSEC range regs do not have valid values
> (!info->mem_enabled, firmware indicates it is not used), just go and
> enable the HDM.
> 
> We try to use the hdm decoders here to be able to use them for a
> non-auto setup. Else, decoder emulation is used
> (should_emulate_decoders()) and decoders are locked
> (CXL_DECODER_F_LOCK will be set).
> 
> Maybe take a look at the whole change with added comments including
> patch 4/18?
> 
> I hope to not add confusion here. :-)
> 
> -Robert
> 
> >   
> > >  	return 0;
> > >  }
> > >  EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, "CXL");  
> >   

Re: [PATCH v3 03/18] cxl/pci: cxl_hdm_decode_init: Move comment

[Robert Richter]

On 14.02.25 15:49:55, Jonathan Cameron wrote:
> On Thu, 13 Feb 2025 01:35:29 +0100
> Robert Richter <rrichter@amd.com> wrote:
> > On 12.02.25 18:09:10, Jonathan Cameron wrote:
> > > On Tue, 11 Feb 2025 10:53:33 +0100
> > > Robert Richter <rrichter@amd.com> wrote:

> > > > +	/*
> > > > +	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
> > > > +	 * [High,Low] when HDM operation is enabled the range register values
> > > > +	 * are ignored by the device, but the spec also recommends matching the
> > > > +	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
> > > > +	 * are expected even though Linux does not require or maintain that
> > > > +	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
> > > > +	 * Decoder Capability Enable.  
> > > 
> > > This check is about mem_enabled. Would be fine to add another comment here to
> > > say.  
> > 
> > The next patch extends the comment for more clarification (I hope so).
> 
> Not to me.  It says 'else' when referring to what happens in the if.

I have dropped this patch and updated the comments in the next patch
along with the patch description.

-Robert

[PATCH v3 04/18] cxl/pci: Add comments to cxl_hdm_decode_init()

[Robert Richter]

There are various configuration cases of HDM decoder registers causing
different code paths. Add comments to cxl_hdm_decode_init() to better
explain them.

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/pci.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index c49efc419285..6333a01e4f19 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -416,9 +416,17 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	if (global_ctrl & CXL_HDM_DECODER_ENABLE || (!hdm && info->mem_enabled))
 		return devm_cxl_enable_mem(&port->dev, cxlds);
 
+	/*
+	 * If the HDM Decoder Capability does not exist and DVSEC was
+	 * not setup, the DVSEC based emulation cannot be used.
+	 */
 	if (!hdm)
 		return -ENODEV;
 
+	/*
+	 * The HDM Decoder Capability exists but is globally disabled.
+	 */
+
 	/*
 	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
 	 * [High,Low] when HDM operation is enabled the range register values
@@ -426,7 +434,8 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
 	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
 	 * are expected even though Linux does not require or maintain that
 	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
-	 * Decoder Capability Enable.
+	 * Decoder Capability Enable. Else, use the HDM Decoder Capability and
+	 * enable it.
 	 */
 	if (!info->mem_enabled) {
 		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
-- 
2.39.5

Re: [PATCH v3 04/18] cxl/pci: Add comments to cxl_hdm_decode_init()

[Jonathan Cameron]

On Tue, 11 Feb 2025 10:53:34 +0100
Robert Richter <rrichter@amd.com> wrote:

> There are various configuration cases of HDM decoder registers causing
> different code paths. Add comments to cxl_hdm_decode_init() to better
> explain them.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Tested-by: Gregory Price <gourry@gourry.net>
> ---
>  drivers/cxl/core/pci.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index c49efc419285..6333a01e4f19 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -416,9 +416,17 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>  	if (global_ctrl & CXL_HDM_DECODER_ENABLE || (!hdm && info->mem_enabled))
>  		return devm_cxl_enable_mem(&port->dev, cxlds);
>  
> +	/*
> +	 * If the HDM Decoder Capability does not exist and DVSEC was
> +	 * not setup, the DVSEC based emulation cannot be used.
> +	 */
>  	if (!hdm)
>  		return -ENODEV;
>  
> +	/*
> +	 * The HDM Decoder Capability exists but is globally disabled.
> +	 */
> +
>  	/*
>  	 * Per CXL 2.0 Section 8.1.3.8.3 and 8.1.3.8.4 DVSEC CXL Range 1 Base
>  	 * [High,Low] when HDM operation is enabled the range register values
> @@ -426,7 +434,8 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
>  	 * DVSEC Range 1,2 to HDM Decoder Range 0,1. So, non-zero info->ranges
>  	 * are expected even though Linux does not require or maintain that
>  	 * match. If at least one DVSEC range is enabled and allowed, skip HDM
> -	 * Decoder Capability Enable.
> +	 * Decoder Capability Enable. Else, use the HDM Decoder Capability and
> +	 * enable it.

As per previous.  Having an 'else' comment that refers to what happens if the
following if is true, just adds to confusion :(

So if you are going to move the comment it needs a more substantial rewrite
to reflect that we care 'here' about that last bit only.

>  	 */
>  	if (!info->mem_enabled) {
>  		rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);

[PATCH v3 05/18] cxl: Introduce parent_port_of() helper

[Robert Richter]

Often a parent port must be determined. Introduce the parent_port_of()
helper function for this.

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/port.c   | 15 +++++++++------
 drivers/cxl/core/region.c | 11 ++---------
 drivers/cxl/cxl.h         |  1 +
 3 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
index f9501a16b390..d19930c009ce 100644
--- a/drivers/cxl/core/port.c
+++ b/drivers/cxl/core/port.c
@@ -606,17 +606,20 @@ struct cxl_port *to_cxl_port(const struct device *dev)
 }
 EXPORT_SYMBOL_NS_GPL(to_cxl_port, "CXL");
 
+struct cxl_port *parent_port_of(struct cxl_port *port)
+{
+	if (!port || !port->parent_dport)
+		return NULL;
+	return port->parent_dport->port;
+}
+EXPORT_SYMBOL_NS_GPL(parent_port_of, "CXL");
+
 static void unregister_port(void *_port)
 {
 	struct cxl_port *port = _port;
-	struct cxl_port *parent;
+	struct cxl_port *parent = parent_port_of(port);
 	struct device *lock_dev;
 
-	if (is_cxl_root(port))
-		parent = NULL;
-	else
-		parent = to_cxl_port(port->dev.parent);
-
 	/*
 	 * CXL root port's and the first level of ports are unregistered
 	 * under the platform firmware device lock, all other ports are
diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 5d252dfae138..54afdb0fa61c 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1734,13 +1734,6 @@ static int cmp_interleave_pos(const void *a, const void *b)
 	return cxled_a->pos - cxled_b->pos;
 }
 
-static struct cxl_port *next_port(struct cxl_port *port)
-{
-	if (!port->parent_dport)
-		return NULL;
-	return port->parent_dport->port;
-}
-
 static int match_switch_decoder_by_range(struct device *dev,
 					 const void *data)
 {
@@ -1767,7 +1760,7 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range,
 	struct device *dev;
 	int rc = -ENXIO;
 
-	parent = next_port(port);
+	parent = parent_port_of(port);
 	if (!parent)
 		return rc;
 
@@ -1847,7 +1840,7 @@ static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
 	 */
 
 	/* Iterate from endpoint to root_port refining the position */
-	for (iter = port; iter; iter = next_port(iter)) {
+	for (iter = port; iter; iter = parent_port_of(iter)) {
 		if (is_cxl_root(iter))
 			break;
 
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 6baec4ba9141..0d7aff8b97b3 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -721,6 +721,7 @@ static inline bool is_cxl_root(struct cxl_port *port)
 int cxl_num_decoders_committed(struct cxl_port *port);
 bool is_cxl_port(const struct device *dev);
 struct cxl_port *to_cxl_port(const struct device *dev);
+struct cxl_port *parent_port_of(struct cxl_port *port);
 void cxl_port_commit_reap(struct cxl_decoder *cxld);
 struct pci_bus;
 int devm_cxl_register_pci_bus(struct device *host, struct device *uport_dev,
-- 
2.39.5

Re: [PATCH v3 05/18] cxl: Introduce parent_port_of() helper

[Dave Jiang]

On 2/11/25 2:53 AM, Robert Richter wrote:
> Often a parent port must be determined. Introduce the parent_port_of()
> helper function for this.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Tested-by: Gregory Price <gourry@gourry.net>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/port.c   | 15 +++++++++------
>  drivers/cxl/core/region.c | 11 ++---------
>  drivers/cxl/cxl.h         |  1 +
>  3 files changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index f9501a16b390..d19930c009ce 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -606,17 +606,20 @@ struct cxl_port *to_cxl_port(const struct device *dev)
>  }
>  EXPORT_SYMBOL_NS_GPL(to_cxl_port, "CXL");
>  
> +struct cxl_port *parent_port_of(struct cxl_port *port)
> +{
> +	if (!port || !port->parent_dport)
> +		return NULL;
> +	return port->parent_dport->port;
> +}
> +EXPORT_SYMBOL_NS_GPL(parent_port_of, "CXL");
> +
>  static void unregister_port(void *_port)
>  {
>  	struct cxl_port *port = _port;
> -	struct cxl_port *parent;
> +	struct cxl_port *parent = parent_port_of(port);
>  	struct device *lock_dev;
>  
> -	if (is_cxl_root(port))
> -		parent = NULL;
> -	else
> -		parent = to_cxl_port(port->dev.parent);
> -
>  	/*
>  	 * CXL root port's and the first level of ports are unregistered
>  	 * under the platform firmware device lock, all other ports are
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 5d252dfae138..54afdb0fa61c 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1734,13 +1734,6 @@ static int cmp_interleave_pos(const void *a, const void *b)
>  	return cxled_a->pos - cxled_b->pos;
>  }
>  
> -static struct cxl_port *next_port(struct cxl_port *port)
> -{
> -	if (!port->parent_dport)
> -		return NULL;
> -	return port->parent_dport->port;
> -}
> -
>  static int match_switch_decoder_by_range(struct device *dev,
>  					 const void *data)
>  {
> @@ -1767,7 +1760,7 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range,
>  	struct device *dev;
>  	int rc = -ENXIO;
>  
> -	parent = next_port(port);
> +	parent = parent_port_of(port);
>  	if (!parent)
>  		return rc;
>  
> @@ -1847,7 +1840,7 @@ static int cxl_calc_interleave_pos(struct cxl_endpoint_decoder *cxled)
>  	 */
>  
>  	/* Iterate from endpoint to root_port refining the position */
> -	for (iter = port; iter; iter = next_port(iter)) {
> +	for (iter = port; iter; iter = parent_port_of(iter)) {
>  		if (is_cxl_root(iter))
>  			break;
>  
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 6baec4ba9141..0d7aff8b97b3 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -721,6 +721,7 @@ static inline bool is_cxl_root(struct cxl_port *port)
>  int cxl_num_decoders_committed(struct cxl_port *port);
>  bool is_cxl_port(const struct device *dev);
>  struct cxl_port *to_cxl_port(const struct device *dev);
> +struct cxl_port *parent_port_of(struct cxl_port *port);
>  void cxl_port_commit_reap(struct cxl_decoder *cxld);
>  struct pci_bus;
>  int devm_cxl_register_pci_bus(struct device *host, struct device *uport_dev,

[PATCH v3 06/18] cxl/region: Rename function to cxl_find_decoder_early()

[Robert Richter]

Current function cxl_region_find_decoder() is used to find a port's
decoder during region setup. The decoder is later used to attach the
port to a region.

Rename function to cxl_find_decoder_early() to emphasize its use only
during region setup in the early setup stage. Once a port is attached
to a region, the region reference can be used to lookup a region's
port and decoder configuration (see struct cxl_region_ref).

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/region.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 54afdb0fa61c..13e3ba984a53 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -850,10 +850,17 @@ static int match_auto_decoder(struct device *dev, const void *data)
 	return 0;
 }
 
+/*
+ * Use cxl_find_decoder_early() only during region setup in the early
+ * setup stage. Once a port is attached to a region, the region
+ * reference can be used to lookup a region's port and decoder
+ * configuration (see struct cxl_region_ref).
+*/
+
 static struct cxl_decoder *
-cxl_region_find_decoder(struct cxl_port *port,
-			struct cxl_endpoint_decoder *cxled,
-			struct cxl_region *cxlr)
+cxl_find_decoder_early(struct cxl_port *port,
+		       struct cxl_endpoint_decoder *cxled,
+		       struct cxl_region *cxlr)
 {
 	struct device *dev;
 
@@ -917,7 +924,7 @@ alloc_region_ref(struct cxl_port *port, struct cxl_region *cxlr,
 		if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
 			struct cxl_decoder *cxld;
 
-			cxld = cxl_region_find_decoder(port, cxled, cxlr);
+			cxld = cxl_find_decoder_early(port, cxled, cxlr);
 			if (auto_order_ok(port, iter->region, cxld))
 				continue;
 		}
@@ -1005,7 +1012,7 @@ static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
 {
 	struct cxl_decoder *cxld;
 
-	cxld = cxl_region_find_decoder(port, cxled, cxlr);
+	cxld = cxl_find_decoder_early(port, cxled, cxlr);
 	if (!cxld) {
 		dev_dbg(&cxlr->dev, "%s: no decoder available\n",
 			dev_name(&port->dev));
-- 
2.39.5

Re: [PATCH v3 06/18] cxl/region: Rename function to cxl_find_decoder_early()

[Jonathan Cameron]

On Tue, 11 Feb 2025 10:53:36 +0100
Robert Richter <rrichter@amd.com> wrote:

> Current function cxl_region_find_decoder() is used to find a port's
> decoder during region setup. The decoder is later used to attach the
> port to a region.
> 
> Rename function to cxl_find_decoder_early() to emphasize its use only
> during region setup in the early setup stage. Once a port is attached
> to a region, the region reference can be used to lookup a region's
> port and decoder configuration (see struct cxl_region_ref).

Early doesn't seem that well defined to me. Can we indicate what the state
is more explicitly?

Or does it actually matter?  Whilst there is a better way to get
it later, does this function then return the wrong answer?

Or if we have both cases of 'finding' it, can we just make this
code do both?

Jonathan

> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
> ---
>  drivers/cxl/core/region.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 54afdb0fa61c..13e3ba984a53 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -850,10 +850,17 @@ static int match_auto_decoder(struct device *dev, const void *data)
>  	return 0;
>  }
>  
> +/*
> + * Use cxl_find_decoder_early() only during region setup in the early
> + * setup stage. Once a port is attached to a region, the region
> + * reference can be used to lookup a region's port and decoder
> + * configuration (see struct cxl_region_ref).
> +*/
> +
>  static struct cxl_decoder *
> -cxl_region_find_decoder(struct cxl_port *port,
> -			struct cxl_endpoint_decoder *cxled,
> -			struct cxl_region *cxlr)
> +cxl_find_decoder_early(struct cxl_port *port,
> +		       struct cxl_endpoint_decoder *cxled,
> +		       struct cxl_region *cxlr)
>  {
>  	struct device *dev;
>  
> @@ -917,7 +924,7 @@ alloc_region_ref(struct cxl_port *port, struct cxl_region *cxlr,
>  		if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
>  			struct cxl_decoder *cxld;
>  
> -			cxld = cxl_region_find_decoder(port, cxled, cxlr);
> +			cxld = cxl_find_decoder_early(port, cxled, cxlr);
>  			if (auto_order_ok(port, iter->region, cxld))
>  				continue;
>  		}
> @@ -1005,7 +1012,7 @@ static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
>  {
>  	struct cxl_decoder *cxld;
>  
> -	cxld = cxl_region_find_decoder(port, cxled, cxlr);
> +	cxld = cxl_find_decoder_early(port, cxled, cxlr);
>  	if (!cxld) {
>  		dev_dbg(&cxlr->dev, "%s: no decoder available\n",
>  			dev_name(&port->dev));

Re: [PATCH v3 06/18] cxl/region: Rename function to cxl_find_decoder_early()

[Robert Richter]

On 14.02.25 15:58:08, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 10:53:36 +0100
> Robert Richter <rrichter@amd.com> wrote:
> 
> > Current function cxl_region_find_decoder() is used to find a port's
> > decoder during region setup. The decoder is later used to attach the
> > port to a region.
> > 
> > Rename function to cxl_find_decoder_early() to emphasize its use only
> > during region setup in the early setup stage. Once a port is attached
> > to a region, the region reference can be used to lookup a region's
> > port and decoder configuration (see struct cxl_region_ref).
> 
> Early doesn't seem that well defined to me. Can we indicate what the state
> is more explicitly?

'early' is used here as common term in the kernel to run pre-init
code. Here that means, an endpoint is not yet attached to a region
(cxl_region_attach()).

> 
> Or does it actually matter?  Whilst there is a better way to get
> it later, does this function then return the wrong answer?

There region references are setup and should be used then.

> 
> Or if we have both cases of 'finding' it, can we just make this
> code do both?

No, there is no user.

I will update description and comment in the code to better explain
the 'early' state.

-Robert

> 
> Jonathan
> 
> > 
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > Reviewed-by: Gregory Price <gourry@gourry.net>
> > Tested-by: Gregory Price <gourry@gourry.net>
> > ---
> >  drivers/cxl/core/region.c | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> > index 54afdb0fa61c..13e3ba984a53 100644
> > --- a/drivers/cxl/core/region.c
> > +++ b/drivers/cxl/core/region.c
> > @@ -850,10 +850,17 @@ static int match_auto_decoder(struct device *dev, const void *data)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Use cxl_find_decoder_early() only during region setup in the early
> > + * setup stage. Once a port is attached to a region, the region
> > + * reference can be used to lookup a region's port and decoder
> > + * configuration (see struct cxl_region_ref).
> > +*/
> > +
> >  static struct cxl_decoder *
> > -cxl_region_find_decoder(struct cxl_port *port,
> > -			struct cxl_endpoint_decoder *cxled,
> > -			struct cxl_region *cxlr)
> > +cxl_find_decoder_early(struct cxl_port *port,
> > +		       struct cxl_endpoint_decoder *cxled,
> > +		       struct cxl_region *cxlr)
> >  {
> >  	struct device *dev;
> >  
> > @@ -917,7 +924,7 @@ alloc_region_ref(struct cxl_port *port, struct cxl_region *cxlr,
> >  		if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> >  			struct cxl_decoder *cxld;
> >  
> > -			cxld = cxl_region_find_decoder(port, cxled, cxlr);
> > +			cxld = cxl_find_decoder_early(port, cxled, cxlr);
> >  			if (auto_order_ok(port, iter->region, cxld))
> >  				continue;
> >  		}
> > @@ -1005,7 +1012,7 @@ static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
> >  {
> >  	struct cxl_decoder *cxld;
> >  
> > -	cxld = cxl_region_find_decoder(port, cxled, cxlr);
> > +	cxld = cxl_find_decoder_early(port, cxled, cxlr);
> >  	if (!cxld) {
> >  		dev_dbg(&cxlr->dev, "%s: no decoder available\n",
> >  			dev_name(&port->dev));
> 

[PATCH v3 07/18] cxl/region: Avoid duplicate call of cxl_find_decoder_early()

[Robert Richter]

Function cxl_find_decoder_early() is called twice, in
alloc_region_ref() and cxl_rr_alloc_decoder(). Move it out there and
instead pass the decoder as function argument to both.

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/region.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 13e3ba984a53..b8201c2faa87 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -908,7 +908,8 @@ static bool auto_order_ok(struct cxl_port *port, struct cxl_region *cxlr_iter,
 
 static struct cxl_region_ref *
 alloc_region_ref(struct cxl_port *port, struct cxl_region *cxlr,
-		 struct cxl_endpoint_decoder *cxled)
+		 struct cxl_endpoint_decoder *cxled,
+		 struct cxl_decoder *cxld)
 {
 	struct cxl_region_params *p = &cxlr->params;
 	struct cxl_region_ref *cxl_rr, *iter;
@@ -922,9 +923,6 @@ alloc_region_ref(struct cxl_port *port, struct cxl_region *cxlr,
 			continue;
 
 		if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
-			struct cxl_decoder *cxld;
-
-			cxld = cxl_find_decoder_early(port, cxled, cxlr);
 			if (auto_order_ok(port, iter->region, cxld))
 				continue;
 		}
@@ -1008,17 +1006,9 @@ static int cxl_rr_ep_add(struct cxl_region_ref *cxl_rr,
 
 static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
 				struct cxl_endpoint_decoder *cxled,
-				struct cxl_region_ref *cxl_rr)
+				struct cxl_region_ref *cxl_rr,
+				struct cxl_decoder *cxld)
 {
-	struct cxl_decoder *cxld;
-
-	cxld = cxl_find_decoder_early(port, cxled, cxlr);
-	if (!cxld) {
-		dev_dbg(&cxlr->dev, "%s: no decoder available\n",
-			dev_name(&port->dev));
-		return -EBUSY;
-	}
-
 	if (cxld->region) {
 		dev_dbg(&cxlr->dev, "%s: %s already attached to %s\n",
 			dev_name(&port->dev), dev_name(&cxld->dev),
@@ -1109,7 +1099,16 @@ static int cxl_port_attach_region(struct cxl_port *port,
 			nr_targets_inc = true;
 		}
 	} else {
-		cxl_rr = alloc_region_ref(port, cxlr, cxled);
+		struct cxl_decoder *cxld;
+
+		cxld = cxl_find_decoder_early(port, cxled, cxlr);
+		if (!cxld) {
+			dev_dbg(&cxlr->dev, "%s: no decoder available\n",
+				dev_name(&port->dev));
+			return -EBUSY;
+		}
+
+		cxl_rr = alloc_region_ref(port, cxlr, cxled, cxld);
 		if (IS_ERR(cxl_rr)) {
 			dev_dbg(&cxlr->dev,
 				"%s: failed to allocate region reference\n",
@@ -1118,7 +1117,7 @@ static int cxl_port_attach_region(struct cxl_port *port,
 		}
 		nr_targets_inc = true;
 
-		rc = cxl_rr_alloc_decoder(port, cxlr, cxled, cxl_rr);
+		rc = cxl_rr_alloc_decoder(port, cxlr, cxled, cxl_rr, cxld);
 		if (rc)
 			goto out_erase;
 	}
-- 
2.39.5

Re: [PATCH v3 07/18] cxl/region: Avoid duplicate call of cxl_find_decoder_early()

[Jonathan Cameron]

On Tue, 11 Feb 2025 10:53:37 +0100
Robert Richter <rrichter@amd.com> wrote:

> Function cxl_find_decoder_early() is called twice, in
> alloc_region_ref() and cxl_rr_alloc_decoder(). Move it out there and

out where?  I'd make it clear that both these calls are in
cxl_port_attach_region()

> instead pass the decoder as function argument to both.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>

I think this is fine but it's not immediately obvious so a request
inline for some more details in this description.

> ---
>  drivers/cxl/core/region.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 13e3ba984a53..b8201c2faa87 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -908,7 +908,8 @@ static bool auto_order_ok(struct cxl_port *port, struct cxl_region *cxlr_iter,
>  
>  static struct cxl_region_ref *
>  alloc_region_ref(struct cxl_port *port, struct cxl_region *cxlr,
> -		 struct cxl_endpoint_decoder *cxled)
> +		 struct cxl_endpoint_decoder *cxled,
> +		 struct cxl_decoder *cxld)
>  {
>  	struct cxl_region_params *p = &cxlr->params;
>  	struct cxl_region_ref *cxl_rr, *iter;
> @@ -922,9 +923,6 @@ alloc_region_ref(struct cxl_port *port, struct cxl_region *cxlr,
>  			continue;
>  
>  		if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags)) {
> -			struct cxl_decoder *cxld;
> -
> -			cxld = cxl_find_decoder_early(port, cxled, cxlr);


This is buried a little deep to be obviously fine to lift out.
Seems like it should always have been done outside the xa_for_each()
loop in here.  So I think this is fine but maybe some more in the
patch description is needed to make that point.

>  			if (auto_order_ok(port, iter->region, cxld))
>  				continue;
>  		}
> @@ -1008,17 +1006,9 @@ static int cxl_rr_ep_add(struct cxl_region_ref *cxl_rr,
>  
>  static int cxl_rr_alloc_decoder(struct cxl_port *port, struct cxl_region *cxlr,
>  				struct cxl_endpoint_decoder *cxled,
> -				struct cxl_region_ref *cxl_rr)
> +				struct cxl_region_ref *cxl_rr,
> +				struct cxl_decoder *cxld)
>  {
> -	struct cxl_decoder *cxld;
> -
> -	cxld = cxl_find_decoder_early(port, cxled, cxlr);
> -	if (!cxld) {
> -		dev_dbg(&cxlr->dev, "%s: no decoder available\n",
> -			dev_name(&port->dev));
> -		return -EBUSY;
> -	}
> -
>  	if (cxld->region) {
>  		dev_dbg(&cxlr->dev, "%s: %s already attached to %s\n",
>  			dev_name(&port->dev), dev_name(&cxld->dev),
> @@ -1109,7 +1099,16 @@ static int cxl_port_attach_region(struct cxl_port *port,
>  			nr_targets_inc = true;
>  		}
>  	} else {
> -		cxl_rr = alloc_region_ref(port, cxlr, cxled);
> +		struct cxl_decoder *cxld;
> +
> +		cxld = cxl_find_decoder_early(port, cxled, cxlr);
> +		if (!cxld) {
> +			dev_dbg(&cxlr->dev, "%s: no decoder available\n",
> +				dev_name(&port->dev));
> +			return -EBUSY;
> +		}
> +
> +		cxl_rr = alloc_region_ref(port, cxlr, cxled, cxld);
>  		if (IS_ERR(cxl_rr)) {
>  			dev_dbg(&cxlr->dev,
>  				"%s: failed to allocate region reference\n",
> @@ -1118,7 +1117,7 @@ static int cxl_port_attach_region(struct cxl_port *port,
>  		}
>  		nr_targets_inc = true;
>  
> -		rc = cxl_rr_alloc_decoder(port, cxlr, cxled, cxl_rr);
> +		rc = cxl_rr_alloc_decoder(port, cxlr, cxled, cxl_rr, cxld);
>  		if (rc)
>  			goto out_erase;
>  	}

Re: [PATCH v3 07/18] cxl/region: Avoid duplicate call of cxl_find_decoder_early()

[Robert Richter]

On 14.02.25 16:07:25, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 10:53:37 +0100
> Robert Richter <rrichter@amd.com> wrote:
> 
> > Function cxl_find_decoder_early() is called twice, in
> > alloc_region_ref() and cxl_rr_alloc_decoder(). Move it out there and
> 
> out where?  I'd make it clear that both these calls are in
> cxl_port_attach_region()
> 
> > instead pass the decoder as function argument to both.
> > 
> > Signed-off-by: Robert Richter <rrichter@amd.com>
> > Reviewed-by: Gregory Price <gourry@gourry.net>
> > Tested-by: Gregory Price <gourry@gourry.net>
> 
> I think this is fine but it's not immediately obvious so a request
> inline for some more details in this description.

I have updated the patch description.

-Robert

[PATCH v3 08/18] cxl/region: Move find_cxl_root() to cxl_add_to_region()

[Robert Richter]

When adding an endpoint to a region, the root port is determined
first. Move this directly into cxl_add_to_region(). This is in
preparation of the initialization of endpoints that iterates the port
hierarchy from the endpoint up to the root port.

As a side-effect the root argument is removed from the argument lists
of cxl_add_to_region() and related functions. Now, the endpoint is the
only parameter to add a region. This simplifies the function
interface.

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/region.c |  6 ++++--
 drivers/cxl/cxl.h         |  6 ++----
 drivers/cxl/port.c        | 15 +++------------
 3 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index b8201c2faa87..0e38bcb43be6 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3312,9 +3312,11 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 	return ERR_PTR(rc);
 }
 
-int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
+int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct cxl_port *port = cxled_to_port(cxled);
+	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
 	struct range *hpa = &cxled->cxld.hpa_range;
 	struct cxl_decoder *cxld = &cxled->cxld;
 	struct device *cxlrd_dev, *region_dev;
@@ -3324,7 +3326,7 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
 	bool attach = false;
 	int rc;
 
-	cxlrd_dev = device_find_child(&root->dev, &cxld->hpa_range,
+	cxlrd_dev = device_find_child(&cxl_root->port.dev, &cxld->hpa_range,
 				      match_root_decoder_by_range);
 	if (!cxlrd_dev) {
 		dev_err(cxlmd->dev.parent,
diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
index 0d7aff8b97b3..85dfc8df0a80 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -854,8 +854,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_port *root,
-		      struct cxl_endpoint_decoder *cxled);
+int cxl_add_to_region(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)
@@ -866,8 +865,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_port *root,
-				    struct cxl_endpoint_decoder *cxled)
+static inline int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
 {
 	return 0;
 }
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index d2bfd1ff5492..74587a403e3d 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -30,7 +30,7 @@ static void schedule_detach(void *cxlmd)
 	schedule_cxl_memdev_detach(cxlmd);
 }
 
-static int discover_region(struct device *dev, void *root)
+static int discover_region(struct device *dev, void *unused)
 {
 	struct cxl_endpoint_decoder *cxled;
 	int rc;
@@ -49,7 +49,7 @@ static int discover_region(struct device *dev, void *root)
 	 * Region enumeration is opportunistic, if this add-event fails,
 	 * continue to the next endpoint decoder.
 	 */
-	rc = cxl_add_to_region(root, cxled);
+	rc = cxl_add_to_region(cxled);
 	if (rc)
 		dev_dbg(dev, "failed to add to region: %#llx-%#llx\n",
 			cxled->cxld.hpa_range.start, cxled->cxld.hpa_range.end);
@@ -95,7 +95,6 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
 	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_hdm *cxlhdm;
-	struct cxl_port *root;
 	int rc;
 
 	rc = cxl_dvsec_rr_decode(cxlds, &info);
@@ -126,19 +125,11 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
 	if (rc)
 		return rc;
 
-	/*
-	 * This can't fail in practice as CXL root exit unregisters all
-	 * descendant ports and that in turn synchronizes with cxl_port_probe()
-	 */
-	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
-
-	root = &cxl_root->port;
-
 	/*
 	 * Now that all endpoint decoders are successfully enumerated, try to
 	 * assemble regions from committed decoders
 	 */
-	device_for_each_child(&port->dev, root, discover_region);
+	device_for_each_child(&port->dev, NULL, discover_region);
 
 	return 0;
 }
-- 
2.39.5

Re: [PATCH v3 08/18] cxl/region: Move find_cxl_root() to cxl_add_to_region()

[Dave Jiang]

On 2/11/25 2:53 AM, Robert Richter wrote:
> When adding an endpoint to a region, the root port is determined
> first. Move this directly into cxl_add_to_region(). This is in
> preparation of the initialization of endpoints that iterates the port
> hierarchy from the endpoint up to the root port.
> 
> As a side-effect the root argument is removed from the argument lists
> of cxl_add_to_region() and related functions. Now, the endpoint is the
> only parameter to add a region. This simplifies the function
> interface.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Tested-by: Gregory Price <gourry@gourry.net>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/region.c |  6 ++++--
>  drivers/cxl/cxl.h         |  6 ++----
>  drivers/cxl/port.c        | 15 +++------------
>  3 files changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index b8201c2faa87..0e38bcb43be6 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3312,9 +3312,11 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>  	return ERR_PTR(rc);
>  }
>  
> -int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
> +int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
>  {
>  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +	struct cxl_port *port = cxled_to_port(cxled);
> +	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
>  	struct range *hpa = &cxled->cxld.hpa_range;
>  	struct cxl_decoder *cxld = &cxled->cxld;
>  	struct device *cxlrd_dev, *region_dev;
> @@ -3324,7 +3326,7 @@ int cxl_add_to_region(struct cxl_port *root, struct cxl_endpoint_decoder *cxled)
>  	bool attach = false;
>  	int rc;
>  
> -	cxlrd_dev = device_find_child(&root->dev, &cxld->hpa_range,
> +	cxlrd_dev = device_find_child(&cxl_root->port.dev, &cxld->hpa_range,
>  				      match_root_decoder_by_range);
>  	if (!cxlrd_dev) {
>  		dev_err(cxlmd->dev.parent,
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 0d7aff8b97b3..85dfc8df0a80 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -854,8 +854,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_port *root,
> -		      struct cxl_endpoint_decoder *cxled);
> +int cxl_add_to_region(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)
> @@ -866,8 +865,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_port *root,
> -				    struct cxl_endpoint_decoder *cxled)
> +static inline int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
>  {
>  	return 0;
>  }
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index d2bfd1ff5492..74587a403e3d 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -30,7 +30,7 @@ static void schedule_detach(void *cxlmd)
>  	schedule_cxl_memdev_detach(cxlmd);
>  }
>  
> -static int discover_region(struct device *dev, void *root)
> +static int discover_region(struct device *dev, void *unused)
>  {
>  	struct cxl_endpoint_decoder *cxled;
>  	int rc;
> @@ -49,7 +49,7 @@ static int discover_region(struct device *dev, void *root)
>  	 * Region enumeration is opportunistic, if this add-event fails,
>  	 * continue to the next endpoint decoder.
>  	 */
> -	rc = cxl_add_to_region(root, cxled);
> +	rc = cxl_add_to_region(cxled);
>  	if (rc)
>  		dev_dbg(dev, "failed to add to region: %#llx-%#llx\n",
>  			cxled->cxld.hpa_range.start, cxled->cxld.hpa_range.end);
> @@ -95,7 +95,6 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport_dev);
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_hdm *cxlhdm;
> -	struct cxl_port *root;
>  	int rc;
>  
>  	rc = cxl_dvsec_rr_decode(cxlds, &info);
> @@ -126,19 +125,11 @@ static int cxl_endpoint_port_probe(struct cxl_port *port)
>  	if (rc)
>  		return rc;
>  
> -	/*
> -	 * This can't fail in practice as CXL root exit unregisters all
> -	 * descendant ports and that in turn synchronizes with cxl_port_probe()
> -	 */
> -	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
> -
> -	root = &cxl_root->port;
> -
>  	/*
>  	 * Now that all endpoint decoders are successfully enumerated, try to
>  	 * assemble regions from committed decoders
>  	 */
> -	device_for_each_child(&port->dev, root, discover_region);
> +	device_for_each_child(&port->dev, NULL, discover_region);
>  
>  	return 0;
>  }

[PATCH v3 09/18] cxl/region: Factor out code to find the root decoder

[Robert Richter]

In function cxl_add_to_region() there is code to determine the root
decoder associated to an endpoint decoder. Factor out that code for
later reuse. This has the benefit of reducing cxl_add_to_region()'s
function complexity.

The reference of cxlrd_dev can be freed earlier. Since the root
decoder exists as long as the root port exists and the endpoint
already holds a reference to the root port, this additional reference
is not needed. Though it looks obvious to use __free() for the
reference of cxlrd_dev here too, this is done in a later rework. So
just move the code.

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/region.c | 55 ++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 18 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 0e38bcb43be6..c641c8922455 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3203,6 +3203,38 @@ static int match_root_decoder_by_range(struct device *dev,
 	return range_contains(r1, r2);
 }
 
+static struct cxl_root_decoder *
+cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
+{
+	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
+	struct cxl_port *port = cxled_to_port(cxled);
+	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
+	struct cxl_decoder *cxld = &cxled->cxld;
+	struct range *hpa = &cxld->hpa_range;
+	struct device *cxlrd_dev;
+
+	cxlrd_dev = device_find_child(&cxl_root->port.dev, hpa,
+				      match_root_decoder_by_range);
+	if (!cxlrd_dev) {
+		dev_err(cxlmd->dev.parent,
+			"%s:%s no CXL window for range %#llx:%#llx\n",
+			dev_name(&cxlmd->dev), dev_name(&cxld->dev),
+			cxld->hpa_range.start, cxld->hpa_range.end);
+		return NULL;
+	}
+
+	/*
+	 * device_find_child() created a reference to the root
+	 * decoder. Since the root decoder exists as long as the root
+	 * port exists and the endpoint already holds a reference to
+	 * the root port, this additional reference is not needed.
+	 * Free it here.
+	 */
+	put_device(cxlrd_dev);
+
+	return to_cxl_root_decoder(cxlrd_dev);
+}
+
 static int match_region_by_range(struct device *dev, const void *data)
 {
 	struct cxl_region_params *p;
@@ -3314,29 +3346,17 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 
 int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
 {
-	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
-	struct cxl_port *port = cxled_to_port(cxled);
-	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
 	struct range *hpa = &cxled->cxld.hpa_range;
-	struct cxl_decoder *cxld = &cxled->cxld;
-	struct device *cxlrd_dev, *region_dev;
+	struct device *region_dev;
 	struct cxl_root_decoder *cxlrd;
 	struct cxl_region_params *p;
 	struct cxl_region *cxlr;
 	bool attach = false;
 	int rc;
 
-	cxlrd_dev = device_find_child(&cxl_root->port.dev, &cxld->hpa_range,
-				      match_root_decoder_by_range);
-	if (!cxlrd_dev) {
-		dev_err(cxlmd->dev.parent,
-			"%s:%s no CXL window for range %#llx:%#llx\n",
-			dev_name(&cxlmd->dev), dev_name(&cxld->dev),
-			cxld->hpa_range.start, cxld->hpa_range.end);
+	cxlrd = cxl_find_root_decoder(cxled);
+	if (!cxlrd)
 		return -ENXIO;
-	}
-
-	cxlrd = to_cxl_root_decoder(cxlrd_dev);
 
 	/*
 	 * Ensure that if multiple threads race to construct_region() for @hpa
@@ -3354,7 +3374,7 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
 
 	rc = PTR_ERR_OR_ZERO(cxlr);
 	if (rc)
-		goto out;
+		return rc;
 
 	attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE);
 
@@ -3375,8 +3395,7 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
 	}
 
 	put_device(region_dev);
-out:
-	put_device(cxlrd_dev);
+
 	return rc;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_add_to_region, "CXL");
-- 
2.39.5

Re: [PATCH v3 09/18] cxl/region: Factor out code to find the root decoder

[Dave Jiang]

On 2/11/25 2:53 AM, Robert Richter wrote:
> In function cxl_add_to_region() there is code to determine the root
> decoder associated to an endpoint decoder. Factor out that code for
> later reuse. This has the benefit of reducing cxl_add_to_region()'s
> function complexity.
> 
> The reference of cxlrd_dev can be freed earlier. Since the root
> decoder exists as long as the root port exists and the endpoint
> already holds a reference to the root port, this additional reference
> is not needed. Though it looks obvious to use __free() for the
> reference of cxlrd_dev here too, this is done in a later rework. So
> just move the code.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Tested-by: Gregory Price <gourry@gourry.net>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/cxl/core/region.c | 55 ++++++++++++++++++++++++++-------------
>  1 file changed, 37 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 0e38bcb43be6..c641c8922455 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3203,6 +3203,38 @@ static int match_root_decoder_by_range(struct device *dev,
>  	return range_contains(r1, r2);
>  }
>  
> +static struct cxl_root_decoder *
> +cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
> +{
> +	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> +	struct cxl_port *port = cxled_to_port(cxled);
> +	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
> +	struct cxl_decoder *cxld = &cxled->cxld;
> +	struct range *hpa = &cxld->hpa_range;
> +	struct device *cxlrd_dev;
> +
> +	cxlrd_dev = device_find_child(&cxl_root->port.dev, hpa,
> +				      match_root_decoder_by_range);
> +	if (!cxlrd_dev) {
> +		dev_err(cxlmd->dev.parent,
> +			"%s:%s no CXL window for range %#llx:%#llx\n",
> +			dev_name(&cxlmd->dev), dev_name(&cxld->dev),
> +			cxld->hpa_range.start, cxld->hpa_range.end);
> +		return NULL;
> +	}
> +
> +	/*
> +	 * device_find_child() created a reference to the root
> +	 * decoder. Since the root decoder exists as long as the root
> +	 * port exists and the endpoint already holds a reference to
> +	 * the root port, this additional reference is not needed.
> +	 * Free it here.
> +	 */
> +	put_device(cxlrd_dev);
> +
> +	return to_cxl_root_decoder(cxlrd_dev);
> +}
> +
>  static int match_region_by_range(struct device *dev, const void *data)
>  {
>  	struct cxl_region_params *p;
> @@ -3314,29 +3346,17 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>  
>  int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
>  {
> -	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> -	struct cxl_port *port = cxled_to_port(cxled);
> -	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
>  	struct range *hpa = &cxled->cxld.hpa_range;
> -	struct cxl_decoder *cxld = &cxled->cxld;
> -	struct device *cxlrd_dev, *region_dev;
> +	struct device *region_dev;
>  	struct cxl_root_decoder *cxlrd;
>  	struct cxl_region_params *p;
>  	struct cxl_region *cxlr;
>  	bool attach = false;
>  	int rc;
>  
> -	cxlrd_dev = device_find_child(&cxl_root->port.dev, &cxld->hpa_range,
> -				      match_root_decoder_by_range);
> -	if (!cxlrd_dev) {
> -		dev_err(cxlmd->dev.parent,
> -			"%s:%s no CXL window for range %#llx:%#llx\n",
> -			dev_name(&cxlmd->dev), dev_name(&cxld->dev),
> -			cxld->hpa_range.start, cxld->hpa_range.end);
> +	cxlrd = cxl_find_root_decoder(cxled);
> +	if (!cxlrd)
>  		return -ENXIO;
> -	}
> -
> -	cxlrd = to_cxl_root_decoder(cxlrd_dev);
>  
>  	/*
>  	 * Ensure that if multiple threads race to construct_region() for @hpa
> @@ -3354,7 +3374,7 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
>  
>  	rc = PTR_ERR_OR_ZERO(cxlr);
>  	if (rc)
> -		goto out;
> +		return rc;
>  
>  	attach_target(cxlr, cxled, -1, TASK_UNINTERRUPTIBLE);
>  
> @@ -3375,8 +3395,7 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
>  	}
>  
>  	put_device(region_dev);
> -out:
> -	put_device(cxlrd_dev);
> +
>  	return rc;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_add_to_region, "CXL");

[PATCH v3 10/18] cxl/region: Factor out code to find a root decoder's region

[Robert Richter]

In function cxl_add_to_region() there is code to determine a root
decoder's region. Factor that code out. This is in preparation to
further rework and simplify function cxl_add_to_region().

No functional changes.

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/region.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index c641c8922455..9ce0282c0042 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3256,6 +3256,19 @@ static int match_region_by_range(struct device *dev, const void *data)
 	return rc;
 }
 
+static struct cxl_region *
+cxl_find_region_by_range(struct cxl_root_decoder *cxlrd, struct range *hpa)
+{
+	struct device *region_dev;
+
+	region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
+				       match_region_by_range);
+	if (!region_dev)
+		return NULL;
+
+	return to_cxl_region(region_dev);
+}
+
 /* Establish an empty region covering the given HPA range */
 static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 					   struct cxl_endpoint_decoder *cxled)
@@ -3347,7 +3360,6 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
 {
 	struct range *hpa = &cxled->cxld.hpa_range;
-	struct device *region_dev;
 	struct cxl_root_decoder *cxlrd;
 	struct cxl_region_params *p;
 	struct cxl_region *cxlr;
@@ -3363,13 +3375,9 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
 	 * one does the construction and the others add to that.
 	 */
 	mutex_lock(&cxlrd->range_lock);
-	region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
-				       match_region_by_range);
-	if (!region_dev) {
+	cxlr = cxl_find_region_by_range(cxlrd, hpa);
+	if (!cxlr)
 		cxlr = construct_region(cxlrd, cxled);
-		region_dev = &cxlr->dev;
-	} else
-		cxlr = to_cxl_region(region_dev);
 	mutex_unlock(&cxlrd->range_lock);
 
 	rc = PTR_ERR_OR_ZERO(cxlr);
@@ -3394,7 +3402,7 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
 				p->res);
 	}
 
-	put_device(region_dev);
+	put_device(&cxlr->dev);			/* cxl_find_region_by_range() */
 
 	return rc;
 }
-- 
2.39.5

Re: [PATCH v3 10/18] cxl/region: Factor out code to find a root decoder's region

[Jonathan Cameron]

On Tue, 11 Feb 2025 10:53:40 +0100
Robert Richter <rrichter@amd.com> wrote:

> In function cxl_add_to_region() there is code to determine a root
> decoder's region. Factor that code out. This is in preparation to
> further rework and simplify function cxl_add_to_region().
> 
> No functional changes.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v3 10/18] cxl/region: Factor out code to find a root decoder's region

[Dave Jiang]

On 2/11/25 2:53 AM, Robert Richter wrote:
> In function cxl_add_to_region() there is code to determine a root
> decoder's region. Factor that code out. This is in preparation to
> further rework and simplify function cxl_add_to_region().
> 
> No functional changes.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/region.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index c641c8922455..9ce0282c0042 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3256,6 +3256,19 @@ static int match_region_by_range(struct device *dev, const void *data)
>  	return rc;
>  }
>  
> +static struct cxl_region *
> +cxl_find_region_by_range(struct cxl_root_decoder *cxlrd, struct range *hpa)
> +{
> +	struct device *region_dev;
> +
> +	region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
> +				       match_region_by_range);
> +	if (!region_dev)
> +		return NULL;
> +
> +	return to_cxl_region(region_dev);
> +}
> +
>  /* Establish an empty region covering the given HPA range */
>  static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>  					   struct cxl_endpoint_decoder *cxled)
> @@ -3347,7 +3360,6 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>  int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
>  {
>  	struct range *hpa = &cxled->cxld.hpa_range;
> -	struct device *region_dev;
>  	struct cxl_root_decoder *cxlrd;
>  	struct cxl_region_params *p;
>  	struct cxl_region *cxlr;
> @@ -3363,13 +3375,9 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
>  	 * one does the construction and the others add to that.
>  	 */
>  	mutex_lock(&cxlrd->range_lock);
> -	region_dev = device_find_child(&cxlrd->cxlsd.cxld.dev, hpa,
> -				       match_region_by_range);
> -	if (!region_dev) {
> +	cxlr = cxl_find_region_by_range(cxlrd, hpa);
> +	if (!cxlr)
>  		cxlr = construct_region(cxlrd, cxled);
> -		region_dev = &cxlr->dev;
> -	} else
> -		cxlr = to_cxl_region(region_dev);
>  	mutex_unlock(&cxlrd->range_lock);
>  
>  	rc = PTR_ERR_OR_ZERO(cxlr);
> @@ -3394,7 +3402,7 @@ int cxl_add_to_region(struct cxl_endpoint_decoder *cxled)
>  				p->res);
>  	}
>  
> -	put_device(region_dev);
> +	put_device(&cxlr->dev);			/* cxl_find_region_by_range() */
>  
>  	return rc;
>  }

[PATCH v3 11/18] cxl/region: Split region registration into an initialization and adding part

[Robert Richter]

Before adding an endpoint to a region, the endpoint is initialized
first. Move that part to a new function
cxl_endpoint_decoder_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_decoder_register().

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/region.c | 36 ++++++++++++++++++++++++++++--------
 drivers/cxl/cxl.h         |  6 ++++--
 drivers/cxl/port.c        |  9 +++++----
 3 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 9ce0282c0042..fb43e154c7b9 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3345,7 +3345,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_decoder_add() */
 	get_device(&cxlr->dev);
 	up_write(&cxl_region_rwsem);
 
@@ -3357,19 +3357,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_decoder_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_decoder_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.
@@ -3406,7 +3415,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_decoder_register(struct cxl_endpoint_decoder *cxled)
+{
+	int rc;
+
+	rc = cxl_endpoint_decoder_initialize(cxled);
+	if (rc)
+		return rc;
+
+	return cxl_endpoint_decoder_add(cxled);
+}
+EXPORT_SYMBOL_NS_GPL(cxl_endpoint_decoder_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 85dfc8df0a80..50e7d878bb6f 100644
--- a/drivers/cxl/cxl.h
+++ b/drivers/cxl/cxl.h
@@ -392,6 +392,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_state state;
@@ -854,7 +855,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_decoder_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)
@@ -865,7 +866,8 @@ 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_decoder_register(struct cxl_endpoint_decoder *cxled)
 {
 	return 0;
 }
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index 74587a403e3d..36e487366c7a 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_decoder_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

Re: [PATCH v3 11/18] cxl/region: Split region registration into an initialization and adding part

[Jonathan Cameron]

On Tue, 11 Feb 2025 10:53:41 +0100
Robert Richter <rrichter@amd.com> wrote:

> Before adding an endpoint to a region, the endpoint is initialized
> first. Move that part to a new function
> cxl_endpoint_decoder_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_decoder_register().
Hi Robert,

Superficially I'd expect a call of that name to be registering
the device for the decoder.  i.e. being the thing that makes
/sys/bus/cxl/devices/decoder3.2 appear.

This register naming is based on the other two being initalize
and add, but they aren't initializing and adding the
endpoint decode device. Hence I don't think those names work either.

> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
> ---
>  drivers/cxl/core/region.c | 36 ++++++++++++++++++++++++++++--------
>  drivers/cxl/cxl.h         |  6 ++++--
>  drivers/cxl/port.c        |  9 +++++----
>  3 files changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 9ce0282c0042..fb43e154c7b9 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3345,7 +3345,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_decoder_add() */
>  	get_device(&cxlr->dev);
>  	up_write(&cxl_region_rwsem);
>  
> @@ -3357,19 +3357,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_decoder_initialize(struct cxl_endpoint_decoder *cxled)
So far this looks like it should be called something like

cxl_endpoint_decoder_init_region_decoder()
or something like that. The cxled is already intialized more generally
and the cxled->cxld.dev is registered.

>  {
> -	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_decoder_add(struct cxl_endpoint_decoder *cxled)
It's not adding what I'd expect such a function to add.
Rather it is performing an association with a region.

> +{
> +	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.
> @@ -3406,7 +3415,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_decoder_register(struct cxl_endpoint_decoder *cxled)
> +{
> +	int rc;
> +
> +	rc = cxl_endpoint_decoder_initialize(cxled);
> +	if (rc)
> +		return rc;
> +
> +	return cxl_endpoint_decoder_add(cxled);
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_endpoint_decoder_register, "CXL");

[PATCH v3 12/18] cxl/region: Use iterator to find the root port in cxl_find_root_decoder()

[Robert Richter]

cxl_find_root_decoder() uses find_cxl_root() to find the root port.
In order to support address translation, an iterator must traverse all
ports from endpoint to root port and filter based on system physical
address. Replace the call to find_cxl_root() with the required logic.

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/region.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index fb43e154c7b9..cfcd235f311e 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3207,13 +3207,18 @@ static struct cxl_root_decoder *
 cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
-	struct cxl_port *port = cxled_to_port(cxled);
-	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
+	struct cxl_port *iter = cxled_to_port(cxled);
 	struct cxl_decoder *cxld = &cxled->cxld;
 	struct range *hpa = &cxld->hpa_range;
 	struct device *cxlrd_dev;
 
-	cxlrd_dev = device_find_child(&cxl_root->port.dev, hpa,
+	while (iter && !is_cxl_root(iter))
+		iter = to_cxl_port(iter->dev.parent);
+
+	if (!iter)
+		return NULL;
+
+	cxlrd_dev = device_find_child(&iter->dev, hpa,
 				      match_root_decoder_by_range);
 	if (!cxlrd_dev) {
 		dev_err(cxlmd->dev.parent,
-- 
2.39.5

Re: [PATCH v3 12/18] cxl/region: Use iterator to find the root port in cxl_find_root_decoder()

[Dave Jiang]

On 2/11/25 2:53 AM, Robert Richter wrote:
> cxl_find_root_decoder() uses find_cxl_root() to find the root port.
> In order to support address translation, an iterator must traverse all
> ports from endpoint to root port and filter based on system physical
> address. Replace the call to find_cxl_root() with the required logic.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Tested-by: Gregory Price <gourry@gourry.net>

Hi Robert,

While some cleanup patches are good to send ahead, some like these would be good to go right before they get used to allow the reviewer to see how the new code is utilized. Otherwise the changes doesn't make a lot of sense stand alone.

If it's not too much of a pain, I'd like to this series only have cleanups/refactorings that would make sense without the address translation needs. And move the other changes near the address translation changes for a smoother review.

Thanks!   

Reviewed-by: Dave Jiang <dave.jiang@intel.com>


> ---
>  drivers/cxl/core/region.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index fb43e154c7b9..cfcd235f311e 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3207,13 +3207,18 @@ static struct cxl_root_decoder *
>  cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
>  {
>  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
> -	struct cxl_port *port = cxled_to_port(cxled);
> -	struct cxl_root *cxl_root __free(put_cxl_root) = find_cxl_root(port);
> +	struct cxl_port *iter = cxled_to_port(cxled);
>  	struct cxl_decoder *cxld = &cxled->cxld;
>  	struct range *hpa = &cxld->hpa_range;
>  	struct device *cxlrd_dev;
>  
> -	cxlrd_dev = device_find_child(&cxl_root->port.dev, hpa,
> +	while (iter && !is_cxl_root(iter))
> +		iter = to_cxl_port(iter->dev.parent);
> +
> +	if (!iter)
> +		return NULL;
> +
> +	cxlrd_dev = device_find_child(&iter->dev, hpa,
>  				      match_root_decoder_by_range);
>  	if (!cxlrd_dev) {
>  		dev_err(cxlmd->dev.parent,

[PATCH v3 13/18] cxl/region: Add function to find a port's switch decoder by range

[Robert Richter]

Factor out code to find the switch decoder of a port for a specific
address range. Reuse the code to search a root decoder, create the
function cxl_port_find_switch_decoder() and rework
match_root_decoder_by_range() to be usable for switch decoders too.

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/region.c | 48 +++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index cfcd235f311e..72e991e7d9ab 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3189,28 +3189,44 @@ static int devm_cxl_add_dax_region(struct cxl_region *cxlr)
 	return rc;
 }
 
-static int match_root_decoder_by_range(struct device *dev,
-				       const void *data)
+static int match_decoder_by_range(struct device *dev, const void *data)
 {
 	const struct range *r1, *r2 = data;
-	struct cxl_root_decoder *cxlrd;
+	struct cxl_decoder *cxld;
 
-	if (!is_root_decoder(dev))
+	if (!is_switch_decoder(dev))
 		return 0;
 
-	cxlrd = to_cxl_root_decoder(dev);
-	r1 = &cxlrd->cxlsd.cxld.hpa_range;
+	cxld = to_cxl_decoder(dev);
+	r1 = &cxld->hpa_range;
 	return range_contains(r1, r2);
 }
 
+static struct cxl_decoder *
+cxl_port_find_switch_decoder(struct cxl_port *port, struct range *hpa)
+{
+	/*
+	 * device_find_child() increments the reference count of the
+	 * the switch decoder's parent port to protect the reference
+	 * to its child. The port is already a parent of the endpoint
+	 * decoder's port, at least indirectly in the port hierarchy.
+	 * Thus, the endpoint already holds a reference for the parent
+	 * port of the switch decoder. Free the unnecessary reference
+	 * here.
+	 */
+	struct device *cxld_dev __free(put_device) =
+		device_find_child(&port->dev, hpa, match_decoder_by_range);
+
+	return cxld_dev ? to_cxl_decoder(cxld_dev) : NULL;
+}
+
 static struct cxl_root_decoder *
 cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct cxl_port *iter = cxled_to_port(cxled);
-	struct cxl_decoder *cxld = &cxled->cxld;
+	struct cxl_decoder *root, *cxld = &cxled->cxld;
 	struct range *hpa = &cxld->hpa_range;
-	struct device *cxlrd_dev;
 
 	while (iter && !is_cxl_root(iter))
 		iter = to_cxl_port(iter->dev.parent);
@@ -3218,9 +3234,8 @@ cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
 	if (!iter)
 		return NULL;
 
-	cxlrd_dev = device_find_child(&iter->dev, hpa,
-				      match_root_decoder_by_range);
-	if (!cxlrd_dev) {
+	root = cxl_port_find_switch_decoder(iter, hpa);
+	if (!root) {
 		dev_err(cxlmd->dev.parent,
 			"%s:%s no CXL window for range %#llx:%#llx\n",
 			dev_name(&cxlmd->dev), dev_name(&cxld->dev),
@@ -3228,16 +3243,9 @@ cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
 		return NULL;
 	}
 
-	/*
-	 * device_find_child() created a reference to the root
-	 * decoder. Since the root decoder exists as long as the root
-	 * port exists and the endpoint already holds a reference to
-	 * the root port, this additional reference is not needed.
-	 * Free it here.
-	 */
-	put_device(cxlrd_dev);
 
-	return to_cxl_root_decoder(cxlrd_dev);
+
+	return to_cxl_root_decoder(&root->dev);
 }
 
 static int match_region_by_range(struct device *dev, const void *data)
-- 
2.39.5

Re: [PATCH v3 13/18] cxl/region: Add function to find a port's switch decoder by range

[Jonathan Cameron]

On Tue, 11 Feb 2025 10:53:43 +0100
Robert Richter <rrichter@amd.com> wrote:

> Factor out code to find the switch decoder of a port for a specific
> address range. Reuse the code to search a root decoder, create the
> function cxl_port_find_switch_decoder() and rework
> match_root_decoder_by_range() to be usable for switch decoders too.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v3 13/18] cxl/region: Add function to find a port's switch decoder by range

[Dave Jiang]

On 2/11/25 2:53 AM, Robert Richter wrote:
> Factor out code to find the switch decoder of a port for a specific
> address range. Reuse the code to search a root decoder, create the
> function cxl_port_find_switch_decoder() and rework
> match_root_decoder_by_range() to be usable for switch decoders too.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/cxl/core/region.c | 48 +++++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index cfcd235f311e..72e991e7d9ab 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3189,28 +3189,44 @@ static int devm_cxl_add_dax_region(struct cxl_region *cxlr)
>  	return rc;
>  }
>  
> -static int match_root_decoder_by_range(struct device *dev,
> -				       const void *data)
> +static int match_decoder_by_range(struct device *dev, const void *data)
>  {
>  	const struct range *r1, *r2 = data;
> -	struct cxl_root_decoder *cxlrd;
> +	struct cxl_decoder *cxld;
>  
> -	if (!is_root_decoder(dev))
> +	if (!is_switch_decoder(dev))
>  		return 0;
>  
> -	cxlrd = to_cxl_root_decoder(dev);
> -	r1 = &cxlrd->cxlsd.cxld.hpa_range;
> +	cxld = to_cxl_decoder(dev);
> +	r1 = &cxld->hpa_range;
>  	return range_contains(r1, r2);
>  }
>  
> +static struct cxl_decoder *
> +cxl_port_find_switch_decoder(struct cxl_port *port, struct range *hpa)
> +{
> +	/*
> +	 * device_find_child() increments the reference count of the
> +	 * the switch decoder's parent port to protect the reference
> +	 * to its child. The port is already a parent of the endpoint
> +	 * decoder's port, at least indirectly in the port hierarchy.
> +	 * Thus, the endpoint already holds a reference for the parent
> +	 * port of the switch decoder. Free the unnecessary reference
> +	 * here.
> +	 */
> +	struct device *cxld_dev __free(put_device) =
> +		device_find_child(&port->dev, hpa, match_decoder_by_range);
> +
> +	return cxld_dev ? to_cxl_decoder(cxld_dev) : NULL;
> +}
> +
>  static struct cxl_root_decoder *
>  cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
>  {
>  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>  	struct cxl_port *iter = cxled_to_port(cxled);
> -	struct cxl_decoder *cxld = &cxled->cxld;
> +	struct cxl_decoder *root, *cxld = &cxled->cxld;
>  	struct range *hpa = &cxld->hpa_range;
> -	struct device *cxlrd_dev;
>  
>  	while (iter && !is_cxl_root(iter))
>  		iter = to_cxl_port(iter->dev.parent);
> @@ -3218,9 +3234,8 @@ cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
>  	if (!iter)
>  		return NULL;
>  
> -	cxlrd_dev = device_find_child(&iter->dev, hpa,
> -				      match_root_decoder_by_range);
> -	if (!cxlrd_dev) {
> +	root = cxl_port_find_switch_decoder(iter, hpa);
> +	if (!root) {
>  		dev_err(cxlmd->dev.parent,
>  			"%s:%s no CXL window for range %#llx:%#llx\n",
>  			dev_name(&cxlmd->dev), dev_name(&cxld->dev),
> @@ -3228,16 +3243,9 @@ cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
>  		return NULL;
>  	}
>  
> -	/*
> -	 * device_find_child() created a reference to the root
> -	 * decoder. Since the root decoder exists as long as the root
> -	 * port exists and the endpoint already holds a reference to
> -	 * the root port, this additional reference is not needed.
> -	 * Free it here.
> -	 */
> -	put_device(cxlrd_dev);
>  
> -	return to_cxl_root_decoder(cxlrd_dev);
> +
> +	return to_cxl_root_decoder(&root->dev);
>  }
>  
>  static int match_region_by_range(struct device *dev, const void *data)

[PATCH v3 14/18] cxl/region: Unfold cxl_find_root_decoder() into cxl_endpoint_decoder_initialize()

[Robert Richter]

To determine other endpoint parameters such as interleaving parameters
during endpoint initialization, the iterator function in
cxl_find_root_decoder() can be used. Unfold this function into
cxl_endpoint_decoder_initialize() and make the iterator available
there.

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/region.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 72e991e7d9ab..ebcfbfe9eafc 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -3220,8 +3220,7 @@ cxl_port_find_switch_decoder(struct cxl_port *port, struct range *hpa)
 	return cxld_dev ? to_cxl_decoder(cxld_dev) : NULL;
 }
 
-static struct cxl_root_decoder *
-cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
+static int cxl_endpoint_decoder_initialize(struct cxl_endpoint_decoder *cxled)
 {
 	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
 	struct cxl_port *iter = cxled_to_port(cxled);
@@ -3232,7 +3231,7 @@ cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
 		iter = to_cxl_port(iter->dev.parent);
 
 	if (!iter)
-		return NULL;
+		return -ENXIO;
 
 	root = cxl_port_find_switch_decoder(iter, hpa);
 	if (!root) {
@@ -3240,12 +3239,12 @@ cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
 			"%s:%s no CXL window for range %#llx:%#llx\n",
 			dev_name(&cxlmd->dev), dev_name(&cxld->dev),
 			cxld->hpa_range.start, cxld->hpa_range.end);
-		return NULL;
+		return -ENXIO;
 	}
 
+	cxled->cxlrd = to_cxl_root_decoder(&root->dev);
 
-
-	return to_cxl_root_decoder(&root->dev);
+	return 0;
 }
 
 static int match_region_by_range(struct device *dev, const void *data)
@@ -3370,19 +3369,6 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
 	return ERR_PTR(rc);
 }
 
-static int cxl_endpoint_decoder_initialize(struct cxl_endpoint_decoder *cxled)
-{
-	struct cxl_root_decoder *cxlrd;
-
-	cxlrd = cxl_find_root_decoder(cxled);
-	if (!cxlrd)
-		return -ENXIO;
-
-	cxled->cxlrd = cxlrd;
-
-	return 0;
-}
-
 static int cxl_endpoint_decoder_add(struct cxl_endpoint_decoder *cxled)
 {
 	struct range *hpa = &cxled->cxld.hpa_range;
-- 
2.39.5

Re: [PATCH v3 14/18] cxl/region: Unfold cxl_find_root_decoder() into cxl_endpoint_decoder_initialize()

[Jonathan Cameron]

On Tue, 11 Feb 2025 10:53:44 +0100
Robert Richter <rrichter@amd.com> wrote:

> To determine other endpoint parameters such as interleaving parameters
> during endpoint initialization, the iterator function in
> cxl_find_root_decoder() can be used. Unfold this function into
> cxl_endpoint_decoder_initialize() and make the iterator available
> there.
I'm not following this description at all. Perhaps this needs
to wait until you have code that is reusing this to find those
interleave parameters and similar.

For now it just looks like a sensible bit of cleanup where there
was just a single caller of cxl_find_root_decoder()

> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
> ---
>  drivers/cxl/core/region.c | 24 +++++-------------------
>  1 file changed, 5 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 72e991e7d9ab..ebcfbfe9eafc 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -3220,8 +3220,7 @@ cxl_port_find_switch_decoder(struct cxl_port *port, struct range *hpa)
>  	return cxld_dev ? to_cxl_decoder(cxld_dev) : NULL;
>  }
>  
> -static struct cxl_root_decoder *
> -cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
> +static int cxl_endpoint_decoder_initialize(struct cxl_endpoint_decoder *cxled)
>  {
>  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>  	struct cxl_port *iter = cxled_to_port(cxled);
> @@ -3232,7 +3231,7 @@ cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
>  		iter = to_cxl_port(iter->dev.parent);
>  
>  	if (!iter)
> -		return NULL;
> +		return -ENXIO;
>  
>  	root = cxl_port_find_switch_decoder(iter, hpa);
>  	if (!root) {
> @@ -3240,12 +3239,12 @@ cxl_find_root_decoder(struct cxl_endpoint_decoder *cxled)
>  			"%s:%s no CXL window for range %#llx:%#llx\n",
>  			dev_name(&cxlmd->dev), dev_name(&cxld->dev),
>  			cxld->hpa_range.start, cxld->hpa_range.end);
> -		return NULL;
> +		return -ENXIO;
>  	}
>  
> +	cxled->cxlrd = to_cxl_root_decoder(&root->dev);
>  
> -
> -	return to_cxl_root_decoder(&root->dev);
> +	return 0;
>  }
>  
>  static int match_region_by_range(struct device *dev, const void *data)
> @@ -3370,19 +3369,6 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>  	return ERR_PTR(rc);
>  }
>  
> -static int cxl_endpoint_decoder_initialize(struct cxl_endpoint_decoder *cxled)
> -{
> -	struct cxl_root_decoder *cxlrd;
> -
> -	cxlrd = cxl_find_root_decoder(cxled);
> -	if (!cxlrd)
> -		return -ENXIO;
> -
> -	cxled->cxlrd = cxlrd;
> -
> -	return 0;
> -}
> -
>  static int cxl_endpoint_decoder_add(struct cxl_endpoint_decoder *cxled)
>  {
>  	struct range *hpa = &cxled->cxld.hpa_range;

Re: [PATCH v3 14/18] cxl/region: Unfold cxl_find_root_decoder() into cxl_endpoint_decoder_initialize()

[Robert Richter]

On 14.02.25 16:33:46, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 10:53:44 +0100
> Robert Richter <rrichter@amd.com> wrote:
> 
> > To determine other endpoint parameters such as interleaving parameters
> > during endpoint initialization, the iterator function in
> > cxl_find_root_decoder() can be used. Unfold this function into
> > cxl_endpoint_decoder_initialize() and make the iterator available
> > there.
> I'm not following this description at all. Perhaps this needs
> to wait until you have code that is reusing this to find those
> interleave parameters and similar.

As suggested by Dave too, I will move that along with some other
patches out of this rework and cleanup series. The motivation of the
changes is better seen then and are part then where they are actually
needed.

-Robert

[PATCH v3 15/18] cxl/region: Add a dev_warn() on registration failure

[Robert Richter]

Esp. in complex system configurations with multiple endpoints and
interleaving setups it is hard to detect region setup failures as its
registration may silently fail. Add messages to show registration
failures.

Example log message:

  cxl region5: region sort successful
  cxl region5: mem0:endpoint5 decoder5.0 add: mem0:decoder5.0 @ 0 next: none nr_eps: 1 nr_targets: 1
  cxl_port endpoint5: decoder5.0: range: 0x22350000000-0x2634fffffff iw: 1 ig: 256
  cxl region5: pci0000:e0:port1 decoder1.2 add: mem0:decoder5.0 @ 0 next: mem0 nr_eps: 1 nr_targets: 1
  cxl region5: pci0000:e0:port1 iw: 1 ig: 256
  cxl region5: pci0000:e0:port1: decoder1.2 expected 0000:e0:01.2 at 0
  cxl endpoint5: failed to attach decoder5.0 to region5: -6
  cxl_port endpoint5: probe: 0

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/region.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index ebcfbfe9eafc..3031d4773274 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2147,6 +2147,12 @@ static int attach_target(struct cxl_region *cxlr,
 	rc = cxl_region_attach(cxlr, cxled, pos);
 	up_read(&cxl_dpa_rwsem);
 	up_write(&cxl_region_rwsem);
+
+	if (rc)
+		dev_warn(cxled->cxld.dev.parent,
+			"failed to attach %s to %s: %d\n",
+			dev_name(&cxled->cxld.dev), dev_name(&cxlr->dev), rc);
+
 	return rc;
 }
 
-- 
2.39.5

Re: [PATCH v3 15/18] cxl/region: Add a dev_warn() on registration failure

[Jonathan Cameron]

On Tue, 11 Feb 2025 10:53:45 +0100
Robert Richter <rrichter@amd.com> wrote:

> Esp. in complex system configurations with multiple endpoints and
> interleaving setups it is hard to detect region setup failures as its
> registration may silently fail. Add messages to show registration
> failures.
> 
> Example log message:
> 
>   cxl region5: region sort successful
>   cxl region5: mem0:endpoint5 decoder5.0 add: mem0:decoder5.0 @ 0 next: none nr_eps: 1 nr_targets: 1
>   cxl_port endpoint5: decoder5.0: range: 0x22350000000-0x2634fffffff iw: 1 ig: 256
>   cxl region5: pci0000:e0:port1 decoder1.2 add: mem0:decoder5.0 @ 0 next: mem0 nr_eps: 1 nr_targets: 1
>   cxl region5: pci0000:e0:port1 iw: 1 ig: 256
>   cxl region5: pci0000:e0:port1: decoder1.2 expected 0000:e0:01.2 at 0
>   cxl endpoint5: failed to attach decoder5.0 to region5: -6
>   cxl_port endpoint5: probe: 0
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
I'm in general fine with this, but we have previously been reluctant in
some cases to go above dev_dbg.  Hence would like input from more
people on this one.

From me though
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/region.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index ebcfbfe9eafc..3031d4773274 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2147,6 +2147,12 @@ static int attach_target(struct cxl_region *cxlr,
>  	rc = cxl_region_attach(cxlr, cxled, pos);
>  	up_read(&cxl_dpa_rwsem);
>  	up_write(&cxl_region_rwsem);
> +
> +	if (rc)
> +		dev_warn(cxled->cxld.dev.parent,
> +			"failed to attach %s to %s: %d\n",
> +			dev_name(&cxled->cxld.dev), dev_name(&cxlr->dev), rc);
> +
>  	return rc;
>  }
>  

Re: [PATCH v3 15/18] cxl/region: Add a dev_warn() on registration failure

[Dave Jiang]

On 2/14/25 9:35 AM, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 10:53:45 +0100
> Robert Richter <rrichter@amd.com> wrote:
> 
>> Esp. in complex system configurations with multiple endpoints and
>> interleaving setups it is hard to detect region setup failures as its
>> registration may silently fail. Add messages to show registration
>> failures.
>>
>> Example log message:
>>
>>   cxl region5: region sort successful
>>   cxl region5: mem0:endpoint5 decoder5.0 add: mem0:decoder5.0 @ 0 next: none nr_eps: 1 nr_targets: 1
>>   cxl_port endpoint5: decoder5.0: range: 0x22350000000-0x2634fffffff iw: 1 ig: 256
>>   cxl region5: pci0000:e0:port1 decoder1.2 add: mem0:decoder5.0 @ 0 next: mem0 nr_eps: 1 nr_targets: 1
>>   cxl region5: pci0000:e0:port1 iw: 1 ig: 256
>>   cxl region5: pci0000:e0:port1: decoder1.2 expected 0000:e0:01.2 at 0
>>   cxl endpoint5: failed to attach decoder5.0 to region5: -6
>>   cxl_port endpoint5: probe: 0
>>
>> Signed-off-by: Robert Richter <rrichter@amd.com>
>> Reviewed-by: Gregory Price <gourry@gourry.net>
>> Tested-by: Gregory Price <gourry@gourry.net>
> I'm in general fine with this, but we have previously been reluctant in
> some cases to go above dev_dbg.  Hence would like input from more
> people on this one.

If this error message has been helpful in determining the cause of issues when debugging a platform, I'm all for it.

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> 
> From me though
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
>> ---
>>  drivers/cxl/core/region.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index ebcfbfe9eafc..3031d4773274 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -2147,6 +2147,12 @@ static int attach_target(struct cxl_region *cxlr,
>>  	rc = cxl_region_attach(cxlr, cxled, pos);
>>  	up_read(&cxl_dpa_rwsem);
>>  	up_write(&cxl_region_rwsem);
>> +
>> +	if (rc)
>> +		dev_warn(cxled->cxld.dev.parent,
>> +			"failed to attach %s to %s: %d\n",
>> +			dev_name(&cxled->cxld.dev), dev_name(&cxlr->dev), rc);
>> +
>>  	return rc;
>>  }
>>  
> 

[PATCH v3 16/18] cxl/region: Add a dev_err() on missing target list entries

[Robert Richter]

Broken target lists are hard to discover as the driver fails at a
later initialization stage. Add an error message for this.

Example log messages:

  cxl_mem mem1: failed to find endpoint6:0000:e0:01.3 in target list of decoder1.1
  cxl_port endpoint6: failed to register decoder6.0: -6
  cxl_port endpoint6: probe: 0

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/core/region.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 3031d4773274..a56b84e7103a 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -1790,6 +1790,13 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range,
 	}
 	put_device(dev);
 
+	if (rc)
+		dev_err(port->uport_dev,
+			"failed to find %s:%s in target list of %s\n",
+			dev_name(&port->dev),
+			dev_name(port->parent_dport->dport_dev),
+			dev_name(&cxlsd->cxld.dev));
+
 	return rc;
 }
 
-- 
2.39.5

Re: [PATCH v3 16/18] cxl/region: Add a dev_err() on missing target list entries

[Jonathan Cameron]

On Tue, 11 Feb 2025 10:53:46 +0100
Robert Richter <rrichter@amd.com> wrote:

> Broken target lists are hard to discover as the driver fails at a
> later initialization stage. Add an error message for this.
> 
> Example log messages:
> 
>   cxl_mem mem1: failed to find endpoint6:0000:e0:01.3 in target list of decoder1.1
>   cxl_port endpoint6: failed to register decoder6.0: -6
>   cxl_port endpoint6: probe: 0
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
Seems reasonable to me as I've also run into fun problems with these
due to setup script bugs...
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v3 16/18] cxl/region: Add a dev_err() on missing target list entries

[Dave Jiang]

On 2/11/25 2:53 AM, Robert Richter wrote:
> Broken target lists are hard to discover as the driver fails at a
> later initialization stage. Add an error message for this.
> 
> Example log messages:
> 
>   cxl_mem mem1: failed to find endpoint6:0000:e0:01.3 in target list of decoder1.1
>   cxl_port endpoint6: failed to register decoder6.0: -6
>   cxl_port endpoint6: probe: 0
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/cxl/core/region.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 3031d4773274..a56b84e7103a 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1790,6 +1790,13 @@ static int find_pos_and_ways(struct cxl_port *port, struct range *range,
>  	}
>  	put_device(dev);
>  
> +	if (rc)
> +		dev_err(port->uport_dev,
> +			"failed to find %s:%s in target list of %s\n",
> +			dev_name(&port->dev),
> +			dev_name(port->parent_dport->dport_dev),
> +			dev_name(&cxlsd->cxld.dev));
> +
>  	return rc;
>  }
>  

[PATCH v3 17/18] cxl: Add a dev_dbg() when a decoder was added to a port

[Robert Richter]

Improve debugging by adding and unifying messages whenever a decoder
was added to a port. It is especially useful to get the decoder
mapping of the involved CXL host bridge or PCI device. This avoids a
complex lookup of the decoder/port/device mappings in sysfs.

Example log messages:

  cxl_acpi ACPI0017:00: decoder0.0 added to root0
  cxl_acpi ACPI0017:00: decoder0.1 added to root0
  ...
   pci0000:e0: decoder1.0 added to port1
   pci0000:e0: decoder1.1 added to port1
  ...
  cxl_mem mem0: decoder5.0 added to endpoint5
  cxl_mem mem0: decoder5.1 added to endpoint5

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/acpi.c     | 10 +++++++++-
 drivers/cxl/core/hdm.c |  3 ++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index cb14829bb9be..3e75e612cbc4 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -421,7 +421,15 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
 	rc = cxl_decoder_add(cxld, target_map);
 	if (rc)
 		return rc;
-	return cxl_root_decoder_autoremove(dev, no_free_ptr(cxlrd));
+
+	rc = cxl_root_decoder_autoremove(dev, no_free_ptr(cxlrd));
+	if (rc)
+		return rc;
+
+	dev_dbg(root_port->dev.parent, "%s added to %s\n",
+		dev_name(&cxld->dev), dev_name(&root_port->dev));
+
+	return 0;
 }
 
 static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index d705dec1471e..467e4fef6a53 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -34,7 +34,8 @@ static int add_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
 	if (rc)
 		return rc;
 
-	dev_dbg(&cxld->dev, "Added to port %s\n", dev_name(&port->dev));
+	dev_dbg(port->uport_dev, "%s added to %s\n",
+		dev_name(&cxld->dev), dev_name(&port->dev));
 
 	return 0;
 }
-- 
2.39.5

Re: [PATCH v3 17/18] cxl: Add a dev_dbg() when a decoder was added to a port

[Jonathan Cameron]

On Tue, 11 Feb 2025 10:53:47 +0100
Robert Richter <rrichter@amd.com> wrote:

> Improve debugging by adding and unifying messages whenever a decoder
> was added to a port. It is especially useful to get the decoder
> mapping of the involved CXL host bridge or PCI device. This avoids a
> complex lookup of the decoder/port/device mappings in sysfs.
> 
> Example log messages:
> 
>   cxl_acpi ACPI0017:00: decoder0.0 added to root0
>   cxl_acpi ACPI0017:00: decoder0.1 added to root0
>   ...
>    pci0000:e0: decoder1.0 added to port1
>    pci0000:e0: decoder1.1 added to port1
>   ...
>   cxl_mem mem0: decoder5.0 added to endpoint5
>   cxl_mem mem0: decoder5.1 added to endpoint5
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
Seems reasonable to me.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v3 17/18] cxl: Add a dev_dbg() when a decoder was added to a port

[Dave Jiang]

On 2/11/25 2:53 AM, Robert Richter wrote:
> Improve debugging by adding and unifying messages whenever a decoder
> was added to a port. It is especially useful to get the decoder
> mapping of the involved CXL host bridge or PCI device. This avoids a
> complex lookup of the decoder/port/device mappings in sysfs.
> 
> Example log messages:
> 
>   cxl_acpi ACPI0017:00: decoder0.0 added to root0
>   cxl_acpi ACPI0017:00: decoder0.1 added to root0
>   ...
>    pci0000:e0: decoder1.0 added to port1
>    pci0000:e0: decoder1.1 added to port1
>   ...
>   cxl_mem mem0: decoder5.0 added to endpoint5
>   cxl_mem mem0: decoder5.1 added to endpoint5
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/cxl/acpi.c     | 10 +++++++++-
>  drivers/cxl/core/hdm.c |  3 ++-
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index cb14829bb9be..3e75e612cbc4 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -421,7 +421,15 @@ static int __cxl_parse_cfmws(struct acpi_cedt_cfmws *cfmws,
>  	rc = cxl_decoder_add(cxld, target_map);
>  	if (rc)
>  		return rc;
> -	return cxl_root_decoder_autoremove(dev, no_free_ptr(cxlrd));
> +
> +	rc = cxl_root_decoder_autoremove(dev, no_free_ptr(cxlrd));
> +	if (rc)
> +		return rc;
> +
> +	dev_dbg(root_port->dev.parent, "%s added to %s\n",
> +		dev_name(&cxld->dev), dev_name(&root_port->dev));
> +
> +	return 0;
>  }
>  
>  static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index d705dec1471e..467e4fef6a53 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -34,7 +34,8 @@ static int add_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
>  	if (rc)
>  		return rc;
>  
> -	dev_dbg(&cxld->dev, "Added to port %s\n", dev_name(&port->dev));
> +	dev_dbg(port->uport_dev, "%s added to %s\n",
> +		dev_name(&cxld->dev), dev_name(&port->dev));
>  
>  	return 0;
>  }

[PATCH v3 18/18] cxl/acpi: Unify CFMWS memory log messages with SRAT messages

[Robert Richter]

CFMWS entries have a similar importance as SRAT table entries to
describe memory regions. For CXL error analysis and memory debugging
information of both is needed. Unify output of both messages to
improve logging. Change the style of CFMWS message according to SRAT
output. Also, turn messages into a dev_info() same as for SRAT.

SRAT pr_info() for reference:

drivers/acpi/numa/srat.c:
  pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s%s\n",

Signed-off-by: Robert Richter <rrichter@amd.com>
Reviewed-by: Gregory Price <gourry@gourry.net>
Tested-by: Gregory Price <gourry@gourry.net>
---
 drivers/cxl/acpi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 3e75e612cbc4..93c73b163c28 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -447,7 +447,7 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
 			cfmws->base_hpa,
 			cfmws->base_hpa + cfmws->window_size - 1, rc);
 	else
-		dev_dbg(dev, "decode range: node: %d range [%#llx - %#llx]\n",
+		dev_info(dev, "ACPI: CFMWS: Node %u [mem %#010Lx-%#010Lx]\n",
 			phys_to_target_node(cfmws->base_hpa), cfmws->base_hpa,
 			cfmws->base_hpa + cfmws->window_size - 1);
 
-- 
2.39.5

Re: [PATCH v3 18/18] cxl/acpi: Unify CFMWS memory log messages with SRAT messages

[Jonathan Cameron]

On Tue, 11 Feb 2025 10:53:48 +0100
Robert Richter <rrichter@amd.com> wrote:

> CFMWS entries have a similar importance as SRAT table entries to
> describe memory regions. For CXL error analysis and memory debugging
> information of both is needed. Unify output of both messages to
> improve logging. Change the style of CFMWS message according to SRAT
> output. Also, turn messages into a dev_info() same as for SRAT.
> 
> SRAT pr_info() for reference:
> 
> drivers/acpi/numa/srat.c:
>   pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s%s\n",
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v3 18/18] cxl/acpi: Unify CFMWS memory log messages with SRAT messages

[Dave Jiang]

On 2/11/25 2:53 AM, Robert Richter wrote:
> CFMWS entries have a similar importance as SRAT table entries to
> describe memory regions. For CXL error analysis and memory debugging
> information of both is needed. Unify output of both messages to
> improve logging. Change the style of CFMWS message according to SRAT
> output. Also, turn messages into a dev_info() same as for SRAT.
> 
> SRAT pr_info() for reference:
> 
> drivers/acpi/numa/srat.c:
>   pr_info("SRAT: Node %u PXM %u [mem %#010Lx-%#010Lx]%s%s\n",
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Reviewed-by: Gregory Price <gourry@gourry.net>
> Tested-by: Gregory Price <gourry@gourry.net>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/acpi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> index 3e75e612cbc4..93c73b163c28 100644
> --- a/drivers/cxl/acpi.c
> +++ b/drivers/cxl/acpi.c
> @@ -447,7 +447,7 @@ static int cxl_parse_cfmws(union acpi_subtable_headers *header, void *arg,
>  			cfmws->base_hpa,
>  			cfmws->base_hpa + cfmws->window_size - 1, rc);
>  	else
> -		dev_dbg(dev, "decode range: node: %d range [%#llx - %#llx]\n",
> +		dev_info(dev, "ACPI: CFMWS: Node %u [mem %#010Lx-%#010Lx]\n",
>  			phys_to_target_node(cfmws->base_hpa), cfmws->base_hpa,
>  			cfmws->base_hpa + cfmws->window_size - 1);
>