From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id BB004C4332F for ; Wed, 8 Nov 2023 15:28:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=2nBpxragxpB7KZacv4YLOWSmcB6dqaqyjwgz9IJEqig=; b=1XhM1AZcyLVsB2 hjS3DSYMlKQD4olMcJO5AETTqRIsrTA0pnrNa7v6WjU9GX+2DvyRFgyS8iehZ5X4VGJnY0Xu6g7Mo Kaq9i+ET+xT9vseFyCRMD5Tocl8d17X1NkM2pFbIQKJV2GTVnNUCr8LfjTojVygNW9VTFS/IKHBv3 mVOK3K8S2KkZG2cS7hLnwwVGuIhrOL+VxOqm6+PwpqobYtGhZ5OUgLsHMNZExOxzhEQk1++jRbZ+L 7BcpeUyi44rGy0xL31Twom0gxFUlrRLdSRGLFRfpaiRPioIyB11TONteii9hoirNS8Oy0ikfWn8bU /FBIJOD7ppSyNMgfQ8fw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1r0kT3-004Bjq-1g; Wed, 08 Nov 2023 15:27:45 +0000 Received: from mail-ej1-x634.google.com ([2a00:1450:4864:20::634]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1r0kT0-004BjG-18 for linux-arm-kernel@lists.infradead.org; Wed, 08 Nov 2023 15:27:44 +0000 Received: by mail-ej1-x634.google.com with SMTP id a640c23a62f3a-9d2d8343dc4so1072606366b.0 for ; Wed, 08 Nov 2023 07:27:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1699457259; x=1700062059; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=GbKer+D8Q+KHc3RawBywAbRhnUO2N5YC2mknZI5eaAk=; b=gSFcnEZWFGyHObvdqEPXjmMEw+hcNuVjQhtQpqt5+w8V7y8JD+rxlxWvX65sOBizgT UpOBFDrGojrIZNzH6evFEbeordAjLa505EZ89VjY5CtriYbOag1aMs2Icy/zJ8PjsDie VdpmRX3K4Vs8yWHazYtqEAHmxIkPxqbV0+THUYJNv+WOUHpbcyTQ8pytYlM8VDN1LZlX 6ovo+3jp8PhuSc/PAk/ZNBX1AjtFgOygtLy73GHcXZeuXER/PLfoch2WMLzv1MOm9ozs X/69O5Z2GvktU5nk/yWG3HmOThNyTPnacjTvq/SENTbxke1320kgA2CpMT1ct5nHPDOw RatA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699457259; x=1700062059; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=GbKer+D8Q+KHc3RawBywAbRhnUO2N5YC2mknZI5eaAk=; b=SVI/n6ejVIEpVeuEZs8OlK2UURiOoIkRHIx1hDR0hvngXlO9lZiZPvejqYD6QxEDGu H9F/suTGYz5Ib8qubozwIdJcRRn8oNMM7beMM7vuhZQNN/qSry/JfJoH78xdsdLuzAcA x8pFlBtASljSix/xRve26P1JZNYwiC8BUZN+Rvq8g8W5gbyP65bjIaWBuVXVqUBwLMci e4IUdnimOVgXkIJq4m2VdxkaoHQzByx3+ncvxjBAMgSiMtj9XyI4z+8WC245nnzAsnK0 /6IeEiTBu/QarXVMijWcVA50Ty3kvT18yDXMn9+x5aqxjCDwkHXa4O3jjU9jhsB71fNY 2tBg== X-Gm-Message-State: AOJu0YxjtSQ0iinH6bwAq6/mG2vM7gpneKvTzGlAEg1UTr1T5KBXwnGR S5UitwQ5Wzhy1+JFqmqhAXA= X-Google-Smtp-Source: AGHT+IGEH7nKLCeA0lDhAwUUQXgn2UhS9sWmWknjymraKE1HIbs3PeGN8y0LRZvFF007+cX+OctJsw== X-Received: by 2002:a17:907:6d20:b0:9be:aebc:d479 with SMTP id sa32-20020a1709076d2000b009beaebcd479mr1813981ejc.19.1699457258882; Wed, 08 Nov 2023 07:27:38 -0800 (PST) Received: from skbuf ([188.26.57.160]) by smtp.gmail.com with ESMTPSA id z17-20020a170906669100b009920e9a3a73sm1203843ejo.115.2023.11.08.07.27.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 08 Nov 2023 07:27:38 -0800 (PST) Date: Wed, 8 Nov 2023 17:27:36 +0200 From: Vladimir Oltean To: Linus Walleij Cc: Hans Ulli Kroll , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , =?utf-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= , Andrew Lunn , linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH net v3 3/4] net: ethernet: cortina: Handle large frames Message-ID: <20231108152736.euqbedurxvq2wben@skbuf> References: <20231107-gemini-largeframe-fix-v3-0-e3803c080b75@linaro.org> <20231107-gemini-largeframe-fix-v3-3-e3803c080b75@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231107-gemini-largeframe-fix-v3-3-e3803c080b75@linaro.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231108_072742_394859_881CB2AC X-CRM114-Status: GOOD ( 44.64 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Nov 07, 2023 at 10:54:28AM +0100, Linus Walleij wrote: > The Gemini ethernet controller provides hardware checksumming > for frames up to 1514 bytes including ethernet headers but not > FCS. > > If we start sending bigger frames (after first bumping up the MTU > on both interfaces sending and receiveing the frames), truncated s/receiveing/receiving/ > packets start to appear on the target such as in this tcpdump > resulting from ping -s 1474: > > 23:34:17.241983 14:d6:4d:a8:3c:4f (oui Unknown) > bc:ae:c5:6b:a8:3d (oui Unknown), > ethertype IPv4 (0x0800), length 1514: truncated-ip - 2 bytes missing! > (tos 0x0, ttl 64, id 32653, offset 0, flags [DF], proto ICMP (1), length 1502) > OpenWrt.lan > Fecusia: ICMP echo request, id 1672, seq 50, length 1482 > > If we bypass the hardware checksumming and provide a software > fallback, everything starts working fine up to the max TX MTU > of 2047 bytes, for example ping -s2000 192.168.1.2: > > 00:44:29.587598 bc:ae:c5:6b:a8:3d (oui Unknown) > 14:d6:4d:a8:3c:4f (oui Unknown), > ethertype IPv4 (0x0800), length 2042: > (tos 0x0, ttl 64, id 51828, offset 0, flags [none], proto ICMP (1), length 2028) > Fecusia > OpenWrt.lan: ICMP echo reply, id 1683, seq 4, length 2008 > > The bit enabling to bypass hardware checksum (or any of the > "TSS" bits) are undocumented in the hardware reference manual. > The entire hardware checksum unit appears undocumented. The > conclusion that we need to use the "bypass" bit was found by > trial-and-error. > > Since no hardware checksum will happen, we slot in a software > checksum fallback. > > Check for the condition where we need to compute checksum on the > skb with either hardware or software using == CHECKSUM_PARTIAL instead > of != CHECKSUM_NONE which is an incomplete check according to > . > > We delete the code disabling the hardware checksum for large > MTU:s: this is suboptimal because it will disable hardware "MTUs" maybe? > checksumming also on small packets which the checksumming > engine can handle just fine, which is a waste of resources. > > On the D-Link DIR-685 router this fixes a bug on the conduit > interface to the RTL8366RB DSA switch: as the switch needs to add > space for its tag it increases the MTU on the conduit interface > to 1504 and that means that when the router sends packages > of 1500 bytes these get an extra 4 bytes of DSA tag and the > transfer fails because of the erroneous hardware checksumming, > affecting such basic functionality as the LuCI web interface. > > Suggested-by: Vladimir Oltean > Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet") > Signed-off-by: Linus Walleij > --- > drivers/net/ethernet/cortina/gemini.c | 34 +++++++++++++++++++++++----------- > 1 file changed, 23 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c > index b21a94b4ab5c..78287cfcbf63 100644 > --- a/drivers/net/ethernet/cortina/gemini.c > +++ b/drivers/net/ethernet/cortina/gemini.c > @@ -1145,6 +1145,7 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, > dma_addr_t mapping; > unsigned short mtu; > void *buffer; > + int ret; > > mtu = ETH_HLEN; > mtu += netdev->mtu; > @@ -1159,9 +1160,30 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, > word3 |= mtu; > } > > - if (skb->ip_summed != CHECKSUM_NONE) { > + if (skb->len >= ETH_FRAME_LEN) { > + /* 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 > + * bigger they get truncated, or the last few bytes get > + * overwritten by the FCS. > + * > + * Just use software checksumming and bypass on bigger frames. > + */ > + if (skb->ip_summed == CHECKSUM_PARTIAL) { > + ret = skb_checksum_help(skb); > + if (ret) > + return ret; > + } > + 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 (skb->protocol == htons(ETH_P_IP)) { > word1 |= TSS_IP_CHKSUM_BIT; > tcp = ip_hdr(skb)->protocol == IPPROTO_TCP; > @@ -1978,15 +2000,6 @@ static int gmac_change_mtu(struct net_device *netdev, int new_mtu) > return 0; > } > > -static netdev_features_t gmac_fix_features(struct net_device *netdev, > - netdev_features_t features) > -{ > - if (netdev->mtu + ETH_HLEN + VLAN_HLEN > MTU_SIZE_BIT_MASK) > - features &= ~GMAC_OFFLOAD_FEATURES; > - > - return features; > -} > - I think this entire ndo_fix_features() can be indeed removed, but your justification was not immediately convincing. I'd point out that after your patch 1/4 "net: ethernet: cortina: Fix MTU max setting", you actually made this dead code, because netdev->mtu can't be larger than netdev->max_mtu. If you reverse the patch order a bit, such that "net: ethernet: cortina: Handle large frames" comes first, I think it would be much more logical for the removal of gmac_fix_features() to be part of the commit "net: ethernet: cortina: Fix MTU max setting", with the simple justification: the new MTU makes the code stop having any role. > static int gmac_set_features(struct net_device *netdev, > netdev_features_t features) > { > @@ -2212,7 +2225,6 @@ static const struct net_device_ops gmac_351x_ops = { > .ndo_set_mac_address = gmac_set_mac_address, > .ndo_get_stats64 = gmac_get_stats64, > .ndo_change_mtu = gmac_change_mtu, > - .ndo_fix_features = gmac_fix_features, > .ndo_set_features = gmac_set_features, > }; > > > -- > 2.34.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel