Re: [PATCH v25 05/11] sfc: create type2 cxl memdev

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

On 4/1/26 22:53, Dan Williams wrote:
> Alejandro Lucero Palau wrote:
>> On 4/1/26 06:17, Dan Williams wrote:
>>> alejandro.lucero-palau@ wrote:
>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>
>>>> Use cxl API for creating a cxl memory device using the type2
>>>> cxl_dev_state struct.
>>>>
>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>>> Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>
>>>> Reviewed-by: Fan Ni <fan.ni@samsung.com>
>>>> Acked-by: Edward Cree <ecree.xilinx@gmail.com>
>>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>>>> ---
>>>>    drivers/net/ethernet/sfc/efx_cxl.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>>>> index 6619084a77d8..63e6f277ae9f 100644
>>>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>>>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>>>> @@ -75,6 +75,12 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>>>>    		return -ENODEV;
>>>>    	}
>>>>    
>>>> +	cxl->cxlmd = devm_cxl_add_memdev(&cxl->cxlds, NULL);
>>> Did this forget about:
>>>
>>> 29317f8dc6ed cxl/mem: Introduce cxl_memdev_attach for CXL-dependent operation
>>>
>>> ?
>>
>> I did not. The idea behind attach does not make sense for sfc, so I do
>> not use that option. I did discuss this with you at LPC and based on
>> previous comments which had not replies (case 1 in here):
>>
>>
>> https://lore.kernel.org/linux-cxl/836d06d6-a36f-4ba3-b7c9-ba8687ba2190@amd.com/
>>
>>
>>   From our conversation at LPC, there is no reason for a CXL device
>> connected to the CXL Root Bridge not having its memdev properly
>> initialized.
> The cxl_core can not make assumptions about attachment topology. It has
> always been the case that the cxl_core needs to be prepared to handle
> everything that the firmware did not.


This is too much generic as an argument. If you want to convince me, I 
would need a real case where this can be a problem.

The current assumption and the only option supported with this last 
version is the BIOS *is* configuring the device HDM and there should be 
nothing to do from the cxl core in that regard:


1) BIOS configured and committed the device HDM: CXL.mem is used.

2) BIOS did not configure the HDM: CXL.mem is not used.


Any problem with the memdev initialization *requiring* your attach 
option is helpless ... if there exists a reason for it being helpful 
because the memdev initialization triggering the port and hdm 
initilization was not completely done, please explain why that could 
happen in the scenario I am presenting with an accelerator connected to 
a CXL Host Bridge port and the BIOS configuring the HDM based on the CFMWS.


> Error handling and kexec also lead to
> situations where the boot configuration gets invalidated and needs
> recovery.


Error handling based on RAS is a follow-up work. CXL reset is not 
supported ... and AFAIK, it is not supported by Type3 either or it has 
not been for a good bunch of time. If so, I do not understand why this 
should stop this patchset.


About kexec, it happens without a shutdown ... so if the system does it 
without proper handling, nothing we can do. If it is properly handled, 
the cxl core initilaization will happen ... is it handling the case of 
finding HDMs already configured (kexec will leave them untouched, won't 
it?)? From a type2 point of view and using this patchset, if kexec was 
executed after removing first the sfc driver, there should not be any 
problem. If I am missing something about this, please explain it in 
detail or give any reference I can look at. FWIW, I did use kexec a lot 
in embedded systems and had to deal with some hardware needing special 
treatment before triggering it, so happy to learn more about the 
implications with CXL.


>
>> I did ask you specifically about this, and you mentioned links on CXL
>> switches not ready, but as I said then and repeat now, this is not
>> what sfc driver expects at all. Only a port from a CXL Root Bridge
>> makes sense due to the latencies involved with more complex CXL
>> topologies.
>>
>>
>> So, from that list I wrote in that old thread where I tried to summarize
>> the problems and clarify the confusion behind different concerns, the
>> only issue is someone removing cxl_acpi. I do not think that should be
>> possible at all,
> ...but it *is* possible to remove cxl_acpi, it *is* possible to invoke
> 'cxl disable-port'. The fact that it is possible contributes to
> complexity, but it also supports flexibility and is a building block for
> error handling / recovery.


It is possible but, should it? When should it be allowed with drivers 
relying on its functionality?


>> so something should be added for other modules depending on it
>> avoiding its removal. I would say any CXL port created is (in X86)
>> dependent on cxl_acpi, so something at port creation invoking a
>> exported cxl_acpi function could do it, like a cxl_acpi_users counter.
>> This could also help for visibility to user space about its usage.
> There are two choices, fail removal or handle removal. There are some
> degrees of freedom that can be pared back, but outright blocking removal
> is not one of them.


Why not?


>> Therefore, I do not like the changes you propose here. But, if this
>> proposal is the only way
> It is not a matter of like or dislike, it is a matter of functional
> correctness relative to the state of the subsystem today. Simply put,
> one proposal handles the locking and lifetime concerns, the other does
> not.