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

[Dan Williams <dan.j.williams@intel.com>]

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. Error handling and kexec also lead to
situations where the boot configuration gets invalidated and needs
recovery.

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

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

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