| Message ID | 20241209185429.54054-24-alejandro.lucero-palau@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: add type2 device basic support | expand |
On 09/12/2024 18:54, alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.com> > > Use cxl api for creating a region using the endpoint decoder related to > a DPA range. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com> Acked-by: Edward Cree <ecree.xilinx@gmail.com> > --- > drivers/net/ethernet/sfc/efx_cxl.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c > index 09827bb9e861..9b34795f7853 100644 > --- a/drivers/net/ethernet/sfc/efx_cxl.c > +++ b/drivers/net/ethernet/sfc/efx_cxl.c > @@ -128,10 +128,19 @@ int efx_cxl_init(struct efx_probe_data *probe_data) > goto err3; > } > > + cxl->efx_region = cxl_create_region(cxl->cxlrd, cxl->cxled); > + if (!cxl->efx_region) { > + pci_err(pci_dev, "CXL accel create region failed"); > + rc = PTR_ERR(cxl->efx_region); > + goto err_region; Little bit weird to have this mixture of numbered and named labels in the failure ladder. Ideally I'd prefer the named kind throughout, in which case e.g. err3: would become three labels called something like err_memdev:, err_freespace:, and err_dpa:, all pointing to the same stmt (the cxl_release_resource call). But up to you whether you want to bother. > + } > + > probe_data->cxl = cxl; > > return 0; > > +err_region: > + cxl_dpa_free(cxl->cxled); > err3: > cxl_release_resource(cxl->cxlds, CXL_RES_RAM); > err2:
On 12/11/24 02:26, Edward Cree wrote: > On 09/12/2024 18:54, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alucerop@amd.com> >> >> Use cxl api for creating a region using the endpoint decoder related to >> a DPA range. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com> > Acked-by: Edward Cree <ecree.xilinx@gmail.com> > >> --- >> drivers/net/ethernet/sfc/efx_cxl.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c >> index 09827bb9e861..9b34795f7853 100644 >> --- a/drivers/net/ethernet/sfc/efx_cxl.c >> +++ b/drivers/net/ethernet/sfc/efx_cxl.c >> @@ -128,10 +128,19 @@ int efx_cxl_init(struct efx_probe_data *probe_data) >> goto err3; >> } >> >> + cxl->efx_region = cxl_create_region(cxl->cxlrd, cxl->cxled); >> + if (!cxl->efx_region) { >> + pci_err(pci_dev, "CXL accel create region failed"); >> + rc = PTR_ERR(cxl->efx_region); >> + goto err_region; > Little bit weird to have this mixture of numbered and named labels in the > failure ladder. Ideally I'd prefer the named kind throughout, in which > case e.g. err3: would become three labels called something like > err_memdev:, err_freespace:, and err_dpa:, all pointing to the same stmt > (the cxl_release_resource call). > But up to you whether you want to bother. > I agree it is not ideal. I will try to use better labeling. Thanks. >> + } >> + >> probe_data->cxl = cxl; >> >> return 0; >> >> +err_region: >> + cxl_dpa_free(cxl->cxled); >> err3: >> cxl_release_resource(cxl->cxlds, CXL_RES_RAM); >> err2:
On Mon, Dec 09, 2024 at 06:54:24PM +0000, alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.com> > > Use cxl api for creating a region using the endpoint decoder related to > a DPA range. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com> > --- > drivers/net/ethernet/sfc/efx_cxl.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c > index 09827bb9e861..9b34795f7853 100644 > --- a/drivers/net/ethernet/sfc/efx_cxl.c > +++ b/drivers/net/ethernet/sfc/efx_cxl.c > @@ -128,10 +128,19 @@ int efx_cxl_init(struct efx_probe_data *probe_data) > goto err3; > } > > + cxl->efx_region = cxl_create_region(cxl->cxlrd, cxl->cxled); > + if (!cxl->efx_region) { > + pci_err(pci_dev, "CXL accel create region failed"); > + rc = PTR_ERR(cxl->efx_region); > + goto err_region; Hi Alejandro, This is similar to my feedback on patch 18/28. Looking over the implementation of cxl_create_region it seems that it returns either a valid pointer or an error pointer, but not NULL. If so, I think the correct condition would be (completely untested): if (IS_ERR(cxl->efx_region) But if cxl->efx_region can be NULL then rc, the return value of this function, will be set to zero. Which doesn't seem correct given the error message above. > + } > + > probe_data->cxl = cxl; > > return 0; > > +err_region: > + cxl_dpa_free(cxl->cxled); > err3: > cxl_release_resource(cxl->cxlds, CXL_RES_RAM); > err2: > @@ -144,6 +153,7 @@ int efx_cxl_init(struct efx_probe_data *probe_data) > void efx_cxl_exit(struct efx_probe_data *probe_data) > { > if (probe_data->cxl) { > + cxl_accel_region_detach(probe_data->cxl->cxled); > cxl_dpa_free(probe_data->cxl->cxled); > cxl_release_resource(probe_data->cxl->cxlds, CXL_RES_RAM); > kfree(probe_data->cxl->cxlds); > -- > 2.17.1 > >
On 12/12/24 18:29, Simon Horman wrote: > On Mon, Dec 09, 2024 at 06:54:24PM +0000, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alucerop@amd.com> >> >> Use cxl api for creating a region using the endpoint decoder related to >> a DPA range. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com> >> --- >> drivers/net/ethernet/sfc/efx_cxl.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c >> index 09827bb9e861..9b34795f7853 100644 >> --- a/drivers/net/ethernet/sfc/efx_cxl.c >> +++ b/drivers/net/ethernet/sfc/efx_cxl.c >> @@ -128,10 +128,19 @@ int efx_cxl_init(struct efx_probe_data *probe_data) >> goto err3; >> } >> >> + cxl->efx_region = cxl_create_region(cxl->cxlrd, cxl->cxled); >> + if (!cxl->efx_region) { >> + pci_err(pci_dev, "CXL accel create region failed"); >> + rc = PTR_ERR(cxl->efx_region); >> + goto err_region; > Hi Alejandro, > > This is similar to my feedback on patch 18/28. > > Looking over the implementation of cxl_create_region it seems > that it returns either a valid pointer or an error pointer, but > not NULL. If so, I think the correct condition would be > (completely untested): > > if (IS_ERR(cxl->efx_region) > > But if cxl->efx_region can be NULL then rc, the return value of this > function, will be set to zero. Which doesn't seem correct given > the error message above. I'll fix it with the IS_ERR check which seems the proper thing to do here. Thanks! >> + } >> + >> probe_data->cxl = cxl; >> >> return 0; >> >> +err_region: >> + cxl_dpa_free(cxl->cxled); >> err3: >> cxl_release_resource(cxl->cxlds, CXL_RES_RAM); >> err2: >> @@ -144,6 +153,7 @@ int efx_cxl_init(struct efx_probe_data *probe_data) >> void efx_cxl_exit(struct efx_probe_data *probe_data) >> { >> if (probe_data->cxl) { >> + cxl_accel_region_detach(probe_data->cxl->cxled); >> cxl_dpa_free(probe_data->cxl->cxled); >> cxl_release_resource(probe_data->cxl->cxlds, CXL_RES_RAM); >> kfree(probe_data->cxl->cxlds); >> -- >> 2.17.1 >> >>
diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c index 09827bb9e861..9b34795f7853 100644 --- a/drivers/net/ethernet/sfc/efx_cxl.c +++ b/drivers/net/ethernet/sfc/efx_cxl.c @@ -128,10 +128,19 @@ int efx_cxl_init(struct efx_probe_data *probe_data) goto err3; } + cxl->efx_region = cxl_create_region(cxl->cxlrd, cxl->cxled); + if (!cxl->efx_region) { + pci_err(pci_dev, "CXL accel create region failed"); + rc = PTR_ERR(cxl->efx_region); + goto err_region; + } + probe_data->cxl = cxl; return 0; +err_region: + cxl_dpa_free(cxl->cxled); err3: cxl_release_resource(cxl->cxlds, CXL_RES_RAM); err2: @@ -144,6 +153,7 @@ int efx_cxl_init(struct efx_probe_data *probe_data) void efx_cxl_exit(struct efx_probe_data *probe_data) { if (probe_data->cxl) { + cxl_accel_region_detach(probe_data->cxl->cxled); cxl_dpa_free(probe_data->cxl->cxled); cxl_release_resource(probe_data->cxl->cxlds, CXL_RES_RAM); kfree(probe_data->cxl->cxlds);