| Message ID | 20240715172835.24757-16-alejandro.lucero-palau@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | cxl: add Type2 device support | expand |
On Mon, 15 Jul 2024 18:28:35 +0100 alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.com> > > With a device supporting CXL and successfully initialised, use the cxl > region to map the memory range and use this mapping for PIO buffers. This explains why you weren't worried about any step of the CXL code failing and why that wasn't a 'bug' as such. I'd argue that you should still have the cxl intialization return an error code and cleanup any state it if hits an error. Then the top level driver can of course elect to use an alternative path given that failure. Logically it belongs there rather than relying on a buffer being mapped or not. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > --- > drivers/net/ethernet/sfc/ef10.c | 25 +++++++++++++++++++++---- > drivers/net/ethernet/sfc/efx_cxl.c | 12 +++++++++++- > drivers/net/ethernet/sfc/mcdi_pcol.h | 3 +++ > drivers/net/ethernet/sfc/nic.h | 1 + > 4 files changed, 36 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c > index 8fa6c0e9195b..3924076d2628 100644 > --- a/drivers/net/ethernet/sfc/ef10.c > +++ b/drivers/net/ethernet/sfc/ef10.c > @@ -24,6 +24,7 @@ > #include <linux/wait.h> > #include <linux/workqueue.h> > #include <net/udp_tunnel.h> > +#include "efx_cxl.h" > > /* Hardware control for EF10 architecture including 'Huntington'. */ > > @@ -177,6 +178,12 @@ static int efx_ef10_init_datapath_caps(struct efx_nic *efx) > efx->num_mac_stats); > } > > + if (outlen < MC_CMD_GET_CAPABILITIES_V7_OUT_LEN) > + nic_data->datapath_caps3 = 0; > + else > + nic_data->datapath_caps3 = MCDI_DWORD(outbuf, > + GET_CAPABILITIES_V7_OUT_FLAGS3); > + > return 0; > } > > @@ -1275,10 +1282,20 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx) > return -ENOMEM; > } > nic_data->pio_write_vi_base = pio_write_vi_base; > - nic_data->pio_write_base = > - nic_data->wc_membase + > - (pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF - > - uc_mem_map_size); > + > + if ((nic_data->datapath_caps3 & > + (1 << MC_CMD_GET_CAPABILITIES_V10_OUT_CXL_CONFIG_ENABLE_LBN)) && > + efx->cxl->ctpio_cxl) As per comment at the top, I'd prefer to see some clean handling of the an error passed up to the caller of the cxl init that then sets a flag that we can clearly see is all about whether we have CXL or not. Using this buffer mapping is a it too much of a detail in my opinion. > + { > + nic_data->pio_write_base = > + efx->cxl->ctpio_cxl + > + (pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF - > + uc_mem_map_size); > + } else { > + nic_data->pio_write_base =nic_data->wc_membase + > + (pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF - > + uc_mem_map_size); > + }
On 8/4/24 19:13, Jonathan Cameron wrote: > On Mon, 15 Jul 2024 18:28:35 +0100 > alejandro.lucero-palau@amd.com wrote: > >> From: Alejandro Lucero <alucerop@amd.com> >> >> With a device supporting CXL and successfully initialised, use the cxl >> region to map the memory range and use this mapping for PIO buffers. > This explains why you weren't worried about any step of the CXL > code failing and why that wasn't a 'bug' as such. > > I'd argue that you should still have the cxl intialization return > an error code and cleanup any state it if hits an error. Ideally, but with devm* being used, this is not easy to do if the error is not fatal. > Then the top level driver can of course elect to use an alternative > path given that failure. Logically it belongs there rather than relying > on a buffer being mapped or not. > Same driver needs to support same functionality which relies on those specific hardware buffers. The functionality is expected to be there with or without CXL. If the hardware has no CXL, the system or the device, the functionality will be there with legacy PCIe BAR regions. The green light for CXL use comes from two sources: the firmware and the kernel. Both need to give the thumbs up. If not, legacy PCIe BAR regions will be used. >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> --- >> drivers/net/ethernet/sfc/ef10.c | 25 +++++++++++++++++++++---- >> drivers/net/ethernet/sfc/efx_cxl.c | 12 +++++++++++- >> drivers/net/ethernet/sfc/mcdi_pcol.h | 3 +++ >> drivers/net/ethernet/sfc/nic.h | 1 + >> 4 files changed, 36 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c >> index 8fa6c0e9195b..3924076d2628 100644 >> --- a/drivers/net/ethernet/sfc/ef10.c >> +++ b/drivers/net/ethernet/sfc/ef10.c >> @@ -24,6 +24,7 @@ >> #include <linux/wait.h> >> #include <linux/workqueue.h> >> #include <net/udp_tunnel.h> >> +#include "efx_cxl.h" >> >> /* Hardware control for EF10 architecture including 'Huntington'. */ >> >> @@ -177,6 +178,12 @@ static int efx_ef10_init_datapath_caps(struct efx_nic *efx) >> efx->num_mac_stats); >> } >> >> + if (outlen < MC_CMD_GET_CAPABILITIES_V7_OUT_LEN) >> + nic_data->datapath_caps3 = 0; >> + else >> + nic_data->datapath_caps3 = MCDI_DWORD(outbuf, >> + GET_CAPABILITIES_V7_OUT_FLAGS3); >> + >> return 0; >> } >> >> @@ -1275,10 +1282,20 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx) >> return -ENOMEM; >> } >> nic_data->pio_write_vi_base = pio_write_vi_base; >> - nic_data->pio_write_base = >> - nic_data->wc_membase + >> - (pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF - >> - uc_mem_map_size); >> + >> + if ((nic_data->datapath_caps3 & >> + (1 << MC_CMD_GET_CAPABILITIES_V10_OUT_CXL_CONFIG_ENABLE_LBN)) && >> + efx->cxl->ctpio_cxl) > As per comment at the top, I'd prefer to see some clean handling of the an > error passed up to the caller of the cxl init that then sets a flag that > we can clearly see is all about whether we have CXL or not. > > Using this buffer mapping is a it too much of a detail in my opinion. Yes, maybe that is clearer than relying on the pointer from the CXL mapping. I will do it. Thanks! >> + { >> + nic_data->pio_write_base = >> + efx->cxl->ctpio_cxl + >> + (pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF - >> + uc_mem_map_size); >> + } else { >> + nic_data->pio_write_base =nic_data->wc_membase + >> + (pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF - >> + uc_mem_map_size); >> + } >
On Mon, 19 Aug 2024 17:28:46 +0100 Alejandro Lucero Palau <alucerop@amd.com> wrote: > On 8/4/24 19:13, Jonathan Cameron wrote: > > On Mon, 15 Jul 2024 18:28:35 +0100 > > alejandro.lucero-palau@amd.com wrote: > > > >> From: Alejandro Lucero <alucerop@amd.com> > >> > >> With a device supporting CXL and successfully initialised, use the cxl > >> region to map the memory range and use this mapping for PIO buffers. > > This explains why you weren't worried about any step of the CXL > > code failing and why that wasn't a 'bug' as such. > > > > I'd argue that you should still have the cxl intialization return > > an error code and cleanup any state it if hits an error. > > > Ideally, but with devm* being used, this is not easy to do if the error > is not fatal. That's usually a strong argument that you shouldn't use devm at that level of abstraction. > > > > Then the top level driver can of course elect to use an alternative > > path given that failure. Logically it belongs there rather than relying > > on a buffer being mapped or not. > > > > Same driver needs to support same functionality which relies on those > specific hardware buffers. > > The functionality is expected to be there with or without CXL. If the > hardware has no CXL, the system or the device, the functionality will be > there with legacy PCIe BAR regions. The green light for CXL use comes > from two sources: the firmware and the kernel. Both need to give the > thumbs up. If not, legacy PCIe BAR regions will be used. Rather than going through full setup, see if you can figure out a minimal (state free) check on whether it should work. If a system is broken, then it's very different from a legacy system with no support for CXL and we can maybe just handle the broken system with errors (or quirks if it's a shipping system). Jonathan >
diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c index 8fa6c0e9195b..3924076d2628 100644 --- a/drivers/net/ethernet/sfc/ef10.c +++ b/drivers/net/ethernet/sfc/ef10.c @@ -24,6 +24,7 @@ #include <linux/wait.h> #include <linux/workqueue.h> #include <net/udp_tunnel.h> +#include "efx_cxl.h" /* Hardware control for EF10 architecture including 'Huntington'. */ @@ -177,6 +178,12 @@ static int efx_ef10_init_datapath_caps(struct efx_nic *efx) efx->num_mac_stats); } + if (outlen < MC_CMD_GET_CAPABILITIES_V7_OUT_LEN) + nic_data->datapath_caps3 = 0; + else + nic_data->datapath_caps3 = MCDI_DWORD(outbuf, + GET_CAPABILITIES_V7_OUT_FLAGS3); + return 0; } @@ -1275,10 +1282,20 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx) return -ENOMEM; } nic_data->pio_write_vi_base = pio_write_vi_base; - nic_data->pio_write_base = - nic_data->wc_membase + - (pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF - - uc_mem_map_size); + + if ((nic_data->datapath_caps3 & + (1 << MC_CMD_GET_CAPABILITIES_V10_OUT_CXL_CONFIG_ENABLE_LBN)) && + efx->cxl->ctpio_cxl) + { + nic_data->pio_write_base = + efx->cxl->ctpio_cxl + + (pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF - + uc_mem_map_size); + } else { + nic_data->pio_write_base =nic_data->wc_membase + + (pio_write_vi_base * efx->vi_stride + ER_DZ_TX_PIOBUF - + uc_mem_map_size); + } rc = efx_ef10_link_piobufs(efx); if (rc) diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c index 4012e3faa298..8e65ef42a572 100644 --- a/drivers/net/ethernet/sfc/efx_cxl.c +++ b/drivers/net/ethernet/sfc/efx_cxl.c @@ -21,8 +21,8 @@ void efx_cxl_init(struct efx_nic *efx) { struct pci_dev *pci_dev = efx->pci_dev; + resource_size_t start, end, max = 0; struct efx_cxl *cxl = efx->cxl; - resource_size_t max = 0; struct resource res; u16 dvsec; @@ -104,6 +104,13 @@ void efx_cxl_init(struct efx_nic *efx) return; } + cxl_accel_get_region_params(cxl->efx_region, &start, &end); + + cxl->ctpio_cxl = ioremap(start, end - start); + if (!cxl->ctpio_cxl) { + pci_info(pci_dev, "CXL accel create region failed"); + cxl_dpa_free(cxl->cxled); + } out: cxl_release_endpoint(cxl->cxlmd, cxl->endpoint); } @@ -112,6 +119,9 @@ void efx_cxl_exit(struct efx_nic *efx) { struct efx_cxl *cxl = efx->cxl; + if (cxl->ctpio_cxl) + iounmap(cxl->ctpio_cxl); + if (cxl->efx_region) cxl_region_detach(cxl->cxled); diff --git a/drivers/net/ethernet/sfc/mcdi_pcol.h b/drivers/net/ethernet/sfc/mcdi_pcol.h index cd297e19cddc..05fd5e021142 100644 --- a/drivers/net/ethernet/sfc/mcdi_pcol.h +++ b/drivers/net/ethernet/sfc/mcdi_pcol.h @@ -18374,6 +18374,9 @@ #define MC_CMD_GET_CAPABILITIES_V10_OUT_DYNAMIC_MPORT_JOURNAL_OFST 148 #define MC_CMD_GET_CAPABILITIES_V10_OUT_DYNAMIC_MPORT_JOURNAL_LBN 14 #define MC_CMD_GET_CAPABILITIES_V10_OUT_DYNAMIC_MPORT_JOURNAL_WIDTH 1 +#define MC_CMD_GET_CAPABILITIES_V10_OUT_CXL_CONFIG_ENABLE_OFST 148 +#define MC_CMD_GET_CAPABILITIES_V10_OUT_CXL_CONFIG_ENABLE_LBN 16 +#define MC_CMD_GET_CAPABILITIES_V10_OUT_CXL_CONFIG_ENABLE_WIDTH 1 /* These bits are reserved for communicating test-specific capabilities to * host-side test software. All production drivers should treat this field as * opaque. diff --git a/drivers/net/ethernet/sfc/nic.h b/drivers/net/ethernet/sfc/nic.h index 1db64fc6e909..cd635f4f7f94 100644 --- a/drivers/net/ethernet/sfc/nic.h +++ b/drivers/net/ethernet/sfc/nic.h @@ -186,6 +186,7 @@ struct efx_ef10_nic_data { bool must_check_datapath_caps; u32 datapath_caps; u32 datapath_caps2; + u32 datapath_caps3; unsigned int rx_dpcpu_fw_id; unsigned int tx_dpcpu_fw_id; bool must_probe_vswitching;