[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250930155105.00001463@huawei.com>
Date: Tue, 30 Sep 2025 15:51:05 +0100
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Alejandro Lucero Palau <alucerop@....com>
CC: <alejandro.lucero-palau@....com>, <linux-cxl@...r.kernel.org>,
<netdev@...r.kernel.org>, <dan.j.williams@...el.com>, <edward.cree@....com>,
<davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
<edumazet@...gle.com>, <dave.jiang@...el.com>
Subject: Re: [PATCH v18 20/20] sfc: support pio mapping based on cxl
On Fri, 26 Sep 2025 10:47:27 +0100
Alejandro Lucero Palau <alucerop@....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 ;)
Powered by blists - more mailing lists