Re: [PATCH v2 15/15] efx: support pio mapping based on cxl

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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);
> +		}