| Message ID | 20241209185429.54054-19-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 getting DPA (Device Physical Address) to use through an > endpoint decoder. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com> Acked-by: Edward Cree <ecree.xilinx@gmail.com> > + cxl->cxled = cxl_request_dpa(cxl->cxlmd, true, EFX_CTPIO_BUFFER_SIZE, > + EFX_CTPIO_BUFFER_SIZE); Just for my education, could you explain what's the difference between cxl_dpa_alloc(..., size) and cxl_request_dpa(..., size, size)? Since you're not really making use of the min/max flexibility here, I assume there's some other reason for using the latter. Is it just that it's a convenience wrapper that saves open-coding some of the boilerplate?
On 12/11/24 00:25, 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 getting DPA (Device Physical Address) to use through an >> endpoint decoder. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com> > Acked-by: Edward Cree <ecree.xilinx@gmail.com> Thanks >> + cxl->cxled = cxl_request_dpa(cxl->cxlmd, true, EFX_CTPIO_BUFFER_SIZE, >> + EFX_CTPIO_BUFFER_SIZE); > Just for my education, could you explain what's the difference between > cxl_dpa_alloc(..., size) and cxl_request_dpa(..., size, size)? Since > you're not really making use of the min/max flexibility here, I assume > there's some other reason for using the latter. Is it just that it's > a convenience wrapper that saves open-coding some of the boilerplate? A device advertises CXL memory but the Host being able to map it depends on the current state of CXL configuration regarding CXL decoders in the path to the device and the request itself. Our case is simple since we rely on having all that memory mapped or not having it at all. And we count on our device being directly connected to the CXL Root Complex or to one of the CXL Root Complex available, but CXL switches and things like memory interleaving can make things far more complex. About min and max, the former specifies the minimal driver requirements for mapping CXL memory, and the latter tells about the maximal size the driver could be interested in. I think this is more a legacy of how Type3 devices are managed and arguably not so important for accelerators, and for some as ours, not needed.
On Mon, Dec 09, 2024 at 06:54:19PM +0000, alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.com> > > Use cxl api for getting DPA (Device Physical Address) to use through an > endpoint decoder. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com> > --- > drivers/net/ethernet/sfc/efx_cxl.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c > index f2dc025c9fbb..09827bb9e861 100644 > --- a/drivers/net/ethernet/sfc/efx_cxl.c > +++ b/drivers/net/ethernet/sfc/efx_cxl.c > @@ -120,6 +120,14 @@ int efx_cxl_init(struct efx_probe_data *probe_data) > goto err3; > } > > + cxl->cxled = cxl_request_dpa(cxl->cxlmd, true, EFX_CTPIO_BUFFER_SIZE, > + EFX_CTPIO_BUFFER_SIZE); > + if (IS_ERR_OR_NULL(cxl->cxled)) { Hi Alejandro, The Kernel doc for cxl_request_dpa says that it returns either a valid pointer or an error pointer. So perhaps IS_ERR() is sufficient here. > + pci_err(pci_dev, "CXL accel request DPA failed"); > + rc = PTR_ERR(cxl->cxlrd); Otherwise, if cxl->cxlrd is NULL here, then rc, the return value for this function, will be 0 here. Which doesn't seem to align with the error message on the line above. Flagged by Smatch. > + goto err3; > + } > + > probe_data->cxl = cxl; > > return 0; > @@ -136,6 +144,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_dpa_free(probe_data->cxl->cxled); > cxl_release_resource(probe_data->cxl->cxlds, CXL_RES_RAM); > kfree(probe_data->cxl->cxlds); > kfree(probe_data->cxl); > -- > 2.17.1 > >
On 12/12/24 18:21, Simon Horman wrote: > On Mon, Dec 09, 2024 at 06:54:19PM +0000, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alucerop@amd.com> >> >> Use cxl api for getting DPA (Device Physical Address) to use through an >> endpoint decoder. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> Reviewed-by: Martin Habets <habetsm.xilinx@gmail.com> >> --- >> drivers/net/ethernet/sfc/efx_cxl.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c >> index f2dc025c9fbb..09827bb9e861 100644 >> --- a/drivers/net/ethernet/sfc/efx_cxl.c >> +++ b/drivers/net/ethernet/sfc/efx_cxl.c >> @@ -120,6 +120,14 @@ int efx_cxl_init(struct efx_probe_data *probe_data) >> goto err3; >> } >> >> + cxl->cxled = cxl_request_dpa(cxl->cxlmd, true, EFX_CTPIO_BUFFER_SIZE, >> + EFX_CTPIO_BUFFER_SIZE); >> + if (IS_ERR_OR_NULL(cxl->cxled)) { > Hi Alejandro, > > The Kernel doc for cxl_request_dpa says that it returns either > a valid pointer or an error pointer. So perhaps IS_ERR() is > sufficient here. > >> + pci_err(pci_dev, "CXL accel request DPA failed"); >> + rc = PTR_ERR(cxl->cxlrd); > Otherwise, if cxl->cxlrd is NULL here, then rc, the return value for > this function, will be 0 here. Which doesn't seem to align with the > error message on the line above. Indisputable. I'll fix it. Thanks! > Flagged by Smatch. > >> + goto err3; >> + } >> + >> probe_data->cxl = cxl; >> >> return 0; >> @@ -136,6 +144,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_dpa_free(probe_data->cxl->cxled); >> cxl_release_resource(probe_data->cxl->cxlds, CXL_RES_RAM); >> kfree(probe_data->cxl->cxlds); >> kfree(probe_data->cxl); >> -- >> 2.17.1 >> >>
diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c index f2dc025c9fbb..09827bb9e861 100644 --- a/drivers/net/ethernet/sfc/efx_cxl.c +++ b/drivers/net/ethernet/sfc/efx_cxl.c @@ -120,6 +120,14 @@ int efx_cxl_init(struct efx_probe_data *probe_data) goto err3; } + cxl->cxled = cxl_request_dpa(cxl->cxlmd, true, EFX_CTPIO_BUFFER_SIZE, + EFX_CTPIO_BUFFER_SIZE); + if (IS_ERR_OR_NULL(cxl->cxled)) { + pci_err(pci_dev, "CXL accel request DPA failed"); + rc = PTR_ERR(cxl->cxlrd); + goto err3; + } + probe_data->cxl = cxl; return 0; @@ -136,6 +144,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_dpa_free(probe_data->cxl->cxled); cxl_release_resource(probe_data->cxl->cxlds, CXL_RES_RAM); kfree(probe_data->cxl->cxlds); kfree(probe_data->cxl);