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

[Alejandro Lucero Palau <alucerop@amd.com>]

On 5/8/25 01:55, Alison Schofield wrote:
> 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
>

I'll fix it.


>> 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];
> 	}
>

Looks good to me and it saves some code lines. I'll do the change.


>
> 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);
> }
>

I have no preference but I can do this change as well.

Thank you!