Re: [PATCH 22/22] sfc: support pio mapping based on cxl

[Alejandro Lucero Palau <alucerop@amd.com>]

On 2/13/26 16:14, Gregory Price wrote:
> This review was generated by kreview-0811365ff2.
>
> This is not an automated email, the reviewer though this report
> looked valid enough to consider discussion.
>
> ----
> I am completely unfamiliar with this code, but the question it poses
> at least seems reasonable.


Yes, and again, it makes sense. We have only tried with one VI, so that 
explains why we have not suffered the issue. But it needs to be fixed.


Thanks!


> ~Gregory
> ----
>
> On Sat, Feb 01, 2026, Alejandro Lucero wrote:
>
>> diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
>> index fcec81f862ec..2bb6d3136c7c 100644
>> --- a/drivers/net/ethernet/sfc/ef10.c
>> +++ b/drivers/net/ethernet/sfc/ef10.c
> [ ... ]
>
>> @@ -1263,8 +1281,25 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx)
>>   	iounmap(efx->membase);
>>   	efx->membase = membase;
>>
>> -	/* Set up the WC mapping if needed */
>> -	if (wc_mem_map_size) {
>> +	if (!wc_mem_map_size)
>> +		goto skip_pio;
>> +
>> +	/* Set up the WC mapping */
>> +
>> +#ifdef CONFIG_SFC_CXL
>> +	probe_data = container_of(efx, struct efx_probe_data, efx);
>> +	if ((nic_data->datapath_caps3 &
>> +	    (1 << MC_CMD_GET_CAPABILITIES_V7_OUT_CXL_CONFIG_ENABLE_LBN)) &&
>> +	    probe_data->cxl_pio_initialised) {
>> +		/* Using PIO through CXL mapping? */
>> +		nic_data->pio_write_base = probe_data->cxl->ctpio_cxl +
>> +					   (pio_write_vi_base * efx->vi_stride +
>> +					    ER_DZ_TX_PIOBUF - uc_mem_map_size);
>> +		probe_data->cxl_pio_in_use = true;
>> +	} else
>> +#endif
>> +	{
>> +		/* Using legacy PIO BAR mapping */
>>   		nic_data->wc_membase = ioremap_wc(efx->membase_phys +
>>   						  uc_mem_map_size,
>>   						  wc_mem_map_size);
>> @@ -1279,12 +1314,13 @@ static int efx_ef10_dimension_resources(struct efx_nic *efx)
>>   			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)
>> -			efx_ef10_free_piobufs(efx);
>>   	}
> The CXL path sets nic_data->pio_write_base but does not set
> nic_data->pio_write_vi_base, while the legacy path does:
>
>      nic_data->pio_write_vi_base = pio_write_vi_base;
>
> Since nic_data is kzalloc'd, pio_write_vi_base stays at 0 in the CXL
> path.  efx_ef10_link_piobufs() then uses nic_data->pio_write_vi_base
> to issue MC_CMD_LINK_PIOBUF commands:
>
>      MCDI_SET_DWORD(inbuf, LINK_PIOBUF_IN_TXQ_INSTANCE,
>                     nic_data->pio_write_vi_base + index);
>
> and also for the special-case check:
>
>      if (tx_queue->queue == nic_data->pio_write_vi_base) {
>
> Wouldn't this link PIO buffers to incorrect VI instances when using
> CXL, since the local variable pio_write_vi_base has the correct
> non-zero value but the struct field was never updated?