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

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Fri, 26 Sep 2025 10:47:27 +0100
Alejandro Lucero Palau <alucerop@amd.com> wrote:

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

Just the white space.  If you want that move it to where that iounmap() is
added.  This is just a patch cleanliness thing.

> 
> 
> 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.
> 
Indeed.  Needed more coffee that day (or less, can't remember which ;)