Re: [PATCH v18 20/20] sfc: support pio mapping based on cxl

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

On 9/18/25 16:08, Jonathan Cameron wrote:
> A few trivial things inline.
>
>> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
>> index 47349c148c0c..7bc854e2d22a 100644
>> --- a/drivers/net/ethernet/sfc/ef10.c
>> +++ b/drivers/net/ethernet/sfc/ef10.c
>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>> index 85490afc7930..3dde59003cd9 100644
>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>> @@ -11,16 +11,23 @@
>>   
>>   #include "net_driver.h"
>>   #include "efx_cxl.h"
>> +#include "efx.h"
>>   
>>   #define EFX_CTPIO_BUFFER_SIZE	SZ_256M
>>   
>>   static void efx_release_cxl_region(void *priv_cxl)
>>   {
>>   	struct efx_probe_data *probe_data = priv_cxl;
>> +	struct efx_nic *efx = &probe_data->efx;
>>   	struct efx_cxl *cxl = probe_data->cxl;
>>   
>> +	/* Next avoid contention with efx_cxl_exit() */
>>   	probe_data->cxl_pio_initialised = false;
>> +
>> +	/* Next makes cxl-based piobus to no be used */
>> +	efx_ef10_disable_piobufs(efx);
>>   	iounmap(cxl->ctpio_cxl);
>> +
> Avoid extra white space changes. Perhaps push to earlier patch.


I'll fix the spaces. Not sure what you mean with the second part of your 
comment, but if I understand it right, I think those changes should be 
added in this patch, just when the final functionality is added.


FWIW, I have decided to drop this driver callback as Dan did not like 
it, and after realizing those Dan's patches this patchset relies on fix 
most of the problem this callback tried to address.


>>   	cxl_put_root_decoder(cxl->cxlrd);
>>   }
>>   
>> @@ -30,6 +37,7 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>>   	struct pci_dev *pci_dev = efx->pci_dev;
>>   	resource_size_t max_size;
>>   	struct efx_cxl *cxl;
>> +	struct range range;
>>   	u16 dvsec;
>>   	int rc;
>>   
>> @@ -133,17 +141,34 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>>   					    &probe_data);
>>   	if (IS_ERR(cxl->efx_region)) {
>>   		pci_err(pci_dev, "CXL accel create region failed");
>> -		cxl_dpa_free(cxl->cxled);
>>   		rc = PTR_ERR(cxl->efx_region);
>> -		goto err_decoder;
>> +		goto err_dpa;
> Why do we now need to call cxl_dpa_free() and didn't previously here? That
> seems like a probably bug in earlier patch.


I think you misread it. We were calling cxl_dpa_free already, just 
moving it to a goto label here.


Thanks!


>> +	}
>> +
>> +	rc = cxl_get_region_range(cxl->efx_region, &range);
>> +	if (rc) {
>> +		pci_err(pci_dev, "CXL getting regions params failed");
>> +		goto err_detach;
>> +	}
>> +
>> +	cxl->ctpio_cxl = ioremap(range.start, range.end - range.start + 1);
>> +	if (!cxl->ctpio_cxl) {
>> +		pci_err(pci_dev, "CXL ioremap region (%pra) failed", &range);
>> +		rc = -ENOMEM;
>> +		goto err_detach;
>>   	}
>>   
>>   	probe_data->cxl = cxl;
>> +	probe_data->cxl_pio_initialised = true;
>>   
>>   	cxl_release_endpoint(cxl->cxlmd, cxl->endpoint);
>>   
>>   	return 0;
>>   
>> +err_detach:
>> +	cxl_decoder_detach(NULL, cxl->cxled, 0, DETACH_INVALIDATE);
>> +err_dpa:
>> +	cxl_dpa_free(cxl->cxled);
>>   err_decoder:
>>   	cxl_put_root_decoder(cxl->cxlrd);
>>   err_release:
>> @@ -154,7 +179,8 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>>   
>>   void efx_cxl_exit(struct efx_probe_data *probe_data)
>>   {
>> -	if (probe_data->cxl) {
>> +	if (probe_data->cxl_pio_initialised) {
>> +		iounmap(probe_data->cxl->ctpio_cxl);
>>   		cxl_decoder_detach(NULL, probe_data->cxl->cxled, 0,
>>   				   DETACH_INVALIDATE);
>>   		cxl_dpa_free(probe_data->cxl->cxled);