Re: [PATCH v7 07/28] sfc: use cxl api for regs setup and checking

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

On 12/12/24 18:04, Simon Horman wrote:
> On Mon, Dec 09, 2024 at 06:54:08PM +0000, alejandro.lucero-palau@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Use cxl code for registers discovery and mapping.
>>
>> Validate capabilities found based on those registers against expected
>> capabilities.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>
>> Reviewed-by: Zhi Wang <zhi@nvidia.com>
>> ---
>>   drivers/net/ethernet/sfc/efx_cxl.c | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>> index 58a6f14565ff..3f15486f99e4 100644
>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>> @@ -21,6 +21,8 @@
>>   int efx_cxl_init(struct efx_probe_data *probe_data)
>>   {
>>   	struct efx_nic *efx = &probe_data->efx;
>> +	DECLARE_BITMAP(expected, CXL_MAX_CAPS);
>> +	DECLARE_BITMAP(found, CXL_MAX_CAPS);
>>   	struct pci_dev *pci_dev;
>>   	struct efx_cxl *cxl;
>>   	struct resource res;
>> @@ -65,6 +67,23 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>>   		goto err2;
>>   	}
>>   
>> +	rc = cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds);
>> +	if (rc) {
>> +		pci_err(pci_dev, "CXL accel setup regs failed");
>> +		goto err2;
>> +	}
>> +
>> +	bitmap_clear(expected, 0, CXL_MAX_CAPS);
>> +	bitmap_set(expected, CXL_DEV_CAP_HDM, 1);
>> +	bitmap_set(expected, CXL_DEV_CAP_RAS, 1);
>> +
>> +	if (!cxl_pci_check_caps(cxl->cxlds, expected, found)) {
>> +		pci_err(pci_dev,
>> +			"CXL device capabilities found(%08lx) not as expected(%08lx)",
>> +			*found, *expected);
>> +		goto err2;
> Hi Alejandro,
>
> goto err2 will result in the function returning rc.
> However, rc is 0 here. Should it be set to a negative error value instead?


Hi Simon,


Right. The code is functionally correct because the driver's variable 
setting the CXL initialization successful is not happening when skipping 
with the goto, which is the key part. Returning an error will log a CXL 
initialization driver's error but it has no impact with the driver's 
initialization or the CXL usage by design.


However, the error should be returned properly.


I will fix it in v8.


Thanks!


> Flagged by Smatch.
>
>> +	}
>> +
>>   	probe_data->cxl = cxl;
>>   
>>   	return 0;
>> -- 
>> 2.17.1
>>
>>