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

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

On 8/4/24 18:31, Jonathan Cameron wrote:
> On Mon, 15 Jul 2024 18:28:27 +0100
> alejandro.lucero-palau@amd.com wrote:
>
>> From: Alejandro Lucero <alucerop@amd.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@amd.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.
>
>>   }
>>   
>>   
>