Re: [PATCH v14 04/22] cxl: move register/capability check to driver

[Alison Schofield <alison.schofield@intel.com>]

On Thu, Apr 17, 2025 at 10:29:07PM +0100, alejandro.lucero-palau@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> Type3 has some mandatory capabilities which are optional for Type2.
> 
> In order to support same register/capability discovery code for both
> types, avoid any assumption about what capabilities should be there, and
> export the capabilities found for the caller doing the capabilities
> check based on the expected ones.
> 
> Add a function for facilitating the report of capabiities missing the
sp. capabilities

> expected ones.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/cxl/core/pci.c  | 35 +++++++++++++++++++++++++++++++++--
>  drivers/cxl/core/port.c |  8 ++++----
>  drivers/cxl/core/regs.c | 35 +++++++++++++++++++----------------
>  drivers/cxl/cxl.h       |  6 +++---
>  drivers/cxl/cxlpci.h    |  2 +-
>  drivers/cxl/pci.c       | 24 +++++++++++++++++++++---
>  include/cxl/cxl.h       | 24 ++++++++++++++++++++++++
>  7 files changed, 105 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 0b8dc34b8300..ed18260ff1c9 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -1061,7 +1061,7 @@ static int cxl_rcrb_get_comp_regs(struct pci_dev *pdev,
>  }
>  
>  int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> -		       struct cxl_register_map *map)
> +		       struct cxl_register_map *map, unsigned long *caps)
>  {
>  	int rc;
>  
> @@ -1091,7 +1091,7 @@ int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>  		return rc;
>  	}
>  
> -	return cxl_setup_regs(map);
> +	return cxl_setup_regs(map, caps);
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, "CXL");
>  
> @@ -1214,3 +1214,34 @@ int cxl_gpf_port_setup(struct device *dport_dev, struct cxl_port *port)
>  
>  	return 0;
>  }
> +
> +int cxl_check_caps(struct pci_dev *pdev, unsigned long *expected,
> +		   unsigned long *found)
> +{
> +	DECLARE_BITMAP(missing, CXL_MAX_CAPS);
> +
> +	if (bitmap_subset(expected, found, CXL_MAX_CAPS))
> +		/* all good */
> +		return 0;
> +
> +	bitmap_andnot(missing, expected, found, CXL_MAX_CAPS);
> +
> +	if (test_bit(CXL_DEV_CAP_RAS, missing))
> +		dev_err(&pdev->dev, "RAS capability not found\n");
> +
> +	if (test_bit(CXL_DEV_CAP_HDM, missing))
> +		dev_err(&pdev->dev, "HDM decoder capability not found\n");
> +
> +	if (test_bit(CXL_DEV_CAP_DEV_STATUS, missing))
> +		dev_err(&pdev->dev, "Device Status capability not found\n");
> +
> +	if (test_bit(CXL_DEV_CAP_MAILBOX_PRIMARY, missing))
> +		dev_err(&pdev->dev, "Primary Mailbox capability not found\n");
> +
> +	if (test_bit(CXL_DEV_CAP_MEMDEV, missing))
> +		dev_err(&pdev->dev,
> +			"Memory Device Status capability not found\n");
> +
> +	return -1;
> +}

Prefer using an array to map the enums to strings, like -

	static const char * const cap_names[CXL_MAX_CAPS] = {
		[CXL_DEV_CAP_RAS]	= "CXL_DEV_CAP_RAS",
		.
		.
		.
	};

and then loop thru that, like: 

	for (int i = 0; i < CXL_MAX_CAPS; i++) {
		if (!test-bit(i, missing))
	 		dev_err(&pdev->dev,"%s capability not found\n",
	 	 		cap_names[i];
	}



snip
>  }
> diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
> index be0ae9aca84a..e409ea06af0b 100644
> --- a/drivers/cxl/core/regs.c
> +++ b/drivers/cxl/core/regs.c
> @@ -4,6 +4,7 @@
>  #include <linux/device.h>
>  #include <linux/slab.h>
>  #include <linux/pci.h>
> +#include <cxl/cxl.h>
>  #include <cxl/pci.h>
>  #include <cxlmem.h>
>  #include <cxlpci.h>
> @@ -11,6 +12,9 @@
>  
>  #include "core.h"
>  
> +#define cxl_cap_set_bit(bit, caps) \
> +	do { if ((caps)) set_bit((bit), (caps)); } while (0)
> +

Prefer a readable and type safe simple fcn:

static void cxl_cap_set_bit(int bit, unsigned long *caps)
{
	if (caps)
		set_bit(bit, caps);
}


snip