Re: [PATCH v16 12/22] sfc: obtain root decoder with enough HPA free space

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

On 5/21/25 20:56, Dan Williams wrote:
> alejandro.lucero-palau@ wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Asking for available HPA space is the previous step to try to obtain
>> an HPA range suitable to accel driver purposes.
>>
>> Add this call to efx cxl initialization.
>>
>> Make sfc cxl build dependent on CXL region.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com>
>> Acked-by: Edward Cree <ecree.xilinx@gmail.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> ---
>>   drivers/net/ethernet/sfc/Kconfig   |  1 +
>>   drivers/net/ethernet/sfc/efx_cxl.c | 19 +++++++++++++++++++
>>   2 files changed, 20 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/sfc/Kconfig b/drivers/net/ethernet/sfc/Kconfig
>> index 979f2801e2a8..e959d9b4f4ce 100644
>> --- a/drivers/net/ethernet/sfc/Kconfig
>> +++ b/drivers/net/ethernet/sfc/Kconfig
>> @@ -69,6 +69,7 @@ config SFC_MCDI_LOGGING
>>   config SFC_CXL
>>   	bool "Solarflare SFC9100-family CXL support"
>>   	depends on SFC && CXL_BUS >= SFC
>> +	depends on CXL_REGION
>>   	default SFC
>>   	help
>>   	  This enables SFC CXL support if the kernel is configuring CXL for
>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>> index 53ff97ad07f5..5635672b3fc3 100644
>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>> @@ -26,6 +26,7 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>>   	struct cxl_dpa_info sfc_dpa_info = {
>>   		.size = EFX_CTPIO_BUFFER_SIZE
>>   	};
>> +	resource_size_t max_size;
>>   	struct efx_cxl *cxl;
>>   	u16 dvsec;
>>   	int rc;
>> @@ -84,6 +85,22 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>>   		return PTR_ERR(cxl->cxlmd);
>>   	}
>>   
>> +	cxl->cxlrd = cxl_get_hpa_freespace(cxl->cxlmd, 1,
>> +					   CXL_DECODER_F_RAM | CXL_DECODER_F_TYPE2,
>> +					   &max_size);
>> +
>> +	if (IS_ERR(cxl->cxlrd)) {
>> +		pci_err(pci_dev, "cxl_get_hpa_freespace failed\n");
>> +		return PTR_ERR(cxl->cxlrd);
>> +	}
> This is a simple enough model, but it does mean that if async-driver
> loading causes this driver to load before cxl_acpi or cxl_mem have
> completed their init work, then it will die here.
>
> It is also worth noting that nothing stops cxl_mem or cxl_acpi from
> detaching immediately after passing the above check. So more work is
> needed here (likely post-merge) to revoke and invalidate usage of that
> freespace when that happens.
>
> Otherwise you can do something like:
>
> Driver1			Driver2			Notes
> cxl_get_hpa_freespace()				"Driver1 gets rangeX"
> 	--- cxl_acpi unloaded ---		"forgets rangeX was assigned"	
> 	--- cxl_acpi reloaded ---			
> 			cxl_get_hpa_freespace() "Driver2 gets rangeX"
> use_cxl(rangeX)		use_cxl(rangeX)		"...uh oh"


I've been thinking about this and other similar comments in later 
patches. I have to admit it is confusing, at least for me, because I did 
not understand why cxl_acpi or cxl_mem can be removed when users/clients 
depend on them. I think (but maybe it is a wrong assumption) they should 
not, but the code is not implementing that restriction. In other words, 
it is not a functionality but something to fix with two options: to not 
allow that to happen implying the removal needs to detect the situation, 
or allow it and the removal unwinding everything depending on them.


If this is the right assumption, then I understand your comments about 
cxl_acquire_endpoint. Maybe it is worth to say I did relate 
cxl_acquire_endpoint to the problem with the initialization and device 
model probing, something that IMO requires further discussion.


> So longer term there needs to be notification back to the creator of the
> memdev to require it to handle cleaning up when the CXL topology is torn
> down either physically or logically.
>
> To date the CXL subsystem has not reset decoders on unload because it
> needs to handle coordinating with HDM decode established by platform
> firmware. Type-2 driver however should be prepared to have their CXL
> range revoked at any moment.
>
> The Type-3 case handles this because cxl_mem is the driver itself, for
> Type-2 that driver wants to coordinate with cxl_mem on these events. To
> me that looks like cxl_mem error handler operation callbacks.