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

Alejandro Lucero Palau
2024-08-16T15:00:49Z

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.
>
>> }
>>
>>
>