netdev - Re: [PATCH v2 07/15] cxl: support type2 memdev creation

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: <05ee01d8-4687-b44a-effc-1c14cd28d79f@amd.com>
Date: Fri, 16 Aug 2024 16:00:06 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 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, richard.hughes@....com
Subject: Re: [PATCH v2 07/15] cxl: support type2 memdev creation


On 8/4/24 18:31, Jonathan Cameron wrote:
> On Mon, 15 Jul 2024 18:28:27 +0100
> alejandro.lucero-palau@....com wrote:
>
>> From: Alejandro Lucero <alucerop@....com>
>>
>> Add memdev creation from sfc driver.
>>
>> Current cxl core is relying on a CXL_DEVTYPE_CLASSMEM type device when
>> creating a memdev leading to problems when obtaining cxl_memdev_state
>> references from a CXL_DEVTYPE_DEVMEM type. This last device type is
>> managed by a specific vendor driver and does not need same sysfs files
>> since not userspace intervention is expected. This patch checks for the
>> right device type in those functions using cxl_memdev_state.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@....com>
> Same general comment about treating failure to get things you expect
> as proper driver probe errors.  Very unlikely we'd ever want to carry
> on if these fail. If we do want to, that should be a high level decision
> and the chances are the driver needs to know that the error occurred
> so it can take some mitigating measures (using some alternative mechanisms
> etc).


OK


Other comments below already addressed when replying to fan.

Thanks!


>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>> index a84fe7992c53..0abe66490ef5 100644
>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>> @@ -57,10 +57,16 @@ void efx_cxl_init(struct efx_nic *efx)
>>   	if (cxl_accel_request_resource(cxl->cxlds, true))
>>   		pci_info(pci_dev, "CXL accel resource request failed");
>>   
>> -	if (!cxl_await_media_ready(cxl->cxlds))
>> +	if (!cxl_await_media_ready(cxl->cxlds)) {
>>   		cxl_accel_set_media_ready(cxl->cxlds);
>> -	else
>> +	} else {
>>   		pci_info(pci_dev, "CXL accel media not active");
>> +		return;
> Once you are returning an error in this path you can just have
> 		return -ETIMEDOUT; or similar here adn avoid
> this code changing in this patch.
>> +	}
>> +
>> +	cxl->cxlmd = devm_cxl_add_memdev(&pci_dev->dev, cxl->cxlds);
>> +	if (IS_ERR(cxl->cxlmd))
>> +		pci_info(pci_dev, "CXL accel memdev creation failed");
> I'd treat this one as fatal as well.
>
> People argue in favor of muddling on to allow firmware upgrade etc.
> That is fine, but pass up the errors then decide to ignore them
> at the higher levels.
>
>>   }
>>   
>>   
>