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

Alejandro Lucero Palau
2024-08-19T16:29:32Z

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