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 (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Fix large frames in the Gemini ethernet driver | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1312 this patch: 1312
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 1340 this patch: 1340
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1340 this patch: 1340
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 62 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

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;
 	}