Re: [PATCH net 1/2] amd-xgbe: Fix data loss when RX checksum offloading is disabled

["Badole, Vishal" <vishal.badole@amd.com>]

On 4/14/2025 8:04 PM, Lendacky, Thomas wrote:
> On 4/14/25 07:02, Vishal Badole wrote:
>> To properly disable checksum offloading, the split header mode must also
>> be disabled. When split header mode is disabled, the network device stores
>> received packets (with size <= 1536 bytes) entirely in buffer1, leaving
>> buffer2 empty. However, with the current DMA configuration, only 256 bytes
>> from buffer1 are copied from the network device to system memory,
>> resulting in the loss of the remaining packet data.
>>
>> Address the issue by programming the ARBS field to 256 bytes, which aligns
>> with the socket buffer size, and setting the SPH bit in the control
>> register to disable split header mode. With this configuration, the
>> network device stores the first 256 bytes of the received packet in
>> buffer1 and the remaining data in buffer2. The DMA is then able to
>> transfer the full packet from the network device to system memory without
>> any data loss.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: c5aa9e3b8156 ("amd-xgbe: Initial AMD 10GbE platform driver")
> 
> Arguably, this patch doesn't fix anything as all you've done is defined
> some routines that aren't called by anything yet. You can probably
> combine this with the actual fix in the next patch and make this a
> single patch.
> 
Sure, Thomas, we will combine these patches into a single patch and 
resend it with version V2.
> Split-header support wasn't added until commit 174fd2597b0b ("amd-xgbe:
> Implement split header receive support") and receive side scaling wasn't
> added until commit 5b9dfe299e55 ("amd-xgbe: Provide support for receive
> side scaling"), so you'll want to double check your Fixes: tag.
> 
Thank you for your observation. We will recheck the Fixes: tag and 
update it in the V2 patch version.
> Also, the ARBS field wasn't always present, so you might need to
> investigate that this is truly backwards compatible and earlier versions
> don't require a different fix.
We will update the patch and resend it with the necessary changes to 
support earlier versions of the hardware as well.
> 
> Thanks,
> Tom
> 
>> Signed-off-by: Vishal Badole <Vishal.Badole@amd.com>
>> ---
>>   drivers/net/ethernet/amd/xgbe/xgbe-common.h |  2 ++
>>   drivers/net/ethernet/amd/xgbe/xgbe-dev.c    | 18 ++++++++++++++++++
>>   drivers/net/ethernet/amd/xgbe/xgbe.h        |  5 +++++
>>   3 files changed, 25 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-common.h b/drivers/net/ethernet/amd/xgbe/xgbe-common.h
>> index bcb221f74875..d92453ee2505 100644
>> --- a/drivers/net/ethernet/amd/xgbe/xgbe-common.h
>> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-common.h
>> @@ -232,6 +232,8 @@
>>   #define DMA_CH_IER_TIE_WIDTH		1
>>   #define DMA_CH_IER_TXSE_INDEX		1
>>   #define DMA_CH_IER_TXSE_WIDTH		1
>> +#define DMA_CH_RCR_ARBS_INDEX		28
>> +#define DMA_CH_RCR_ARBS_WIDTH		3
>>   #define DMA_CH_RCR_PBL_INDEX		16
>>   #define DMA_CH_RCR_PBL_WIDTH		6
>>   #define DMA_CH_RCR_RBSZ_INDEX		1
>> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
>> index 7a923b6e83df..429c5e1444d8 100644
>> --- a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
>> +++ b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
>> @@ -292,6 +292,8 @@ static void xgbe_config_rx_buffer_size(struct xgbe_prv_data *pdata)
>>   
>>   		XGMAC_DMA_IOWRITE_BITS(pdata->channel[i], DMA_CH_RCR, RBSZ,
>>   				       pdata->rx_buf_size);
>> +		XGMAC_DMA_IOWRITE_BITS(pdata->channel[i], DMA_CH_RCR, ARBS,
>> +				       XGBE_ARBS_SIZE);
>>   	}
>>   }
>>   
>> @@ -321,6 +323,18 @@ static void xgbe_config_sph_mode(struct xgbe_prv_data *pdata)
>>   	XGMAC_IOWRITE_BITS(pdata, MAC_RCR, HDSMS, XGBE_SPH_HDSMS_SIZE);
>>   }
>>   
>> +static void xgbe_disable_sph_mode(struct xgbe_prv_data *pdata)
>> +{
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < pdata->channel_count; i++) {
>> +		if (!pdata->channel[i]->rx_ring)
>> +			break;
>> +
>> +		XGMAC_DMA_IOWRITE_BITS(pdata->channel[i], DMA_CH_CR, SPH, 0);
>> +	}
>> +}
>> +
>>   static int xgbe_write_rss_reg(struct xgbe_prv_data *pdata, unsigned int type,
>>   			      unsigned int index, unsigned int val)
>>   {
>> @@ -3910,5 +3924,9 @@ void xgbe_init_function_ptrs_dev(struct xgbe_hw_if *hw_if)
>>   	hw_if->disable_vxlan = xgbe_disable_vxlan;
>>   	hw_if->set_vxlan_id = xgbe_set_vxlan_id;
>>   
>> +	/* For Split Header*/
>> +	hw_if->enable_sph = xgbe_config_sph_mode;
>> +	hw_if->disable_sph = xgbe_disable_sph_mode;
>> +
>>   	DBGPR("<--xgbe_init_function_ptrs\n");
>>   }
>> diff --git a/drivers/net/ethernet/amd/xgbe/xgbe.h b/drivers/net/ethernet/amd/xgbe/xgbe.h
>> index db73c8f8b139..1b9c679453fb 100755
>> --- a/drivers/net/ethernet/amd/xgbe/xgbe.h
>> +++ b/drivers/net/ethernet/amd/xgbe/xgbe.h
>> @@ -166,6 +166,7 @@
>>   #define XGBE_RX_BUF_ALIGN	64
>>   #define XGBE_SKB_ALLOC_SIZE	256
>>   #define XGBE_SPH_HDSMS_SIZE	2	/* Keep in sync with SKB_ALLOC_SIZE */
>> +#define XGBE_ARBS_SIZE	        3
>>   
>>   #define XGBE_MAX_DMA_CHANNELS	16
>>   #define XGBE_MAX_QUEUES		16
>> @@ -902,6 +903,10 @@ struct xgbe_hw_if {
>>   	void (*enable_vxlan)(struct xgbe_prv_data *);
>>   	void (*disable_vxlan)(struct xgbe_prv_data *);
>>   	void (*set_vxlan_id)(struct xgbe_prv_data *);
>> +
>> +	/* For Split Header */
>> +	void (*enable_sph)(struct xgbe_prv_data *pdata);
>> +	void (*disable_sph)(struct xgbe_prv_data *pdata);
>>   };
>>   
>>   /* This structure represents implementation specific routines for an