[PATCH 00/14] cxl: Fix "mem_enable" handling

[PATCH 00/14] cxl: Fix "mem_enable" handling

[Dan Williams]

Jonathan reports [1] that after he changed QEMU to stop setting
Mem_Enable (8.1.3.2 DVSEC CXL Control (Bit 2)) by default the following
problems arose:

    1. Nothing in the Linux code actually sets Mem_Enable to 1.
    2. Probing fails in mem.c as wait_for_media() checks for
       info->mem_enabled (cached value of this bit).

The investigation turned up more issues:

- DVSEC ranges are always non-zero size, so it is ambiguous, just
  looking at the registers, as to whether platform firmware is trying to
  route the first 256M of memory to CXL, or just failed to change the
  registers from the default.

- No driver consideration for clearing "mem_enabled" and / or HDM
  Decoder Enable.

- The cxl_test mock override for cxl_hdm_decode_init() was hiding bugs
  in this path.

The end goal of these reworks are to improve detection for cases where
platform firmware is actually operating in legacy CXL DVSEC Range mode,
take ownership for setting and clearing "mem_enable" and HDM Decoder
Enable, and cleanup the indirections / mocking for cxl_test.

The new flow is described in patch 14:

    Previously, the cxl_mem driver was relying on platform-firmware to set
    "mem_enable". That is an invalid assumption as there is no requirement
    that platform-firmware sets the bit before the driver sees a device,
    especially in hot-plug scenarios. Additionally, ACPI-platforms that
    support CXL 2.0 devices also support the ACPI CEDT (CXL Early Discovery
    Table). That table outlines the platform permissible address ranges for
    CXL operation. So, there is a need for the driver to set "mem_enable",
    and there is information available to determine the validity of the CXL
    DVSEC Ranges. Note that the DVSEC Ranges can not be shut off completely.
    They always decode at least 256MB if "mem_enable" is set and the HDM
    Decoder capability is disabled.

    Arrange for the driver to optionally enable the HDM Decoder Capability
    if "mem_enable" was not set by platform firmware, or the CXL DVSEC Range
    configuration was invalid. Be careful to only disable memory decode if
    the kernel was the one to enable it. In other words, if CXL is backing
    all of kernel memory at boot the device needs to maintain "mem_enable"
    and "HDM Decoder enable" all the way up to handoff back to platform
    firmware (e.g. ACPI S5 state entry may require CXL memory to stay
    active).

Link: https://lore.kernel.org/r/20220426180832.00005f0b@Huawei.com [1]

---

Dan Williams (14):
      cxl/mem: Drop mem_enabled check from wait_for_media()
      cxl/pci: Consolidate wait_for_media() and wait_for_media_ready()
      cxl/pci: Drop wait_for_valid() from cxl_await_media_ready()
      cxl/mem: Fix cxl_mem_probe() error exit
      cxl/mem: Validate port connectivity before dvsec ranges
      cxl/pci: Move cxl_await_media_ready() to the core
      cxl/mem: Consolidate CXL DVSEC Range enumeration in the core
      cxl/mem: Skip range enumeration if mem_enable clear
      cxl/mem: Fix CXL DVSEC Range Sizing
      cxl/mem: Merge cxl_dvsec_ranges() and cxl_hdm_decode_init()
      cxl/pci: Drop @info argument to cxl_hdm_decode_init()
      cxl/port: Move endpoint HDM Decoder Capability init to port driver
      cxl/port: Reuse 'struct cxl_hdm' context for hdm init
      cxl/port: Enable HDM Capability after validating DVSEC Ranges


 drivers/cxl/core/pci.c        |  362 +++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxlmem.h          |    4 
 drivers/cxl/cxlpci.h          |    2 
 drivers/cxl/mem.c             |  115 -------------
 drivers/cxl/pci.c             |  184 ---------------------
 drivers/cxl/port.c            |   28 ++-
 tools/testing/cxl/Kbuild      |    3 
 tools/testing/cxl/mock_mem.c  |   10 -
 tools/testing/cxl/test/mem.c  |   17 --
 tools/testing/cxl/test/mock.c |   29 +++
 10 files changed, 422 insertions(+), 332 deletions(-)
 delete mode 100644 tools/testing/cxl/mock_mem.c

base-commit: e6829d1bd3c4b58296ee9e412f7ed4d6cb390192

[PATCH 01/14] cxl/mem: Drop mem_enabled check from wait_for_media()

[Dan Williams]

Media ready is asserted by the device independent of whether mem_enabled
was ever set. Drop this check to allow for dropping wait_for_media() in
favor of ->wait_media_ready().

Fixes: 8dd2bc0f8e02 ("cxl/mem: Add the cxl_mem driver")
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/mem.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 401b0fbe21db..c2d9dadf4a2e 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -27,12 +27,8 @@
 static int wait_for_media(struct cxl_memdev *cxlmd)
 {
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
-	struct cxl_endpoint_dvsec_info *info = &cxlds->info;
 	int rc;
 
-	if (!info->mem_enabled)
-		return -EBUSY;
-
 	rc = cxlds->wait_media_ready(cxlds);
 	if (rc)
 		return rc;

Re: [PATCH 01/14] cxl/mem: Drop mem_enabled check from wait_for_media()

[Jonathan Cameron]

On Thu, 12 May 2022 11:14:21 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Media ready is asserted by the device independent of whether mem_enabled
> was ever set. Drop this check to allow for dropping wait_for_media() in
> favor of ->wait_media_ready().
> 
> Fixes: 8dd2bc0f8e02 ("cxl/mem: Add the cxl_mem driver")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/mem.c |    4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 401b0fbe21db..c2d9dadf4a2e 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -27,12 +27,8 @@
>  static int wait_for_media(struct cxl_memdev *cxlmd)
>  {
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> -	struct cxl_endpoint_dvsec_info *info = &cxlds->info;
>  	int rc;
>  
> -	if (!info->mem_enabled)
> -		return -EBUSY;
> -
>  	rc = cxlds->wait_media_ready(cxlds);
>  	if (rc)
>  		return rc;
> 

[PATCH 02/14] cxl/pci: Consolidate wait_for_media() and wait_for_media_ready()

[Dan Williams]

Now that wait_for_media() does nothing supplemental to
wait_for_media_ready() just promote wait_for_media_ready() to a common
helper and drop wait_for_media().

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/mem.c |   19 +------------------
 drivers/cxl/pci.c |    4 ++--
 2 files changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index c2d9dadf4a2e..7622cfefa1b0 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -24,23 +24,6 @@
  * in higher level operations.
  */
 
-static int wait_for_media(struct cxl_memdev *cxlmd)
-{
-	struct cxl_dev_state *cxlds = cxlmd->cxlds;
-	int rc;
-
-	rc = cxlds->wait_media_ready(cxlds);
-	if (rc)
-		return rc;
-
-	/*
-	 * We know the device is active, and enabled, if any ranges are non-zero
-	 * we'll need to check later before adding the port since that owns the
-	 * HDM decoder registers.
-	 */
-	return 0;
-}
-
 static int create_endpoint(struct cxl_memdev *cxlmd,
 			   struct cxl_port *parent_port)
 {
@@ -157,7 +140,7 @@ static int cxl_mem_probe(struct device *dev)
 	if (work_pending(&cxlmd->detach_work))
 		return -EBUSY;
 
-	rc = wait_for_media(cxlmd);
+	rc = cxlds->wait_media_ready(cxlds);
 	if (rc) {
 		dev_err(dev, "Media not active (%d)\n", rc);
 		return rc;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index e7ab9a34d718..435f9f89b793 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -423,7 +423,7 @@ static int wait_for_valid(struct cxl_dev_state *cxlds)
  * Wait up to @mbox_ready_timeout for the device to report memory
  * active.
  */
-static int wait_for_media_ready(struct cxl_dev_state *cxlds)
+static int cxl_await_media_ready(struct cxl_dev_state *cxlds)
 {
 	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
 	int d = cxlds->cxl_dvsec;
@@ -593,7 +593,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		dev_warn(&pdev->dev,
 			 "Device DVSEC not present, skip CXL.mem init\n");
 
-	cxlds->wait_media_ready = wait_for_media_ready;
+	cxlds->wait_media_ready = cxl_await_media_ready;
 
 	rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
 	if (rc)

Re: [PATCH 02/14] cxl/pci: Consolidate wait_for_media() and wait_for_media_ready()

[Jonathan Cameron]

On Thu, 12 May 2022 11:14:27 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Now that wait_for_media() does nothing supplemental to
> wait_for_media_ready() just promote wait_for_media_ready() to a common
> helper and drop wait_for_media().
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/mem.c |   19 +------------------
>  drivers/cxl/pci.c |    4 ++--
>  2 files changed, 3 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index c2d9dadf4a2e..7622cfefa1b0 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -24,23 +24,6 @@
>   * in higher level operations.
>   */
>  
> -static int wait_for_media(struct cxl_memdev *cxlmd)
> -{
> -	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> -	int rc;
> -
> -	rc = cxlds->wait_media_ready(cxlds);
> -	if (rc)
> -		return rc;
> -
> -	/*
> -	 * We know the device is active, and enabled, if any ranges are non-zero
> -	 * we'll need to check later before adding the port since that owns the
> -	 * HDM decoder registers.
> -	 */
> -	return 0;
> -}
> -
>  static int create_endpoint(struct cxl_memdev *cxlmd,
>  			   struct cxl_port *parent_port)
>  {
> @@ -157,7 +140,7 @@ static int cxl_mem_probe(struct device *dev)
>  	if (work_pending(&cxlmd->detach_work))
>  		return -EBUSY;
>  
> -	rc = wait_for_media(cxlmd);
> +	rc = cxlds->wait_media_ready(cxlds);
>  	if (rc) {
>  		dev_err(dev, "Media not active (%d)\n", rc);
>  		return rc;
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index e7ab9a34d718..435f9f89b793 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -423,7 +423,7 @@ static int wait_for_valid(struct cxl_dev_state *cxlds)
>   * Wait up to @mbox_ready_timeout for the device to report memory
>   * active.
>   */
> -static int wait_for_media_ready(struct cxl_dev_state *cxlds)
> +static int cxl_await_media_ready(struct cxl_dev_state *cxlds)
>  {
>  	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>  	int d = cxlds->cxl_dvsec;
> @@ -593,7 +593,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		dev_warn(&pdev->dev,
>  			 "Device DVSEC not present, skip CXL.mem init\n");
>  
> -	cxlds->wait_media_ready = wait_for_media_ready;
> +	cxlds->wait_media_ready = cxl_await_media_ready;
>  
>  	rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
>  	if (rc)
> 

[PATCH 03/14] cxl/pci: Drop wait_for_valid() from cxl_await_media_ready()

[Dan Williams]

A check mem_info_valid already happens in __cxl_dvsec_ranges(). Rely on
that instead of calling wait_for_valid again.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/pci.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 435f9f89b793..91b266911e52 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -431,10 +431,6 @@ static int cxl_await_media_ready(struct cxl_dev_state *cxlds)
 	u64 md_status;
 	int rc, i;
 
-	rc = wait_for_valid(cxlds);
-	if (rc)
-		return rc;
-
 	for (i = mbox_ready_timeout; i; i--) {
 		u32 temp;
 

Re: [PATCH 03/14] cxl/pci: Drop wait_for_valid() from cxl_await_media_ready()

[Jonathan Cameron]

On Thu, 12 May 2022 11:14:32 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> A check mem_info_valid already happens in __cxl_dvsec_ranges(). Rely on
> that instead of calling wait_for_valid again.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/pci.c |    4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 435f9f89b793..91b266911e52 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -431,10 +431,6 @@ static int cxl_await_media_ready(struct cxl_dev_state *cxlds)
>  	u64 md_status;
>  	int rc, i;
>  
> -	rc = wait_for_valid(cxlds);
> -	if (rc)
> -		return rc;
> -
>  	for (i = mbox_ready_timeout; i; i--) {
>  		u32 temp;
>  
> 

[PATCH 04/14] cxl/mem: Fix cxl_mem_probe() error exit

[Dan Williams]

The addition of cxl_mem_active() broke error exit scenarios for
cxl_mem_probe(). Return early rather than proceed with disabling suspend.

Fixes: 9ea4dcf49878 ("PM: CXL: Disable suspend")
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/mem.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 7622cfefa1b0..fed7f10ef9b2 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -178,6 +178,8 @@ static int cxl_mem_probe(struct device *dev)
 out:
 	device_unlock(&parent_port->dev);
 	put_device(&parent_port->dev);
+	if (rc)
+		return rc;
 
 	/*
 	 * The kernel may be operating out of CXL memory on this device,

Re: [PATCH 04/14] cxl/mem: Fix cxl_mem_probe() error exit

[Jonathan Cameron]

On Thu, 12 May 2022 11:14:37 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> The addition of cxl_mem_active() broke error exit scenarios for
> cxl_mem_probe(). Return early rather than proceed with disabling suspend.
> 
> Fixes: 9ea4dcf49878 ("PM: CXL: Disable suspend")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

With or without rename of label

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

> ---
>  drivers/cxl/mem.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 7622cfefa1b0..fed7f10ef9b2 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -178,6 +178,8 @@ static int cxl_mem_probe(struct device *dev)
>  out:
>  	device_unlock(&parent_port->dev);
>  	put_device(&parent_port->dev);
> +	if (rc)
> +		return rc;
>  
>  	/*
>  	 * The kernel may be operating out of CXL memory on this device,
> 

[PATCH 05/14] cxl/mem: Validate port connectivity before dvsec ranges

[Dan Williams]

In preparation for validating DVSEC ranges against the platform declared
CXL memory ranges (ACPI CFMWS) move port enumeration before the
endpoint's decoder validation. Ultimately this logic will move to the
port driver, but create a bisect point before that larger move.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/mem.c |   36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index fed7f10ef9b2..80e75a410499 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -140,22 +140,6 @@ static int cxl_mem_probe(struct device *dev)
 	if (work_pending(&cxlmd->detach_work))
 		return -EBUSY;
 
-	rc = cxlds->wait_media_ready(cxlds);
-	if (rc) {
-		dev_err(dev, "Media not active (%d)\n", rc);
-		return rc;
-	}
-
-	/*
-	 * If DVSEC ranges are being used instead of HDM decoder registers there
-	 * is no use in trying to manage those.
-	 */
-	if (!cxl_hdm_decode_init(cxlds)) {
-		dev_err(dev,
-			"Legacy range registers configuration prevents HDM operation.\n");
-		return -EBUSY;
-	}
-
 	rc = devm_cxl_enumerate_ports(cxlmd);
 	if (rc)
 		return rc;
@@ -171,16 +155,32 @@ static int cxl_mem_probe(struct device *dev)
 		dev_err(dev, "CXL port topology %s not enabled\n",
 			dev_name(&parent_port->dev));
 		rc = -ENXIO;
-		goto out;
+		goto unlock;
 	}
 
 	rc = create_endpoint(cxlmd, parent_port);
-out:
+unlock:
 	device_unlock(&parent_port->dev);
 	put_device(&parent_port->dev);
 	if (rc)
 		return rc;
 
+	rc = cxlds->wait_media_ready(cxlds);
+	if (rc) {
+		dev_err(dev, "Media not active (%d)\n", rc);
+		return rc;
+	}
+
+	/*
+	 * If DVSEC ranges are being used instead of HDM decoder registers there
+	 * is no use in trying to manage those.
+	 */
+	if (!cxl_hdm_decode_init(cxlds)) {
+		dev_err(dev,
+			"Legacy range registers configuration prevents HDM operation.\n");
+		return -EBUSY;
+	}
+
 	/*
 	 * The kernel may be operating out of CXL memory on this device,
 	 * there is no spec defined way to determine whether this device

Re: [PATCH 05/14] cxl/mem: Validate port connectivity before dvsec ranges

[Jonathan Cameron]

On Thu, 12 May 2022 11:14:43 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> In preparation for validating DVSEC ranges against the platform declared
> CXL memory ranges (ACPI CFMWS) move port enumeration before the
> endpoint's decoder validation. Ultimately this logic will move to the
> port driver, but create a bisect point before that larger move.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/mem.c |   36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index fed7f10ef9b2..80e75a410499 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -140,22 +140,6 @@ static int cxl_mem_probe(struct device *dev)
>  	if (work_pending(&cxlmd->detach_work))
>  		return -EBUSY;
>  
> -	rc = cxlds->wait_media_ready(cxlds);
> -	if (rc) {
> -		dev_err(dev, "Media not active (%d)\n", rc);
> -		return rc;
> -	}
> -
> -	/*
> -	 * If DVSEC ranges are being used instead of HDM decoder registers there
> -	 * is no use in trying to manage those.
> -	 */
> -	if (!cxl_hdm_decode_init(cxlds)) {
> -		dev_err(dev,
> -			"Legacy range registers configuration prevents HDM operation.\n");
> -		return -EBUSY;
> -	}
> -
>  	rc = devm_cxl_enumerate_ports(cxlmd);
>  	if (rc)
>  		return rc;
> @@ -171,16 +155,32 @@ static int cxl_mem_probe(struct device *dev)
>  		dev_err(dev, "CXL port topology %s not enabled\n",
>  			dev_name(&parent_port->dev));
>  		rc = -ENXIO;
> -		goto out;
> +		goto unlock;
>  	}
>  
>  	rc = create_endpoint(cxlmd, parent_port);
> -out:
> +unlock:

Trivial but I think this rename of the label would be better in the earlier
patch where you first added the if (rc) below.
I suppose it is arguable that the previous patch is a fix so should be minimal
though so I'm not that bothered if you leave it here.


>  	device_unlock(&parent_port->dev);
>  	put_device(&parent_port->dev);
>  	if (rc)
>  		return rc;
>  
> +	rc = cxlds->wait_media_ready(cxlds);
> +	if (rc) {
> +		dev_err(dev, "Media not active (%d)\n", rc);
> +		return rc;
> +	}
> +
> +	/*
> +	 * If DVSEC ranges are being used instead of HDM decoder registers there
> +	 * is no use in trying to manage those.
> +	 */
> +	if (!cxl_hdm_decode_init(cxlds)) {
> +		dev_err(dev,
> +			"Legacy range registers configuration prevents HDM operation.\n");
> +		return -EBUSY;
> +	}
> +
>  	/*
>  	 * The kernel may be operating out of CXL memory on this device,
>  	 * there is no spec defined way to determine whether this device
> 

Re: [PATCH 05/14] cxl/mem: Validate port connectivity before dvsec ranges

[Dan Williams]

On Wed, May 18, 2022 at 9:15 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Thu, 12 May 2022 11:14:43 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > In preparation for validating DVSEC ranges against the platform declared
> > CXL memory ranges (ACPI CFMWS) move port enumeration before the
> > endpoint's decoder validation. Ultimately this logic will move to the
> > port driver, but create a bisect point before that larger move.
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/mem.c |   36 ++++++++++++++++++------------------
> >  1 file changed, 18 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > index fed7f10ef9b2..80e75a410499 100644
> > --- a/drivers/cxl/mem.c
> > +++ b/drivers/cxl/mem.c
> > @@ -140,22 +140,6 @@ static int cxl_mem_probe(struct device *dev)
> >       if (work_pending(&cxlmd->detach_work))
> >               return -EBUSY;
> >
> > -     rc = cxlds->wait_media_ready(cxlds);
> > -     if (rc) {
> > -             dev_err(dev, "Media not active (%d)\n", rc);
> > -             return rc;
> > -     }
> > -
> > -     /*
> > -      * If DVSEC ranges are being used instead of HDM decoder registers there
> > -      * is no use in trying to manage those.
> > -      */
> > -     if (!cxl_hdm_decode_init(cxlds)) {
> > -             dev_err(dev,
> > -                     "Legacy range registers configuration prevents HDM operation.\n");
> > -             return -EBUSY;
> > -     }
> > -
> >       rc = devm_cxl_enumerate_ports(cxlmd);
> >       if (rc)
> >               return rc;
> > @@ -171,16 +155,32 @@ static int cxl_mem_probe(struct device *dev)
> >               dev_err(dev, "CXL port topology %s not enabled\n",
> >                       dev_name(&parent_port->dev));
> >               rc = -ENXIO;
> > -             goto out;
> > +             goto unlock;
> >       }
> >
> >       rc = create_endpoint(cxlmd, parent_port);
> > -out:
> > +unlock:
>
> Trivial but I think this rename of the label would be better in the earlier
> patch where you first added the if (rc) below.
> I suppose it is arguable that the previous patch is a fix so should be minimal
> though so I'm not that bothered if you leave it here.
>

True, the rename is just as much a part of what "9ea4dcf49878 PM: CXL:
Disable suspend" was missing as the fix, I'll go ahead and make the
adjustment.

Re: [PATCH 05/14] cxl/mem: Validate port connectivity before dvsec ranges

[Jonathan Cameron]

On Wed, 18 May 2022 09:41:43 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Wed, May 18, 2022 at 9:15 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Thu, 12 May 2022 11:14:43 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >  
> > > In preparation for validating DVSEC ranges against the platform declared
> > > CXL memory ranges (ACPI CFMWS) move port enumeration before the
> > > endpoint's decoder validation. Ultimately this logic will move to the
> > > port driver, but create a bisect point before that larger move.
> > >
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > ---
> > >  drivers/cxl/mem.c |   36 ++++++++++++++++++------------------
> > >  1 file changed, 18 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > > index fed7f10ef9b2..80e75a410499 100644
> > > --- a/drivers/cxl/mem.c
> > > +++ b/drivers/cxl/mem.c
> > > @@ -140,22 +140,6 @@ static int cxl_mem_probe(struct device *dev)
> > >       if (work_pending(&cxlmd->detach_work))
> > >               return -EBUSY;
> > >
> > > -     rc = cxlds->wait_media_ready(cxlds);
> > > -     if (rc) {
> > > -             dev_err(dev, "Media not active (%d)\n", rc);
> > > -             return rc;
> > > -     }
> > > -
> > > -     /*
> > > -      * If DVSEC ranges are being used instead of HDM decoder registers there
> > > -      * is no use in trying to manage those.
> > > -      */
> > > -     if (!cxl_hdm_decode_init(cxlds)) {
> > > -             dev_err(dev,
> > > -                     "Legacy range registers configuration prevents HDM operation.\n");
> > > -             return -EBUSY;
> > > -     }
> > > -
> > >       rc = devm_cxl_enumerate_ports(cxlmd);
> > >       if (rc)
> > >               return rc;
> > > @@ -171,16 +155,32 @@ static int cxl_mem_probe(struct device *dev)
> > >               dev_err(dev, "CXL port topology %s not enabled\n",
> > >                       dev_name(&parent_port->dev));
> > >               rc = -ENXIO;
> > > -             goto out;
> > > +             goto unlock;
> > >       }
> > >
> > >       rc = create_endpoint(cxlmd, parent_port);
> > > -out:
> > > +unlock:  
> >
> > Trivial but I think this rename of the label would be better in the earlier
> > patch where you first added the if (rc) below.
> > I suppose it is arguable that the previous patch is a fix so should be minimal
> > though so I'm not that bothered if you leave it here.
> >  
> 
> True, the rename is just as much a part of what "9ea4dcf49878 PM: CXL:
> Disable suspend" was missing as the fix, I'll go ahead and make the
> adjustment.

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

I'm lazy enough not to look again for something so trivial ;)

[PATCH 06/14] cxl/pci: Move cxl_await_media_ready() to the core

[Dan Williams]

Allow cxl_await_media_ready() to be mocked for testing purposes rather
than carrying the maintenance burden of an indirect function call in the
mainline driver.

With the move cxl_await_media_ready() can no longer reuse the mailbox
timeout override, so add a media_ready_timeout module parameter to the
core to backfill.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/pci.c        |   48 +++++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxlmem.h          |    3 +--
 drivers/cxl/mem.c             |    2 +-
 drivers/cxl/pci.c             |   45 +-------------------------------------
 tools/testing/cxl/Kbuild      |    1 +
 tools/testing/cxl/test/mem.c  |    7 ------
 tools/testing/cxl/test/mock.c |   15 +++++++++++++
 7 files changed, 67 insertions(+), 54 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index c9a494d6976a..603945f49174 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -1,8 +1,11 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2021 Intel Corporation. All rights reserved. */
+#include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/device.h>
+#include <linux/delay.h>
 #include <linux/pci.h>
 #include <cxlpci.h>
+#include <cxlmem.h>
 #include <cxl.h>
 #include "core.h"
 
@@ -13,6 +16,10 @@
  * a set of helpers for CXL interactions which occur via PCIe.
  */
 
+static unsigned short media_ready_timeout = 60;
+module_param(media_ready_timeout, ushort, 0644);
+MODULE_PARM_DESC(media_ready_timeout, "seconds to wait for media ready");
+
 struct cxl_walk_context {
 	struct pci_bus *bus;
 	struct cxl_port *port;
@@ -94,3 +101,44 @@ int devm_cxl_port_enumerate_dports(struct cxl_port *port)
 	return ctx.count;
 }
 EXPORT_SYMBOL_NS_GPL(devm_cxl_port_enumerate_dports, CXL);
+
+/*
+ * Wait up to @media_ready_timeout for the device to report memory
+ * active.
+ */
+int cxl_await_media_ready(struct cxl_dev_state *cxlds)
+{
+	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
+	int d = cxlds->cxl_dvsec;
+	bool active = false;
+	u64 md_status;
+	int rc, i;
+
+	for (i = media_ready_timeout; i; i--) {
+		u32 temp;
+
+		rc = pci_read_config_dword(
+			pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &temp);
+		if (rc)
+			return rc;
+
+		active = FIELD_GET(CXL_DVSEC_MEM_ACTIVE, temp);
+		if (active)
+			break;
+		msleep(1000);
+	}
+
+	if (!active) {
+		dev_err(&pdev->dev,
+			"timeout awaiting memory active after %d seconds\n",
+			media_ready_timeout);
+		return -ETIMEDOUT;
+	}
+
+	md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
+	if (!CXLMDEV_READY(md_status))
+		return -EIO;
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_await_media_ready, CXL);
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 7235d2f976e5..843916c1dab6 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -192,7 +192,6 @@ struct cxl_endpoint_dvsec_info {
  * @info: Cached DVSEC information about the device.
  * @serial: PCIe Device Serial Number
  * @mbox_send: @dev specific transport for transmitting mailbox commands
- * @wait_media_ready: @dev specific method to await media ready
  *
  * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
  * details on capacity parameters.
@@ -227,7 +226,6 @@ struct cxl_dev_state {
 	u64 serial;
 
 	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
-	int (*wait_media_ready)(struct cxl_dev_state *cxlds);
 };
 
 enum cxl_opcode {
@@ -348,6 +346,7 @@ struct cxl_mem_command {
 int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
 		      size_t in_size, void *out, size_t out_size);
 int cxl_dev_state_identify(struct cxl_dev_state *cxlds);
+int cxl_await_media_ready(struct cxl_dev_state *cxlds);
 int cxl_enumerate_cmds(struct cxl_dev_state *cxlds);
 int cxl_mem_create_range_info(struct cxl_dev_state *cxlds);
 struct cxl_dev_state *cxl_dev_state_create(struct device *dev);
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 80e75a410499..8c3a1c85a7ae 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -165,7 +165,7 @@ static int cxl_mem_probe(struct device *dev)
 	if (rc)
 		return rc;
 
-	rc = cxlds->wait_media_ready(cxlds);
+	rc = cxl_await_media_ready(cxlds);
 	if (rc) {
 		dev_err(dev, "Media not active (%d)\n", rc);
 		return rc;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 91b266911e52..1bf880fa1fb8 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -48,8 +48,7 @@
  */
 static unsigned short mbox_ready_timeout = 60;
 module_param(mbox_ready_timeout, ushort, 0644);
-MODULE_PARM_DESC(mbox_ready_timeout,
-		 "seconds to wait for mailbox ready / memory active status");
+MODULE_PARM_DESC(mbox_ready_timeout, "seconds to wait for mailbox ready");
 
 static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds)
 {
@@ -419,46 +418,6 @@ static int wait_for_valid(struct cxl_dev_state *cxlds)
 	return -ETIMEDOUT;
 }
 
-/*
- * Wait up to @mbox_ready_timeout for the device to report memory
- * active.
- */
-static int cxl_await_media_ready(struct cxl_dev_state *cxlds)
-{
-	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
-	int d = cxlds->cxl_dvsec;
-	bool active = false;
-	u64 md_status;
-	int rc, i;
-
-	for (i = mbox_ready_timeout; i; i--) {
-		u32 temp;
-
-		rc = pci_read_config_dword(
-			pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &temp);
-		if (rc)
-			return rc;
-
-		active = FIELD_GET(CXL_DVSEC_MEM_ACTIVE, temp);
-		if (active)
-			break;
-		msleep(1000);
-	}
-
-	if (!active) {
-		dev_err(&pdev->dev,
-			"timeout awaiting memory active after %d seconds\n",
-			mbox_ready_timeout);
-		return -ETIMEDOUT;
-	}
-
-	md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
-	if (!CXLMDEV_READY(md_status))
-		return -EIO;
-
-	return 0;
-}
-
 /*
  * Return positive number of non-zero ranges on success and a negative
  * error code on failure. The cxl_mem driver depends on ranges == 0 to
@@ -589,8 +548,6 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		dev_warn(&pdev->dev,
 			 "Device DVSEC not present, skip CXL.mem init\n");
 
-	cxlds->wait_media_ready = cxl_await_media_ready;
-
 	rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
 	if (rc)
 		return rc;
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 82e49ab0937d..6007fe770122 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -8,6 +8,7 @@ ldflags-y += --wrap=devm_cxl_port_enumerate_dports
 ldflags-y += --wrap=devm_cxl_setup_hdm
 ldflags-y += --wrap=devm_cxl_add_passthrough_decoder
 ldflags-y += --wrap=devm_cxl_enumerate_decoders
+ldflags-y += --wrap=cxl_await_media_ready
 
 DRIVERS := ../../../drivers
 CXL_SRC := $(DRIVERS)/cxl
diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index b6b726eff3e2..c519ace17b41 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -237,12 +237,6 @@ static int cxl_mock_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *
 	return rc;
 }
 
-static int cxl_mock_wait_media_ready(struct cxl_dev_state *cxlds)
-{
-	msleep(100);
-	return 0;
-}
-
 static void label_area_release(void *lsa)
 {
 	vfree(lsa);
@@ -278,7 +272,6 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 
 	cxlds->serial = pdev->id;
 	cxlds->mbox_send = cxl_mock_mbox_send;
-	cxlds->wait_media_ready = cxl_mock_wait_media_ready;
 	cxlds->payload_size = SZ_4K;
 
 	rc = cxl_enumerate_cmds(cxlds);
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index 6e8c9d63c92d..2c01d81ab014 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -193,6 +193,21 @@ int __wrap_devm_cxl_port_enumerate_dports(struct cxl_port *port)
 }
 EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_port_enumerate_dports, CXL);
 
+int __wrap_cxl_await_media_ready(struct cxl_dev_state *cxlds)
+{
+	int rc, index;
+	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
+
+	if (ops && ops->is_mock_dev(cxlds->dev))
+		rc = 0;
+	else
+		rc = cxl_await_media_ready(cxlds);
+	put_cxl_mock_ops(index);
+
+	return rc;
+}
+EXPORT_SYMBOL_NS_GPL(__wrap_cxl_await_media_ready, CXL);
+
 MODULE_LICENSE("GPL v2");
 MODULE_IMPORT_NS(ACPI);
 MODULE_IMPORT_NS(CXL);

Re: [PATCH 06/14] cxl/pci: Move cxl_await_media_ready() to the core

[Jonathan Cameron]

On Thu, 12 May 2022 11:14:49 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Allow cxl_await_media_ready() to be mocked for testing purposes rather
> than carrying the maintenance burden of an indirect function call in the
> mainline driver.
> 
> With the move cxl_await_media_ready() can no longer reuse the mailbox
> timeout override, so add a media_ready_timeout module parameter to the
> core to backfill.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/pci.c        |   48 +++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxlmem.h          |    3 +--
>  drivers/cxl/mem.c             |    2 +-
>  drivers/cxl/pci.c             |   45 +-------------------------------------
>  tools/testing/cxl/Kbuild      |    1 +
>  tools/testing/cxl/test/mem.c  |    7 ------
>  tools/testing/cxl/test/mock.c |   15 +++++++++++++
>  7 files changed, 67 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index c9a494d6976a..603945f49174 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -1,8 +1,11 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2021 Intel Corporation. All rights reserved. */
> +#include <linux/io-64-nonatomic-lo-hi.h>

Curiously I see the pending branch no longer has this include
(which makes sense!)

Otherwise looks fine to me.

Jonathan


>  #include <linux/device.h>
> +#include <linux/delay.h>
>  #include <linux/pci.h>
>  #include <cxlpci.h>
> +#include <cxlmem.h>
>  #include <cxl.h>
>  #include "core.h"
>  
> @@ -13,6 +16,10 @@
>   * a set of helpers for CXL interactions which occur via PCIe.
>   */
>  
> +static unsigned short media_ready_timeout = 60;
> +module_param(media_ready_timeout, ushort, 0644);
> +MODULE_PARM_DESC(media_ready_timeout, "seconds to wait for media ready");
> +
>  struct cxl_walk_context {
>  	struct pci_bus *bus;
>  	struct cxl_port *port;
> @@ -94,3 +101,44 @@ int devm_cxl_port_enumerate_dports(struct cxl_port *port)
>  	return ctx.count;
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_cxl_port_enumerate_dports, CXL);
> +
> +/*
> + * Wait up to @media_ready_timeout for the device to report memory
> + * active.
> + */
> +int cxl_await_media_ready(struct cxl_dev_state *cxlds)
> +{
> +	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> +	int d = cxlds->cxl_dvsec;
> +	bool active = false;
> +	u64 md_status;
> +	int rc, i;
> +
> +	for (i = media_ready_timeout; i; i--) {
> +		u32 temp;
> +
> +		rc = pci_read_config_dword(
> +			pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &temp);
> +		if (rc)
> +			return rc;
> +
> +		active = FIELD_GET(CXL_DVSEC_MEM_ACTIVE, temp);
> +		if (active)
> +			break;
> +		msleep(1000);
> +	}
> +
> +	if (!active) {
> +		dev_err(&pdev->dev,
> +			"timeout awaiting memory active after %d seconds\n",
> +			media_ready_timeout);
> +		return -ETIMEDOUT;
> +	}
> +
> +	md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> +	if (!CXLMDEV_READY(md_status))
> +		return -EIO;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_await_media_ready, CXL);
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 7235d2f976e5..843916c1dab6 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -192,7 +192,6 @@ struct cxl_endpoint_dvsec_info {
>   * @info: Cached DVSEC information about the device.
>   * @serial: PCIe Device Serial Number
>   * @mbox_send: @dev specific transport for transmitting mailbox commands
> - * @wait_media_ready: @dev specific method to await media ready
>   *
>   * See section 8.2.9.5.2 Capacity Configuration and Label Storage for
>   * details on capacity parameters.
> @@ -227,7 +226,6 @@ struct cxl_dev_state {
>  	u64 serial;
>  
>  	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
> -	int (*wait_media_ready)(struct cxl_dev_state *cxlds);
>  };
>  
>  enum cxl_opcode {
> @@ -348,6 +346,7 @@ struct cxl_mem_command {
>  int cxl_mbox_send_cmd(struct cxl_dev_state *cxlds, u16 opcode, void *in,
>  		      size_t in_size, void *out, size_t out_size);
>  int cxl_dev_state_identify(struct cxl_dev_state *cxlds);
> +int cxl_await_media_ready(struct cxl_dev_state *cxlds);
>  int cxl_enumerate_cmds(struct cxl_dev_state *cxlds);
>  int cxl_mem_create_range_info(struct cxl_dev_state *cxlds);
>  struct cxl_dev_state *cxl_dev_state_create(struct device *dev);
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 80e75a410499..8c3a1c85a7ae 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -165,7 +165,7 @@ static int cxl_mem_probe(struct device *dev)
>  	if (rc)
>  		return rc;
>  
> -	rc = cxlds->wait_media_ready(cxlds);
> +	rc = cxl_await_media_ready(cxlds);
>  	if (rc) {
>  		dev_err(dev, "Media not active (%d)\n", rc);
>  		return rc;
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 91b266911e52..1bf880fa1fb8 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -48,8 +48,7 @@
>   */
>  static unsigned short mbox_ready_timeout = 60;
>  module_param(mbox_ready_timeout, ushort, 0644);
> -MODULE_PARM_DESC(mbox_ready_timeout,
> -		 "seconds to wait for mailbox ready / memory active status");
> +MODULE_PARM_DESC(mbox_ready_timeout, "seconds to wait for mailbox ready");
>  
>  static int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds)
>  {
> @@ -419,46 +418,6 @@ static int wait_for_valid(struct cxl_dev_state *cxlds)
>  	return -ETIMEDOUT;
>  }
>  
> -/*
> - * Wait up to @mbox_ready_timeout for the device to report memory
> - * active.
> - */
> -static int cxl_await_media_ready(struct cxl_dev_state *cxlds)
> -{
> -	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> -	int d = cxlds->cxl_dvsec;
> -	bool active = false;
> -	u64 md_status;
> -	int rc, i;
> -
> -	for (i = mbox_ready_timeout; i; i--) {
> -		u32 temp;
> -
> -		rc = pci_read_config_dword(
> -			pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &temp);
> -		if (rc)
> -			return rc;
> -
> -		active = FIELD_GET(CXL_DVSEC_MEM_ACTIVE, temp);
> -		if (active)
> -			break;
> -		msleep(1000);
> -	}
> -
> -	if (!active) {
> -		dev_err(&pdev->dev,
> -			"timeout awaiting memory active after %d seconds\n",
> -			mbox_ready_timeout);
> -		return -ETIMEDOUT;
> -	}
> -
> -	md_status = readq(cxlds->regs.memdev + CXLMDEV_STATUS_OFFSET);
> -	if (!CXLMDEV_READY(md_status))
> -		return -EIO;
> -
> -	return 0;
> -}
> -
>  /*
>   * Return positive number of non-zero ranges on success and a negative
>   * error code on failure. The cxl_mem driver depends on ranges == 0 to
> @@ -589,8 +548,6 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		dev_warn(&pdev->dev,
>  			 "Device DVSEC not present, skip CXL.mem init\n");
>  
> -	cxlds->wait_media_ready = cxl_await_media_ready;
> -
>  	rc = cxl_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
>  	if (rc)
>  		return rc;
> diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
> index 82e49ab0937d..6007fe770122 100644
> --- a/tools/testing/cxl/Kbuild
> +++ b/tools/testing/cxl/Kbuild
> @@ -8,6 +8,7 @@ ldflags-y += --wrap=devm_cxl_port_enumerate_dports
>  ldflags-y += --wrap=devm_cxl_setup_hdm
>  ldflags-y += --wrap=devm_cxl_add_passthrough_decoder
>  ldflags-y += --wrap=devm_cxl_enumerate_decoders
> +ldflags-y += --wrap=cxl_await_media_ready
>  
>  DRIVERS := ../../../drivers
>  CXL_SRC := $(DRIVERS)/cxl
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index b6b726eff3e2..c519ace17b41 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -237,12 +237,6 @@ static int cxl_mock_mbox_send(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *
>  	return rc;
>  }
>  
> -static int cxl_mock_wait_media_ready(struct cxl_dev_state *cxlds)
> -{
> -	msleep(100);
> -	return 0;
> -}
> -
>  static void label_area_release(void *lsa)
>  {
>  	vfree(lsa);
> @@ -278,7 +272,6 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
>  
>  	cxlds->serial = pdev->id;
>  	cxlds->mbox_send = cxl_mock_mbox_send;
> -	cxlds->wait_media_ready = cxl_mock_wait_media_ready;
>  	cxlds->payload_size = SZ_4K;
>  
>  	rc = cxl_enumerate_cmds(cxlds);
> diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
> index 6e8c9d63c92d..2c01d81ab014 100644
> --- a/tools/testing/cxl/test/mock.c
> +++ b/tools/testing/cxl/test/mock.c
> @@ -193,6 +193,21 @@ int __wrap_devm_cxl_port_enumerate_dports(struct cxl_port *port)
>  }
>  EXPORT_SYMBOL_NS_GPL(__wrap_devm_cxl_port_enumerate_dports, CXL);
>  
> +int __wrap_cxl_await_media_ready(struct cxl_dev_state *cxlds)
> +{
> +	int rc, index;
> +	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
> +
> +	if (ops && ops->is_mock_dev(cxlds->dev))
> +		rc = 0;
> +	else
> +		rc = cxl_await_media_ready(cxlds);
> +	put_cxl_mock_ops(index);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_NS_GPL(__wrap_cxl_await_media_ready, CXL);
> +
>  MODULE_LICENSE("GPL v2");
>  MODULE_IMPORT_NS(ACPI);
>  MODULE_IMPORT_NS(CXL);
> 

Re: [PATCH 06/14] cxl/pci: Move cxl_await_media_ready() to the core

[Dan Williams]

On Wed, May 18, 2022 at 9:21 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Thu, 12 May 2022 11:14:49 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > Allow cxl_await_media_ready() to be mocked for testing purposes rather
> > than carrying the maintenance burden of an indirect function call in the
> > mainline driver.
> >
> > With the move cxl_await_media_ready() can no longer reuse the mailbox
> > timeout override, so add a media_ready_timeout module parameter to the
> > core to backfill.
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/core/pci.c        |   48 +++++++++++++++++++++++++++++++++++++++++
> >  drivers/cxl/cxlmem.h          |    3 +--
> >  drivers/cxl/mem.c             |    2 +-
> >  drivers/cxl/pci.c             |   45 +-------------------------------------
> >  tools/testing/cxl/Kbuild      |    1 +
> >  tools/testing/cxl/test/mem.c  |    7 ------
> >  tools/testing/cxl/test/mock.c |   15 +++++++++++++
> >  7 files changed, 67 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index c9a494d6976a..603945f49174 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -1,8 +1,11 @@
> >  // SPDX-License-Identifier: GPL-2.0-only
> >  /* Copyright(c) 2021 Intel Corporation. All rights reserved. */
> > +#include <linux/io-64-nonatomic-lo-hi.h>
>
> Curiously I see the pending branch no longer has this include
> (which makes sense!)

It never had it before since there were no readq calls in core/pci.c
until this point.

> Otherwise looks fine to me.

Thanks!

Re: [PATCH 06/14] cxl/pci: Move cxl_await_media_ready() to the core

[Jonathan Cameron]

On Wed, 18 May 2022 09:37:24 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Wed, May 18, 2022 at 9:21 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Thu, 12 May 2022 11:14:49 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >  
> > > Allow cxl_await_media_ready() to be mocked for testing purposes rather
> > > than carrying the maintenance burden of an indirect function call in the
> > > mainline driver.
> > >
> > > With the move cxl_await_media_ready() can no longer reuse the mailbox
> > > timeout override, so add a media_ready_timeout module parameter to the
> > > core to backfill.
> > >
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > ---
> > >  drivers/cxl/core/pci.c        |   48 +++++++++++++++++++++++++++++++++++++++++
> > >  drivers/cxl/cxlmem.h          |    3 +--
> > >  drivers/cxl/mem.c             |    2 +-
> > >  drivers/cxl/pci.c             |   45 +-------------------------------------
> > >  tools/testing/cxl/Kbuild      |    1 +
> > >  tools/testing/cxl/test/mem.c  |    7 ------
> > >  tools/testing/cxl/test/mock.c |   15 +++++++++++++
> > >  7 files changed, 67 insertions(+), 54 deletions(-)
> > >
> > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > > index c9a494d6976a..603945f49174 100644
> > > --- a/drivers/cxl/core/pci.c
> > > +++ b/drivers/cxl/core/pci.c
> > > @@ -1,8 +1,11 @@
> > >  // SPDX-License-Identifier: GPL-2.0-only
> > >  /* Copyright(c) 2021 Intel Corporation. All rights reserved. */
> > > +#include <linux/io-64-nonatomic-lo-hi.h>  
> >
> > Curiously I see the pending branch no longer has this include
> > (which makes sense!)  
> 
> It never had it before since there were no readq calls in core/pci.c
> until this point.

I meant the cxl/pending tree seems to have a subtle difference
from what you have emailed out here in that the unnecessary extra
include isn't there.  Editorial change I guess :)

> 
> > Otherwise looks fine to me.  
> 
> Thanks!

Was thinking I'd give RB for the series, but as a few outstanding
bits and bobs I'll do it per patch.

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

Re: [PATCH 06/14] cxl/pci: Move cxl_await_media_ready() to the core

[Dan Williams]

On Wed, May 18, 2022 at 11:02 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Wed, 18 May 2022 09:37:24 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > On Wed, May 18, 2022 at 9:21 AM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:
> > >
> > > On Thu, 12 May 2022 11:14:49 -0700
> > > Dan Williams <dan.j.williams@intel.com> wrote:
> > >
> > > > Allow cxl_await_media_ready() to be mocked for testing purposes rather
> > > > than carrying the maintenance burden of an indirect function call in the
> > > > mainline driver.
> > > >
> > > > With the move cxl_await_media_ready() can no longer reuse the mailbox
> > > > timeout override, so add a media_ready_timeout module parameter to the
> > > > core to backfill.
> > > >
> > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > > ---
> > > >  drivers/cxl/core/pci.c        |   48 +++++++++++++++++++++++++++++++++++++++++
> > > >  drivers/cxl/cxlmem.h          |    3 +--
> > > >  drivers/cxl/mem.c             |    2 +-
> > > >  drivers/cxl/pci.c             |   45 +-------------------------------------
> > > >  tools/testing/cxl/Kbuild      |    1 +
> > > >  tools/testing/cxl/test/mem.c  |    7 ------
> > > >  tools/testing/cxl/test/mock.c |   15 +++++++++++++
> > > >  7 files changed, 67 insertions(+), 54 deletions(-)
> > > >
> > > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > > > index c9a494d6976a..603945f49174 100644
> > > > --- a/drivers/cxl/core/pci.c
> > > > +++ b/drivers/cxl/core/pci.c
> > > > @@ -1,8 +1,11 @@
> > > >  // SPDX-License-Identifier: GPL-2.0-only
> > > >  /* Copyright(c) 2021 Intel Corporation. All rights reserved. */
> > > > +#include <linux/io-64-nonatomic-lo-hi.h>
> > >
> > > Curiously I see the pending branch no longer has this include
> > > (which makes sense!)
> >
> > It never had it before since there were no readq calls in core/pci.c
> > until this point.
>
> I meant the cxl/pending tree seems to have a subtle difference
> from what you have emailed out here in that the unnecessary extra
> include isn't there.  Editorial change I guess :)

Do you mean the cxl/preview branch? That's the only one that might get
work-in-progress patches, but 'pending' and 'next' should have 'Link:'
lines for everything that was pulled off the list. The pending branch
has not picked up anything from this series yet.

[PATCH 07/14] cxl/mem: Consolidate CXL DVSEC Range enumeration in the core

[Dan Williams]

In preparation for fixing the setting of the 'mem_enabled' bit in CXL
DVSEC Control register, move all CXL DVSEC range enumeration into the
same source file.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/pci.c        |  129 +++++++++++++++++++++++++++++++++++++++
 drivers/cxl/cxlmem.h          |    1 
 drivers/cxl/cxlpci.h          |    4 +
 drivers/cxl/mem.c             |   14 ++--
 drivers/cxl/pci.c             |  135 -----------------------------------------
 tools/testing/cxl/Kbuild      |    1 
 tools/testing/cxl/test/mem.c  |   10 ---
 tools/testing/cxl/test/mock.c |   16 +++++
 8 files changed, 158 insertions(+), 152 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 603945f49174..ea6711721901 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -142,3 +142,132 @@ int cxl_await_media_ready(struct cxl_dev_state *cxlds)
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_await_media_ready, CXL);
+
+static int wait_for_valid(struct cxl_dev_state *cxlds)
+{
+	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
+	int d = cxlds->cxl_dvsec, rc;
+	u32 val;
+
+	/*
+	 * Memory_Info_Valid: When set, indicates that the CXL Range 1 Size high
+	 * and Size Low registers are valid. Must be set within 1 second of
+	 * deassertion of reset to CXL device. Likely it is already set by the
+	 * time this runs, but otherwise give a 1.5 second timeout in case of
+	 * clock skew.
+	 */
+	rc = pci_read_config_dword(pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &val);
+	if (rc)
+		return rc;
+
+	if (val & CXL_DVSEC_MEM_INFO_VALID)
+		return 0;
+
+	msleep(1500);
+
+	rc = pci_read_config_dword(pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &val);
+	if (rc)
+		return rc;
+
+	if (val & CXL_DVSEC_MEM_INFO_VALID)
+		return 0;
+
+	return -ETIMEDOUT;
+}
+
+/*
+ * Return positive number of non-zero ranges on success and a negative
+ * error code on failure. The cxl_mem driver depends on ranges == 0 to
+ * init HDM operation.
+ */
+int cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
+		     struct cxl_endpoint_dvsec_info *info)
+{
+	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
+	int hdm_count, rc, i, ranges = 0;
+	struct device *dev = &pdev->dev;
+	int d = cxlds->cxl_dvsec;
+	u16 cap, ctrl;
+
+	if (!d) {
+		dev_dbg(dev, "No DVSEC Capability\n");
+		return -ENXIO;
+	}
+
+	rc = pci_read_config_word(pdev, d + CXL_DVSEC_CAP_OFFSET, &cap);
+	if (rc)
+		return rc;
+
+	rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
+	if (rc)
+		return rc;
+
+	if (!(cap & CXL_DVSEC_MEM_CAPABLE)) {
+		dev_dbg(dev, "Not MEM Capable\n");
+		return -ENXIO;
+	}
+
+	/*
+	 * It is not allowed by spec for MEM.capable to be set and have 0 legacy
+	 * HDM decoders (values > 2 are also undefined as of CXL 2.0). As this
+	 * driver is for a spec defined class code which must be CXL.mem
+	 * capable, there is no point in continuing to enable CXL.mem.
+	 */
+	hdm_count = FIELD_GET(CXL_DVSEC_HDM_COUNT_MASK, cap);
+	if (!hdm_count || hdm_count > 2)
+		return -EINVAL;
+
+	rc = wait_for_valid(cxlds);
+	if (rc) {
+		dev_dbg(dev, "Failure awaiting MEM_INFO_VALID (%d)\n", rc);
+		return rc;
+	}
+
+	info->mem_enabled = FIELD_GET(CXL_DVSEC_MEM_ENABLE, ctrl);
+
+	for (i = 0; i < hdm_count; i++) {
+		u64 base, size;
+		u32 temp;
+
+		rc = pci_read_config_dword(
+			pdev, d + CXL_DVSEC_RANGE_SIZE_HIGH(i), &temp);
+		if (rc)
+			return rc;
+
+		size = (u64)temp << 32;
+
+		rc = pci_read_config_dword(
+			pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(i), &temp);
+		if (rc)
+			return rc;
+
+		size |= temp & CXL_DVSEC_MEM_SIZE_LOW_MASK;
+
+		rc = pci_read_config_dword(
+			pdev, d + CXL_DVSEC_RANGE_BASE_HIGH(i), &temp);
+		if (rc)
+			return rc;
+
+		base = (u64)temp << 32;
+
+		rc = pci_read_config_dword(
+			pdev, d + CXL_DVSEC_RANGE_BASE_LOW(i), &temp);
+		if (rc)
+			return rc;
+
+		base |= temp & CXL_DVSEC_MEM_BASE_LOW_MASK;
+
+		info->dvsec_range[i] = (struct range) {
+			.start = base,
+			.end = base + size - 1
+		};
+
+		if (size)
+			ranges++;
+	}
+
+	info->ranges = ranges;
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_dvsec_ranges, CXL);
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 843916c1dab6..60d10ee1e7fc 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -222,7 +222,6 @@ struct cxl_dev_state {
 	u64 next_persistent_bytes;
 
 	resource_size_t component_reg_phys;
-	struct cxl_endpoint_dvsec_info info;
 	u64 serial;
 
 	int (*mbox_send)(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd *cmd);
diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
index 329e7ea3f36a..ad1b62843195 100644
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -72,4 +72,8 @@ static inline resource_size_t cxl_regmap_to_base(struct pci_dev *pdev,
 }
 
 int devm_cxl_port_enumerate_dports(struct cxl_port *port);
+struct cxl_dev_state;
+struct cxl_endpoint_dvsec_info;
+int cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
+		     struct cxl_endpoint_dvsec_info *info);
 #endif /* __CXL_PCI_H__ */
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 8c3a1c85a7ae..0cfbde134fc7 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -58,18 +58,15 @@ static int create_endpoint(struct cxl_memdev *cxlmd,
  * decoders, or if it can not be determined if DVSEC Ranges are in use.
  * Otherwise, returns true.
  */
-__mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
+__mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
+				struct cxl_endpoint_dvsec_info *info)
 {
-	struct cxl_endpoint_dvsec_info *info = &cxlds->info;
 	struct cxl_register_map map;
 	struct cxl_component_reg_map *cmap = &map.component_map;
 	bool global_enable, retval = false;
 	void __iomem *crb;
 	u32 global_ctrl;
 
-	if (info->ranges < 0)
-		return false;
-
 	/* map hdm decoder */
 	crb = ioremap(cxlds->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
 	if (!crb) {
@@ -125,6 +122,7 @@ static void enable_suspend(void *data)
 static int cxl_mem_probe(struct device *dev)
 {
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
+	struct cxl_endpoint_dvsec_info info = { 0 };
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_port *parent_port;
 	int rc;
@@ -165,6 +163,10 @@ static int cxl_mem_probe(struct device *dev)
 	if (rc)
 		return rc;
 
+	rc = cxl_dvsec_ranges(cxlds, &info);
+	if (rc)
+		return rc;
+
 	rc = cxl_await_media_ready(cxlds);
 	if (rc) {
 		dev_err(dev, "Media not active (%d)\n", rc);
@@ -175,7 +177,7 @@ static int cxl_mem_probe(struct device *dev)
 	 * If DVSEC ranges are being used instead of HDM decoder registers there
 	 * is no use in trying to manage those.
 	 */
-	if (!cxl_hdm_decode_init(cxlds)) {
+	if (!cxl_hdm_decode_init(cxlds, &info)) {
 		dev_err(dev,
 			"Legacy range registers configuration prevents HDM operation.\n");
 		return -EBUSY;
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 1bf880fa1fb8..5a0ae46d4989 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -386,139 +386,6 @@ static int cxl_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
 	return rc;
 }
 
-static int wait_for_valid(struct cxl_dev_state *cxlds)
-{
-	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
-	int d = cxlds->cxl_dvsec, rc;
-	u32 val;
-
-	/*
-	 * Memory_Info_Valid: When set, indicates that the CXL Range 1 Size high
-	 * and Size Low registers are valid. Must be set within 1 second of
-	 * deassertion of reset to CXL device. Likely it is already set by the
-	 * time this runs, but otherwise give a 1.5 second timeout in case of
-	 * clock skew.
-	 */
-	rc = pci_read_config_dword(pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &val);
-	if (rc)
-		return rc;
-
-	if (val & CXL_DVSEC_MEM_INFO_VALID)
-		return 0;
-
-	msleep(1500);
-
-	rc = pci_read_config_dword(pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(0), &val);
-	if (rc)
-		return rc;
-
-	if (val & CXL_DVSEC_MEM_INFO_VALID)
-		return 0;
-
-	return -ETIMEDOUT;
-}
-
-/*
- * Return positive number of non-zero ranges on success and a negative
- * error code on failure. The cxl_mem driver depends on ranges == 0 to
- * init HDM operation.
- */
-static int __cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
-			      struct cxl_endpoint_dvsec_info *info)
-{
-	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
-	int hdm_count, rc, i, ranges = 0;
-	struct device *dev = &pdev->dev;
-	int d = cxlds->cxl_dvsec;
-	u16 cap, ctrl;
-
-	if (!d) {
-		dev_dbg(dev, "No DVSEC Capability\n");
-		return -ENXIO;
-	}
-
-	rc = pci_read_config_word(pdev, d + CXL_DVSEC_CAP_OFFSET, &cap);
-	if (rc)
-		return rc;
-
-	rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
-	if (rc)
-		return rc;
-
-	if (!(cap & CXL_DVSEC_MEM_CAPABLE)) {
-		dev_dbg(dev, "Not MEM Capable\n");
-		return -ENXIO;
-	}
-
-	/*
-	 * It is not allowed by spec for MEM.capable to be set and have 0 legacy
-	 * HDM decoders (values > 2 are also undefined as of CXL 2.0). As this
-	 * driver is for a spec defined class code which must be CXL.mem
-	 * capable, there is no point in continuing to enable CXL.mem.
-	 */
-	hdm_count = FIELD_GET(CXL_DVSEC_HDM_COUNT_MASK, cap);
-	if (!hdm_count || hdm_count > 2)
-		return -EINVAL;
-
-	rc = wait_for_valid(cxlds);
-	if (rc) {
-		dev_dbg(dev, "Failure awaiting MEM_INFO_VALID (%d)\n", rc);
-		return rc;
-	}
-
-	info->mem_enabled = FIELD_GET(CXL_DVSEC_MEM_ENABLE, ctrl);
-
-	for (i = 0; i < hdm_count; i++) {
-		u64 base, size;
-		u32 temp;
-
-		rc = pci_read_config_dword(
-			pdev, d + CXL_DVSEC_RANGE_SIZE_HIGH(i), &temp);
-		if (rc)
-			return rc;
-
-		size = (u64)temp << 32;
-
-		rc = pci_read_config_dword(
-			pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(i), &temp);
-		if (rc)
-			return rc;
-
-		size |= temp & CXL_DVSEC_MEM_SIZE_LOW_MASK;
-
-		rc = pci_read_config_dword(
-			pdev, d + CXL_DVSEC_RANGE_BASE_HIGH(i), &temp);
-		if (rc)
-			return rc;
-
-		base = (u64)temp << 32;
-
-		rc = pci_read_config_dword(
-			pdev, d + CXL_DVSEC_RANGE_BASE_LOW(i), &temp);
-		if (rc)
-			return rc;
-
-		base |= temp & CXL_DVSEC_MEM_BASE_LOW_MASK;
-
-		info->dvsec_range[i] = (struct range) {
-			.start = base,
-			.end = base + size - 1
-		};
-
-		if (size)
-			ranges++;
-	}
-
-	return ranges;
-}
-
-static void cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
-{
-	struct cxl_endpoint_dvsec_info *info = &cxlds->info;
-
-	info->ranges = __cxl_dvsec_ranges(cxlds, info);
-}
-
 static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct cxl_register_map map;
@@ -583,8 +450,6 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
-	cxl_dvsec_ranges(cxlds);
-
 	cxlmd = devm_cxl_add_memdev(cxlds);
 	if (IS_ERR(cxlmd))
 		return PTR_ERR(cxlmd);
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 6007fe770122..2ea6fcb8baa5 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -9,6 +9,7 @@ ldflags-y += --wrap=devm_cxl_setup_hdm
 ldflags-y += --wrap=devm_cxl_add_passthrough_decoder
 ldflags-y += --wrap=devm_cxl_enumerate_decoders
 ldflags-y += --wrap=cxl_await_media_ready
+ldflags-y += --wrap=cxl_dvsec_ranges
 
 DRIVERS := ../../../drivers
 CXL_SRC := $(DRIVERS)/cxl
diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index c519ace17b41..6b9239b2afd4 100644
--- a/tools/testing/cxl/test/mem.c
+++ b/tools/testing/cxl/test/mem.c
@@ -242,14 +242,6 @@ static void label_area_release(void *lsa)
 	vfree(lsa);
 }
 
-static void mock_validate_dvsec_ranges(struct cxl_dev_state *cxlds)
-{
-	struct cxl_endpoint_dvsec_info *info;
-
-	info = &cxlds->info;
-	info->mem_enabled = true;
-}
-
 static int cxl_mock_mem_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -286,8 +278,6 @@ static int cxl_mock_mem_probe(struct platform_device *pdev)
 	if (rc)
 		return rc;
 
-	mock_validate_dvsec_ranges(cxlds);
-
 	cxlmd = devm_cxl_add_memdev(cxlds);
 	if (IS_ERR(cxlmd))
 		return PTR_ERR(cxlmd);
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index 2c01d81ab014..d6aa644822db 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -208,6 +208,22 @@ int __wrap_cxl_await_media_ready(struct cxl_dev_state *cxlds)
 }
 EXPORT_SYMBOL_NS_GPL(__wrap_cxl_await_media_ready, CXL);
 
+int __wrap_cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
+			    struct cxl_endpoint_dvsec_info *info)
+{
+	int rc = 0, index;
+	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
+
+	if (ops && ops->is_mock_dev(cxlds->dev))
+		info->mem_enabled = 1;
+	else
+		rc = cxl_dvsec_ranges(cxlds, info);
+	put_cxl_mock_ops(index);
+
+	return rc;
+}
+EXPORT_SYMBOL_NS_GPL(__wrap_cxl_dvsec_ranges, CXL);
+
 MODULE_LICENSE("GPL v2");
 MODULE_IMPORT_NS(ACPI);
 MODULE_IMPORT_NS(CXL);

Re: [PATCH 07/14] cxl/mem: Consolidate CXL DVSEC Range enumeration in the core

[Jonathan Cameron]

On Thu, 12 May 2022 11:14:54 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> In preparation for fixing the setting of the 'mem_enabled' bit in CXL
> DVSEC Control register, move all CXL DVSEC range enumeration into the
> same source file.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Comment inline but not something to necessarily do anything about as
it's a transient thing mid series.

> ---
>  drivers/cxl/core/pci.c        |  129 +++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxlmem.h          |    1 
>  drivers/cxl/cxlpci.h          |    4 +
>  drivers/cxl/mem.c             |   14 ++--
>  drivers/cxl/pci.c             |  135 -----------------------------------------
>  tools/testing/cxl/Kbuild      |    1 
>  tools/testing/cxl/test/mem.c  |   10 ---
>  tools/testing/cxl/test/mock.c |   16 +++++
>  8 files changed, 158 insertions(+), 152 deletions(-)
> 

...


> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 8c3a1c85a7ae..0cfbde134fc7 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -58,18 +58,15 @@ static int create_endpoint(struct cxl_memdev *cxlmd,
>   * decoders, or if it can not be determined if DVSEC Ranges are in use.
>   * Otherwise, returns true.
>   */
> -__mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
> +__mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
> +				struct cxl_endpoint_dvsec_info *info)

Seems a little over the top to bring in info just to get to info->ranges.
Mind you you get rid of it again later so it's just temporary ugly.

>  {
> -	struct cxl_endpoint_dvsec_info *info = &cxlds->info;
>  	struct cxl_register_map map;
>  	struct cxl_component_reg_map *cmap = &map.component_map;
>  	bool global_enable, retval = false;
>  	void __iomem *crb;
>  	u32 global_ctrl;
>  
> -	if (info->ranges < 0)
> -		return false;
> -
>  	/* map hdm decoder */
>  	crb = ioremap(cxlds->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);

Re: [PATCH 07/14] cxl/mem: Consolidate CXL DVSEC Range enumeration in the core

[Dan Williams]

On Wed, May 18, 2022 at 9:37 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Thu, 12 May 2022 11:14:54 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > In preparation for fixing the setting of the 'mem_enabled' bit in CXL
> > DVSEC Control register, move all CXL DVSEC range enumeration into the
> > same source file.
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Comment inline but not something to necessarily do anything about as
> it's a transient thing mid series.
>
> > ---
> >  drivers/cxl/core/pci.c        |  129 +++++++++++++++++++++++++++++++++++++++
> >  drivers/cxl/cxlmem.h          |    1
> >  drivers/cxl/cxlpci.h          |    4 +
> >  drivers/cxl/mem.c             |   14 ++--
> >  drivers/cxl/pci.c             |  135 -----------------------------------------
> >  tools/testing/cxl/Kbuild      |    1
> >  tools/testing/cxl/test/mem.c  |   10 ---
> >  tools/testing/cxl/test/mock.c |   16 +++++
> >  8 files changed, 158 insertions(+), 152 deletions(-)
> >
>
> ...
>
>
> > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > index 8c3a1c85a7ae..0cfbde134fc7 100644
> > --- a/drivers/cxl/mem.c
> > +++ b/drivers/cxl/mem.c
> > @@ -58,18 +58,15 @@ static int create_endpoint(struct cxl_memdev *cxlmd,
> >   * decoders, or if it can not be determined if DVSEC Ranges are in use.
> >   * Otherwise, returns true.
> >   */
> > -__mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
> > +__mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
> > +                             struct cxl_endpoint_dvsec_info *info)
>
> Seems a little over the top to bring in info just to get to info->ranges.
> Mind you you get rid of it again later so it's just temporary ugly.

Yes, just a little ugliness to get through the impedance barrier to a
more pretty state.

> >  {
> > -     struct cxl_endpoint_dvsec_info *info = &cxlds->info;
> >       struct cxl_register_map map;
> >       struct cxl_component_reg_map *cmap = &map.component_map;
> >       bool global_enable, retval = false;
> >       void __iomem *crb;
> >       u32 global_ctrl;
> >
> > -     if (info->ranges < 0)
> > -             return false;
> > -
> >       /* map hdm decoder */
> >       crb = ioremap(cxlds->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);

Re: [PATCH 07/14] cxl/mem: Consolidate CXL DVSEC Range enumeration in the core

[Jonathan Cameron]

On Wed, 18 May 2022 09:52:03 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Wed, May 18, 2022 at 9:37 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Thu, 12 May 2022 11:14:54 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >  
> > > In preparation for fixing the setting of the 'mem_enabled' bit in CXL
> > > DVSEC Control register, move all CXL DVSEC range enumeration into the
> > > same source file.
> > >
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>  
> > Comment inline but not something to necessarily do anything about as
> > it's a transient thing mid series.

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

> >  
> > > ---
> > >  drivers/cxl/core/pci.c        |  129 +++++++++++++++++++++++++++++++++++++++
> > >  drivers/cxl/cxlmem.h          |    1
> > >  drivers/cxl/cxlpci.h          |    4 +
> > >  drivers/cxl/mem.c             |   14 ++--
> > >  drivers/cxl/pci.c             |  135 -----------------------------------------
> > >  tools/testing/cxl/Kbuild      |    1
> > >  tools/testing/cxl/test/mem.c  |   10 ---
> > >  tools/testing/cxl/test/mock.c |   16 +++++
> > >  8 files changed, 158 insertions(+), 152 deletions(-)
> > >  
> >
> > ...
> >
> >  
> > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > > index 8c3a1c85a7ae..0cfbde134fc7 100644
> > > --- a/drivers/cxl/mem.c
> > > +++ b/drivers/cxl/mem.c
> > > @@ -58,18 +58,15 @@ static int create_endpoint(struct cxl_memdev *cxlmd,
> > >   * decoders, or if it can not be determined if DVSEC Ranges are in use.
> > >   * Otherwise, returns true.
> > >   */
> > > -__mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
> > > +__mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
> > > +                             struct cxl_endpoint_dvsec_info *info)  
> >
> > Seems a little over the top to bring in info just to get to info->ranges.
> > Mind you you get rid of it again later so it's just temporary ugly.  
> 
> Yes, just a little ugliness to get through the impedance barrier to a
> more pretty state.
> 
> > >  {
> > > -     struct cxl_endpoint_dvsec_info *info = &cxlds->info;
> > >       struct cxl_register_map map;
> > >       struct cxl_component_reg_map *cmap = &map.component_map;
> > >       bool global_enable, retval = false;
> > >       void __iomem *crb;
> > >       u32 global_ctrl;
> > >
> > > -     if (info->ranges < 0)
> > > -             return false;
> > > -
> > >       /* map hdm decoder */
> > >       crb = ioremap(cxlds->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);  

[PATCH 08/14] cxl/mem: Skip range enumeration if mem_enable clear

[Dan Williams]

When a device does not have mem_enable set then the current range
settings are moot. Skip the enumeration and cause cxl_hdm_decode_init()
to proceed directly to enable the HDM Decoder Capability.

Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/pci.c |    2 ++
 drivers/cxl/mem.c      |    2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index ea6711721901..f3e59f8b6621 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -224,6 +224,8 @@ int cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
 	}
 
 	info->mem_enabled = FIELD_GET(CXL_DVSEC_MEM_ENABLE, ctrl);
+	if (!info->mem_enabled)
+		return 0;
 
 	for (i = 0; i < hdm_count; i++) {
 		u64 base, size;
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 0cfbde134fc7..902d1f6e189e 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -92,7 +92,7 @@ __mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
 	 * are expected even though Linux does not require or maintain that
 	 * match.
 	 */
-	if (!global_enable && info->ranges)
+	if (!global_enable && info->mem_enabled && info->ranges)
 		goto out;
 
 	retval = true;

Re: [PATCH 08/14] cxl/mem: Skip range enumeration if mem_enable clear

[Jonathan Cameron]

On Thu, 12 May 2022 11:14:59 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> When a device does not have mem_enable set then the current range
> settings are moot. Skip the enumeration and cause cxl_hdm_decode_init()
> to proceed directly to enable the HDM Decoder Capability.
> 
> Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

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

> ---
>  drivers/cxl/core/pci.c |    2 ++
>  drivers/cxl/mem.c      |    2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index ea6711721901..f3e59f8b6621 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -224,6 +224,8 @@ int cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
>  	}
>  
>  	info->mem_enabled = FIELD_GET(CXL_DVSEC_MEM_ENABLE, ctrl);
> +	if (!info->mem_enabled)
> +		return 0;
>  
>  	for (i = 0; i < hdm_count; i++) {
>  		u64 base, size;
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 0cfbde134fc7..902d1f6e189e 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -92,7 +92,7 @@ __mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
>  	 * are expected even though Linux does not require or maintain that
>  	 * match.
>  	 */
> -	if (!global_enable && info->ranges)
> +	if (!global_enable && info->mem_enabled && info->ranges)
>  		goto out;
>  
>  	retval = true;
> 

[PATCH 09/14] cxl/mem: Fix CXL DVSEC Range Sizing

[Dan Williams]

Per CXL 2.0 Section 8.1.3.8.4 "DVSEC CXL Range 1 Base Low" there is no
way to specify decode sizes smaller than 256M. Fix cxl_dvsec_ranges()
and cxl_hdm_decode_init() to account for that default decode range.
Note, that this means that any BIOS implementation that sets mem_enable,
but not HDM Decoder Capability enable will cause the driver to fail to
attach. A later change validates the DVSEC ranges against platform CXL
decode (CXL CFMWS) to make a decision about overriding the default DVSEC
Range configuration.

Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/pci.c |   10 +++++++---
 drivers/cxl/mem.c      |   10 +---------
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index f3e59f8b6621..f1c0677a4f52 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -236,7 +236,12 @@ int cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
 		if (rc)
 			return rc;
 
-		size = (u64)temp << 32;
+		/*
+		 * Per CXL 2.0 Section 8.1.3.8.4 "DVSEC CXL Range 1 Base
+		 * Low", the minimum decode size is 256MB
+		 */
+		size = SZ_256M;
+		size |= (u64)temp << 32;
 
 		rc = pci_read_config_dword(
 			pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(i), &temp);
@@ -264,8 +269,7 @@ int cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
 			.end = base + size - 1
 		};
 
-		if (size)
-			ranges++;
+		ranges++;
 	}
 
 	info->ranges = ranges;
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 902d1f6e189e..af4a88d3c5fa 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -84,15 +84,7 @@ __mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
 			    CXL_HDM_DECODER_CTRL_OFFSET);
 	global_enable = global_ctrl & CXL_HDM_DECODER_ENABLE;
 
-	/*
-	 * 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 (!global_enable && info->mem_enabled && info->ranges)
+	if (!global_enable && info->mem_enabled)
 		goto out;
 
 	retval = true;

Re: [PATCH 09/14] cxl/mem: Fix CXL DVSEC Range Sizing

[Jonathan Cameron]

On Thu, 12 May 2022 11:15:05 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Per CXL 2.0 Section 8.1.3.8.4 "DVSEC CXL Range 1 Base Low" there is no
> way to specify decode sizes smaller than 256M. Fix cxl_dvsec_ranges()
> and cxl_hdm_decode_init() to account for that default decode range.

This is effectively the same as the discussion on patch 14.

My reading of the spec suggests that size can be 0 and that would mean
no access is passed on to the hardware.  It's a rather odd corner case
and would mean the device would only work with HDM decoders and might
well fail some compliance tests (I haven't checked)

It's not a corner case I care about... 

> Note, that this means that any BIOS implementation that sets mem_enable,
> but not HDM Decoder Capability enable will cause the driver to fail to
> attach. A later change validates the DVSEC ranges against platform CXL
> decode (CXL CFMWS) to make a decision about overriding the default DVSEC
> Range configuration.

> 
> Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/pci.c |   10 +++++++---
>  drivers/cxl/mem.c      |   10 +---------
>  2 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index f3e59f8b6621..f1c0677a4f52 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -236,7 +236,12 @@ int cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
>  		if (rc)
>  			return rc;
>  
> -		size = (u64)temp << 32;
> +		/*
> +		 * Per CXL 2.0 Section 8.1.3.8.4 "DVSEC CXL Range 1 Base
> +		 * Low", the minimum decode size is 256MB
> +		 */
> +		size = SZ_256M;

This is not how I read the spec.  The match is base <= Addr < base + size.
If size == 0 then there are no matches as base = base + size and so the
right condition isn't met.  Maybe I'm missing something though...


> +		size |= (u64)temp << 32;
>  
>  		rc = pci_read_config_dword(
>  			pdev, d + CXL_DVSEC_RANGE_SIZE_LOW(i), &temp);
> @@ -264,8 +269,7 @@ int cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
>  			.end = base + size - 1
>  		};
>  
> -		if (size)
> -			ranges++;
> +		ranges++;
>  	}
>  
>  	info->ranges = ranges;
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 902d1f6e189e..af4a88d3c5fa 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -84,15 +84,7 @@ __mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
>  			    CXL_HDM_DECODER_CTRL_OFFSET);
>  	global_enable = global_ctrl & CXL_HDM_DECODER_ENABLE;
>  
> -	/*
> -	 * 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 (!global_enable && info->mem_enabled && info->ranges)
> +	if (!global_enable && info->mem_enabled)
>  		goto out;
>  
>  	retval = true;
> 

Re: [PATCH 09/14] cxl/mem: Fix CXL DVSEC Range Sizing

[Dan Williams]

On Wed, May 18, 2022 at 9:42 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Thu, 12 May 2022 11:15:05 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > Per CXL 2.0 Section 8.1.3.8.4 "DVSEC CXL Range 1 Base Low" there is no
> > way to specify decode sizes smaller than 256M. Fix cxl_dvsec_ranges()
> > and cxl_hdm_decode_init() to account for that default decode range.
>
> This is effectively the same as the discussion on patch 14.
>
> My reading of the spec suggests that size can be 0 and that would mean
> no access is passed on to the hardware.  It's a rather odd corner case
> and would mean the device would only work with HDM decoders and might
> well fail some compliance tests (I haven't checked)
>
> It's not a corner case I care about...
>
> > Note, that this means that any BIOS implementation that sets mem_enable,
> > but not HDM Decoder Capability enable will cause the driver to fail to
> > attach. A later change validates the DVSEC ranges against platform CXL
> > decode (CXL CFMWS) to make a decision about overriding the default DVSEC
> > Range configuration.
>
> >
> > Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/core/pci.c |   10 +++++++---
> >  drivers/cxl/mem.c      |   10 +---------
> >  2 files changed, 8 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index f3e59f8b6621..f1c0677a4f52 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -236,7 +236,12 @@ int cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
> >               if (rc)
> >                       return rc;
> >
> > -             size = (u64)temp << 32;
> > +             /*
> > +              * Per CXL 2.0 Section 8.1.3.8.4 "DVSEC CXL Range 1 Base
> > +              * Low", the minimum decode size is 256MB
> > +              */
> > +             size = SZ_256M;
>
> This is not how I read the spec.  The match is base <= Addr < base + size.
> If size == 0 then there are no matches as base = base + size and so the
> right condition isn't met.  Maybe I'm missing something though...

That's how Ariel reads it too, so I'll just drop this as it doesn't
affect the logic all that much, and hopefully the spec gets cleaned up
to preclude the possibility of sub-256M sized devices.

[PATCH 10/14] cxl/mem: Merge cxl_dvsec_ranges() and cxl_hdm_decode_init()

[Dan Williams]

In preparation for changing how the driver handles 'mem_enable' in the CXL
DVSEC control register. Merge the contents of cxl_hdm_decode_init() into
cxl_dvsec_ranges() and rename the combined function cxl_hdm_decode_init().
The possible cleanups and fixes that result from this merge are saved for a
follow-on change.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/pci.c        |   74 +++++++++++++++++++++++++++++++++++++----
 drivers/cxl/cxlpci.h          |    4 +-
 drivers/cxl/mem.c             |   72 +---------------------------------------
 tools/testing/cxl/Kbuild      |    3 +-
 tools/testing/cxl/mock_mem.c  |   10 ------
 tools/testing/cxl/test/mock.c |    8 ++--
 6 files changed, 75 insertions(+), 96 deletions(-)
 delete mode 100644 tools/testing/cxl/mock_mem.c

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index f1c0677a4f52..6146764ac68e 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -175,13 +175,63 @@ static int wait_for_valid(struct cxl_dev_state *cxlds)
 	return -ETIMEDOUT;
 }
 
-/*
- * Return positive number of non-zero ranges on success and a negative
- * error code on failure. The cxl_mem driver depends on ranges == 0 to
- * init HDM operation.
+static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
+				  struct cxl_endpoint_dvsec_info *info)
+{
+	struct cxl_register_map map;
+	struct cxl_component_reg_map *cmap = &map.component_map;
+	bool global_enable, retval = false;
+	void __iomem *crb;
+	u32 global_ctrl;
+
+	/* map hdm decoder */
+	crb = ioremap(cxlds->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
+	if (!crb) {
+		dev_dbg(cxlds->dev, "Failed to map component registers\n");
+		return false;
+	}
+
+	cxl_probe_component_regs(cxlds->dev, crb, cmap);
+	if (!cmap->hdm_decoder.valid) {
+		dev_dbg(cxlds->dev, "Invalid HDM decoder registers\n");
+		goto out;
+	}
+
+	global_ctrl = readl(crb + cmap->hdm_decoder.offset +
+			    CXL_HDM_DECODER_CTRL_OFFSET);
+	global_enable = global_ctrl & CXL_HDM_DECODER_ENABLE;
+
+	if (!global_enable && info->mem_enabled)
+		goto out;
+
+	retval = true;
+
+	/*
+	 * Permanently (for this boot at least) opt the device into HDM
+	 * operation. Individual HDM decoders still need to be enabled after
+	 * this point.
+	 */
+	if (!global_enable) {
+		dev_dbg(cxlds->dev, "Enabling HDM decode\n");
+		writel(global_ctrl | CXL_HDM_DECODER_ENABLE,
+		       crb + cmap->hdm_decoder.offset +
+			       CXL_HDM_DECODER_CTRL_OFFSET);
+	}
+
+out:
+	iounmap(crb);
+	return retval;
+}
+
+/**
+ * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
+ * @cxlds: Device state
+ * @info: DVSEC Range cached enumeration
+ *
+ * Try to enable the endpoint's HDM Decoder Capability
  */
-int cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
-		     struct cxl_endpoint_dvsec_info *info)
+int cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
+			struct cxl_endpoint_dvsec_info *info)
 {
 	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
 	int hdm_count, rc, i, ranges = 0;
@@ -274,6 +324,16 @@ int cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
 
 	info->ranges = ranges;
 
+	/*
+	 * If DVSEC ranges are being used instead of HDM decoder registers there
+	 * is no use in trying to manage those.
+	 */
+	if (!__cxl_hdm_decode_init(cxlds, info)) {
+		dev_err(dev,
+			"Legacy range registers configuration prevents HDM operation.\n");
+		return -EBUSY;
+	}
+
 	return 0;
 }
-EXPORT_SYMBOL_NS_GPL(cxl_dvsec_ranges, CXL);
+EXPORT_SYMBOL_NS_GPL(cxl_hdm_decode_init, CXL);
diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
index ad1b62843195..202fdaa8d293 100644
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -74,6 +74,6 @@ static inline resource_size_t cxl_regmap_to_base(struct pci_dev *pdev,
 int devm_cxl_port_enumerate_dports(struct cxl_port *port);
 struct cxl_dev_state;
 struct cxl_endpoint_dvsec_info;
-int cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
-		     struct cxl_endpoint_dvsec_info *info);
+int cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
+			struct cxl_endpoint_dvsec_info *info);
 #endif /* __CXL_PCI_H__ */
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index af4a88d3c5fa..2a5dc92d566f 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -46,66 +46,6 @@ static int create_endpoint(struct cxl_memdev *cxlmd,
 	return cxl_endpoint_autoremove(cxlmd, endpoint);
 }
 
-/**
- * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
- * @cxlds: Device state
- *
- * Additionally, enables global HDM decoding. Warning: don't call this outside
- * of probe. Once probe is complete, the port driver owns all access to the HDM
- * decoder registers.
- *
- * Returns: false if DVSEC Ranges are being used instead of HDM
- * decoders, or if it can not be determined if DVSEC Ranges are in use.
- * Otherwise, returns true.
- */
-__mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
-				struct cxl_endpoint_dvsec_info *info)
-{
-	struct cxl_register_map map;
-	struct cxl_component_reg_map *cmap = &map.component_map;
-	bool global_enable, retval = false;
-	void __iomem *crb;
-	u32 global_ctrl;
-
-	/* map hdm decoder */
-	crb = ioremap(cxlds->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
-	if (!crb) {
-		dev_dbg(cxlds->dev, "Failed to map component registers\n");
-		return false;
-	}
-
-	cxl_probe_component_regs(cxlds->dev, crb, cmap);
-	if (!cmap->hdm_decoder.valid) {
-		dev_dbg(cxlds->dev, "Invalid HDM decoder registers\n");
-		goto out;
-	}
-
-	global_ctrl = readl(crb + cmap->hdm_decoder.offset +
-			    CXL_HDM_DECODER_CTRL_OFFSET);
-	global_enable = global_ctrl & CXL_HDM_DECODER_ENABLE;
-
-	if (!global_enable && info->mem_enabled)
-		goto out;
-
-	retval = true;
-
-	/*
-	 * Permanently (for this boot at least) opt the device into HDM
-	 * operation. Individual HDM decoders still need to be enabled after
-	 * this point.
-	 */
-	if (!global_enable) {
-		dev_dbg(cxlds->dev, "Enabling HDM decode\n");
-		writel(global_ctrl | CXL_HDM_DECODER_ENABLE,
-		       crb + cmap->hdm_decoder.offset +
-			       CXL_HDM_DECODER_CTRL_OFFSET);
-	}
-
-out:
-	iounmap(crb);
-	return retval;
-}
-
 static void enable_suspend(void *data)
 {
 	cxl_mem_active_dec();
@@ -155,7 +95,7 @@ static int cxl_mem_probe(struct device *dev)
 	if (rc)
 		return rc;
 
-	rc = cxl_dvsec_ranges(cxlds, &info);
+	rc = cxl_hdm_decode_init(cxlds, &info);
 	if (rc)
 		return rc;
 
@@ -165,16 +105,6 @@ static int cxl_mem_probe(struct device *dev)
 		return rc;
 	}
 
-	/*
-	 * If DVSEC ranges are being used instead of HDM decoder registers there
-	 * is no use in trying to manage those.
-	 */
-	if (!cxl_hdm_decode_init(cxlds, &info)) {
-		dev_err(dev,
-			"Legacy range registers configuration prevents HDM operation.\n");
-		return -EBUSY;
-	}
-
 	/*
 	 * The kernel may be operating out of CXL memory on this device,
 	 * there is no spec defined way to determine whether this device
diff --git a/tools/testing/cxl/Kbuild b/tools/testing/cxl/Kbuild
index 2ea6fcb8baa5..33543231d453 100644
--- a/tools/testing/cxl/Kbuild
+++ b/tools/testing/cxl/Kbuild
@@ -9,7 +9,7 @@ ldflags-y += --wrap=devm_cxl_setup_hdm
 ldflags-y += --wrap=devm_cxl_add_passthrough_decoder
 ldflags-y += --wrap=devm_cxl_enumerate_decoders
 ldflags-y += --wrap=cxl_await_media_ready
-ldflags-y += --wrap=cxl_dvsec_ranges
+ldflags-y += --wrap=cxl_hdm_decode_init
 
 DRIVERS := ../../../drivers
 CXL_SRC := $(DRIVERS)/cxl
@@ -36,7 +36,6 @@ cxl_port-y += config_check.o
 obj-m += cxl_mem.o
 
 cxl_mem-y := $(CXL_SRC)/mem.o
-cxl_mem-y += mock_mem.o
 cxl_mem-y += config_check.o
 
 obj-m += cxl_core.o
diff --git a/tools/testing/cxl/mock_mem.c b/tools/testing/cxl/mock_mem.c
deleted file mode 100644
index 69946f678cfa..000000000000
--- a/tools/testing/cxl/mock_mem.c
+++ /dev/null
@@ -1,10 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/* Copyright(c) 2022 Intel Corporation. All rights reserved. */
-
-#include <linux/types.h>
-
-struct cxl_dev_state;
-bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
-{
-	return true;
-}
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index d6aa644822db..ddf0e7dd9249 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -208,8 +208,8 @@ int __wrap_cxl_await_media_ready(struct cxl_dev_state *cxlds)
 }
 EXPORT_SYMBOL_NS_GPL(__wrap_cxl_await_media_ready, CXL);
 
-int __wrap_cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
-			    struct cxl_endpoint_dvsec_info *info)
+int __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
+				struct cxl_endpoint_dvsec_info *info)
 {
 	int rc = 0, index;
 	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
@@ -217,12 +217,12 @@ int __wrap_cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
 	if (ops && ops->is_mock_dev(cxlds->dev))
 		info->mem_enabled = 1;
 	else
-		rc = cxl_dvsec_ranges(cxlds, info);
+		rc = cxl_hdm_decode_init(cxlds, info);
 	put_cxl_mock_ops(index);
 
 	return rc;
 }
-EXPORT_SYMBOL_NS_GPL(__wrap_cxl_dvsec_ranges, CXL);
+EXPORT_SYMBOL_NS_GPL(__wrap_cxl_hdm_decode_init, CXL);
 
 MODULE_LICENSE("GPL v2");
 MODULE_IMPORT_NS(ACPI);

[PATCH 11/14] cxl/pci: Drop @info argument to cxl_hdm_decode_init()

[Dan Williams]

Now that nothing external to cxl_hdm_decode_init() considers
'struct cxl_endpoint_dvec_info' move it internal to
cxl_hdm_decode_init().

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/pci.c        |   15 +++++++--------
 drivers/cxl/cxlpci.h          |    4 +---
 drivers/cxl/mem.c             |    3 +--
 tools/testing/cxl/test/mock.c |    9 +++------
 4 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 6146764ac68e..8f14d846713c 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -226,14 +226,13 @@ static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
 /**
  * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
  * @cxlds: Device state
- * @info: DVSEC Range cached enumeration
  *
  * Try to enable the endpoint's HDM Decoder Capability
  */
-int cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
-			struct cxl_endpoint_dvsec_info *info)
+int cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
 {
 	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
+	struct cxl_endpoint_dvsec_info info = { 0 };
 	int hdm_count, rc, i, ranges = 0;
 	struct device *dev = &pdev->dev;
 	int d = cxlds->cxl_dvsec;
@@ -273,8 +272,8 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
 		return rc;
 	}
 
-	info->mem_enabled = FIELD_GET(CXL_DVSEC_MEM_ENABLE, ctrl);
-	if (!info->mem_enabled)
+	info.mem_enabled = FIELD_GET(CXL_DVSEC_MEM_ENABLE, ctrl);
+	if (!info.mem_enabled)
 		return 0;
 
 	for (i = 0; i < hdm_count; i++) {
@@ -314,7 +313,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
 
 		base |= temp & CXL_DVSEC_MEM_BASE_LOW_MASK;
 
-		info->dvsec_range[i] = (struct range) {
+		info.dvsec_range[i] = (struct range) {
 			.start = base,
 			.end = base + size - 1
 		};
@@ -322,13 +321,13 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
 		ranges++;
 	}
 
-	info->ranges = ranges;
+	info.ranges = ranges;
 
 	/*
 	 * If DVSEC ranges are being used instead of HDM decoder registers there
 	 * is no use in trying to manage those.
 	 */
-	if (!__cxl_hdm_decode_init(cxlds, info)) {
+	if (!__cxl_hdm_decode_init(cxlds, &info)) {
 		dev_err(dev,
 			"Legacy range registers configuration prevents HDM operation.\n");
 		return -EBUSY;
diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
index 202fdaa8d293..53cd34f8813c 100644
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -73,7 +73,5 @@ static inline resource_size_t cxl_regmap_to_base(struct pci_dev *pdev,
 
 int devm_cxl_port_enumerate_dports(struct cxl_port *port);
 struct cxl_dev_state;
-struct cxl_endpoint_dvsec_info;
-int cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
-			struct cxl_endpoint_dvsec_info *info);
+int cxl_hdm_decode_init(struct cxl_dev_state *cxlds);
 #endif /* __CXL_PCI_H__ */
diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 2a5dc92d566f..8ce89d128e36 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -54,7 +54,6 @@ static void enable_suspend(void *data)
 static int cxl_mem_probe(struct device *dev)
 {
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
-	struct cxl_endpoint_dvsec_info info = { 0 };
 	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_port *parent_port;
 	int rc;
@@ -95,7 +94,7 @@ static int cxl_mem_probe(struct device *dev)
 	if (rc)
 		return rc;
 
-	rc = cxl_hdm_decode_init(cxlds, &info);
+	rc = cxl_hdm_decode_init(cxlds);
 	if (rc)
 		return rc;
 
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index ddf0e7dd9249..45ffbb8f519a 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -208,16 +208,13 @@ int __wrap_cxl_await_media_ready(struct cxl_dev_state *cxlds)
 }
 EXPORT_SYMBOL_NS_GPL(__wrap_cxl_await_media_ready, CXL);
 
-int __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
-				struct cxl_endpoint_dvsec_info *info)
+bool __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
 {
 	int rc = 0, index;
 	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
 
-	if (ops && ops->is_mock_dev(cxlds->dev))
-		info->mem_enabled = 1;
-	else
-		rc = cxl_hdm_decode_init(cxlds, info);
+	if (!ops || !ops->is_mock_dev(cxlds->dev))
+		rc = cxl_hdm_decode_init(cxlds);
 	put_cxl_mock_ops(index);
 
 	return rc;

Re: [PATCH 11/14] cxl/pci: Drop @info argument to cxl_hdm_decode_init()

[Jonathan Cameron]

On Thu, 12 May 2022 11:15:15 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Now that nothing external to cxl_hdm_decode_init() considers
> 'struct cxl_endpoint_dvec_info' move it internal to
> cxl_hdm_decode_init().
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Makes sense.

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

> ---
>  drivers/cxl/core/pci.c        |   15 +++++++--------
>  drivers/cxl/cxlpci.h          |    4 +---
>  drivers/cxl/mem.c             |    3 +--
>  tools/testing/cxl/test/mock.c |    9 +++------
>  4 files changed, 12 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 6146764ac68e..8f14d846713c 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -226,14 +226,13 @@ static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
>  /**
>   * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
>   * @cxlds: Device state
> - * @info: DVSEC Range cached enumeration
>   *
>   * Try to enable the endpoint's HDM Decoder Capability
>   */
> -int cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
> -			struct cxl_endpoint_dvsec_info *info)
> +int cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
>  {
>  	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> +	struct cxl_endpoint_dvsec_info info = { 0 };
>  	int hdm_count, rc, i, ranges = 0;
>  	struct device *dev = &pdev->dev;
>  	int d = cxlds->cxl_dvsec;
> @@ -273,8 +272,8 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
>  		return rc;
>  	}
>  
> -	info->mem_enabled = FIELD_GET(CXL_DVSEC_MEM_ENABLE, ctrl);
> -	if (!info->mem_enabled)
> +	info.mem_enabled = FIELD_GET(CXL_DVSEC_MEM_ENABLE, ctrl);
> +	if (!info.mem_enabled)
>  		return 0;
>  
>  	for (i = 0; i < hdm_count; i++) {
> @@ -314,7 +313,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
>  
>  		base |= temp & CXL_DVSEC_MEM_BASE_LOW_MASK;
>  
> -		info->dvsec_range[i] = (struct range) {
> +		info.dvsec_range[i] = (struct range) {
>  			.start = base,
>  			.end = base + size - 1
>  		};
> @@ -322,13 +321,13 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
>  		ranges++;
>  	}
>  
> -	info->ranges = ranges;
> +	info.ranges = ranges;
>  
>  	/*
>  	 * If DVSEC ranges are being used instead of HDM decoder registers there
>  	 * is no use in trying to manage those.
>  	 */
> -	if (!__cxl_hdm_decode_init(cxlds, info)) {
> +	if (!__cxl_hdm_decode_init(cxlds, &info)) {
>  		dev_err(dev,
>  			"Legacy range registers configuration prevents HDM operation.\n");
>  		return -EBUSY;
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index 202fdaa8d293..53cd34f8813c 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -73,7 +73,5 @@ static inline resource_size_t cxl_regmap_to_base(struct pci_dev *pdev,
>  
>  int devm_cxl_port_enumerate_dports(struct cxl_port *port);
>  struct cxl_dev_state;
> -struct cxl_endpoint_dvsec_info;
> -int cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
> -			struct cxl_endpoint_dvsec_info *info);
> +int cxl_hdm_decode_init(struct cxl_dev_state *cxlds);
>  #endif /* __CXL_PCI_H__ */
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 2a5dc92d566f..8ce89d128e36 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -54,7 +54,6 @@ static void enable_suspend(void *data)
>  static int cxl_mem_probe(struct device *dev)
>  {
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> -	struct cxl_endpoint_dvsec_info info = { 0 };
>  	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_port *parent_port;
>  	int rc;
> @@ -95,7 +94,7 @@ static int cxl_mem_probe(struct device *dev)
>  	if (rc)
>  		return rc;
>  
> -	rc = cxl_hdm_decode_init(cxlds, &info);
> +	rc = cxl_hdm_decode_init(cxlds);
>  	if (rc)
>  		return rc;
>  
> diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
> index ddf0e7dd9249..45ffbb8f519a 100644
> --- a/tools/testing/cxl/test/mock.c
> +++ b/tools/testing/cxl/test/mock.c
> @@ -208,16 +208,13 @@ int __wrap_cxl_await_media_ready(struct cxl_dev_state *cxlds)
>  }
>  EXPORT_SYMBOL_NS_GPL(__wrap_cxl_await_media_ready, CXL);
>  
> -int __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
> -				struct cxl_endpoint_dvsec_info *info)
> +bool __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
>  {
>  	int rc = 0, index;
>  	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
>  
> -	if (ops && ops->is_mock_dev(cxlds->dev))
> -		info->mem_enabled = 1;
> -	else
> -		rc = cxl_hdm_decode_init(cxlds, info);
> +	if (!ops || !ops->is_mock_dev(cxlds->dev))
> +		rc = cxl_hdm_decode_init(cxlds);
>  	put_cxl_mock_ops(index);
>  
>  	return rc;
> 

[PATCH 12/14] cxl/port: Move endpoint HDM Decoder Capability init to port driver

[Dan Williams]

The responsibility for establishing HDM Decoder Capability based
operation is more closely tied to port enabling than memdev enabling
which is concerned with port enumeration. This later enables reusing
@cxlhdm for probing / controlling "global enable" for the HDM Decoder
Capability. For now, just do the nominal move.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/mem.c  |   11 -----------
 drivers/cxl/port.c |   11 +++++++++++
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 8ce89d128e36..c310f1fd3db0 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -54,7 +54,6 @@ static void enable_suspend(void *data)
 static int cxl_mem_probe(struct device *dev)
 {
 	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
-	struct cxl_dev_state *cxlds = cxlmd->cxlds;
 	struct cxl_port *parent_port;
 	int rc;
 
@@ -94,16 +93,6 @@ static int cxl_mem_probe(struct device *dev)
 	if (rc)
 		return rc;
 
-	rc = cxl_hdm_decode_init(cxlds);
-	if (rc)
-		return rc;
-
-	rc = cxl_await_media_ready(cxlds);
-	if (rc) {
-		dev_err(dev, "Media not active (%d)\n", rc);
-		return rc;
-	}
-
 	/*
 	 * The kernel may be operating out of CXL memory on this device,
 	 * there is no spec defined way to determine whether this device
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index d420da5fc39c..a7deaeaf0276 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -38,11 +38,22 @@ static int cxl_port_probe(struct device *dev)
 
 	if (is_cxl_endpoint(port)) {
 		struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport);
+		struct cxl_dev_state *cxlds = cxlmd->cxlds;
 
 		get_device(&cxlmd->dev);
 		rc = devm_add_action_or_reset(dev, schedule_detach, cxlmd);
 		if (rc)
 			return rc;
+
+		rc = cxl_hdm_decode_init(cxlds);
+		if (rc)
+			return rc;
+
+		rc = cxl_await_media_ready(cxlds);
+		if (rc) {
+			dev_err(dev, "Media not active (%d)\n", rc);
+			return rc;
+		}
 	} else {
 		rc = devm_cxl_port_enumerate_dports(port);
 		if (rc < 0)

Re: [PATCH 12/14] cxl/port: Move endpoint HDM Decoder Capability init to port driver

[Jonathan Cameron]

On Thu, 12 May 2022 11:15:21 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> The responsibility for establishing HDM Decoder Capability based
> operation is more closely tied to port enabling than memdev enabling
> which is concerned with port enumeration. This later enables reusing
> @cxlhdm for probing / controlling "global enable" for the HDM Decoder
> Capability. For now, just do the nominal move.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/mem.c  |   11 -----------
>  drivers/cxl/port.c |   11 +++++++++++
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 8ce89d128e36..c310f1fd3db0 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -54,7 +54,6 @@ static void enable_suspend(void *data)
>  static int cxl_mem_probe(struct device *dev)
>  {
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> -	struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  	struct cxl_port *parent_port;
>  	int rc;
>  
> @@ -94,16 +93,6 @@ static int cxl_mem_probe(struct device *dev)
>  	if (rc)
>  		return rc;
>  
> -	rc = cxl_hdm_decode_init(cxlds);
> -	if (rc)
> -		return rc;
> -
> -	rc = cxl_await_media_ready(cxlds);
> -	if (rc) {
> -		dev_err(dev, "Media not active (%d)\n", rc);
> -		return rc;
> -	}
> -
>  	/*
>  	 * The kernel may be operating out of CXL memory on this device,
>  	 * there is no spec defined way to determine whether this device
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index d420da5fc39c..a7deaeaf0276 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -38,11 +38,22 @@ static int cxl_port_probe(struct device *dev)
>  
>  	if (is_cxl_endpoint(port)) {
>  		struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport);
> +		struct cxl_dev_state *cxlds = cxlmd->cxlds;
>  
>  		get_device(&cxlmd->dev);
>  		rc = devm_add_action_or_reset(dev, schedule_detach, cxlmd);
>  		if (rc)
>  			return rc;
> +
> +		rc = cxl_hdm_decode_init(cxlds);
> +		if (rc)
> +			return rc;
> +
> +		rc = cxl_await_media_ready(cxlds);
> +		if (rc) {
> +			dev_err(dev, "Media not active (%d)\n", rc);
> +			return rc;
> +		}
>  	} else {
>  		rc = devm_cxl_port_enumerate_dports(port);
>  		if (rc < 0)
> 

[PATCH 13/14] cxl/port: Reuse 'struct cxl_hdm' context for hdm init

[Dan Williams]

The port driver maps component registers for port operations. Reuse that
mapping for HDM Decoder Capability setup / enable. Move
devm_cxl_setup_hdm() before cxl_hdm_decode_init() and plumb @cxlhdm
through the hdm init helpers.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/pci.c        |   39 ++++++++++-----------------------------
 drivers/cxl/cxlpci.h          |    2 +-
 drivers/cxl/port.c            |   25 ++++++++++++++-----------
 tools/testing/cxl/test/mock.c |    5 +++--
 4 files changed, 28 insertions(+), 43 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 8f14d846713c..a697c48fc830 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -176,35 +176,18 @@ static int wait_for_valid(struct cxl_dev_state *cxlds)
 }
 
 static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
+				  struct cxl_hdm *cxlhdm,
 				  struct cxl_endpoint_dvsec_info *info)
 {
-	struct cxl_register_map map;
-	struct cxl_component_reg_map *cmap = &map.component_map;
-	bool global_enable, retval = false;
-	void __iomem *crb;
+	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
+	bool global_enable;
 	u32 global_ctrl;
 
-	/* map hdm decoder */
-	crb = ioremap(cxlds->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
-	if (!crb) {
-		dev_dbg(cxlds->dev, "Failed to map component registers\n");
-		return false;
-	}
-
-	cxl_probe_component_regs(cxlds->dev, crb, cmap);
-	if (!cmap->hdm_decoder.valid) {
-		dev_dbg(cxlds->dev, "Invalid HDM decoder registers\n");
-		goto out;
-	}
-
-	global_ctrl = readl(crb + cmap->hdm_decoder.offset +
-			    CXL_HDM_DECODER_CTRL_OFFSET);
+	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
 	global_enable = global_ctrl & CXL_HDM_DECODER_ENABLE;
 
 	if (!global_enable && info->mem_enabled)
-		goto out;
-
-	retval = true;
+		return false;
 
 	/*
 	 * Permanently (for this boot at least) opt the device into HDM
@@ -214,22 +197,20 @@ static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
 	if (!global_enable) {
 		dev_dbg(cxlds->dev, "Enabling HDM decode\n");
 		writel(global_ctrl | CXL_HDM_DECODER_ENABLE,
-		       crb + cmap->hdm_decoder.offset +
-			       CXL_HDM_DECODER_CTRL_OFFSET);
+		       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
 	}
 
-out:
-	iounmap(crb);
-	return retval;
+	return true;
 }
 
 /**
  * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
  * @cxlds: Device state
+ * @cxlhdm: Mapped HDM decoder Capability
  *
  * Try to enable the endpoint's HDM Decoder Capability
  */
-int cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
+int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
 {
 	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
 	struct cxl_endpoint_dvsec_info info = { 0 };
@@ -327,7 +308,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
 	 * If DVSEC ranges are being used instead of HDM decoder registers there
 	 * is no use in trying to manage those.
 	 */
-	if (!__cxl_hdm_decode_init(cxlds, &info)) {
+	if (!__cxl_hdm_decode_init(cxlds, cxlhdm, &info)) {
 		dev_err(dev,
 			"Legacy range registers configuration prevents HDM operation.\n");
 		return -EBUSY;
diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
index 53cd34f8813c..fce1c11729c2 100644
--- a/drivers/cxl/cxlpci.h
+++ b/drivers/cxl/cxlpci.h
@@ -73,5 +73,5 @@ static inline resource_size_t cxl_regmap_to_base(struct pci_dev *pdev,
 
 int devm_cxl_port_enumerate_dports(struct cxl_port *port);
 struct cxl_dev_state;
-int cxl_hdm_decode_init(struct cxl_dev_state *cxlds);
+int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm);
 #endif /* __CXL_PCI_H__ */
diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
index a7deaeaf0276..3cf308f114c4 100644
--- a/drivers/cxl/port.c
+++ b/drivers/cxl/port.c
@@ -36,6 +36,19 @@ static int cxl_port_probe(struct device *dev)
 	struct cxl_hdm *cxlhdm;
 	int rc;
 
+
+	if (!is_cxl_endpoint(port)) {
+		rc = devm_cxl_port_enumerate_dports(port);
+		if (rc < 0)
+			return rc;
+		if (rc == 1)
+			return devm_cxl_add_passthrough_decoder(port);
+	}
+
+	cxlhdm = devm_cxl_setup_hdm(port);
+	if (IS_ERR(cxlhdm))
+		return PTR_ERR(cxlhdm);
+
 	if (is_cxl_endpoint(port)) {
 		struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport);
 		struct cxl_dev_state *cxlds = cxlmd->cxlds;
@@ -45,7 +58,7 @@ static int cxl_port_probe(struct device *dev)
 		if (rc)
 			return rc;
 
-		rc = cxl_hdm_decode_init(cxlds);
+		rc = cxl_hdm_decode_init(cxlds, cxlhdm);
 		if (rc)
 			return rc;
 
@@ -54,18 +67,8 @@ static int cxl_port_probe(struct device *dev)
 			dev_err(dev, "Media not active (%d)\n", rc);
 			return rc;
 		}
-	} else {
-		rc = devm_cxl_port_enumerate_dports(port);
-		if (rc < 0)
-			return rc;
-		if (rc == 1)
-			return devm_cxl_add_passthrough_decoder(port);
 	}
 
-	cxlhdm = devm_cxl_setup_hdm(port);
-	if (IS_ERR(cxlhdm))
-		return PTR_ERR(cxlhdm);
-
 	rc = devm_cxl_enumerate_decoders(cxlhdm);
 	if (rc) {
 		dev_err(dev, "Couldn't enumerate decoders (%d)\n", rc);
diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
index 45ffbb8f519a..f1f8c40948c5 100644
--- a/tools/testing/cxl/test/mock.c
+++ b/tools/testing/cxl/test/mock.c
@@ -208,13 +208,14 @@ int __wrap_cxl_await_media_ready(struct cxl_dev_state *cxlds)
 }
 EXPORT_SYMBOL_NS_GPL(__wrap_cxl_await_media_ready, CXL);
 
-bool __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
+bool __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
+				struct cxl_hdm *cxlhdm)
 {
 	int rc = 0, index;
 	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
 
 	if (!ops || !ops->is_mock_dev(cxlds->dev))
-		rc = cxl_hdm_decode_init(cxlds);
+		rc = cxl_hdm_decode_init(cxlds, cxlhdm);
 	put_cxl_mock_ops(index);
 
 	return rc;

Re: [PATCH 13/14] cxl/port: Reuse 'struct cxl_hdm' context for hdm init

[Jonathan Cameron]

On Thu, 12 May 2022 11:15:26 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> The port driver maps component registers for port operations. Reuse that
> mapping for HDM Decoder Capability setup / enable. Move
> devm_cxl_setup_hdm() before cxl_hdm_decode_init() and plumb @cxlhdm
> through the hdm init helpers.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
LGTM

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

> ---
>  drivers/cxl/core/pci.c        |   39 ++++++++++-----------------------------
>  drivers/cxl/cxlpci.h          |    2 +-
>  drivers/cxl/port.c            |   25 ++++++++++++++-----------
>  tools/testing/cxl/test/mock.c |    5 +++--
>  4 files changed, 28 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 8f14d846713c..a697c48fc830 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -176,35 +176,18 @@ static int wait_for_valid(struct cxl_dev_state *cxlds)
>  }
>  
>  static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
> +				  struct cxl_hdm *cxlhdm,
>  				  struct cxl_endpoint_dvsec_info *info)
>  {
> -	struct cxl_register_map map;
> -	struct cxl_component_reg_map *cmap = &map.component_map;
> -	bool global_enable, retval = false;
> -	void __iomem *crb;
> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> +	bool global_enable;
>  	u32 global_ctrl;
>  
> -	/* map hdm decoder */
> -	crb = ioremap(cxlds->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
> -	if (!crb) {
> -		dev_dbg(cxlds->dev, "Failed to map component registers\n");
> -		return false;
> -	}
> -
> -	cxl_probe_component_regs(cxlds->dev, crb, cmap);
> -	if (!cmap->hdm_decoder.valid) {
> -		dev_dbg(cxlds->dev, "Invalid HDM decoder registers\n");
> -		goto out;
> -	}
> -
> -	global_ctrl = readl(crb + cmap->hdm_decoder.offset +
> -			    CXL_HDM_DECODER_CTRL_OFFSET);
> +	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
>  	global_enable = global_ctrl & CXL_HDM_DECODER_ENABLE;
>  
>  	if (!global_enable && info->mem_enabled)
> -		goto out;
> -
> -	retval = true;
> +		return false;
>  
>  	/*
>  	 * Permanently (for this boot at least) opt the device into HDM
> @@ -214,22 +197,20 @@ static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
>  	if (!global_enable) {
>  		dev_dbg(cxlds->dev, "Enabling HDM decode\n");
>  		writel(global_ctrl | CXL_HDM_DECODER_ENABLE,
> -		       crb + cmap->hdm_decoder.offset +
> -			       CXL_HDM_DECODER_CTRL_OFFSET);
> +		       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
>  	}
>  
> -out:
> -	iounmap(crb);
> -	return retval;
> +	return true;
>  }
>  
>  /**
>   * cxl_hdm_decode_init() - Setup HDM decoding for the endpoint
>   * @cxlds: Device state
> + * @cxlhdm: Mapped HDM decoder Capability
>   *
>   * Try to enable the endpoint's HDM Decoder Capability
>   */
> -int cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
> +int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
>  {
>  	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
>  	struct cxl_endpoint_dvsec_info info = { 0 };
> @@ -327,7 +308,7 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
>  	 * If DVSEC ranges are being used instead of HDM decoder registers there
>  	 * is no use in trying to manage those.
>  	 */
> -	if (!__cxl_hdm_decode_init(cxlds, &info)) {
> +	if (!__cxl_hdm_decode_init(cxlds, cxlhdm, &info)) {
>  		dev_err(dev,
>  			"Legacy range registers configuration prevents HDM operation.\n");
>  		return -EBUSY;
> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h
> index 53cd34f8813c..fce1c11729c2 100644
> --- a/drivers/cxl/cxlpci.h
> +++ b/drivers/cxl/cxlpci.h
> @@ -73,5 +73,5 @@ static inline resource_size_t cxl_regmap_to_base(struct pci_dev *pdev,
>  
>  int devm_cxl_port_enumerate_dports(struct cxl_port *port);
>  struct cxl_dev_state;
> -int cxl_hdm_decode_init(struct cxl_dev_state *cxlds);
> +int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm);
>  #endif /* __CXL_PCI_H__ */
> diff --git a/drivers/cxl/port.c b/drivers/cxl/port.c
> index a7deaeaf0276..3cf308f114c4 100644
> --- a/drivers/cxl/port.c
> +++ b/drivers/cxl/port.c
> @@ -36,6 +36,19 @@ static int cxl_port_probe(struct device *dev)
>  	struct cxl_hdm *cxlhdm;
>  	int rc;
>  
> +
> +	if (!is_cxl_endpoint(port)) {
> +		rc = devm_cxl_port_enumerate_dports(port);
> +		if (rc < 0)
> +			return rc;
> +		if (rc == 1)
> +			return devm_cxl_add_passthrough_decoder(port);
> +	}
> +
> +	cxlhdm = devm_cxl_setup_hdm(port);
> +	if (IS_ERR(cxlhdm))
> +		return PTR_ERR(cxlhdm);
> +
>  	if (is_cxl_endpoint(port)) {
>  		struct cxl_memdev *cxlmd = to_cxl_memdev(port->uport);
>  		struct cxl_dev_state *cxlds = cxlmd->cxlds;
> @@ -45,7 +58,7 @@ static int cxl_port_probe(struct device *dev)
>  		if (rc)
>  			return rc;
>  
> -		rc = cxl_hdm_decode_init(cxlds);
> +		rc = cxl_hdm_decode_init(cxlds, cxlhdm);
>  		if (rc)
>  			return rc;
>  
> @@ -54,18 +67,8 @@ static int cxl_port_probe(struct device *dev)
>  			dev_err(dev, "Media not active (%d)\n", rc);
>  			return rc;
>  		}
> -	} else {
> -		rc = devm_cxl_port_enumerate_dports(port);
> -		if (rc < 0)
> -			return rc;
> -		if (rc == 1)
> -			return devm_cxl_add_passthrough_decoder(port);
>  	}
>  
> -	cxlhdm = devm_cxl_setup_hdm(port);
> -	if (IS_ERR(cxlhdm))
> -		return PTR_ERR(cxlhdm);
> -
>  	rc = devm_cxl_enumerate_decoders(cxlhdm);
>  	if (rc) {
>  		dev_err(dev, "Couldn't enumerate decoders (%d)\n", rc);
> diff --git a/tools/testing/cxl/test/mock.c b/tools/testing/cxl/test/mock.c
> index 45ffbb8f519a..f1f8c40948c5 100644
> --- a/tools/testing/cxl/test/mock.c
> +++ b/tools/testing/cxl/test/mock.c
> @@ -208,13 +208,14 @@ int __wrap_cxl_await_media_ready(struct cxl_dev_state *cxlds)
>  }
>  EXPORT_SYMBOL_NS_GPL(__wrap_cxl_await_media_ready, CXL);
>  
> -bool __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
> +bool __wrap_cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
> +				struct cxl_hdm *cxlhdm)
>  {
>  	int rc = 0, index;
>  	struct cxl_mock_ops *ops = get_cxl_mock_ops(&index);
>  
>  	if (!ops || !ops->is_mock_dev(cxlds->dev))
> -		rc = cxl_hdm_decode_init(cxlds);
> +		rc = cxl_hdm_decode_init(cxlds, cxlhdm);
>  	put_cxl_mock_ops(index);
>  
>  	return rc;
> 

[PATCH 14/14] cxl/port: Enable HDM Capability after validating DVSEC Ranges

[Dan Williams]

CXL memory expanders that support the CXL 2.0 memory device class code
include an "HDM Decoder Capability" mechanism to supplant the "CXL DVSEC
Range" mechanism originally defined in CXL 1.1. Both mechanisms depend
on a "mem_enable" bit being set in configuration space before either
mechanism activates. When the HDM Decoder Capability is enabled the CXL
DVSEC Range settings are ignored.

Previously, the cxl_mem driver was relying on platform-firmware to set
"mem_enable". That is an invalid assumption as there is no requirement
that platform-firmware sets the bit before the driver sees a device,
especially in hot-plug scenarios. Additionally, ACPI-platforms that
support CXL 2.0 devices also support the ACPI CEDT (CXL Early Discovery
Table). That table outlines the platform permissible address ranges for
CXL operation. So, there is a need for the driver to set "mem_enable",
and there is information available to determine the validity of the CXL
DVSEC Ranges. Note that the DVSEC Ranges can not be shut off completely.
They always decode at least 256MB if "mem_enable" is set and the HDM
Decoder capability is disabled.

Arrange for the driver to optionally enable the HDM Decoder Capability
if "mem_enable" was not set by platform firmware, or the CXL DVSEC Range
configuration was invalid. Be careful to only disable memory decode if
the kernel was the one to enable it. In other words, if CXL is backing
all of kernel memory at boot the device needs to maintain "mem_enable"
and "HDM Decoder enable" all the way up to handoff back to platform
firmware (e.g. ACPI S5 state entry may require CXL memory to stay
active).

Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/pci.c |  163 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 151 insertions(+), 12 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index a697c48fc830..cf53370d3d20 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -175,30 +175,164 @@ static int wait_for_valid(struct cxl_dev_state *cxlds)
 	return -ETIMEDOUT;
 }
 
+static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val)
+{
+	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
+	int d = cxlds->cxl_dvsec;
+	u16 ctrl;
+	int rc;
+
+	rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
+	if (rc < 0)
+		return rc;
+
+	if ((ctrl & CXL_DVSEC_MEM_ENABLE) == val)
+		return 1;
+	ctrl &= ~CXL_DVSEC_MEM_ENABLE;
+	ctrl |= val;
+
+	rc = pci_write_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, ctrl);
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+
+static void clear_mem_enable(void *cxlds)
+{
+	cxl_set_mem_enable(cxlds, 0);
+}
+
+static int devm_cxl_enable_mem(struct device *host, struct cxl_dev_state *cxlds)
+{
+	int rc;
+
+	rc = cxl_set_mem_enable(cxlds, CXL_DVSEC_MEM_ENABLE);
+	if (rc < 0)
+		return rc;
+	if (rc > 0)
+		return 0;
+	return devm_add_action_or_reset(host, clear_mem_enable, cxlds);
+}
+
+static bool range_contains(struct range *r1, struct range *r2)
+{
+	return r1->start <= r2->start && r1->end >= r2->end;
+}
+
+/* require dvsec ranges to be covered by a locked platform window */
+static int dvsec_range_allowed(struct device *dev, void *arg)
+{
+	struct range *dev_range = arg;
+	struct cxl_decoder *cxld;
+	struct range root_range;
+
+	if (!is_root_decoder(dev))
+		return 0;
+
+	cxld = to_cxl_decoder(dev);
+
+	if (!(cxld->flags & CXL_DECODER_F_LOCK))
+		return 0;
+	if (!(cxld->flags & CXL_DECODER_F_RAM))
+		return 0;
+
+	root_range = (struct range) {
+		.start = cxld->platform_res.start,
+		.end = cxld->platform_res.end,
+	};
+
+	return range_contains(&root_range, dev_range);
+}
+
+static void disable_hdm(void *_cxlhdm)
+{
+	u32 global_ctrl;
+	struct cxl_hdm *cxlhdm = _cxlhdm;
+	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
+
+	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
+	writel(global_ctrl & ~CXL_HDM_DECODER_ENABLE,
+	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
+}
+
+static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm)
+{
+	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
+	u32 global_ctrl;
+
+	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
+	writel(global_ctrl | CXL_HDM_DECODER_ENABLE,
+	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
+
+	return devm_add_action_or_reset(host, disable_hdm, cxlhdm);
+}
+
 static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
 				  struct cxl_hdm *cxlhdm,
 				  struct cxl_endpoint_dvsec_info *info)
 {
 	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
-	bool global_enable;
+	struct cxl_port *port = cxlhdm->port;
+	struct device *dev = cxlds->dev;
+	struct cxl_port *root;
 	u32 global_ctrl;
+	int i, rc;
 
 	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
-	global_enable = global_ctrl & CXL_HDM_DECODER_ENABLE;
 
-	if (!global_enable && info->mem_enabled)
+	/*
+	 * If the HDM Decoder Capability is already enabled then assume
+	 * that some other agent like platform firmware set it up.
+	 */
+	if (global_ctrl & CXL_HDM_DECODER_ENABLE) {
+		rc = devm_cxl_enable_mem(&port->dev, cxlds);
+		if (rc)
+			return false;
+		return true;
+	}
+
+	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 false;
+	}
+
+	for (i = 0; i < info->ranges; i++) {
+		struct device *cxld_dev;
+
+		if (!info->mem_enabled)
+			break;
+
+		cxld_dev = device_find_child(&root->dev, &info->dvsec_range[i],
+					     dvsec_range_allowed);
+		if (!cxld_dev) {
+			dev_dbg(dev, "Range%d disallowed by platform\n", i);
+			cxl_set_mem_enable(cxlds, 0);
+			info->mem_enabled = 0;
+			break;
+		}
+		put_device(cxld_dev);
+		break;
+	}
+	put_device(&root->dev);
 
 	/*
-	 * Permanently (for this boot at least) opt the device into HDM
-	 * operation. Individual HDM decoders still need to be enabled after
-	 * this point.
+	 * At least one DVSEC range is enabled and allowed, skip HDM
+	 * Decoder Capability Enable
 	 */
-	if (!global_enable) {
-		dev_dbg(cxlds->dev, "Enabling HDM decode\n");
-		writel(global_ctrl | CXL_HDM_DECODER_ENABLE,
-		       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
-	}
+	if (info->mem_enabled)
+		return false;
+
+	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
+	if (rc)
+		return false;
+
+	rc = devm_cxl_enable_mem(&port->dev, cxlds);
+	if (rc)
+		return false;
 
 	return true;
 }
@@ -253,9 +387,14 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
 		return rc;
 	}
 
+	/*
+	 * The current DVSEC values are moot if the memory capability is
+	 * disabled, and they will remain moot after the HDM Decoder
+	 * capability is enabled.
+	 */
 	info.mem_enabled = FIELD_GET(CXL_DVSEC_MEM_ENABLE, ctrl);
 	if (!info.mem_enabled)
-		return 0;
+		return __cxl_hdm_decode_init(cxlds, cxlhdm, &info);
 
 	for (i = 0; i < hdm_count; i++) {
 		u64 base, size;

RE: [PATCH 14/14] cxl/port: Enable HDM Capability after validating DVSEC Ranges

[Ariel.Sibley]

> Previously, the cxl_mem driver was relying on platform-firmware to set
> "mem_enable". That is an invalid assumption as there is no requirement
> that platform-firmware sets the bit before the driver sees a device,
> especially in hot-plug scenarios. Additionally, ACPI-platforms that
> support CXL 2.0 devices also support the ACPI CEDT (CXL Early Discovery
> Table). That table outlines the platform permissible address ranges for
> CXL operation. So, there is a need for the driver to set "mem_enable",
> and there is information available to determine the validity of the CXL
> DVSEC Ranges. Note that the DVSEC Ranges can not be shut off completely.
> They always decode at least 256MB if "mem_enable" is set and the HDM
> Decoder capability is disabled.

Regarding "can not be shut off completely", CXL 2.0 Section 8.1.3.8.4
has this statement:
---
A CXL.mem capable device that does not implement CXL HDM Decoder
Capability registers directs host accesses to an address A within its
local HDM memory if the following two equations are satisfied -
Memory_Base[63:28] <= (A >> 28) < Memory_Base[63:28]+Memory_Size[63:28]
Memory_Active AND Mem_Enable=1
---

Per the above, if a device advertises Memory_Size = 0, then the first
equation (second part) can never be true. So, a device advertising
Memory_Size = 0 will effectively shut off this range, even if
Mem_Enable = 1 and the HDM Decoder capability is disabled.

Regards,
Ariel

Re: [PATCH 14/14] cxl/port: Enable HDM Capability after validating DVSEC Ranges

[Dan Williams]

On Mon, May 16, 2022 at 11:41 AM <Ariel.Sibley@microchip.com> wrote:
>
> > Previously, the cxl_mem driver was relying on platform-firmware to set
> > "mem_enable". That is an invalid assumption as there is no requirement
> > that platform-firmware sets the bit before the driver sees a device,
> > especially in hot-plug scenarios. Additionally, ACPI-platforms that
> > support CXL 2.0 devices also support the ACPI CEDT (CXL Early Discovery
> > Table). That table outlines the platform permissible address ranges for
> > CXL operation. So, there is a need for the driver to set "mem_enable",
> > and there is information available to determine the validity of the CXL
> > DVSEC Ranges. Note that the DVSEC Ranges can not be shut off completely.
> > They always decode at least 256MB if "mem_enable" is set and the HDM
> > Decoder capability is disabled.
>
> Regarding "can not be shut off completely", CXL 2.0 Section 8.1.3.8.4
> has this statement:
> ---
> A CXL.mem capable device that does not implement CXL HDM Decoder
> Capability registers directs host accesses to an address A within its
> local HDM memory if the following two equations are satisfied -
> Memory_Base[63:28] <= (A >> 28) < Memory_Base[63:28]+Memory_Size[63:28]
> Memory_Active AND Mem_Enable=1
> ---
>
> Per the above, if a device advertises Memory_Size = 0, then the first
> equation (second part) can never be true. So, a device advertising
> Memory_Size = 0 will effectively shut off this range, even if
> Mem_Enable = 1 and the HDM Decoder capability is disabled.

Hmm, so now I am confused as to why the spec goes on to say:

"If the address A is not backed by real memory (e.g. a device with
less than 256 MB of
memory), a device that does not implement CXL HDM Decoder Capability registers
must handle those accesses gracefully i.e. return all 1’s on reads and
drop writes."

This is what led me to conclude that even though Memory_Size is 0, the
device is still actively decoding [0, 256MB).

RE: [PATCH 14/14] cxl/port: Enable HDM Capability after validating DVSEC Ranges

[Ariel.Sibley]

> > > Previously, the cxl_mem driver was relying on platform-firmware to set
> > > "mem_enable". That is an invalid assumption as there is no requirement
> > > that platform-firmware sets the bit before the driver sees a device,
> > > especially in hot-plug scenarios. Additionally, ACPI-platforms that
> > > support CXL 2.0 devices also support the ACPI CEDT (CXL Early Discovery
> > > Table). That table outlines the platform permissible address ranges for
> > > CXL operation. So, there is a need for the driver to set "mem_enable",
> > > and there is information available to determine the validity of the CXL
> > > DVSEC Ranges. Note that the DVSEC Ranges can not be shut off completely.
> > > They always decode at least 256MB if "mem_enable" is set and the HDM
> > > Decoder capability is disabled.
> >
> > Regarding "can not be shut off completely", CXL 2.0 Section 8.1.3.8.4
> > has this statement:
> > ---
> > A CXL.mem capable device that does not implement CXL HDM Decoder
> > Capability registers directs host accesses to an address A within its
> > local HDM memory if the following two equations are satisfied -
> > Memory_Base[63:28] <= (A >> 28) < Memory_Base[63:28]+Memory_Size[63:28]
> > Memory_Active AND Mem_Enable=1
> > ---
> >
> > Per the above, if a device advertises Memory_Size = 0, then the first
> > equation (second part) can never be true. So, a device advertising
> > Memory_Size = 0 will effectively shut off this range, even if
> > Mem_Enable = 1 and the HDM Decoder capability is disabled.
> 
> Hmm, so now I am confused as to why the spec goes on to say:
> 
> "If the address A is not backed by real memory (e.g. a device with
> less than 256 MB of
> memory), a device that does not implement CXL HDM Decoder Capability registers
> must handle those accesses gracefully i.e. return all 1’s on reads and
> drop writes."
> 
> This is what led me to conclude that even though Memory_Size is 0, the
> device is still actively decoding [0, 256MB).

My interpretation of that statement is that such a device would need to
advertise a capacity of 256M, and behave per the text for the portion of
the 256M range not backed by physical memory. Such a device would be 
problematic for the host to handle, as I'm not sure how the host would be 
able to determine the portion of the range that is usable. As such, I
don't think building such a device would be practical.

There are several other locations in the CXL 2.0 spec that indicate that
devices must manage capacity in units of 256M. E.g. Table 175. Identify
Memory Device Output Payload.

Regards,
Ariel

Re: [PATCH 14/14] cxl/port: Enable HDM Capability after validating DVSEC Ranges

[Dan Williams]

On Mon, May 16, 2022 at 12:32 PM <Ariel.Sibley@microchip.com> wrote:
>
> > > > Previously, the cxl_mem driver was relying on platform-firmware to set
> > > > "mem_enable". That is an invalid assumption as there is no requirement
> > > > that platform-firmware sets the bit before the driver sees a device,
> > > > especially in hot-plug scenarios. Additionally, ACPI-platforms that
> > > > support CXL 2.0 devices also support the ACPI CEDT (CXL Early Discovery
> > > > Table). That table outlines the platform permissible address ranges for
> > > > CXL operation. So, there is a need for the driver to set "mem_enable",
> > > > and there is information available to determine the validity of the CXL
> > > > DVSEC Ranges. Note that the DVSEC Ranges can not be shut off completely.
> > > > They always decode at least 256MB if "mem_enable" is set and the HDM
> > > > Decoder capability is disabled.
> > >
> > > Regarding "can not be shut off completely", CXL 2.0 Section 8.1.3.8.4
> > > has this statement:
> > > ---
> > > A CXL.mem capable device that does not implement CXL HDM Decoder
> > > Capability registers directs host accesses to an address A within its
> > > local HDM memory if the following two equations are satisfied -
> > > Memory_Base[63:28] <= (A >> 28) < Memory_Base[63:28]+Memory_Size[63:28]
> > > Memory_Active AND Mem_Enable=1
> > > ---
> > >
> > > Per the above, if a device advertises Memory_Size = 0, then the first
> > > equation (second part) can never be true. So, a device advertising
> > > Memory_Size = 0 will effectively shut off this range, even if
> > > Mem_Enable = 1 and the HDM Decoder capability is disabled.
> >
> > Hmm, so now I am confused as to why the spec goes on to say:
> >
> > "If the address A is not backed by real memory (e.g. a device with
> > less than 256 MB of
> > memory), a device that does not implement CXL HDM Decoder Capability registers
> > must handle those accesses gracefully i.e. return all 1’s on reads and
> > drop writes."
> >
> > This is what led me to conclude that even though Memory_Size is 0, the
> > device is still actively decoding [0, 256MB).
>
> My interpretation of that statement is that such a device would need to
> advertise a capacity of 256M, and behave per the text for the portion of
> the 256M range not backed by physical memory. Such a device would be
> problematic for the host to handle, as I'm not sure how the host would be
> able to determine the portion of the range that is usable. As such, I
> don't think building such a device would be practical.

Right, I don't expect it to happen, but it's easy enough for the
kernel to exclude the possibility.

It just so happens that one of the early revs of the QEMU patch set
introduced this case, so it may not be feasible for hardware, but it's
already happened for emulated devices.

> There are several other locations in the CXL 2.0 spec that indicate that
> devices must manage capacity in units of 256M. E.g. Table 175. Identify
> Memory Device Output Payload.

Yes, for this patch it is just trying to find a reasonable heuristic
for when "Mem_enable = 1" and "HDM Decoder Capabilty Enable = 0" to
determine that the kernel can go ahead and set "HDM Decoder Capability
Enable = 1"

[PATCH v2 14/14] cxl/port: Enable HDM Capability after validating DVSEC Ranges

[Dan Williams]

CXL memory expanders that support the CXL 2.0 memory device class code
include an "HDM Decoder Capability" mechanism to supplant the "CXL DVSEC
Range" mechanism originally defined in CXL 1.1. Both mechanisms depend
on a "mem_enable" bit being set in configuration space before either
mechanism activates. When the HDM Decoder Capability is enabled the CXL
DVSEC Range settings are ignored.

Previously, the cxl_mem driver was relying on platform-firmware to set
"mem_enable". That is an invalid assumption as there is no requirement
that platform-firmware sets the bit before the driver sees a device,
especially in hot-plug scenarios. Additionally, ACPI-platforms that
support CXL 2.0 devices also support the ACPI CEDT (CXL Early Discovery
Table). That table outlines the platform permissible address ranges for
CXL operation. So, there is a need for the driver to set "mem_enable",
and there is information available to determine the validity of the CXL
DVSEC Ranges. While DVSEC Ranges are expected to be at least
256M in size, the specification (CXL 2.0 Section 8.1.3.8.4 DVSEC CXL
Range 1 Base Low) allows for the possibilty of devices smaller than
256M. So the range [0, 256M) is considered active even if Memory_size
is 0.

Arrange for the driver to optionally enable the HDM Decoder Capability
if "mem_enable" was not set by platform firmware, or the CXL DVSEC Range
configuration was invalid. Be careful to only disable memory decode if
the kernel was the one to enable it. In other words, if CXL is backing
all of kernel memory at boot the device needs to maintain "mem_enable"
and "HDM Decoder enable" all the way up to handoff back to platform
firmware (e.g. ACPI S5 state entry may require CXL memory to stay
active).

Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
Cc: Dan Carpenter <dan.carpenter@oracle.com>
[dan: fix early terminiation of range-allowed loop]
Cc: Ariel Sibley <ariel.sibley@microchip.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v1:
- Fix range-allowed loop termination (Smatch / Dan)
- Clean up changeloe wording around why [0, 256M) is considered always
  active (Ariel)

 drivers/cxl/core/pci.c |  163 ++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 151 insertions(+), 12 deletions(-)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index a697c48fc830..528430da0e77 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -175,30 +175,164 @@ static int wait_for_valid(struct cxl_dev_state *cxlds)
 	return -ETIMEDOUT;
 }
 
+static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val)
+{
+	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
+	int d = cxlds->cxl_dvsec;
+	u16 ctrl;
+	int rc;
+
+	rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
+	if (rc < 0)
+		return rc;
+
+	if ((ctrl & CXL_DVSEC_MEM_ENABLE) == val)
+		return 1;
+	ctrl &= ~CXL_DVSEC_MEM_ENABLE;
+	ctrl |= val;
+
+	rc = pci_write_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, ctrl);
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+
+static void clear_mem_enable(void *cxlds)
+{
+	cxl_set_mem_enable(cxlds, 0);
+}
+
+static int devm_cxl_enable_mem(struct device *host, struct cxl_dev_state *cxlds)
+{
+	int rc;
+
+	rc = cxl_set_mem_enable(cxlds, CXL_DVSEC_MEM_ENABLE);
+	if (rc < 0)
+		return rc;
+	if (rc > 0)
+		return 0;
+	return devm_add_action_or_reset(host, clear_mem_enable, cxlds);
+}
+
+static bool range_contains(struct range *r1, struct range *r2)
+{
+	return r1->start <= r2->start && r1->end >= r2->end;
+}
+
+/* require dvsec ranges to be covered by a locked platform window */
+static int dvsec_range_allowed(struct device *dev, void *arg)
+{
+	struct range *dev_range = arg;
+	struct cxl_decoder *cxld;
+	struct range root_range;
+
+	if (!is_root_decoder(dev))
+		return 0;
+
+	cxld = to_cxl_decoder(dev);
+
+	if (!(cxld->flags & CXL_DECODER_F_LOCK))
+		return 0;
+	if (!(cxld->flags & CXL_DECODER_F_RAM))
+		return 0;
+
+	root_range = (struct range) {
+		.start = cxld->platform_res.start,
+		.end = cxld->platform_res.end,
+	};
+
+	return range_contains(&root_range, dev_range);
+}
+
+static void disable_hdm(void *_cxlhdm)
+{
+	u32 global_ctrl;
+	struct cxl_hdm *cxlhdm = _cxlhdm;
+	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
+
+	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
+	writel(global_ctrl & ~CXL_HDM_DECODER_ENABLE,
+	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
+}
+
+static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm)
+{
+	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
+	u32 global_ctrl;
+
+	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
+	writel(global_ctrl | CXL_HDM_DECODER_ENABLE,
+	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
+
+	return devm_add_action_or_reset(host, disable_hdm, cxlhdm);
+}
+
 static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
 				  struct cxl_hdm *cxlhdm,
 				  struct cxl_endpoint_dvsec_info *info)
 {
 	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
-	bool global_enable;
+	struct cxl_port *port = cxlhdm->port;
+	struct device *dev = cxlds->dev;
+	struct cxl_port *root;
+	int i, rc, allowed;
 	u32 global_ctrl;
 
 	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
-	global_enable = global_ctrl & CXL_HDM_DECODER_ENABLE;
 
-	if (!global_enable && info->mem_enabled)
+	/*
+	 * If the HDM Decoder Capability is already enabled then assume
+	 * that some other agent like platform firmware set it up.
+	 */
+	if (global_ctrl & CXL_HDM_DECODER_ENABLE) {
+		rc = devm_cxl_enable_mem(&port->dev, cxlds);
+		if (rc)
+			return false;
+		return true;
+	}
+
+	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 false;
+	}
+
+	for (i = 0, allowed = 0; info->mem_enabled && i < info->ranges; i++) {
+		struct device *cxld_dev;
+
+		cxld_dev = device_find_child(&root->dev, &info->dvsec_range[i],
+					     dvsec_range_allowed);
+		dev_dbg(dev, "DVSEC Range%d %sallowed by platform\n", i,
+			cxld_dev ? "" : "dis");
+		if (!cxld_dev)
+			continue;
+		put_device(cxld_dev);
+		allowed++;
+	}
+	put_device(&root->dev);
+
+	if (!allowed) {
+		cxl_set_mem_enable(cxlds, 0);
+		info->mem_enabled = 0;
+	}
 
 	/*
-	 * Permanently (for this boot at least) opt the device into HDM
-	 * operation. Individual HDM decoders still need to be enabled after
-	 * this point.
+	 * At least one DVSEC range is enabled and allowed, skip HDM
+	 * Decoder Capability Enable
 	 */
-	if (!global_enable) {
-		dev_dbg(cxlds->dev, "Enabling HDM decode\n");
-		writel(global_ctrl | CXL_HDM_DECODER_ENABLE,
-		       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
-	}
+	if (info->mem_enabled)
+		return false;
+
+	rc = devm_cxl_enable_hdm(&port->dev, cxlhdm);
+	if (rc)
+		return false;
+
+	rc = devm_cxl_enable_mem(&port->dev, cxlds);
+	if (rc)
+		return false;
 
 	return true;
 }
@@ -253,9 +387,14 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm)
 		return rc;
 	}
 
+	/*
+	 * The current DVSEC values are moot if the memory capability is
+	 * disabled, and they will remain moot after the HDM Decoder
+	 * capability is enabled.
+	 */
 	info.mem_enabled = FIELD_GET(CXL_DVSEC_MEM_ENABLE, ctrl);
 	if (!info.mem_enabled)
-		return 0;
+		return __cxl_hdm_decode_init(cxlds, cxlhdm, &info);
 
 	for (i = 0; i < hdm_count; i++) {
 		u64 base, size;

RE: [PATCH v2 14/14] cxl/port: Enable HDM Capability after validating DVSEC Ranges

[Ariel.Sibley]

> Previously, the cxl_mem driver was relying on platform-firmware to set
> "mem_enable". That is an invalid assumption as there is no requirement
> that platform-firmware sets the bit before the driver sees a device,
> especially in hot-plug scenarios. Additionally, ACPI-platforms that
> support CXL 2.0 devices also support the ACPI CEDT (CXL Early Discovery
> Table). That table outlines the platform permissible address ranges for
> CXL operation. So, there is a need for the driver to set "mem_enable",
> and there is information available to determine the validity of the CXL
> DVSEC Ranges. While DVSEC Ranges are expected to be at least
> 256M in size, the specification (CXL 2.0 Section 8.1.3.8.4 DVSEC CXL
> Range 1 Base Low) allows for the possibilty of devices smaller than
> 256M. So the range [0, 256M) is considered active even if Memory_size
> is 0.

Regarding "So the range [0, 256M) is considered active even if
Memory_size is 0."

Since Memory_Base is included in address A, this portion of the equation
from CXL 2.0 Section 8.1.3.8.4 mandates that for host access to address A
to be directed to local HDM memory, Memory_Size[63:28] must be > 0:

(A >> 28) < Memory_Base[63:28] + Memory_Size[63:28]

This means if a device advertises Memory_Size = 0, no host access will
result in access to the HDM memory.

I would also note this text from CXL 2.0 Section 8.1.3.8:
"A CXL.mem capable device is permitted to report zero memory size."

For a device with a non-zero capacity less than 256M to satisfy the
equation, it would need to advertise a Memory_Size of at least 256M.

Regards,
Ariel

Re: [PATCH v2 14/14] cxl/port: Enable HDM Capability after validating DVSEC Ranges

[Dan Williams]

On Tue, May 17, 2022 at 7:08 PM <Ariel.Sibley@microchip.com> wrote:
>
> > Previously, the cxl_mem driver was relying on platform-firmware to set
> > "mem_enable". That is an invalid assumption as there is no requirement
> > that platform-firmware sets the bit before the driver sees a device,
> > especially in hot-plug scenarios. Additionally, ACPI-platforms that
> > support CXL 2.0 devices also support the ACPI CEDT (CXL Early Discovery
> > Table). That table outlines the platform permissible address ranges for
> > CXL operation. So, there is a need for the driver to set "mem_enable",
> > and there is information available to determine the validity of the CXL
> > DVSEC Ranges. While DVSEC Ranges are expected to be at least
> > 256M in size, the specification (CXL 2.0 Section 8.1.3.8.4 DVSEC CXL
> > Range 1 Base Low) allows for the possibilty of devices smaller than
> > 256M. So the range [0, 256M) is considered active even if Memory_size
> > is 0.
>
> Regarding "So the range [0, 256M) is considered active even if
> Memory_size is 0."
>
> Since Memory_Base is included in address A, this portion of the equation
> from CXL 2.0 Section 8.1.3.8.4 mandates that for host access to address A
> to be directed to local HDM memory, Memory_Size[63:28] must be > 0:
>
> (A >> 28) < Memory_Base[63:28] + Memory_Size[63:28]
>
> This means if a device advertises Memory_Size = 0, no host access will
> result in access to the HDM memory.
>
> I would also note this text from CXL 2.0 Section 8.1.3.8:
> "A CXL.mem capable device is permitted to report zero memory size."
>
> For a device with a non-zero capacity less than 256M to satisfy the
> equation, it would need to advertise a Memory_Size of at least 256M.

I think we need an errata to delete the "(e.g. a device with less than
256 MB of memory)" mention. I otherwise do not see how such a device
can exist if Memory_size must be >= 256M.

Re: [PATCH v2 14/14] cxl/port: Enable HDM Capability after validating DVSEC Ranges

[Jonathan Cameron]

On Tue, 17 May 2022 19:44:28 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Tue, May 17, 2022 at 7:08 PM <Ariel.Sibley@microchip.com> wrote:
> >  
> > > Previously, the cxl_mem driver was relying on platform-firmware to set
> > > "mem_enable". That is an invalid assumption as there is no requirement
> > > that platform-firmware sets the bit before the driver sees a device,
> > > especially in hot-plug scenarios. Additionally, ACPI-platforms that
> > > support CXL 2.0 devices also support the ACPI CEDT (CXL Early Discovery
> > > Table). That table outlines the platform permissible address ranges for
> > > CXL operation. So, there is a need for the driver to set "mem_enable",
> > > and there is information available to determine the validity of the CXL
> > > DVSEC Ranges. While DVSEC Ranges are expected to be at least
> > > 256M in size, the specification (CXL 2.0 Section 8.1.3.8.4 DVSEC CXL
> > > Range 1 Base Low) allows for the possibilty of devices smaller than
> > > 256M. So the range [0, 256M) is considered active even if Memory_size
> > > is 0.  
> >
> > Regarding "So the range [0, 256M) is considered active even if
> > Memory_size is 0."
> >
> > Since Memory_Base is included in address A, this portion of the equation
> > from CXL 2.0 Section 8.1.3.8.4 mandates that for host access to address A
> > to be directed to local HDM memory, Memory_Size[63:28] must be > 0:
> >
> > (A >> 28) < Memory_Base[63:28] + Memory_Size[63:28]
> >
> > This means if a device advertises Memory_Size = 0, no host access will
> > result in access to the HDM memory.
> >
> > I would also note this text from CXL 2.0 Section 8.1.3.8:
> > "A CXL.mem capable device is permitted to report zero memory size."
> >
> > For a device with a non-zero capacity less than 256M to satisfy the
> > equation, it would need to advertise a Memory_Size of at least 256M.  
> 
> I think we need an errata to delete the "(e.g. a device with less than
> 256 MB of memory)" mention. I otherwise do not see how such a device
> can exist if Memory_size must be >= 256M.

My reading of that is it is permissible to implement a device that
has say 16MiB or actual memory, report it as 256MiB and follow this
behavior for the 16-256 MiB range.  It also covers a 300MiB device
where the size of the HDM decoder is set to 512MiB etc.

As such I don't think it's wrong, but rather just not relevant to us
here (0 is a valid setting for Memory_Size).
Would need impdef means to establish the actual size of the
memory to do anything useful with that corner case.

Jonathan

Re: [PATCH v2 14/14] cxl/port: Enable HDM Capability after validating DVSEC Ranges

[Jonathan Cameron]

On Tue, 17 May 2022 17:38:10 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> CXL memory expanders that support the CXL 2.0 memory device class code
> include an "HDM Decoder Capability" mechanism to supplant the "CXL DVSEC
> Range" mechanism originally defined in CXL 1.1. Both mechanisms depend
> on a "mem_enable" bit being set in configuration space before either
> mechanism activates. When the HDM Decoder Capability is enabled the CXL
> DVSEC Range settings are ignored.
> 
> Previously, the cxl_mem driver was relying on platform-firmware to set
> "mem_enable". That is an invalid assumption as there is no requirement
> that platform-firmware sets the bit before the driver sees a device,
> especially in hot-plug scenarios. Additionally, ACPI-platforms that
> support CXL 2.0 devices also support the ACPI CEDT (CXL Early Discovery
> Table). That table outlines the platform permissible address ranges for
> CXL operation. So, there is a need for the driver to set "mem_enable",
> and there is information available to determine the validity of the CXL
> DVSEC Ranges. While DVSEC Ranges are expected to be at least
> 256M in size, the specification (CXL 2.0 Section 8.1.3.8.4 DVSEC CXL
> Range 1 Base Low) allows for the possibilty of devices smaller than
> 256M. So the range [0, 256M) is considered active even if Memory_size
> is 0.
> 
> Arrange for the driver to optionally enable the HDM Decoder Capability
> if "mem_enable" was not set by platform firmware, or the CXL DVSEC Range
> configuration was invalid. Be careful to only disable memory decode if
> the kernel was the one to enable it. In other words, if CXL is backing
> all of kernel memory at boot the device needs to maintain "mem_enable"
> and "HDM Decoder enable" all the way up to handoff back to platform
> firmware (e.g. ACPI S5 state entry may require CXL memory to stay
> active).
> 
> Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> [dan: fix early terminiation of range-allowed loop]
> Cc: Ariel Sibley <ariel.sibley@microchip.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> Changes since v1:
> - Fix range-allowed loop termination (Smatch / Dan)
That had me confused before I saw v2 :)

I'm not keen on the trick to do disallowed in the debug message...

Other than ongoing discussion around the range being always allowed
(or not) this looks good to me.

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

> - Clean up changeloe wording around why [0, 256M) is considered always
>   active (Ariel)
> 
>  drivers/cxl/core/pci.c |  163 ++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 151 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index a697c48fc830..528430da0e77 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -175,30 +175,164 @@ static int wait_for_valid(struct cxl_dev_state *cxlds)
>  	return -ETIMEDOUT;
>  }
>  
> +static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val)
> +{
> +	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> +	int d = cxlds->cxl_dvsec;
> +	u16 ctrl;
> +	int rc;
> +
> +	rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
> +	if (rc < 0)
> +		return rc;
> +
> +	if ((ctrl & CXL_DVSEC_MEM_ENABLE) == val)
> +		return 1;
> +	ctrl &= ~CXL_DVSEC_MEM_ENABLE;
> +	ctrl |= val;
> +
> +	rc = pci_write_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, ctrl);
> +	if (rc < 0)
> +		return rc;
> +
> +	return 0;
> +}
> +
> +static void clear_mem_enable(void *cxlds)
> +{
> +	cxl_set_mem_enable(cxlds, 0);
> +}
> +
> +static int devm_cxl_enable_mem(struct device *host, struct cxl_dev_state *cxlds)
> +{
> +	int rc;
> +
> +	rc = cxl_set_mem_enable(cxlds, CXL_DVSEC_MEM_ENABLE);
> +	if (rc < 0)
> +		return rc;
> +	if (rc > 0)
> +		return 0;
> +	return devm_add_action_or_reset(host, clear_mem_enable, cxlds);
> +}
> +
> +static bool range_contains(struct range *r1, struct range *r2)
> +{
> +	return r1->start <= r2->start && r1->end >= r2->end;
> +}
> +
> +/* require dvsec ranges to be covered by a locked platform window */
> +static int dvsec_range_allowed(struct device *dev, void *arg)
> +{
> +	struct range *dev_range = arg;
> +	struct cxl_decoder *cxld;
> +	struct range root_range;
> +
> +	if (!is_root_decoder(dev))
> +		return 0;
> +
> +	cxld = to_cxl_decoder(dev);
> +
> +	if (!(cxld->flags & CXL_DECODER_F_LOCK))
> +		return 0;
> +	if (!(cxld->flags & CXL_DECODER_F_RAM))
> +		return 0;
> +
> +	root_range = (struct range) {
> +		.start = cxld->platform_res.start,
> +		.end = cxld->platform_res.end,
> +	};
> +
> +	return range_contains(&root_range, dev_range);
> +}
> +
> +static void disable_hdm(void *_cxlhdm)
> +{
> +	u32 global_ctrl;
> +	struct cxl_hdm *cxlhdm = _cxlhdm;
> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> +
> +	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> +	writel(global_ctrl & ~CXL_HDM_DECODER_ENABLE,
> +	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> +}
> +
> +static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm)
> +{
> +	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> +	u32 global_ctrl;
> +
> +	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> +	writel(global_ctrl | CXL_HDM_DECODER_ENABLE,
> +	       hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> +
> +	return devm_add_action_or_reset(host, disable_hdm, cxlhdm);
> +}
> +
>  static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
>  				  struct cxl_hdm *cxlhdm,
>  				  struct cxl_endpoint_dvsec_info *info)
>  {
>  	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> -	bool global_enable;
> +	struct cxl_port *port = cxlhdm->port;
> +	struct device *dev = cxlds->dev;
> +	struct cxl_port *root;
> +	int i, rc, allowed;
>  	u32 global_ctrl;
>  
>  	global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> -	global_enable = global_ctrl & CXL_HDM_DECODER_ENABLE;
>  
> -	if (!global_enable && info->mem_enabled)
> +	/*
> +	 * If the HDM Decoder Capability is already enabled then assume
> +	 * that some other agent like platform firmware set it up.
> +	 */
> +	if (global_ctrl & CXL_HDM_DECODER_ENABLE) {
> +		rc = devm_cxl_enable_mem(&port->dev, cxlds);
> +		if (rc)
> +			return false;
> +		return true;
> +	}
> +
> +	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 false;
> +	}
> +
> +	for (i = 0, allowed = 0; info->mem_enabled && i < info->ranges; i++) {
> +		struct device *cxld_dev;
> +
> +		cxld_dev = device_find_child(&root->dev, &info->dvsec_range[i],
> +					     dvsec_range_allowed);
> +		dev_dbg(dev, "DVSEC Range%d %sallowed by platform\n", i,
> +			cxld_dev ? "" : "dis");

Ouch.  Not worth doing that to save a few chars. Makes the message
harder to grep for.

> +		if (!cxld_dev)
> +			continue;
> +		put_device(cxld_dev);
> +		allowed++;
> +	}
> +	put_device(&root->dev);
> +
> +	if (!allowed) {
> +		cxl_set_mem_enable(cxlds, 0);
> +		info->mem_enabled = 0;
> +	}

Re: [PATCH v2 14/14] cxl/port: Enable HDM Capability after validating DVSEC Ranges

[Dan Williams]

On Wed, May 18, 2022 at 10:55 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 17 May 2022 17:38:10 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > CXL memory expanders that support the CXL 2.0 memory device class code
> > include an "HDM Decoder Capability" mechanism to supplant the "CXL DVSEC
> > Range" mechanism originally defined in CXL 1.1. Both mechanisms depend
> > on a "mem_enable" bit being set in configuration space before either
> > mechanism activates. When the HDM Decoder Capability is enabled the CXL
> > DVSEC Range settings are ignored.
> >
> > Previously, the cxl_mem driver was relying on platform-firmware to set
> > "mem_enable". That is an invalid assumption as there is no requirement
> > that platform-firmware sets the bit before the driver sees a device,
> > especially in hot-plug scenarios. Additionally, ACPI-platforms that
> > support CXL 2.0 devices also support the ACPI CEDT (CXL Early Discovery
> > Table). That table outlines the platform permissible address ranges for
> > CXL operation. So, there is a need for the driver to set "mem_enable",
> > and there is information available to determine the validity of the CXL
> > DVSEC Ranges. While DVSEC Ranges are expected to be at least
> > 256M in size, the specification (CXL 2.0 Section 8.1.3.8.4 DVSEC CXL
> > Range 1 Base Low) allows for the possibilty of devices smaller than
> > 256M. So the range [0, 256M) is considered active even if Memory_size
> > is 0.
> >
> > Arrange for the driver to optionally enable the HDM Decoder Capability
> > if "mem_enable" was not set by platform firmware, or the CXL DVSEC Range
> > configuration was invalid. Be careful to only disable memory decode if
> > the kernel was the one to enable it. In other words, if CXL is backing
> > all of kernel memory at boot the device needs to maintain "mem_enable"
> > and "HDM Decoder enable" all the way up to handoff back to platform
> > firmware (e.g. ACPI S5 state entry may require CXL memory to stay
> > active).
> >
> > Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > [dan: fix early terminiation of range-allowed loop]
> > Cc: Ariel Sibley <ariel.sibley@microchip.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> > Changes since v1:
> > - Fix range-allowed loop termination (Smatch / Dan)
> That had me confused before I saw v2 :)
>
> I'm not keen on the trick to do disallowed in the debug message...
>
> Other than ongoing discussion around the range being always allowed
> (or not) this looks good to me.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> > - Clean up changeloe wording around why [0, 256M) is considered always
> >   active (Ariel)
> >
> >  drivers/cxl/core/pci.c |  163 ++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 151 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index a697c48fc830..528430da0e77 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -175,30 +175,164 @@ static int wait_for_valid(struct cxl_dev_state *cxlds)
> >       return -ETIMEDOUT;
> >  }
> >
> > +static int cxl_set_mem_enable(struct cxl_dev_state *cxlds, u16 val)
> > +{
> > +     struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> > +     int d = cxlds->cxl_dvsec;
> > +     u16 ctrl;
> > +     int rc;
> > +
> > +     rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl);
> > +     if (rc < 0)
> > +             return rc;
> > +
> > +     if ((ctrl & CXL_DVSEC_MEM_ENABLE) == val)
> > +             return 1;
> > +     ctrl &= ~CXL_DVSEC_MEM_ENABLE;
> > +     ctrl |= val;
> > +
> > +     rc = pci_write_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, ctrl);
> > +     if (rc < 0)
> > +             return rc;
> > +
> > +     return 0;
> > +}
> > +
> > +static void clear_mem_enable(void *cxlds)
> > +{
> > +     cxl_set_mem_enable(cxlds, 0);
> > +}
> > +
> > +static int devm_cxl_enable_mem(struct device *host, struct cxl_dev_state *cxlds)
> > +{
> > +     int rc;
> > +
> > +     rc = cxl_set_mem_enable(cxlds, CXL_DVSEC_MEM_ENABLE);
> > +     if (rc < 0)
> > +             return rc;
> > +     if (rc > 0)
> > +             return 0;
> > +     return devm_add_action_or_reset(host, clear_mem_enable, cxlds);
> > +}
> > +
> > +static bool range_contains(struct range *r1, struct range *r2)
> > +{
> > +     return r1->start <= r2->start && r1->end >= r2->end;
> > +}
> > +
> > +/* require dvsec ranges to be covered by a locked platform window */
> > +static int dvsec_range_allowed(struct device *dev, void *arg)
> > +{
> > +     struct range *dev_range = arg;
> > +     struct cxl_decoder *cxld;
> > +     struct range root_range;
> > +
> > +     if (!is_root_decoder(dev))
> > +             return 0;
> > +
> > +     cxld = to_cxl_decoder(dev);
> > +
> > +     if (!(cxld->flags & CXL_DECODER_F_LOCK))
> > +             return 0;
> > +     if (!(cxld->flags & CXL_DECODER_F_RAM))
> > +             return 0;
> > +
> > +     root_range = (struct range) {
> > +             .start = cxld->platform_res.start,
> > +             .end = cxld->platform_res.end,
> > +     };
> > +
> > +     return range_contains(&root_range, dev_range);
> > +}
> > +
> > +static void disable_hdm(void *_cxlhdm)
> > +{
> > +     u32 global_ctrl;
> > +     struct cxl_hdm *cxlhdm = _cxlhdm;
> > +     void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> > +
> > +     global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> > +     writel(global_ctrl & ~CXL_HDM_DECODER_ENABLE,
> > +            hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> > +}
> > +
> > +static int devm_cxl_enable_hdm(struct device *host, struct cxl_hdm *cxlhdm)
> > +{
> > +     void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> > +     u32 global_ctrl;
> > +
> > +     global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> > +     writel(global_ctrl | CXL_HDM_DECODER_ENABLE,
> > +            hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> > +
> > +     return devm_add_action_or_reset(host, disable_hdm, cxlhdm);
> > +}
> > +
> >  static bool __cxl_hdm_decode_init(struct cxl_dev_state *cxlds,
> >                                 struct cxl_hdm *cxlhdm,
> >                                 struct cxl_endpoint_dvsec_info *info)
> >  {
> >       void __iomem *hdm = cxlhdm->regs.hdm_decoder;
> > -     bool global_enable;
> > +     struct cxl_port *port = cxlhdm->port;
> > +     struct device *dev = cxlds->dev;
> > +     struct cxl_port *root;
> > +     int i, rc, allowed;
> >       u32 global_ctrl;
> >
> >       global_ctrl = readl(hdm + CXL_HDM_DECODER_CTRL_OFFSET);
> > -     global_enable = global_ctrl & CXL_HDM_DECODER_ENABLE;
> >
> > -     if (!global_enable && info->mem_enabled)
> > +     /*
> > +      * If the HDM Decoder Capability is already enabled then assume
> > +      * that some other agent like platform firmware set it up.
> > +      */
> > +     if (global_ctrl & CXL_HDM_DECODER_ENABLE) {
> > +             rc = devm_cxl_enable_mem(&port->dev, cxlds);
> > +             if (rc)
> > +                     return false;
> > +             return true;
> > +     }
> > +
> > +     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 false;
> > +     }
> > +
> > +     for (i = 0, allowed = 0; info->mem_enabled && i < info->ranges; i++) {
> > +             struct device *cxld_dev;
> > +
> > +             cxld_dev = device_find_child(&root->dev, &info->dvsec_range[i],
> > +                                          dvsec_range_allowed);
> > +             dev_dbg(dev, "DVSEC Range%d %sallowed by platform\n", i,
> > +                     cxld_dev ? "" : "dis");
>
> Ouch.  Not worth doing that to save a few chars. Makes the message
> harder to grep for.

Ok, will drop, along with the "always enabled" change.

Re: [PATCH 00/14] cxl: Fix "mem_enable" handling

[Ira Weiny]

On Thu, May 12, 2022 at 11:14:16AM -0700, Dan Williams wrote:
> Jonathan reports [1] that after he changed QEMU to stop setting
> Mem_Enable (8.1.3.2 DVSEC CXL Control (Bit 2)) by default the following
> problems arose:
> 
>     1. Nothing in the Linux code actually sets Mem_Enable to 1.
>     2. Probing fails in mem.c as wait_for_media() checks for
>        info->mem_enabled (cached value of this bit).
> 
> The investigation turned up more issues:
> 
> - DVSEC ranges are always non-zero size, so it is ambiguous, just
>   looking at the registers, as to whether platform firmware is trying to
>   route the first 256M of memory to CXL, or just failed to change the
>   registers from the default.
> 
> - No driver consideration for clearing "mem_enabled" and / or HDM
>   Decoder Enable.
> 
> - The cxl_test mock override for cxl_hdm_decode_init() was hiding bugs
>   in this path.
> 
> The end goal of these reworks are to improve detection for cases where
> platform firmware is actually operating in legacy CXL DVSEC Range mode,
> take ownership for setting and clearing "mem_enable" and HDM Decoder
> Enable, and cleanup the indirections / mocking for cxl_test.
> 
> The new flow is described in patch 14:
> 
>     Previously, the cxl_mem driver was relying on platform-firmware to set
>     "mem_enable". That is an invalid assumption as there is no requirement
>     that platform-firmware sets the bit before the driver sees a device,
>     especially in hot-plug scenarios. Additionally, ACPI-platforms that
>     support CXL 2.0 devices also support the ACPI CEDT (CXL Early Discovery
>     Table). That table outlines the platform permissible address ranges for
>     CXL operation. So, there is a need for the driver to set "mem_enable",
>     and there is information available to determine the validity of the CXL
>     DVSEC Ranges. Note that the DVSEC Ranges can not be shut off completely.
>     They always decode at least 256MB if "mem_enable" is set and the HDM
>     Decoder capability is disabled.
> 
>     Arrange for the driver to optionally enable the HDM Decoder Capability
>     if "mem_enable" was not set by platform firmware, or the CXL DVSEC Range
>     configuration was invalid. Be careful to only disable memory decode if
>     the kernel was the one to enable it. In other words, if CXL is backing
>     all of kernel memory at boot the device needs to maintain "mem_enable"
>     and "HDM Decoder enable" all the way up to handoff back to platform
>     firmware (e.g. ACPI S5 state entry may require CXL memory to stay
>     active).
> 
> Link: https://lore.kernel.org/r/20220426180832.00005f0b@Huawei.com [1]

For the series:

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> 
> ---
> 
> Dan Williams (14):
>       cxl/mem: Drop mem_enabled check from wait_for_media()
>       cxl/pci: Consolidate wait_for_media() and wait_for_media_ready()
>       cxl/pci: Drop wait_for_valid() from cxl_await_media_ready()
>       cxl/mem: Fix cxl_mem_probe() error exit
>       cxl/mem: Validate port connectivity before dvsec ranges
>       cxl/pci: Move cxl_await_media_ready() to the core
>       cxl/mem: Consolidate CXL DVSEC Range enumeration in the core
>       cxl/mem: Skip range enumeration if mem_enable clear
>       cxl/mem: Fix CXL DVSEC Range Sizing
>       cxl/mem: Merge cxl_dvsec_ranges() and cxl_hdm_decode_init()
>       cxl/pci: Drop @info argument to cxl_hdm_decode_init()
>       cxl/port: Move endpoint HDM Decoder Capability init to port driver
>       cxl/port: Reuse 'struct cxl_hdm' context for hdm init
>       cxl/port: Enable HDM Capability after validating DVSEC Ranges
> 
> 
>  drivers/cxl/core/pci.c        |  362 +++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/cxlmem.h          |    4 
>  drivers/cxl/cxlpci.h          |    2 
>  drivers/cxl/mem.c             |  115 -------------
>  drivers/cxl/pci.c             |  184 ---------------------
>  drivers/cxl/port.c            |   28 ++-
>  tools/testing/cxl/Kbuild      |    3 
>  tools/testing/cxl/mock_mem.c  |   10 -
>  tools/testing/cxl/test/mem.c  |   17 --
>  tools/testing/cxl/test/mock.c |   29 +++
>  10 files changed, 422 insertions(+), 332 deletions(-)
>  delete mode 100644 tools/testing/cxl/mock_mem.c
> 
> base-commit: e6829d1bd3c4b58296ee9e412f7ed4d6cb390192