[PATCH 0/3] net: xilinx: axienet: Partial checksum offload improvements

[PATCH 0/3] net: xilinx: axienet: Partial checksum offload improvements

[Sean Anderson]

Partial checksum offload is not always used when it could be. Enable it
in more cases.


Sean Anderson (3):
  net: xilinx: axienet: Remove unused checksum variables
  net: xilinx: axienet: Enable NETIF_F_HW_CSUM for partial tx
    checksumming
  net: xilinx: axienet: Relax partial rx checksum checks

 drivers/net/ethernet/xilinx/xilinx_axienet.h  |  5 -----
 .../net/ethernet/xilinx/xilinx_axienet_main.c | 20 +++----------------
 2 files changed, 3 insertions(+), 22 deletions(-)

-- 
2.35.1.1320.gc452695387.dirty

[PATCH 1/3] net: xilinx: axienet: Remove unused checksum variables

[Sean Anderson]

These variables are set but never used. Remove them

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/net/ethernet/xilinx/xilinx_axienet.h      |  5 -----
 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 12 ------------
 2 files changed, 17 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h b/drivers/net/ethernet/xilinx/xilinx_axienet.h
index c301dd2ee083..b9d2d7319220 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
@@ -527,8 +527,6 @@ struct skbuf_dma_descriptor {
  *		  supported, the maximum frame size would be 9k. Else it is
  *		  1522 bytes (assuming support for basic VLAN)
  * @rxmem:	Stores rx memory size for jumbo frame handling.
- * @csum_offload_on_tx_path:	Stores the checksum selection on TX side.
- * @csum_offload_on_rx_path:	Stores the checksum selection on RX side.
  * @coalesce_count_rx:	Store the irq coalesce on RX side.
  * @coalesce_usec_rx:	IRQ coalesce delay for RX
  * @coalesce_count_tx:	Store the irq coalesce on TX side.
@@ -606,9 +604,6 @@ struct axienet_local {
 	u32 max_frm_size;
 	u32 rxmem;
 
-	int csum_offload_on_tx_path;
-	int csum_offload_on_rx_path;
-
 	u32 coalesce_count_rx;
 	u32 coalesce_usec_rx;
 	u32 coalesce_count_tx;
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index fe6a0e2e463f..60ec430f3eb0 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -2631,38 +2631,26 @@ static int axienet_probe(struct platform_device *pdev)
 	if (!ret) {
 		switch (value) {
 		case 1:
-			lp->csum_offload_on_tx_path =
-				XAE_FEATURE_PARTIAL_TX_CSUM;
 			lp->features |= XAE_FEATURE_PARTIAL_TX_CSUM;
 			/* Can checksum TCP/UDP over IPv4. */
 			ndev->features |= NETIF_F_IP_CSUM;
 			break;
 		case 2:
-			lp->csum_offload_on_tx_path =
-				XAE_FEATURE_FULL_TX_CSUM;
 			lp->features |= XAE_FEATURE_FULL_TX_CSUM;
 			/* Can checksum TCP/UDP over IPv4. */
 			ndev->features |= NETIF_F_IP_CSUM;
 			break;
-		default:
-			lp->csum_offload_on_tx_path = XAE_NO_CSUM_OFFLOAD;
 		}
 	}
 	ret = of_property_read_u32(pdev->dev.of_node, "xlnx,rxcsum", &value);
 	if (!ret) {
 		switch (value) {
 		case 1:
-			lp->csum_offload_on_rx_path =
-				XAE_FEATURE_PARTIAL_RX_CSUM;
 			lp->features |= XAE_FEATURE_PARTIAL_RX_CSUM;
 			break;
 		case 2:
-			lp->csum_offload_on_rx_path =
-				XAE_FEATURE_FULL_RX_CSUM;
 			lp->features |= XAE_FEATURE_FULL_RX_CSUM;
 			break;
-		default:
-			lp->csum_offload_on_rx_path = XAE_NO_CSUM_OFFLOAD;
 		}
 	}
 	/* For supporting jumbo frames, the Axi Ethernet hardware must have
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH 1/3] net: xilinx: axienet: Remove unused checksum variables

[Simon Horman]

On Tue, Sep 03, 2024 at 02:43:32PM -0400, Sean Anderson wrote:
> These variables are set but never used. Remove them
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>

Reviewed-by: Simon Horman <horms@kernel.org>

RE: [PATCH 1/3] net: xilinx: axienet: Remove unused checksum variables

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Sean Anderson <sean.anderson@linux.dev>
> Sent: Wednesday, September 4, 2024 12:14 AM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; David S .
> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
> netdev@vger.kernel.org
> Cc: Simek, Michal <michal.simek@amd.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Sean Anderson
> <sean.anderson@linux.dev>
> Subject: [PATCH 1/3] net: xilinx: axienet: Remove unused checksum variables
> 
> These variables are set but never used. Remove them
> 

Nit - Missing "." at the end. Also, just curious how you found these unused vars ? 
using some tool or in code walkthrough?

> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>

Rest seems fine.
Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>


> ---
> 
>  drivers/net/ethernet/xilinx/xilinx_axienet.h      |  5 -----
>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 12 ------------
>  2 files changed, 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> index c301dd2ee083..b9d2d7319220 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
> @@ -527,8 +527,6 @@ struct skbuf_dma_descriptor {
>   *		  supported, the maximum frame size would be 9k. Else it is
>   *		  1522 bytes (assuming support for basic VLAN)
>   * @rxmem:	Stores rx memory size for jumbo frame handling.
> - * @csum_offload_on_tx_path:	Stores the checksum selection on TX
> side.
> - * @csum_offload_on_rx_path:	Stores the checksum selection on RX
> side.
>   * @coalesce_count_rx:	Store the irq coalesce on RX side.
>   * @coalesce_usec_rx:	IRQ coalesce delay for RX
>   * @coalesce_count_tx:	Store the irq coalesce on TX side.
> @@ -606,9 +604,6 @@ struct axienet_local {
>  	u32 max_frm_size;
>  	u32 rxmem;
> 
> -	int csum_offload_on_tx_path;
> -	int csum_offload_on_rx_path;
> -
>  	u32 coalesce_count_rx;
>  	u32 coalesce_usec_rx;
>  	u32 coalesce_count_tx;
> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> index fe6a0e2e463f..60ec430f3eb0 100644
> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
> @@ -2631,38 +2631,26 @@ static int axienet_probe(struct platform_device
> *pdev)
>  	if (!ret) {
>  		switch (value) {
>  		case 1:
> -			lp->csum_offload_on_tx_path =
> -				XAE_FEATURE_PARTIAL_TX_CSUM;
>  			lp->features |= XAE_FEATURE_PARTIAL_TX_CSUM;
>  			/* Can checksum TCP/UDP over IPv4. */
>  			ndev->features |= NETIF_F_IP_CSUM;
>  			break;
>  		case 2:
> -			lp->csum_offload_on_tx_path =
> -				XAE_FEATURE_FULL_TX_CSUM;
>  			lp->features |= XAE_FEATURE_FULL_TX_CSUM;
>  			/* Can checksum TCP/UDP over IPv4. */
>  			ndev->features |= NETIF_F_IP_CSUM;
>  			break;
> -		default:
> -			lp->csum_offload_on_tx_path =
> XAE_NO_CSUM_OFFLOAD;
>  		}
>  	}
>  	ret = of_property_read_u32(pdev->dev.of_node, "xlnx,rxcsum",
> &value);
>  	if (!ret) {
>  		switch (value) {
>  		case 1:
> -			lp->csum_offload_on_rx_path =
> -				XAE_FEATURE_PARTIAL_RX_CSUM;
>  			lp->features |= XAE_FEATURE_PARTIAL_RX_CSUM;
>  			break;
>  		case 2:
> -			lp->csum_offload_on_rx_path =
> -				XAE_FEATURE_FULL_RX_CSUM;
>  			lp->features |= XAE_FEATURE_FULL_RX_CSUM;
>  			break;
> -		default:
> -			lp->csum_offload_on_rx_path =
> XAE_NO_CSUM_OFFLOAD;
>  		}
>  	}
>  	/* For supporting jumbo frames, the Axi Ethernet hardware must
> have
> --
> 2.35.1.1320.gc452695387.dirty

Re: [PATCH 1/3] net: xilinx: axienet: Remove unused checksum variables

[Sean Anderson]

On 9/4/24 13:03, Pandey, Radhey Shyam wrote:
>> -----Original Message-----
>> From: Sean Anderson <sean.anderson@linux.dev>
>> Sent: Wednesday, September 4, 2024 12:14 AM
>> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; David S .
>> Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
>> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
>> netdev@vger.kernel.org
>> Cc: Simek, Michal <michal.simek@amd.com>; linux-arm-
>> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Sean Anderson
>> <sean.anderson@linux.dev>
>> Subject: [PATCH 1/3] net: xilinx: axienet: Remove unused checksum variables
>> 
>> These variables are set but never used. Remove them
>> 
> 
> Nit - Missing "." at the end.

Will fix if I send a v2.

> Also, just curious how you found these unused vars ? 
> using some tool or in code walkthrough?

I read the code and noticed it :)

--Sean

>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> 
> Rest seems fine.
> Reviewed-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> 
> 
>> ---
>> 
>>  drivers/net/ethernet/xilinx/xilinx_axienet.h      |  5 -----
>>  drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 12 ------------
>>  2 files changed, 17 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> b/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> index c301dd2ee083..b9d2d7319220 100644
>> --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h
>> @@ -527,8 +527,6 @@ struct skbuf_dma_descriptor {
>>   *		  supported, the maximum frame size would be 9k. Else it is
>>   *		  1522 bytes (assuming support for basic VLAN)
>>   * @rxmem:	Stores rx memory size for jumbo frame handling.
>> - * @csum_offload_on_tx_path:	Stores the checksum selection on TX
>> side.
>> - * @csum_offload_on_rx_path:	Stores the checksum selection on RX
>> side.
>>   * @coalesce_count_rx:	Store the irq coalesce on RX side.
>>   * @coalesce_usec_rx:	IRQ coalesce delay for RX
>>   * @coalesce_count_tx:	Store the irq coalesce on TX side.
>> @@ -606,9 +604,6 @@ struct axienet_local {
>>  	u32 max_frm_size;
>>  	u32 rxmem;
>> 
>> -	int csum_offload_on_tx_path;
>> -	int csum_offload_on_rx_path;
>> -
>>  	u32 coalesce_count_rx;
>>  	u32 coalesce_usec_rx;
>>  	u32 coalesce_count_tx;
>> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> index fe6a0e2e463f..60ec430f3eb0 100644
>> --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>> @@ -2631,38 +2631,26 @@ static int axienet_probe(struct platform_device
>> *pdev)
>>  	if (!ret) {
>>  		switch (value) {
>>  		case 1:
>> -			lp->csum_offload_on_tx_path =
>> -				XAE_FEATURE_PARTIAL_TX_CSUM;
>>  			lp->features |= XAE_FEATURE_PARTIAL_TX_CSUM;
>>  			/* Can checksum TCP/UDP over IPv4. */
>>  			ndev->features |= NETIF_F_IP_CSUM;
>>  			break;
>>  		case 2:
>> -			lp->csum_offload_on_tx_path =
>> -				XAE_FEATURE_FULL_TX_CSUM;
>>  			lp->features |= XAE_FEATURE_FULL_TX_CSUM;
>>  			/* Can checksum TCP/UDP over IPv4. */
>>  			ndev->features |= NETIF_F_IP_CSUM;
>>  			break;
>> -		default:
>> -			lp->csum_offload_on_tx_path =
>> XAE_NO_CSUM_OFFLOAD;
>>  		}
>>  	}
>>  	ret = of_property_read_u32(pdev->dev.of_node, "xlnx,rxcsum",
>> &value);
>>  	if (!ret) {
>>  		switch (value) {
>>  		case 1:
>> -			lp->csum_offload_on_rx_path =
>> -				XAE_FEATURE_PARTIAL_RX_CSUM;
>>  			lp->features |= XAE_FEATURE_PARTIAL_RX_CSUM;
>>  			break;
>>  		case 2:
>> -			lp->csum_offload_on_rx_path =
>> -				XAE_FEATURE_FULL_RX_CSUM;
>>  			lp->features |= XAE_FEATURE_FULL_RX_CSUM;
>>  			break;
>> -		default:
>> -			lp->csum_offload_on_rx_path =
>> XAE_NO_CSUM_OFFLOAD;
>>  		}
>>  	}
>>  	/* For supporting jumbo frames, the Axi Ethernet hardware must
>> have
>> --
>> 2.35.1.1320.gc452695387.dirty
> 

[PATCH 2/3] net: xilinx: axienet: Enable NETIF_F_HW_CSUM for partial tx checksumming

[Sean Anderson]

Partial tx chechsumming is completely generic and does not depend on the
L3/L4 protocol. Signal this to the net subsystem by enabling the
more-generic offload feature (instead of restricting ourselves to
TCP/UDP over IPv4 checksumming only like is necessary with full
checksumming).

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 60ec430f3eb0..74fade5a95c2 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -2632,8 +2632,8 @@ static int axienet_probe(struct platform_device *pdev)
 		switch (value) {
 		case 1:
 			lp->features |= XAE_FEATURE_PARTIAL_TX_CSUM;
-			/* Can checksum TCP/UDP over IPv4. */
-			ndev->features |= NETIF_F_IP_CSUM;
+			/* Can checksum any contiguous range */
+			ndev->features |= NETIF_F_HW_CSUM;
 			break;
 		case 2:
 			lp->features |= XAE_FEATURE_FULL_TX_CSUM;
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH 2/3] net: xilinx: axienet: Enable NETIF_F_HW_CSUM for partial tx checksumming

[Simon Horman]

On Tue, Sep 03, 2024 at 02:43:33PM -0400, Sean Anderson wrote:
> Partial tx chechsumming is completely generic and does not depend on the
> L3/L4 protocol. Signal this to the net subsystem by enabling the
> more-generic offload feature (instead of restricting ourselves to
> TCP/UDP over IPv4 checksumming only like is necessary with full
> checksumming).
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH 3/3] net: xilinx: axienet: Relax partial rx checksum checks

[Sean Anderson]

The partial rx checksum feature computes a checksum over the entire
packet, regardless of the L3 protocol. Remove the check for IPv4.
Additionally, packets under 64 bytes should have been dropped by the
MAC, so we can remove the length check as well.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
index 74fade5a95c2..99d08a775520 100644
--- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
+++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
@@ -1188,9 +1188,7 @@ static int axienet_rx_poll(struct napi_struct *napi, int budget)
 				    csumstatus == XAE_IP_UDP_CSUM_VALIDATED) {
 					skb->ip_summed = CHECKSUM_UNNECESSARY;
 				}
-			} else if ((lp->features & XAE_FEATURE_PARTIAL_RX_CSUM) != 0 &&
-				   skb->protocol == htons(ETH_P_IP) &&
-				   skb->len > 64) {
+			} else if (lp->features & XAE_FEATURE_PARTIAL_RX_CSUM) {
 				skb->csum = be32_to_cpu(cur_p->app3 & 0xFFFF);
 				skb->ip_summed = CHECKSUM_COMPLETE;
 			}
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH 3/3] net: xilinx: axienet: Relax partial rx checksum checks

[Simon Horman]

On Tue, Sep 03, 2024 at 02:43:34PM -0400, Sean Anderson wrote:
> The partial rx checksum feature computes a checksum over the entire
> packet, regardless of the L3 protocol. Remove the check for IPv4.
> Additionally, packets under 64 bytes should have been dropped by the
> MAC, so we can remove the length check as well.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH 3/3] net: xilinx: axienet: Relax partial rx checksum checks

[Eric Dumazet]

On Tue, Sep 3, 2024 at 8:43 PM Sean Anderson <sean.anderson@linux.dev> wrote:
>
> The partial rx checksum feature computes a checksum over the entire
> packet, regardless of the L3 protocol. Remove the check for IPv4.
> Additionally, packets under 64 bytes should have been dropped by the
> MAC, so we can remove the length check as well.

Some packets have a smaller len (than 64).

For instance, TCP pure ACK and no options over IPv4 would be 54 bytes long.

Presumably they are not dropped by the MAC ?

Re: [PATCH 3/3] net: xilinx: axienet: Relax partial rx checksum checks

[Sean Anderson]

On 9/4/24 12:30, Eric Dumazet wrote:
> On Tue, Sep 3, 2024 at 8:43 PM Sean Anderson <sean.anderson@linux.dev> wrote:
>>
>> The partial rx checksum feature computes a checksum over the entire
>> packet, regardless of the L3 protocol. Remove the check for IPv4.
>> Additionally, packets under 64 bytes should have been dropped by the
>> MAC, so we can remove the length check as well.
> 
> Some packets have a smaller len (than 64).
> 
> For instance, TCP pure ACK and no options over IPv4 would be 54 bytes long.
> 
> Presumably they are not dropped by the MAC ?

Ethernet frames have a minimum size on the wire of 64 bytes. From 802.3
section 4.2.4.2.2:

| The shortest valid transmission in full duplex mode must be at least
| minFrameSize in length. While collisions do not occur in full duplex
| mode MACs, a full duplex MAC nevertheless discards received frames
| containing less than minFrameSize bits. The discarding of such a frame
| by a MAC is not reported as an error.

where minFrameSize is 512 bits (64 bytes).

On the transmit side, undersize frames are padded. From 802.3 section
4.2.3.3:

| The CSMA/CD Media Access mechanism requires that a minimum frame
| length of minFrameSize bits be transmitted. If frameSize is less than
| minFrameSize, then the CSMA/CD MAC sublayer shall append extra bits in
| units of octets (Pad), after the end of the MAC Client Data field but
| prior to calculating and appending the FCS (if not provided by the MAC
| client).

That said, I could not find any mention of a minimum frame size
limitation for partial checksums in the AXI Ethernet documentation.
RX_CSRAW is calculated over the whole packet, so it's possible that this
check is trying to avoid passing it to the net subsystem when the frame
has been padded. However, skb->len is the length of the Ethernet packet,
so we can't tell how long the original packet was at this point. That
can only be determined from the L3 header, which isn't parsed yet. I
assume this is handled by the net subsystem.

--Sean

Re: [PATCH 3/3] net: xilinx: axienet: Relax partial rx checksum checks

[Eric Dumazet]

On Thu, Sep 5, 2024 at 4:24 PM Sean Anderson <sean.anderson@linux.dev> wrote:
>
> On 9/4/24 12:30, Eric Dumazet wrote:
> > On Tue, Sep 3, 2024 at 8:43 PM Sean Anderson <sean.anderson@linux.dev> wrote:
> >>
> >> The partial rx checksum feature computes a checksum over the entire
> >> packet, regardless of the L3 protocol. Remove the check for IPv4.
> >> Additionally, packets under 64 bytes should have been dropped by the
> >> MAC, so we can remove the length check as well.
> >
> > Some packets have a smaller len (than 64).
> >
> > For instance, TCP pure ACK and no options over IPv4 would be 54 bytes long.
> >
> > Presumably they are not dropped by the MAC ?
>
> Ethernet frames have a minimum size on the wire of 64 bytes. From 802.3
> section 4.2.4.2.2:
>
> | The shortest valid transmission in full duplex mode must be at least
> | minFrameSize in length. While collisions do not occur in full duplex
> | mode MACs, a full duplex MAC nevertheless discards received frames
> | containing less than minFrameSize bits. The discarding of such a frame
> | by a MAC is not reported as an error.
>
> where minFrameSize is 512 bits (64 bytes).
>
> On the transmit side, undersize frames are padded. From 802.3 section
> 4.2.3.3:
>
> | The CSMA/CD Media Access mechanism requires that a minimum frame
> | length of minFrameSize bits be transmitted. If frameSize is less than
> | minFrameSize, then the CSMA/CD MAC sublayer shall append extra bits in
> | units of octets (Pad), after the end of the MAC Client Data field but
> | prior to calculating and appending the FCS (if not provided by the MAC
> | client).
>
> That said, I could not find any mention of a minimum frame size
> limitation for partial checksums in the AXI Ethernet documentation.
> RX_CSRAW is calculated over the whole packet, so it's possible that this
> check is trying to avoid passing it to the net subsystem when the frame
> has been padded. However, skb->len is the length of the Ethernet packet,
> so we can't tell how long the original packet was at this point. That
> can only be determined from the L3 header, which isn't parsed yet. I
> assume this is handled by the net subsystem.
>

The fact there was a check in the driver hints about something.

It is possible the csum is incorrect if a 'padding' is added at the
receiver, if the padding has non zero bytes, and is not included in
the csum.

Look at this relevant patch :

Author: Saeed Mahameed <saeedm@mellanox.com>
Date:   Mon Feb 11 18:04:17 2019 +0200

    net/mlx4_en: Force CHECKSUM_NONE for short ethernet frames

Re: [PATCH 3/3] net: xilinx: axienet: Relax partial rx checksum checks

[Sean Anderson]

On 9/5/24 10:59, Eric Dumazet wrote:
> On Thu, Sep 5, 2024 at 4:24 PM Sean Anderson <sean.anderson@linux.dev> wrote:
>>
>> On 9/4/24 12:30, Eric Dumazet wrote:
>> > On Tue, Sep 3, 2024 at 8:43 PM Sean Anderson <sean.anderson@linux.dev> wrote:
>> >>
>> >> The partial rx checksum feature computes a checksum over the entire
>> >> packet, regardless of the L3 protocol. Remove the check for IPv4.
>> >> Additionally, packets under 64 bytes should have been dropped by the
>> >> MAC, so we can remove the length check as well.
>> >
>> > Some packets have a smaller len (than 64).
>> >
>> > For instance, TCP pure ACK and no options over IPv4 would be 54 bytes long.
>> >
>> > Presumably they are not dropped by the MAC ?
>>
>> Ethernet frames have a minimum size on the wire of 64 bytes. From 802.3
>> section 4.2.4.2.2:
>>
>> | The shortest valid transmission in full duplex mode must be at least
>> | minFrameSize in length. While collisions do not occur in full duplex
>> | mode MACs, a full duplex MAC nevertheless discards received frames
>> | containing less than minFrameSize bits. The discarding of such a frame
>> | by a MAC is not reported as an error.
>>
>> where minFrameSize is 512 bits (64 bytes).
>>
>> On the transmit side, undersize frames are padded. From 802.3 section
>> 4.2.3.3:
>>
>> | The CSMA/CD Media Access mechanism requires that a minimum frame
>> | length of minFrameSize bits be transmitted. If frameSize is less than
>> | minFrameSize, then the CSMA/CD MAC sublayer shall append extra bits in
>> | units of octets (Pad), after the end of the MAC Client Data field but
>> | prior to calculating and appending the FCS (if not provided by the MAC
>> | client).
>>
>> That said, I could not find any mention of a minimum frame size
>> limitation for partial checksums in the AXI Ethernet documentation.
>> RX_CSRAW is calculated over the whole packet, so it's possible that this
>> check is trying to avoid passing it to the net subsystem when the frame
>> has been padded. However, skb->len is the length of the Ethernet packet,
>> so we can't tell how long the original packet was at this point. That
>> can only be determined from the L3 header, which isn't parsed yet. I
>> assume this is handled by the net subsystem.
>>
> 
> The fact there was a check in the driver hints about something.
> 
> It is possible the csum is incorrect if a 'padding' is added at the
> receiver, if the padding has non zero bytes, and is not included in
> the csum.
> 
> Look at this relevant patch :
> 
> Author: Saeed Mahameed <saeedm@mellanox.com>
> Date:   Mon Feb 11 18:04:17 2019 +0200
> 
>     net/mlx4_en: Force CHECKSUM_NONE for short ethernet frames

Well, I tested UDP and it appears to be working fine. First I ran

# nc -lu

on the DUT. On the other host I used scapy to send a packet with some
non-zero padding:

  >>> port = RandShort()
  >>> send(IP(dst="10.0.0.2")/UDP(sport=port, dport=4444)/Raw(b'data\r\n')/Padding(load=b'padding'))

I verified that the packet was received correctly, both in netcat and
with tcpdump:

    # tcpdump -i net4 -xXn 
    tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
    listening on net4, link-type EN10MB (Ethernet), snapshot length 262144 bytes
    16:07:45.083795 IP 10.0.0.1.27365 > 10.0.0.2.4444: UDP, length 6
            0x0000:  4500 0022 0001 0000 4011 66c8 0a00 0001  E.."....@.f.....
            0x0010:  0a00 0002 6ae5 115c 000e 0005 6461 7461  ....j..\....data
            0x0020:  0d0a 7061 6464 696e 6700 0000 0000       ..padding.....

and also checked for checksum errors:

  # netstat -s | grep InCsumErrors
      InCsumErrors: 0

to verify that checksums were being checked properly, I also sent a
packet with an invalid checksum:

  >>> send(IP(dst="10.0.0.2")/UDP(sport=port, dport=4444, chksum=5)/Raw(b'data\r\n')/Padding(load=b'padding'))

and confirmed that there was no output on netcat, and that I had gotten
a UDP checksum error:

  # netstat -s | grep InCsumErrors
      InCsumErrors: 1

I can try to test TCP as well, but it is a bit trickier due to the 3-way
handshake. From the documentation, partial checksums should be agnostic
to the L3 protocol, so I don't think there should be any difference.

--Sean

Re: [PATCH 3/3] net: xilinx: axienet: Relax partial rx checksum checks

[Sean Anderson]

Hi Eric,

On 9/5/24 12:32, Sean Anderson wrote:
> On 9/5/24 10:59, Eric Dumazet wrote:
>> On Thu, Sep 5, 2024 at 4:24 PM Sean Anderson <sean.anderson@linux.dev> wrote:
>>>
>>> On 9/4/24 12:30, Eric Dumazet wrote:
>>> > On Tue, Sep 3, 2024 at 8:43 PM Sean Anderson <sean.anderson@linux.dev> wrote:
>>> >>
>>> >> The partial rx checksum feature computes a checksum over the entire
>>> >> packet, regardless of the L3 protocol. Remove the check for IPv4.
>>> >> Additionally, packets under 64 bytes should have been dropped by the
>>> >> MAC, so we can remove the length check as well.
>>> >
>>> > Some packets have a smaller len (than 64).
>>> >
>>> > For instance, TCP pure ACK and no options over IPv4 would be 54 bytes long.
>>> >
>>> > Presumably they are not dropped by the MAC ?
>>>
>>> Ethernet frames have a minimum size on the wire of 64 bytes. From 802.3
>>> section 4.2.4.2.2:
>>>
>>> | The shortest valid transmission in full duplex mode must be at least
>>> | minFrameSize in length. While collisions do not occur in full duplex
>>> | mode MACs, a full duplex MAC nevertheless discards received frames
>>> | containing less than minFrameSize bits. The discarding of such a frame
>>> | by a MAC is not reported as an error.
>>>
>>> where minFrameSize is 512 bits (64 bytes).
>>>
>>> On the transmit side, undersize frames are padded. From 802.3 section
>>> 4.2.3.3:
>>>
>>> | The CSMA/CD Media Access mechanism requires that a minimum frame
>>> | length of minFrameSize bits be transmitted. If frameSize is less than
>>> | minFrameSize, then the CSMA/CD MAC sublayer shall append extra bits in
>>> | units of octets (Pad), after the end of the MAC Client Data field but
>>> | prior to calculating and appending the FCS (if not provided by the MAC
>>> | client).
>>>
>>> That said, I could not find any mention of a minimum frame size
>>> limitation for partial checksums in the AXI Ethernet documentation.
>>> RX_CSRAW is calculated over the whole packet, so it's possible that this
>>> check is trying to avoid passing it to the net subsystem when the frame
>>> has been padded. However, skb->len is the length of the Ethernet packet,
>>> so we can't tell how long the original packet was at this point. That
>>> can only be determined from the L3 header, which isn't parsed yet. I
>>> assume this is handled by the net subsystem.
>>>
>> 
>> The fact there was a check in the driver hints about something.
>> 
>> It is possible the csum is incorrect if a 'padding' is added at the
>> receiver, if the padding has non zero bytes, and is not included in
>> the csum.
>> 
>> Look at this relevant patch :
>> 
>> Author: Saeed Mahameed <saeedm@mellanox.com>
>> Date:   Mon Feb 11 18:04:17 2019 +0200
>> 
>>     net/mlx4_en: Force CHECKSUM_NONE for short ethernet frames
> 
> Well, I tested UDP and it appears to be working fine. First I ran
> 
> # nc -lu
> 
> on the DUT. On the other host I used scapy to send a packet with some
> non-zero padding:
> 
>   >>> port = RandShort()
>   >>> send(IP(dst="10.0.0.2")/UDP(sport=port, dport=4444)/Raw(b'data\r\n')/Padding(load=b'padding'))
> 
> I verified that the packet was received correctly, both in netcat and
> with tcpdump:
> 
>     # tcpdump -i net4 -xXn 
>     tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
>     listening on net4, link-type EN10MB (Ethernet), snapshot length 262144 bytes
>     16:07:45.083795 IP 10.0.0.1.27365 > 10.0.0.2.4444: UDP, length 6
>             0x0000:  4500 0022 0001 0000 4011 66c8 0a00 0001  E.."....@.f.....
>             0x0010:  0a00 0002 6ae5 115c 000e 0005 6461 7461  ....j..\....data
>             0x0020:  0d0a 7061 6464 696e 6700 0000 0000       ..padding.....
> 
> and also checked for checksum errors:
> 
>   # netstat -s | grep InCsumErrors
>       InCsumErrors: 0
> 
> to verify that checksums were being checked properly, I also sent a
> packet with an invalid checksum:
> 
>   >>> send(IP(dst="10.0.0.2")/UDP(sport=port, dport=4444, chksum=5)/Raw(b'data\r\n')/Padding(load=b'padding'))
> 
> and confirmed that there was no output on netcat, and that I had gotten
> a UDP checksum error:
> 
>   # netstat -s | grep InCsumErrors
>       InCsumErrors: 1
> 
> I can try to test TCP as well, but it is a bit trickier due to the 3-way
> handshake. From the documentation, partial checksums should be agnostic
> to the L3 protocol, so I don't think there should be any difference.
> 
> --Sean

I saw that there was a checksum selftest today, so I went back and ran
that as well. I managed to get it to pass:

# NETIF=net LOCAL_V4=10.0.0.1 LOCAL_V6=fc00::1 REMOTE_V4=10.0.0.2 REMOTE_V6=fc00::2 REMOTE_TYPE=netns REMOTE_ARGS=ns2 ip netns exec ns1 kselftest_install/drivers/net/hw/csum.py
KTAP version 1
1..12
ok 1 csum.ipv4_rx_tcp
ok 2 csum.ipv4_rx_tcp_invalid
ok 3 csum.ipv4_rx_udp
ok 4 csum.ipv4_rx_udp_invalid
ok 5 csum.ipv4_tx_udp_csum_offload
ok 6 csum.ipv4_tx_udp_zero_checksum
ok 7 csum.ipv6_rx_tcp
ok 8 csum.ipv6_rx_tcp_invalid
ok 9 csum.ipv6_rx_udp
ok 10 csum.ipv6_rx_udp_invalid
ok 11 csum.ipv6_tx_udp_csum_offload
ok 12 csum.ipv6_tx_udp_zero_checksum
# Totals: pass:12 fail:0 xfail:0 xpass:0 skip:0 error:0

But ended up having to modify the test [1] to handle exactly this
situation (but in the test's reference checksum). I also had to add
another patch to set NETIF_F_RXCSUM for this driver. I think this shows
that there should be no hardware issue with removing the length check.
I'll send a v2 on Monday with the RXCSUM patch unless you have any
objections.

--Sean

[1] https://lore.kernel.org/netdev/20240906210743.627413-1-sean.anderson@linux.dev

Re: [PATCH 0/3] net: xilinx: axienet: Partial checksum offload improvements

[Sean Anderson]

On 9/3/24 14:43, Sean Anderson wrote:
> Partial checksum offload is not always used when it could be. Enable it
> in more cases.
> 
> 
> Sean Anderson (3):
>   net: xilinx: axienet: Remove unused checksum variables
>   net: xilinx: axienet: Enable NETIF_F_HW_CSUM for partial tx
>     checksumming
>   net: xilinx: axienet: Relax partial rx checksum checks
> 
>  drivers/net/ethernet/xilinx/xilinx_axienet.h  |  5 -----
>  .../net/ethernet/xilinx/xilinx_axienet_main.c | 20 +++----------------
>  2 files changed, 3 insertions(+), 22 deletions(-)
> 

Sorry I forgot to add this to the subject, but it should be for net-next.
I can resend if necessary.

--Sean