Re: [PATCH v12 04/23] cxl: move register/capability check to driver

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Mon, 31 Mar 2025 15:45:36 +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.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
A few minor things inline.

> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 0fd6646c1a2e..7adf2cff43b6 100644

> @@ -37,7 +39,8 @@
>   * Probe for component register information and return it in map object.
>   */
>  void cxl_probe_component_regs(struct device *dev, void __iomem *base,
> -			      struct cxl_component_reg_map *map)
> +			      struct cxl_component_reg_map *map,
> +			      unsigned long *caps)
>  {
>  	int cap, cap_count;
>  	u32 cap_array;
> @@ -85,6 +88,8 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
>  			decoder_cnt = cxl_hdm_decoder_count(hdr);
>  			length = 0x20 * decoder_cnt + 0x10;
>  			rmap = &map->hdm_decoder;
> +			if (caps)
> +				set_bit(CXL_DEV_CAP_HDM, caps);

Maybe it's worth a local helper?
			cxl_set_cap_bit() that checks for NULL cap

#define cxl_set_cap_bit(bit, caps) if (caps) { set_bit((bit), (caps)); }

Or just always provide caps.  Do we have use cases where we really don't
care about what is found?  

>  			break;
>  		}
>  		case CXL_CM_CAP_CAP_ID_RAS:
> @@ -92,6 +97,8 @@ void cxl_probe_component_regs(struct device *dev, void __iomem *base,
>  				offset);
>  			length = CXL_RAS_CAPABILITY_LENGTH;
>  			rmap = &map->ras;
> +			if (caps)
> +				set_bit(CXL_DEV_CAP_RAS, caps);
>  			break;
>  		default:
>  			dev_dbg(dev, "Unknown CM cap ID: %d (0x%x)\n", cap_id,

>  #endif /* __CXL_PCI_H__ */
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index e8c0efb3a12f..2a52556bd568 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -836,6 +836,8 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)

>  	else if (!cxlds->reg_map.component_map.ras.valid)
> @@ -895,6 +909,17 @@ 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");
>  
> +	/*
> +	 * Checking mandatory caps are there as, at least, a subset of those
> +	 * found.
> +	 */
> +	if (!bitmap_subset(expected, found, CXL_MAX_CAPS)) {

> +		dev_err(&pdev->dev,
> +			"Found capabilities (%pb) not containing mandatory expected: (%pb)\n",

The only obvious reason I can see for an expected bitmap is this print and this isn't
that helpful as requires anyone seeing it to dig into what the bitmap means.  Maybe

	if (!test_bit(CXL_DEV_CAP_HDM, found))
		return dev_err_probe(&pdev->dev, -ENXIO "HDM decoder capability not found\n");
	etc.

That will only print the first once not found though and avoiding that adds complexity we
probably don't want here. 

> +			found, expected);
> +		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 1383fd724cf6..b9cd98950a38 100644
> --- a/include/cxl/cxl.h
> +++ b/include/cxl/cxl.h
> @@ -25,6 +25,26 @@ enum cxl_devtype {
>  
>  struct device;
>  
> +
> +/* Capabilities as defined for:
CXL code so 
/*
 * Capabilities...

> + *
> + *	Component Registers (Table 8-22 CXL 3.2 specification)
> + *	Device Registers (8.2.8.2.1 CXL 3.2 specification)
> + *
> + * and currently being used for kernel CXL support.
> + */
> +
> +enum cxl_dev_cap {
> +	/* capabilities from Component Registers */
> +	CXL_DEV_CAP_RAS,
> +	CXL_DEV_CAP_HDM,
> +	/* capabilities from Device Registers */
> +	CXL_DEV_CAP_DEV_STATUS,
> +	CXL_DEV_CAP_MAILBOX_PRIMARY,
> +	CXL_DEV_CAP_MEMDEV,
> +	CXL_MAX_CAPS,

No comma as this is at terminating entry. Seems unlikely to make
sense to ever have anything after it so let us make that harder /
more obvious in future patches, but not having the comma.

> +};
> +
>  /*
>   * Using struct_group() allows for per register-block-type helper routines,
>   * without requiring block-type agnostic code to include the prefix.