Re: [PATCH v9 06/27] cxl: add function for type2 cxl regs setup

Alejandro Lucero Palau wrote:

On 1/18/25 01:51, Dan Williams wrote:

quoted

alejandro.lucero-palau@ wrote:

quoted

From: Alejandro Lucero <redacted>

Create a new function for a type2 device initialising
cxl_dev_state struct regarding cxl regs setup and mapping.

Signed-off-by: Alejandro Lucero <redacted>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Fan Ni <redacted>
---
  drivers/cxl/core/pci.c | 51 ++++++++++++++++++++++++++++++++++++++++++
  include/cxl/cxl.h      |  2 ++
  2 files changed, 53 insertions(+)

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 5821d582c520..493ab33fe771 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c

@@ -1107,6 +1107,57 @@ int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
  }
  EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, "CXL");
  
+static int cxl_pci_setup_memdev_regs(struct pci_dev *pdev,
+				     struct cxl_dev_state *cxlds)
+{
+	struct cxl_register_map map;
+	int rc;
+
+	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map,
+				cxlds->capabilities);
+	/*
+	 * This call can return -ENODEV if regs not found. This is not an error
+	 * for Type2 since these regs are not mandatory. If they do exist then
+	 * mapping them should not fail. If they should exist, it is with driver
+	 * calling cxl_pci_check_caps where the problem should be found.
+	 */

There is no common definition of type-2 so the core should not try to
assume it knows, or be told what is mandatory. Just export the raw
helpers and leave it to the caller to make these decisions.

The code does not know, but it knows it does not know, therefore handles 
this new situation not needed before Type2 support in the generic code 
for the pci driver and Type3.

This is added to the API for accel drivers following the design 
restrictions I have commented earlier in another patch. Your suggestion 
seems to go against that decision what was implicitly taken after the 
first versions and which had no complains until now.

Apologies for that, I had not looked at the implications of that general
decision until now, but the result is going in the wrong direction from
what it is doing to the core.

quoted

quoted

+		return 0;
+
+	if (rc)
+		return rc;
+
+	return cxl_map_device_regs(&map, &cxlds->regs.device_regs);
+}
+
+int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds)
+{
+	int rc;
+
+	rc = cxl_pci_setup_memdev_regs(pdev, cxlds);
+	if (rc)
+		return rc;
+
+	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
+				&cxlds->reg_map, cxlds->capabilities);
+	if (rc) {
+		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
+		return rc;
+	}
+
+	if (!test_bit(CXL_CM_CAP_CAP_ID_RAS, cxlds->capabilities))
+		return rc;

This is injecting logic in a bitmap and a new CXL core exported ABI just
to avoid the driver optionally skipping RAS register enumeration.

The core should not care how and whether endpoint drivers (accel or
cxl_pci) consume register blocks, just arrange for their enumeration and
let the leaf driver logic take it from there.