[PATCH v2 0/6] cxl: Handle DVSEC range init failures

[PATCH v2 0/6] cxl: Handle DVSEC range init failures

[Dan Williams]

Changes since v1 [1]:
- Split the debug message additions to their own patch (Ben)
- Split the cxl_dvsec_range() handling in the cxl_mem driver to its own
  patch.
- Add a patch to rename cxl_dvsec_decode_init()
- Add a patch to clarify global HDM decoder control vs DVSEC range
  configuration.
- Pick up David's Reviewed-by, thanks David!

[1]: https://lore.kernel.org/r/164690155138.3326488.16049914482944930295.stgit@dwillia2-desk3.amr.corp.intel.com

---

Krzysztof reports a case where a timeout waiting for the "memory info
valid" indication in the DVSEC range register also results in the PCI
driver failing to load. Fix that scenario by making DVSEC range register
probe failures non-fatal to the cxl_pci driver.

Instead, convey the state of the DVSEC range registers to the cxl_mem
driver which can decide if it wants to proceed. Recall that the reason
cxl_mem relies on cxl_pci to do the register access is to keep PCI
configuration space knowledge in the cxl_pci driver and leave MMIO based
CXL.mem operations to the cxl_mem driver.

While pulling on the above threads a few more fixup and clarification
opportunities fell out.

---

Dan Williams (6):
      cxl/mem: Drop DVSEC vs EFI Memory Map sanity check
      cxl/pci: Add debug for DVSEC range init failures
      cxl/mem: Make cxl_dvsec_range() init failure fatal
      cxl/pci: Make cxl_dvsec_ranges() failure not fatal to cxl_pci
      cxl/mem: Rename cxl_dvsec_decode_init() to cxl_hdm_decode_init()
      cxl/mem: Replace redundant debug message with a comment


 drivers/cxl/mem.c            |   52 +++++++++++++++---------------------------
 drivers/cxl/pci.c            |   40 +++++++++++++++++++++++---------
 tools/testing/cxl/mock_mem.c |    2 +-
 3 files changed, 48 insertions(+), 46 deletions(-)

base-commit: 74be98774dfbc5b8b795db726bd772e735d2edd4

[PATCH v2 1/6] cxl/mem: Drop DVSEC vs EFI Memory Map sanity check

[Dan Williams]

When the driver finds legacy DVSEC ranges active on a CXL Memory
Expander it indicates that platform firmware is not aware of, or is
deliberately disabling common CXL 2.0 operation. In this case Linux
generally has no choice, but to leave the device alone.

The driver attempts to validate that the DVSEC range is in the EFI
memory map. Remove that logic since there is no requirement that the
BIOS publish DVSEC ranges in the EFI Memory Map.

In the future the driver will want to permanently reserve this capacity
out of the available CFMWS capacity and hide it from
request_free_mem_region(), but it serves no purpose to warn about the
range not appearing in the EFI Memory Map.

Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/mem.c |   24 +-----------------------
 1 file changed, 1 insertion(+), 23 deletions(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 49a4b1c47299..cd4e8bba82aa 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -158,30 +158,8 @@ static int cxl_mem_probe(struct device *dev)
 	 * is no use in trying to manage those.
 	 */
 	if (!cxl_dvsec_decode_init(cxlds)) {
-		struct cxl_endpoint_dvsec_info *info = &cxlds->info;
-		int i;
-
-		/* */
-		for (i = 0; i < 2; i++) {
-			u64 base, size;
-
-			/*
-			 * Give a nice warning to the user that BIOS has really
-			 * botched things for them if it didn't place DVSEC
-			 * ranges in the memory map.
-			 */
-			base = info->dvsec_range[i].start;
-			size = range_len(&info->dvsec_range[i]);
-			if (size && !region_intersects(base, size,
-						       IORESOURCE_SYSTEM_RAM,
-						       IORES_DESC_NONE)) {
-				dev_err(dev,
-					"DVSEC range %#llx-%#llx must be reserved by BIOS, but isn't\n",
-					base, base + size - 1);
-			}
-		}
 		dev_err(dev,
-			"Active DVSEC range registers in use. Will not bind.\n");
+			"Legacy range registers configuration prevents HDM operation.\n");
 		return -EBUSY;
 	}
 

[PATCH v2 2/6] cxl/pci: Add debug for DVSEC range init failures

[Dan Williams]

In preparation for not treating DVSEC range initialization failures as
fatal to cxl_pci_probe() add individual dev_dbg() statements for each of
the major failure reasons in cxl_dvsec_ranges().

The rationale for cxl_dvsec_ranges() failure not being fatal is that
there is still value for cxl_pci to enable mailbox operations even if
CXL.mem operation is disabled.

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

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 8a7267d116b7..257cf735505d 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -467,12 +467,15 @@ static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
 {
 	struct cxl_endpoint_dvsec_info *info = &cxlds->info;
 	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
+	struct device *dev = &pdev->dev;
 	int d = cxlds->cxl_dvsec;
 	int hdm_count, rc, i;
 	u16 cap, ctrl;
 
-	if (!d)
+	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)
@@ -482,8 +485,10 @@ static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
 	if (rc)
 		return rc;
 
-	if (!(cap & CXL_DVSEC_MEM_CAPABLE))
+	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
@@ -496,8 +501,10 @@ static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
 		return -EINVAL;
 
 	rc = wait_for_valid(cxlds);
-	if (rc)
+	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);
 

[PATCH v2 3/6] cxl/mem: Make cxl_dvsec_range() init failure fatal

[Dan Williams]

In preparation for the cxl_pci driver to continue operation after
cxl_dvsec_range() failure, update cxl_mem to check for negative error
codes in info->ranges. Treat that condition as fatal regardless of the
state of the HDM configuration since cxl_mem needs positive confirmation
that legacy ranges were not established by platform firmware or another
agent.

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

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index cd4e8bba82aa..50704deb2ff0 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -88,6 +88,9 @@ __mock bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
 	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) {

[PATCH v2 4/6] cxl/pci: Make cxl_dvsec_ranges() failure not fatal to cxl_pci

[Dan Williams]

cxl_dvsec_ranges(), the helper for enumerating the presence of an active
legacy CXL.mem configuration on a CXL 2.0 Memory Expander, is not fatal
for cxl_pci because there is still value to enable mailbox operations
even if CXL.mem operation is disabled. Recall that the reason cxl_pci
does this initialization and not cxl_mem is to preserve the useful
property (for unit testing) that cxl_mem is cxl_memdev + mmio generic,
and does not require access to a 'struct pci_dev' to issue config
cycles.

Update 'struct cxl_endpoint_dvsec_info' to carry either a positive
number of non-zero size legacy CXL DVSEC ranges, or the negative error
code from __cxl_dvsec_ranges() in its @ranges member.

Reported-by: Krzysztof Zach <krzysztof.zach@intel.com>
Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/pci.c |   27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 257cf735505d..994c79bf6afd 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -463,13 +463,18 @@ static int wait_for_media_ready(struct cxl_dev_state *cxlds)
 	return 0;
 }
 
-static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
+/*
+ * 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 cxl_endpoint_dvsec_info *info = &cxlds->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;
-	int hdm_count, rc, i;
 	u16 cap, ctrl;
 
 	if (!d) {
@@ -546,10 +551,17 @@ static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
 		};
 
 		if (size)
-			info->ranges++;
+			ranges++;
 	}
 
-	return 0;
+	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)
@@ -618,10 +630,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
-	rc = cxl_dvsec_ranges(cxlds);
-	if (rc)
-		dev_warn(&pdev->dev,
-			 "Failed to get DVSEC range information (%d)\n", rc);
+	cxl_dvsec_ranges(cxlds);
 
 	cxlmd = devm_cxl_add_memdev(cxlds);
 	if (IS_ERR(cxlmd))

[PATCH v2 5/6] cxl/mem: Rename cxl_dvsec_decode_init() to cxl_hdm_decode_init()

[Dan Williams]

cxl_dvsec_decode_init() is tasked with checking whether legacy DVSEC
range based decode is in effect, or whether HDM can be enabled / already
is enabled. As such it either succeeds or fails and that result is the
return value. The @do_hdm_init variable is misleading in the case where
HDM operation is already found to be active, so just call it @retval.

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

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 50704deb2ff0..3baae1332760 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -68,7 +68,7 @@ static int create_endpoint(struct cxl_memdev *cxlmd,
 }
 
 /**
- * cxl_dvsec_decode_init() - Setup HDM decoding for the 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
@@ -79,12 +79,12 @@ 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_dvsec_decode_init(struct cxl_dev_state *cxlds)
+__mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
 {
 	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, do_hdm_init = false;
+	bool global_enable, retval = false;
 	void __iomem *crb;
 	u32 global_ctrl;
 
@@ -113,7 +113,7 @@ __mock bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
 		goto out;
 	}
 
-	do_hdm_init = true;
+	retval = true;
 
 	/*
 	 * Permanently (for this boot at least) opt the device into HDM
@@ -129,7 +129,7 @@ __mock bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
 
 out:
 	iounmap(crb);
-	return do_hdm_init;
+	return retval;
 }
 
 static int cxl_mem_probe(struct device *dev)
@@ -160,7 +160,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_dvsec_decode_init(cxlds)) {
+	if (!cxl_hdm_decode_init(cxlds)) {
 		dev_err(dev,
 			"Legacy range registers configuration prevents HDM operation.\n");
 		return -EBUSY;
diff --git a/tools/testing/cxl/mock_mem.c b/tools/testing/cxl/mock_mem.c
index d1dec5845139..69946f678cfa 100644
--- a/tools/testing/cxl/mock_mem.c
+++ b/tools/testing/cxl/mock_mem.c
@@ -4,7 +4,7 @@
 #include <linux/types.h>
 
 struct cxl_dev_state;
-bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
+bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
 {
 	return true;
 }

[PATCH v2 6/6] cxl/mem: Replace redundant debug message with a comment

[Dan Williams]

cxl_mem_probe() already emits a log message when HDM operation can not
be established. Delete the similar one in cxl_hdm_decode_init().

What is less obvious is why global_ctrl being enabled makes positive
values of info->ranges irrelevant.

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

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 3baae1332760..6def7d7b6bfd 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -107,11 +107,16 @@ __mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
 	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->ranges) {
-		dev_dbg(cxlds->dev,
-			"DVSEC ranges already programmed and HDM decoders not enabled.\n");
+
+	/*
+	 * 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.
+	 */
+	if (!global_enable && info->ranges)
 		goto out;
-	}
 
 	retval = true;
 

Re: [PATCH v2 3/6] cxl/mem: Make cxl_dvsec_range() init failure fatal

[Davidlohr Bueso]

On Mon, 14 Mar 2022, Dan Williams wrote:

>In preparation for the cxl_pci driver to continue operation after
>cxl_dvsec_range() failure, update cxl_mem to check for negative error
>codes in info->ranges. Treat that condition as fatal regardless of the
>state of the HDM configuration since cxl_mem needs positive confirmation
>that legacy ranges were not established by platform firmware or another
>agent.
>
>Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>---
> drivers/cxl/mem.c |    3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
>index cd4e8bba82aa..50704deb2ff0 100644
>--- a/drivers/cxl/mem.c
>+++ b/drivers/cxl/mem.c
>@@ -88,6 +88,9 @@ __mock bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
>	void __iomem *crb;
>	u32 global_ctrl;
>
>+	if (info->ranges < 0)
>+		return false;
>+

Nit: shouldn't this be part of the next patch that actually implements negative
info->ranges?

Thanks,
Davidlohr

Re: [PATCH v2 3/6] cxl/mem: Make cxl_dvsec_range() init failure fatal

[Dan Williams]

On Tue, Mar 15, 2022 at 7:01 PM Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> On Mon, 14 Mar 2022, Dan Williams wrote:
>
> >In preparation for the cxl_pci driver to continue operation after
> >cxl_dvsec_range() failure, update cxl_mem to check for negative error
> >codes in info->ranges. Treat that condition as fatal regardless of the
> >state of the HDM configuration since cxl_mem needs positive confirmation
> >that legacy ranges were not established by platform firmware or another
> >agent.
> >
> >Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >---
> > drivers/cxl/mem.c |    3 +++
> > 1 file changed, 3 insertions(+)
> >
> >diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> >index cd4e8bba82aa..50704deb2ff0 100644
> >--- a/drivers/cxl/mem.c
> >+++ b/drivers/cxl/mem.c
> >@@ -88,6 +88,9 @@ __mock bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
> >       void __iomem *crb;
> >       u32 global_ctrl;
> >
> >+      if (info->ranges < 0)
> >+              return false;
> >+
>
> Nit: shouldn't this be part of the next patch that actually implements negative
> info->ranges?

I considered that. Ben had asked for a split and this patch is able to
stand alone since info->ranges is always >= 0 until the next patch.

Re: [PATCH v2 0/6] cxl: Handle DVSEC range init failures

[Davidlohr Bueso]

On Mon, 14 Mar 2022, Dan Williams wrote:

>Dan Williams (6):
>      cxl/mem: Drop DVSEC vs EFI Memory Map sanity check
>      cxl/pci: Add debug for DVSEC range init failures
>      cxl/mem: Make cxl_dvsec_range() init failure fatal
>      cxl/pci: Make cxl_dvsec_ranges() failure not fatal to cxl_pci
>      cxl/mem: Rename cxl_dvsec_decode_init() to cxl_hdm_decode_init()
>      cxl/mem: Replace redundant debug message with a comment

I have gone through the remaining patches and they look good to me.
Feel free to add my:

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

>
>
> drivers/cxl/mem.c            |   52 +++++++++++++++---------------------------
> drivers/cxl/pci.c            |   40 +++++++++++++++++++++++---------
> tools/testing/cxl/mock_mem.c |    2 +-
> 3 files changed, 48 insertions(+), 46 deletions(-)
>
>base-commit: 74be98774dfbc5b8b795db726bd772e735d2edd4

Re: [PATCH v2 1/6] cxl/mem: Drop DVSEC vs EFI Memory Map sanity check

[Ben Widawsky]

On 22-03-14 18:22:22, Dan Williams wrote:
> When the driver finds legacy DVSEC ranges active on a CXL Memory
> Expander it indicates that platform firmware is not aware of, or is
> deliberately disabling common CXL 2.0 operation. In this case Linux
> generally has no choice, but to leave the device alone.
> 
> The driver attempts to validate that the DVSEC range is in the EFI
> memory map. Remove that logic since there is no requirement that the
> BIOS publish DVSEC ranges in the EFI Memory Map.
> 
> In the future the driver will want to permanently reserve this capacity
> out of the available CFMWS capacity and hide it from
> request_free_mem_region(), but it serves no purpose to warn about the
> range not appearing in the EFI Memory Map.
> 
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Ben Widawsky <ben.widawsky@intel.com>

Re: [PATCH v2 2/6] cxl/pci: Add debug for DVSEC range init failures

[Ben Widawsky]

On 22-03-14 18:22:28, Dan Williams wrote:
> In preparation for not treating DVSEC range initialization failures as
> fatal to cxl_pci_probe() add individual dev_dbg() statements for each of
> the major failure reasons in cxl_dvsec_ranges().
> 
> The rationale for cxl_dvsec_ranges() failure not being fatal is that
> there is still value for cxl_pci to enable mailbox operations even if
> CXL.mem operation is disabled.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/pci.c |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 8a7267d116b7..257cf735505d 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -467,12 +467,15 @@ static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
>  {
>  	struct cxl_endpoint_dvsec_info *info = &cxlds->info;
>  	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> +	struct device *dev = &pdev->dev;
>  	int d = cxlds->cxl_dvsec;
>  	int hdm_count, rc, i;
>  	u16 cap, ctrl;
>  
> -	if (!d)
> +	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)
> @@ -482,8 +485,10 @@ static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
>  	if (rc)
>  		return rc;
>  
> -	if (!(cap & CXL_DVSEC_MEM_CAPABLE))
> +	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
> @@ -496,8 +501,10 @@ static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
>  		return -EINVAL;
>  
>  	rc = wait_for_valid(cxlds);
> -	if (rc)
> +	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);
>  
> 

It kind of stinks now that this function has some pdev->dev usages and plain dev
usages now. Either way, it's correct and an improvement.

Reviewed-by: Ben Widawsky <ben.widawsky@intel.com>

Re: [PATCH v2 3/6] cxl/mem: Make cxl_dvsec_range() init failure fatal

[Ben Widawsky]

On 22-03-14 18:22:33, Dan Williams wrote:
> In preparation for the cxl_pci driver to continue operation after
> cxl_dvsec_range() failure, update cxl_mem to check for negative error
> codes in info->ranges. Treat that condition as fatal regardless of the
> state of the HDM configuration since cxl_mem needs positive confirmation
> that legacy ranges were not established by platform firmware or another
> agent.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Ben Widawsky <ben.widawsky@intel.com>

Re: [PATCH v2 4/6] cxl/pci: Make cxl_dvsec_ranges() failure not fatal to cxl_pci

[Ben Widawsky]

On 22-03-14 18:22:38, Dan Williams wrote:
> cxl_dvsec_ranges(), the helper for enumerating the presence of an active
> legacy CXL.mem configuration on a CXL 2.0 Memory Expander, is not fatal
> for cxl_pci because there is still value to enable mailbox operations
> even if CXL.mem operation is disabled. Recall that the reason cxl_pci
> does this initialization and not cxl_mem is to preserve the useful
> property (for unit testing) that cxl_mem is cxl_memdev + mmio generic,
> and does not require access to a 'struct pci_dev' to issue config
> cycles.
> 
> Update 'struct cxl_endpoint_dvsec_info' to carry either a positive
> number of non-zero size legacy CXL DVSEC ranges, or the negative error
> code from __cxl_dvsec_ranges() in its @ranges member.
> 
> Reported-by: Krzysztof Zach <krzysztof.zach@intel.com>
> Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/pci.c |   27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 257cf735505d..994c79bf6afd 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -463,13 +463,18 @@ static int wait_for_media_ready(struct cxl_dev_state *cxlds)
>  	return 0;
>  }
>  
> -static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
> +/*
> + * 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.
> + */

It shouldn't depend on 0 ranges, it depends on ranges matching existing HDM
decoder ranges. I took a shortcut by just checking global enable originally
because we didn't yet have code to enumerate decoders (anything else would be a
crazy BIOS bug that's probably not worth working around).

> +static int __cxl_dvsec_ranges(struct cxl_dev_state *cxlds,
> +			      struct cxl_endpoint_dvsec_info *info)
>  {
> -	struct cxl_endpoint_dvsec_info *info = &cxlds->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;
> -	int hdm_count, rc, i;
>  	u16 cap, ctrl;
>  
>  	if (!d) {
> @@ -546,10 +551,17 @@ static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
>  		};
>  
>  		if (size)
> -			info->ranges++;
> +			ranges++;
>  	}
>  
> -	return 0;
> +	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)
> @@ -618,10 +630,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> -	rc = cxl_dvsec_ranges(cxlds);
> -	if (rc)
> -		dev_warn(&pdev->dev,
> -			 "Failed to get DVSEC range information (%d)\n", rc);
> +	cxl_dvsec_ranges(cxlds);
>  
>  	cxlmd = devm_cxl_add_memdev(cxlds);
>  	if (IS_ERR(cxlmd))
> 

Re: [PATCH v2 5/6] cxl/mem: Rename cxl_dvsec_decode_init() to cxl_hdm_decode_init()

[Ben Widawsky]

On 22-03-14 18:22:44, Dan Williams wrote:
> cxl_dvsec_decode_init() is tasked with checking whether legacy DVSEC
> range based decode is in effect, or whether HDM can be enabled / already
> is enabled. As such it either succeeds or fails and that result is the
> return value. The @do_hdm_init variable is misleading in the case where
> HDM operation is already found to be active, so just call it @retval.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

If we're doing the rename, which I'm in favor of, maybe making it something else
instead? I think init is usually something which cannot fail. Perhaps
cxl_hdm_decode_probe()?

> ---
>  drivers/cxl/mem.c            |   12 ++++++------
>  tools/testing/cxl/mock_mem.c |    2 +-
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 50704deb2ff0..3baae1332760 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -68,7 +68,7 @@ static int create_endpoint(struct cxl_memdev *cxlmd,
>  }
>  
>  /**
> - * cxl_dvsec_decode_init() - Setup HDM decoding for the 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
> @@ -79,12 +79,12 @@ 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_dvsec_decode_init(struct cxl_dev_state *cxlds)
> +__mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
>  {
>  	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, do_hdm_init = false;
> +	bool global_enable, retval = false;
>  	void __iomem *crb;
>  	u32 global_ctrl;
>  
> @@ -113,7 +113,7 @@ __mock bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
>  		goto out;
>  	}
>  
> -	do_hdm_init = true;
> +	retval = true;
>  
>  	/*
>  	 * Permanently (for this boot at least) opt the device into HDM
> @@ -129,7 +129,7 @@ __mock bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
>  
>  out:
>  	iounmap(crb);
> -	return do_hdm_init;
> +	return retval;
>  }
>  
>  static int cxl_mem_probe(struct device *dev)
> @@ -160,7 +160,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_dvsec_decode_init(cxlds)) {
> +	if (!cxl_hdm_decode_init(cxlds)) {
>  		dev_err(dev,
>  			"Legacy range registers configuration prevents HDM operation.\n");
>  		return -EBUSY;
> diff --git a/tools/testing/cxl/mock_mem.c b/tools/testing/cxl/mock_mem.c
> index d1dec5845139..69946f678cfa 100644
> --- a/tools/testing/cxl/mock_mem.c
> +++ b/tools/testing/cxl/mock_mem.c
> @@ -4,7 +4,7 @@
>  #include <linux/types.h>
>  
>  struct cxl_dev_state;
> -bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
> +bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
>  {
>  	return true;
>  }
> 

Re: [PATCH v2 4/6] cxl/pci: Make cxl_dvsec_ranges() failure not fatal to cxl_pci

[Dan Williams]

On Thu, Mar 17, 2022 at 10:52 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 22-03-14 18:22:38, Dan Williams wrote:
> > cxl_dvsec_ranges(), the helper for enumerating the presence of an active
> > legacy CXL.mem configuration on a CXL 2.0 Memory Expander, is not fatal
> > for cxl_pci because there is still value to enable mailbox operations
> > even if CXL.mem operation is disabled. Recall that the reason cxl_pci
> > does this initialization and not cxl_mem is to preserve the useful
> > property (for unit testing) that cxl_mem is cxl_memdev + mmio generic,
> > and does not require access to a 'struct pci_dev' to issue config
> > cycles.
> >
> > Update 'struct cxl_endpoint_dvsec_info' to carry either a positive
> > number of non-zero size legacy CXL DVSEC ranges, or the negative error
> > code from __cxl_dvsec_ranges() in its @ranges member.
> >
> > Reported-by: Krzysztof Zach <krzysztof.zach@intel.com>
> > Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/cxl/pci.c |   27 ++++++++++++++++++---------
> >  1 file changed, 18 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index 257cf735505d..994c79bf6afd 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -463,13 +463,18 @@ static int wait_for_media_ready(struct cxl_dev_state *cxlds)
> >       return 0;
> >  }
> >
> > -static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
> > +/*
> > + * 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.
> > + */
>
> It shouldn't depend on 0 ranges, it depends on ranges matching existing HDM
> decoder ranges. I took a shortcut by just checking global enable originally
> because we didn't yet have code to enumerate decoders (anything else would be a
> crazy BIOS bug that's probably not worth working around).

Hmm, I don't see that as a shortcut. If global enable is set then
there is no requirement that CXL DVSEC matches the HDM decoder ranges,
if global enable is not set then the HDM decoder ranges are not in
effect.

This is what the new comment in cxl_hdm_decode_init() is trying to
clarify. My proposal is that Linux ignores the recommendation which I
think is trying to accommodate legacy CXL 1.1 software stacks which
Linux never had.

        /*
         * 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.
         */

Re: [PATCH v2 4/6] cxl/pci: Make cxl_dvsec_ranges() failure not fatal to cxl_pci

[Ben Widawsky]

On 22-03-17 11:20:48, Dan Williams wrote:
> On Thu, Mar 17, 2022 at 10:52 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > On 22-03-14 18:22:38, Dan Williams wrote:
> > > cxl_dvsec_ranges(), the helper for enumerating the presence of an active
> > > legacy CXL.mem configuration on a CXL 2.0 Memory Expander, is not fatal
> > > for cxl_pci because there is still value to enable mailbox operations
> > > even if CXL.mem operation is disabled. Recall that the reason cxl_pci
> > > does this initialization and not cxl_mem is to preserve the useful
> > > property (for unit testing) that cxl_mem is cxl_memdev + mmio generic,
> > > and does not require access to a 'struct pci_dev' to issue config
> > > cycles.
> > >
> > > Update 'struct cxl_endpoint_dvsec_info' to carry either a positive
> > > number of non-zero size legacy CXL DVSEC ranges, or the negative error
> > > code from __cxl_dvsec_ranges() in its @ranges member.
> > >
> > > Reported-by: Krzysztof Zach <krzysztof.zach@intel.com>
> > > Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > ---
> > >  drivers/cxl/pci.c |   27 ++++++++++++++++++---------
> > >  1 file changed, 18 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > > index 257cf735505d..994c79bf6afd 100644
> > > --- a/drivers/cxl/pci.c
> > > +++ b/drivers/cxl/pci.c
> > > @@ -463,13 +463,18 @@ static int wait_for_media_ready(struct cxl_dev_state *cxlds)
> > >       return 0;
> > >  }
> > >
> > > -static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
> > > +/*
> > > + * 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.
> > > + */
> >
> > It shouldn't depend on 0 ranges, it depends on ranges matching existing HDM
> > decoder ranges. I took a shortcut by just checking global enable originally
> > because we didn't yet have code to enumerate decoders (anything else would be a
> > crazy BIOS bug that's probably not worth working around).
> 
> Hmm, I don't see that as a shortcut. If global enable is set then
> there is no requirement that CXL DVSEC matches the HDM decoder ranges,
> if global enable is not set then the HDM decoder ranges are not in
> effect.

It's true that there is no requirement that the DVSEC matches HDM decoder
ranges, except as you point out before it's going against spec recommendation.

> 
> This is what the new comment in cxl_hdm_decode_init() is trying to
> clarify. My proposal is that Linux ignores the recommendation which I
> think is trying to accommodate legacy CXL 1.1 software stacks which
> Linux never had.
> 

Okay, that's fine.

>         /*
>          * 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.
>          */

Maybe explicitly say the driver does not attempt to check this recommendation.

Re: [PATCH v2 4/6] cxl/pci: Make cxl_dvsec_ranges() failure not fatal to cxl_pci

[Dan Williams]

On Thu, Mar 17, 2022 at 11:29 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 22-03-17 11:20:48, Dan Williams wrote:
> > On Thu, Mar 17, 2022 at 10:52 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > >
> > > On 22-03-14 18:22:38, Dan Williams wrote:
> > > > cxl_dvsec_ranges(), the helper for enumerating the presence of an active
> > > > legacy CXL.mem configuration on a CXL 2.0 Memory Expander, is not fatal
> > > > for cxl_pci because there is still value to enable mailbox operations
> > > > even if CXL.mem operation is disabled. Recall that the reason cxl_pci
> > > > does this initialization and not cxl_mem is to preserve the useful
> > > > property (for unit testing) that cxl_mem is cxl_memdev + mmio generic,
> > > > and does not require access to a 'struct pci_dev' to issue config
> > > > cycles.
> > > >
> > > > Update 'struct cxl_endpoint_dvsec_info' to carry either a positive
> > > > number of non-zero size legacy CXL DVSEC ranges, or the negative error
> > > > code from __cxl_dvsec_ranges() in its @ranges member.
> > > >
> > > > Reported-by: Krzysztof Zach <krzysztof.zach@intel.com>
> > > > Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
> > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > > ---
> > > >  drivers/cxl/pci.c |   27 ++++++++++++++++++---------
> > > >  1 file changed, 18 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > > > index 257cf735505d..994c79bf6afd 100644
> > > > --- a/drivers/cxl/pci.c
> > > > +++ b/drivers/cxl/pci.c
> > > > @@ -463,13 +463,18 @@ static int wait_for_media_ready(struct cxl_dev_state *cxlds)
> > > >       return 0;
> > > >  }
> > > >
> > > > -static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
> > > > +/*
> > > > + * 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.
> > > > + */
> > >
> > > It shouldn't depend on 0 ranges, it depends on ranges matching existing HDM
> > > decoder ranges. I took a shortcut by just checking global enable originally
> > > because we didn't yet have code to enumerate decoders (anything else would be a
> > > crazy BIOS bug that's probably not worth working around).
> >
> > Hmm, I don't see that as a shortcut. If global enable is set then
> > there is no requirement that CXL DVSEC matches the HDM decoder ranges,
> > if global enable is not set then the HDM decoder ranges are not in
> > effect.
>
> It's true that there is no requirement that the DVSEC matches HDM decoder
> ranges, except as you point out before it's going against spec recommendation.
>
> >
> > This is what the new comment in cxl_hdm_decode_init() is trying to
> > clarify. My proposal is that Linux ignores the recommendation which I
> > think is trying to accommodate legacy CXL 1.1 software stacks which
> > Linux never had.
> >
>
> Okay, that's fine.
>
> >         /*
> >          * 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.
> >          */
>
> Maybe explicitly say the driver does not attempt to check this recommendation.

Sure, I can add that.

Re: [PATCH v2 5/6] cxl/mem: Rename cxl_dvsec_decode_init() to cxl_hdm_decode_init()

[Dan Williams]

On Thu, Mar 17, 2022 at 10:55 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
>
> On 22-03-14 18:22:44, Dan Williams wrote:
> > cxl_dvsec_decode_init() is tasked with checking whether legacy DVSEC
> > range based decode is in effect, or whether HDM can be enabled / already
> > is enabled. As such it either succeeds or fails and that result is the
> > return value. The @do_hdm_init variable is misleading in the case where
> > HDM operation is already found to be active, so just call it @retval.
> >
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> If we're doing the rename, which I'm in favor of, maybe making it something else
> instead? I think init is usually something which cannot fail. Perhaps
> cxl_hdm_decode_probe()?

I think it's init, or at least not "probe", because the "probe" is
what devm_cxl_enumerate_decoders() is doing, right?

Re: [PATCH v2 1/6] cxl/mem: Drop DVSEC vs EFI Memory Map sanity check

[Jonathan Cameron]

On Mon, 14 Mar 2022 18:22:22 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> When the driver finds legacy DVSEC ranges active on a CXL Memory
> Expander it indicates that platform firmware is not aware of, or is
> deliberately disabling common CXL 2.0 operation. In this case Linux
> generally has no choice, but to leave the device alone.
> 
> The driver attempts to validate that the DVSEC range is in the EFI
> memory map. Remove that logic since there is no requirement that the
> BIOS publish DVSEC ranges in the EFI Memory Map.
> 
> In the future the driver will want to permanently reserve this capacity
> out of the available CFMWS capacity and hide it from
> request_free_mem_region(), but it serves no purpose to warn about the
> range not appearing in the EFI Memory Map.
> 
> Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

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

> ---
>  drivers/cxl/mem.c |   24 +-----------------------
>  1 file changed, 1 insertion(+), 23 deletions(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 49a4b1c47299..cd4e8bba82aa 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -158,30 +158,8 @@ static int cxl_mem_probe(struct device *dev)
>  	 * is no use in trying to manage those.
>  	 */
>  	if (!cxl_dvsec_decode_init(cxlds)) {
> -		struct cxl_endpoint_dvsec_info *info = &cxlds->info;
> -		int i;
> -
> -		/* */
> -		for (i = 0; i < 2; i++) {
> -			u64 base, size;
> -
> -			/*
> -			 * Give a nice warning to the user that BIOS has really
> -			 * botched things for them if it didn't place DVSEC
> -			 * ranges in the memory map.
> -			 */
> -			base = info->dvsec_range[i].start;
> -			size = range_len(&info->dvsec_range[i]);
> -			if (size && !region_intersects(base, size,
> -						       IORESOURCE_SYSTEM_RAM,
> -						       IORES_DESC_NONE)) {
> -				dev_err(dev,
> -					"DVSEC range %#llx-%#llx must be reserved by BIOS, but isn't\n",
> -					base, base + size - 1);
> -			}
> -		}
>  		dev_err(dev,
> -			"Active DVSEC range registers in use. Will not bind.\n");
> +			"Legacy range registers configuration prevents HDM operation.\n");
>  		return -EBUSY;
>  	}
>  
> 

Re: [PATCH v2 2/6] cxl/pci: Add debug for DVSEC range init failures

[Jonathan Cameron]

On Mon, 14 Mar 2022 18:22:28 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> In preparation for not treating DVSEC range initialization failures as
> fatal to cxl_pci_probe() add individual dev_dbg() statements for each of
> the major failure reasons in cxl_dvsec_ranges().
> 
> The rationale for cxl_dvsec_ranges() failure not being fatal is that
> there is still value for cxl_pci to enable mailbox operations even if
> CXL.mem operation is disabled.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
LGTM
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/pci.c |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 8a7267d116b7..257cf735505d 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -467,12 +467,15 @@ static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
>  {
>  	struct cxl_endpoint_dvsec_info *info = &cxlds->info;
>  	struct pci_dev *pdev = to_pci_dev(cxlds->dev);
> +	struct device *dev = &pdev->dev;
>  	int d = cxlds->cxl_dvsec;
>  	int hdm_count, rc, i;
>  	u16 cap, ctrl;
>  
> -	if (!d)
> +	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)
> @@ -482,8 +485,10 @@ static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
>  	if (rc)
>  		return rc;
>  
> -	if (!(cap & CXL_DVSEC_MEM_CAPABLE))
> +	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
> @@ -496,8 +501,10 @@ static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
>  		return -EINVAL;
>  
>  	rc = wait_for_valid(cxlds);
> -	if (rc)
> +	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);
>  
> 

Re: [PATCH v2 3/6] cxl/mem: Make cxl_dvsec_range() init failure fatal

[Jonathan Cameron]

On Mon, 14 Mar 2022 18:22:33 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> In preparation for the cxl_pci driver to continue operation after
> cxl_dvsec_range() failure, update cxl_mem to check for negative error
> codes in info->ranges. Treat that condition as fatal regardless of the
> state of the HDM configuration since cxl_mem needs positive confirmation
> that legacy ranges were not established by platform firmware or another
> agent.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com.

> ---
>  drivers/cxl/mem.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index cd4e8bba82aa..50704deb2ff0 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -88,6 +88,9 @@ __mock bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
>  	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) {
> 

Re: [PATCH v2 4/6] cxl/pci: Make cxl_dvsec_ranges() failure not fatal to cxl_pci

[Jonathan Cameron]

On Mon, 14 Mar 2022 18:22:38 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> cxl_dvsec_ranges(), the helper for enumerating the presence of an active
> legacy CXL.mem configuration on a CXL 2.0 Memory Expander, is not fatal
> for cxl_pci because there is still value to enable mailbox operations
> even if CXL.mem operation is disabled. Recall that the reason cxl_pci
> does this initialization and not cxl_mem is to preserve the useful
> property (for unit testing) that cxl_mem is cxl_memdev + mmio generic,
> and does not require access to a 'struct pci_dev' to issue config
> cycles.
> 
> Update 'struct cxl_endpoint_dvsec_info' to carry either a positive
> number of non-zero size legacy CXL DVSEC ranges, or the negative error
> code from __cxl_dvsec_ranges() in its @ranges member.
> 
> Reported-by: Krzysztof Zach <krzysztof.zach@intel.com>
> Fixes: 560f78559006 ("cxl/pci: Retrieve CXL DVSEC memory info")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
with comment Ben requested

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

> ---
>  drivers/cxl/pci.c |   27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 257cf735505d..994c79bf6afd 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -463,13 +463,18 @@ static int wait_for_media_ready(struct cxl_dev_state *cxlds)
>  	return 0;
>  }
>  
> -static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
> +/*
> + * 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 cxl_endpoint_dvsec_info *info = &cxlds->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;
> -	int hdm_count, rc, i;
>  	u16 cap, ctrl;
>  
>  	if (!d) {
> @@ -546,10 +551,17 @@ static int cxl_dvsec_ranges(struct cxl_dev_state *cxlds)
>  		};
>  
>  		if (size)
> -			info->ranges++;
> +			ranges++;
>  	}
>  
> -	return 0;
> +	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)
> @@ -618,10 +630,7 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	if (rc)
>  		return rc;
>  
> -	rc = cxl_dvsec_ranges(cxlds);
> -	if (rc)
> -		dev_warn(&pdev->dev,
> -			 "Failed to get DVSEC range information (%d)\n", rc);
> +	cxl_dvsec_ranges(cxlds);
>  
>  	cxlmd = devm_cxl_add_memdev(cxlds);
>  	if (IS_ERR(cxlmd))
> 

Re: [PATCH v2 5/6] cxl/mem: Rename cxl_dvsec_decode_init() to cxl_hdm_decode_init()

[Jonathan Cameron]

On Mon, 14 Mar 2022 18:22:44 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> cxl_dvsec_decode_init() is tasked with checking whether legacy DVSEC
> range based decode is in effect, or whether HDM can be enabled / already
> is enabled. As such it either succeeds or fails and that result is the
> return value. The @do_hdm_init variable is misleading in the case where
> HDM operation is already found to be active, so just call it @retval.

The text above is confusing for me.  Which part is justifying the
function rename and which part is for the retval?

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

> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/mem.c            |   12 ++++++------
>  tools/testing/cxl/mock_mem.c |    2 +-
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 50704deb2ff0..3baae1332760 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -68,7 +68,7 @@ static int create_endpoint(struct cxl_memdev *cxlmd,
>  }
>  
>  /**
> - * cxl_dvsec_decode_init() - Setup HDM decoding for the 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
> @@ -79,12 +79,12 @@ 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_dvsec_decode_init(struct cxl_dev_state *cxlds)
> +__mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
>  {
>  	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, do_hdm_init = false;
> +	bool global_enable, retval = false;
>  	void __iomem *crb;
>  	u32 global_ctrl;
>  
> @@ -113,7 +113,7 @@ __mock bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
>  		goto out;
>  	}
>  
> -	do_hdm_init = true;
> +	retval = true;
>  
>  	/*
>  	 * Permanently (for this boot at least) opt the device into HDM
> @@ -129,7 +129,7 @@ __mock bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
>  
>  out:
>  	iounmap(crb);
> -	return do_hdm_init;
> +	return retval;
>  }
>  
>  static int cxl_mem_probe(struct device *dev)
> @@ -160,7 +160,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_dvsec_decode_init(cxlds)) {
> +	if (!cxl_hdm_decode_init(cxlds)) {
>  		dev_err(dev,
>  			"Legacy range registers configuration prevents HDM operation.\n");
>  		return -EBUSY;
> diff --git a/tools/testing/cxl/mock_mem.c b/tools/testing/cxl/mock_mem.c
> index d1dec5845139..69946f678cfa 100644
> --- a/tools/testing/cxl/mock_mem.c
> +++ b/tools/testing/cxl/mock_mem.c
> @@ -4,7 +4,7 @@
>  #include <linux/types.h>
>  
>  struct cxl_dev_state;
> -bool cxl_dvsec_decode_init(struct cxl_dev_state *cxlds)
> +bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
>  {
>  	return true;
>  }
> 

Re: [PATCH v2 6/6] cxl/mem: Replace redundant debug message with a comment

[Jonathan Cameron]

On Mon, 14 Mar 2022 18:22:49 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> cxl_mem_probe() already emits a log message when HDM operation can not
> be established. Delete the similar one in cxl_hdm_decode_init().
> 
> What is less obvious is why global_ctrl being enabled makes positive
> values of info->ranges irrelevant.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Assuming comment on ignore recommendation added for v3.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/mem.c |   13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 3baae1332760..6def7d7b6bfd 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -107,11 +107,16 @@ __mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
>  	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->ranges) {
> -		dev_dbg(cxlds->dev,
> -			"DVSEC ranges already programmed and HDM decoders not enabled.\n");
> +
> +	/*
> +	 * 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.
> +	 */
> +	if (!global_enable && info->ranges)
>  		goto out;
> -	}
>  
>  	retval = true;
>  
> 

[PATCH v3 6/6] cxl/mem: Replace redundant debug message with a comment

[Dan Williams]

cxl_mem_probe() already emits a log message when HDM operation can not
be established. Delete the similar one in cxl_hdm_decode_init().

What is less obvious is why global_ctrl being enabled makes positive
values of info->ranges irrelevant, and the Linux behavior with respect
to the spec recommendation to mirror CXL Range registers with HDM
Decoder Base + Size registers.

Cc: Ben Widawsky <ben.widawsky@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Changes since v2:
- Clarify that Linux does not maintain the spec recommended match
  between HDM decoders and CXL DVSEC range registers. (Ben and Jonathan)

 drivers/cxl/mem.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
index 3baae1332760..43e73d259207 100644
--- a/drivers/cxl/mem.c
+++ b/drivers/cxl/mem.c
@@ -107,11 +107,17 @@ __mock bool cxl_hdm_decode_init(struct cxl_dev_state *cxlds)
 	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->ranges) {
-		dev_dbg(cxlds->dev,
-			"DVSEC ranges already programmed and HDM decoders not enabled.\n");
+
+	/*
+	 * 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->ranges)
 		goto out;
-	}
 
 	retval = true;