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

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

Hi Ben,


I forgot to reply to this one. Apologies.


My comments below.


On 3/11/25 20:05, Ben Cheatham wrote:
> On 3/10/25 4:03 PM, 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>
>> ---
>>   drivers/cxl/core/pci.c  |  4 ++--
>>   drivers/cxl/core/port.c |  8 ++++----
>>   drivers/cxl/core/regs.c | 37 +++++++++++++++++++++----------------
>>   drivers/cxl/cxl.h       |  6 +++---
>>   drivers/cxl/cxlpci.h    |  2 +-
>>   drivers/cxl/pci.c       | 31 ++++++++++++++++++++++++++++---
>>   include/cxl/cxl.h       | 20 ++++++++++++++++++++
>>   7 files changed, 79 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 0b8dc34b8300..05399292209a 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");
>>   
>> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
>> index 6a44b6dad3c7..ede36f7168ed 100644
>> --- a/drivers/cxl/core/port.c
>> +++ b/drivers/cxl/core/port.c
>> @@ -755,7 +755,7 @@ static struct cxl_port *cxl_port_alloc(struct device *uport_dev,
>>   }
>>   
>>   static int cxl_setup_comp_regs(struct device *host, struct cxl_register_map *map,
>> -			       resource_size_t component_reg_phys)
>> +			       resource_size_t component_reg_phys, unsigned long *caps)
>>   {
>>   	*map = (struct cxl_register_map) {
>>   		.host = host,
>> @@ -769,7 +769,7 @@ static int cxl_setup_comp_regs(struct device *host, struct cxl_register_map *map
>>   	map->reg_type = CXL_REGLOC_RBI_COMPONENT;
>>   	map->max_size = CXL_COMPONENT_REG_BLOCK_SIZE;
>>   
>> -	return cxl_setup_regs(map);
>> +	return cxl_setup_regs(map, caps);
>>   }
>>   
>>   static int cxl_port_setup_regs(struct cxl_port *port,
>> @@ -778,7 +778,7 @@ static int cxl_port_setup_regs(struct cxl_port *port,
>>   	if (dev_is_platform(port->uport_dev))
>>   		return 0;
>>   	return cxl_setup_comp_regs(&port->dev, &port->reg_map,
>> -				   component_reg_phys);
>> +				   component_reg_phys, NULL);
>>   }
>>   
>>   static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
>> @@ -795,7 +795,7 @@ static int cxl_dport_setup_regs(struct device *host, struct cxl_dport *dport,
>>   	 * NULL.
>>   	 */
>>   	rc = cxl_setup_comp_regs(dport->dport_dev, &dport->reg_map,
>> -				 component_reg_phys);
>> +				 component_reg_phys, NULL);
> Nit here, but if you just pass in a unsigned long here, and in cxl_port_setup_regs() above, instead of NULL
> you can get rid of the null pointer checks in the register probe functions.
>

Not sure this kind of tricks are usual, but I do not like it. It avoids 
the check, but it can confuse the reader, and the checks are not so many.


<snip>

>>   
>> +	/*
>> +	 * 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,
>> +			"Expected mandatory capabilities not found: (%pb - %pb)\n",
> I think this will just print the bitmaps, so it would probably be good to highlight which is
> the mandatory capabilites map and which is the found. Maybe something like:
>
> "Found capabilities (%pb) are missing mandatory capabilities (%pb)\n"
>

I can do that.


>> +			expected, found);
>> +		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 5c6481136f93..02b73c82e5d8 100644
>> --- a/include/cxl/cxl.h
>> +++ b/include/cxl/cxl.h
>> @@ -25,6 +25,26 @@ enum cxl_devtype {
>>   
>>   struct device;
>>   
>> +
>> +/* Capabilities as defined for:
>> + *
>> + *	Component Registers (Table 8-22 CXL 3.1 specification)
>> + *	Device Registers (8.2.8.2.1 CXL 3.1 specification)
> Update to 3.2 spec?


Sure.


Thanks!