On 12/24/24 17:15, Jonathan Cameron wrote:

On Mon, 16 Dec 2024 16:10:19 +0000
alejandro.lucero-palau@amd.com wrote:

quoted

From: Alejandro Lucero <redacted>

During CXL device initialization supported capabilities by the device
are discovered. Type3 and Type2 devices have different mandatory
capabilities and a Type2 expects a specific set including optional
capabilities.

Add a function for checking expected capabilities against those found
during initialization and allow those mandatory/expected capabilities to
be a subset of the capabilities found.

Rely on this function for validating capabilities instead of when CXL
regs are probed.

Signed-off-by: Alejandro Lucero <redacted>
Reviewed-by: Zhi Wang <redacted>

Some follow on comments in how to handle bitmaps.

Jonathan

quoted

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index dbc1cd9bec09..1fcc53df1217 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c

@@ -903,6 +903,8 @@ __ATTRIBUTE_GROUPS(cxl_rcd);
  static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
  {
  	struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus);
+	DECLARE_BITMAP(expected, CXL_MAX_CAPS);
+	DECLARE_BITMAP(found, CXL_MAX_CAPS);
  	struct cxl_memdev_state *mds;
  	struct cxl_dev_state *cxlds;
  	struct cxl_register_map map;

@@ -964,6 +966,28 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
  	if (rc)
  		dev_dbg(&pdev->dev, "Failed to map RAS capability.\n");
  
+	bitmap_clear(expected, 0, CXL_MAX_CAPS);
+
+	/*
+	 * These are the mandatory capabilities for a Type3 device.
+	 * Only checking capabilities used by current Linux drivers.
+	 */
+	bitmap_set(expected, CXL_DEV_CAP_HDM, 1);

set_bit()  - see comments in bitmap.h, these are fine applied to bitmaps
and make more sense for setting a single bit.

OK.

quoted

+	bitmap_set(expected, CXL_DEV_CAP_DEV_STATUS, 1);
+	bitmap_set(expected, CXL_DEV_CAP_MAILBOX_PRIMARY, 1);
+	bitmap_set(expected, CXL_DEV_CAP_MEMDEV, 1);
+
+	/*
+	 * Checking mandatory caps are there as, at least, a subset of those
+	 * found.
+	 */
+	if (!cxl_pci_check_caps(cxlds, expected, found)) {
+		dev_err(&pdev->dev,
+			"Expected mandatory capabilities not found: (%08lx - %08lx)\n",
+			*expected, *found);

There are printk formats for bitmaps that should be used here. %*pb

That is more convenient. I'll use them.

Thanks!

quoted

+		return -ENXIO;
+	}
+
  	rc = cxl_pci_type3_init_mailbox(cxlds);
  	if (rc)
  		return rc;

diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
index f656fcd4945f..05f06bfd2c29 100644
--- a/include/cxl/cxl.h
+++ b/include/cxl/cxl.h

@@ -37,4 +37,7 @@ void cxl_set_dvsec(struct cxl_dev_state *cxlds, u16 dvsec);
  void cxl_set_serial(struct cxl_dev_state *cxlds, u64 serial);
  int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res,
  		     enum cxl_resource);
+bool cxl_pci_check_caps(struct cxl_dev_state *cxlds,
+			unsigned long *expected_caps,
+			unsigned long *current_caps);
  #endif