diff mbox series

[net,v3,4/4] net: ethernet: cortina: Checksum only TCP and UDP

Message ID 20231107-gemini-largeframe-fix-v3-4-e3803c080b75@linaro.org
State New
Headers
Series Fix large frames in the Gemini ethernet driver |

Commit Message

Linus Walleij Nov. 7, 2023, 9:54 a.m. UTC 
  It is a bit odd that frames that are neither TCP or UDP
(such as ICMP or ARP) are still sent to the checksumming
engine, where they are clearly just ignored.

Rewrite the logic slightly so that we first check if the
frame is TCP or UDP, in that case bypass the checksum
engine.

Reported-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/ethernet/cortina/gemini.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

Comments

Vladimir Oltean Nov. 8, 2023, 3:19 p.m. UTC | #1 
On Tue, Nov 07, 2023 at 10:54:29AM +0100, Linus Walleij wrote:
> It is a bit odd that frames that are neither TCP or UDP
> (such as ICMP or ARP) are still sent to the checksumming
> engine, where they are clearly just ignored.
> 
> Rewrite the logic slightly so that we first check if the
> frame is TCP or UDP, in that case bypass the checksum
> engine.
> 
> Reported-by: Vladimir Oltean <olteanv@gmail.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

If this patch doesn't fix anything and isn't a dependency for anything,
it shouldn't be present in a series targeted for "net". And anyway, I
think it's not needed in general after the discussion on v2.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
index 78287cfcbf63..1bf07505653b 100644
--- a/drivers/net/ethernet/cortina/gemini.c
+++ b/drivers/net/ethernet/cortina/gemini.c
@@ -1144,6 +1144,7 @@  static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
 	skb_frag_t *skb_frag;
 	dma_addr_t mapping;
 	unsigned short mtu;
+	bool tcp, udp;
 	void *buffer;
 	int ret;
 
@@ -1160,7 +1161,18 @@  static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
 		word3 |= mtu;
 	}
 
-	if (skb->len >= ETH_FRAME_LEN) {
+	/* Check if the protocol is TCP or UDP */
+	tcp = false;
+	udp = false;
+	if (skb->protocol == htons(ETH_P_IP)) {
+		tcp = ip_hdr(skb)->protocol == IPPROTO_TCP;
+		udp = ip_hdr(skb)->protocol == IPPROTO_UDP;
+	} else { /* IPv6 */
+		tcp = ipv6_hdr(skb)->nexthdr == IPPROTO_TCP;
+		udp = ipv6_hdr(skb)->nexthdr == IPPROTO_UDP;
+	}
+
+	if (skb->len >= ETH_FRAME_LEN || (!tcp && !udp)) {
 		/* Hardware offloaded checksumming isn't working on frames
 		 * bigger than 1514 bytes. A hypothesis about this is that the
 		 * checksum buffer is only 1518 bytes, so when the frames get
@@ -1168,6 +1180,9 @@  static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
 		 * overwritten by the FCS.
 		 *
 		 * Just use software checksumming and bypass on bigger frames.
+		 *
+		 * Bypass the checksumming engine for any protocols that are
+		 * not TCP or UDP.
 		 */
 		if (skb->ip_summed == CHECKSUM_PARTIAL) {
 			ret = skb_checksum_help(skb);
@@ -1176,22 +1191,14 @@  static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
 		}
 		word1 |= TSS_BYPASS_BIT;
 	} else if (skb->ip_summed == CHECKSUM_PARTIAL) {
-		int tcp = 0;
-
-		/* We do not switch off the checksumming on non TCP/UDP
-		 * frames: as is shown from tests, the checksumming engine
-		 * is smart enough to see that a frame is not actually TCP
-		 * or UDP and then just pass it through without any changes
-		 * to the frame.
+		/* If we get here we are dealing with a TCP or UDP frame
+		 * which is small enough to be processed by the checkumming
+		 * engine.
 		 */
-		if (skb->protocol == htons(ETH_P_IP)) {
+		if (skb->protocol == htons(ETH_P_IP))
 			word1 |= TSS_IP_CHKSUM_BIT;
-			tcp = ip_hdr(skb)->protocol == IPPROTO_TCP;
-		} else { /* IPv6 */
+		else
 			word1 |= TSS_IPV6_ENABLE_BIT;
-			tcp = ipv6_hdr(skb)->nexthdr == IPPROTO_TCP;
-		}
-
 		word1 |= tcp ? TSS_TCP_CHKSUM_BIT : TSS_UDP_CHKSUM_BIT;
 	}