[PATCH net 0/2] amd-xgbe: Fixes for RX checksum offloading and split header mode

[PATCH net 0/2] amd-xgbe: Fixes for RX checksum offloading and split header mode

[Vishal Badole]

Address two issues in the AMD XGBE driver related to checksum offloading
and split header mode. These changes ensure proper handling of dependent
features and resolve data loss caused by mismatched configurations.

- Patch 1/2 resolves an issue where disabling checksum offloading
  with disabling split header mode leads to packet data loss.
- Patch 2/2 ensures that enabling or disabling RX checksum offload
  properly toggles all dependent features to prevents abnormal behavior
  caused by mismatched feature dependencies.

Vishal Badole (2):
  amd-xgbe: Fix data loss when RX checksum offloading is disabled
  amd-xgbe: Fix to ensure dependent features are toggled with RX
    checksum offload

 drivers/net/ethernet/amd/xgbe/xgbe-common.h |  2 ++
 drivers/net/ethernet/amd/xgbe/xgbe-dev.c    | 26 +++++++++++++++++++--
 drivers/net/ethernet/amd/xgbe/xgbe-drv.c    | 19 +++++++++++++--
 drivers/net/ethernet/amd/xgbe/xgbe.h        |  5 ++++
 4 files changed, 48 insertions(+), 4 deletions(-)

-- 
2.34.1

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

[Vishal Badole]

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")
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
-- 
2.34.1

[PATCH net 2/2] amd-xgbe: Fix to ensure dependent features are toggled with RX checksum offload

[Vishal Badole]

According to the XGMAC specification, enabling features such as Layer 3
and Layer 4 Packet Filtering, Split Header, Receive Side Scaling (RSS),
and Virtualized Network support automatically selects the IPC Full
Checksum Offload Engine on the receive side.

When RX checksum offload is disabled, these dependent features must also
be disabled to prevent abnormal behavior caused by mismatched feature
dependencies.

Ensure that toggling RX checksum offload (disabling or enabling) properly
disables or enables all dependent features, maintaining consistent and
expected behavior in the network device.

Cc: stable@vger.kernel.org
Fixes: c5aa9e3b8156 ("amd-xgbe: Initial AMD 10GbE platform driver")
Signed-off-by: Vishal Badole <Vishal.Badole@amd.com>
---
 drivers/net/ethernet/amd/xgbe/xgbe-dev.c |  8 ++++++--
 drivers/net/ethernet/amd/xgbe/xgbe-drv.c | 19 +++++++++++++++++--
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
index 429c5e1444d8..48a474337d96 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-dev.c
@@ -3764,8 +3764,12 @@ static int xgbe_init(struct xgbe_prv_data *pdata)
 	xgbe_config_tx_coalesce(pdata);
 	xgbe_config_rx_buffer_size(pdata);
 	xgbe_config_tso_mode(pdata);
-	xgbe_config_sph_mode(pdata);
-	xgbe_config_rss(pdata);
+
+	if (pdata->netdev->features & NETIF_F_RXCSUM) {
+		xgbe_config_sph_mode(pdata);
+		xgbe_config_rss(pdata);
+	}
+
 	desc_if->wrapper_tx_desc_init(pdata);
 	desc_if->wrapper_rx_desc_init(pdata);
 	xgbe_enable_dma_interrupts(pdata);
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
index f249f89fec38..0146af7f93cd 100755
--- a/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-drv.c
@@ -2267,6 +2267,16 @@ static netdev_features_t xgbe_fix_features(struct net_device *netdev,
 		}
 	}
 
+	if (features & NETIF_F_RXCSUM) {
+		netdev_notice(netdev,
+			      "forcing receive hashing on\n");
+		features |= NETIF_F_RXHASH;
+	} else {
+		netdev_notice(netdev,
+			      "forcing receive hashing off\n");
+		features &= ~NETIF_F_RXHASH;
+	}
+
 	return features;
 }
 
@@ -2290,10 +2300,15 @@ static int xgbe_set_features(struct net_device *netdev,
 	if (ret)
 		return ret;
 
-	if ((features & NETIF_F_RXCSUM) && !rxcsum)
+	if ((features & NETIF_F_RXCSUM) && !rxcsum) {
+		hw_if->enable_sph(pdata);
+		hw_if->enable_vxlan(pdata);
 		hw_if->enable_rx_csum(pdata);
-	else if (!(features & NETIF_F_RXCSUM) && rxcsum)
+	} else if (!(features & NETIF_F_RXCSUM) && rxcsum) {
+		hw_if->disable_sph(pdata);
+		hw_if->disable_vxlan(pdata);
 		hw_if->disable_rx_csum(pdata);
+	}
 
 	if ((features & NETIF_F_HW_VLAN_CTAG_RX) && !rxvlan)
 		hw_if->enable_rx_vlan_stripping(pdata);
-- 
2.34.1

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

[Tom Lendacky]

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.

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.

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.

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

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

[Badole, Vishal]

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