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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7fa95860-5d4b-c19c-b9e6-94d8d8130793@amd.com>
Date: Fri, 13 Dec 2024 09:17:54 +0000
From: Alejandro Lucero Palau <alucerop@....com>
To: Simon Horman <horms@...nel.org>, alejandro.lucero-palau@....com
Cc: linux-cxl@...r.kernel.org, netdev@...r.kernel.org,
 dan.j.williams@...el.com, martin.habets@...inx.com, edward.cree@....com,
 davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
 edumazet@...gle.com, dave.jiang@...el.com
Subject: Re: [PATCH v7 07/28] sfc: use cxl api for regs setup and checking


On 12/12/24 18:04, Simon Horman wrote:
> On Mon, Dec 09, 2024 at 06:54:08PM +0000, alejandro.lucero-palau@....com wrote:
>> From: Alejandro Lucero <alucerop@....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@....com>
>> Reviewed-by: Martin Habets <habetsm.xilinx@...il.com>
>> Reviewed-by: Zhi Wang <zhi@...dia.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
>>
>>