| Message ID | 20240106-new-gemini-ethernet-regression-v6-1-889e98d3deb7@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ac631873c9e7a50d2a8de457cfc4b9f86666403e |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v6] net: ethernet: cortina: Drop TSO support | expand |
On Sat, Jan 6, 2024 at 1:12 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > The recent change to allow large frames without hardware checksumming > slotted in software checksumming in the driver if hardware could not > do it. > > This will however upset TSO (TCP Segment Offloading). Typical > error dumps includes this: > > skb len=2961 headroom=222 headlen=66 tailroom=0 > (...) > WARNING: CPU: 0 PID: 956 at net/core/dev.c:3259 skb_warn_bad_offload+0x7c/0x108 > gemini-ethernet-port: caps=(0x0000010000154813, 0x00002007ffdd7889) > > And the packets do not go through. > > The TSO implementation is bogus: a TSO enabled driver must propagate > the skb_shinfo(skb)->gso_size value to the TSO engine on the NIC. > > Drop the size check and TSO offloading features for now: this > needs to be fixed up properly. > > After this ethernet works fine on Gemini devices with a direct connected > PHY such as D-Link DNS-313. > > Also tested to still be working with a DSA switch using the Gemini > ethernet as conduit interface. > > Link: https://lore.kernel.org/netdev/CANn89iJLfxng1sYL5Zk0mknXpyYQPCp83m3KgD2KJ2_hKCpEUg@mail.gmail.com/ > Suggested-by: Eric Dumazet <edumazet@google.com> > Fixes: d4d0c5b4d279 ("net: ethernet: cortina: Handle large frames") > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > This fix was developed on top of the earlier fixes. > > Finding the right solution is hard because the Gemini checksumming > engine is completely undocumented in the datasheets. > --- > Changes in v6: > - Keep the software checksum on larger frames, just drop the > TSO support which is bogus anyway. > - Drop the heuristics in the second patch. Just dropping TSO > makes everything work right. > - Drop adding the length in word3. > - Link to v5: https://lore.kernel.org/r/20240102-new-gemini-ethernet-regression-v5-0-cf61ab3aa8cd@linaro.org > > Changes in v5: > - Drop the patch re-implementing eth_header_parse_protocol() > - Link to v4: https://lore.kernel.org/r/20231222-new-gemini-ethernet-regression-v4-0-a36e71b0f32b@linaro.org > > Changes in v4: > - Properly drop all MTU/TSO muckery in the TX function, the > whole approach is bogus. > - Make the raw etherype retrieveal return __be16, it is the > callers job to deal with endianness (as per the pattern > from if_vlan.h) > - Use __vlan_get_protocol() instead of vlan_get_protocol() > - Only actively bypass the TSS if the frame is over a certain > size. > - Drop comment that no longer applies. > - Link to v3: https://lore.kernel.org/r/20231221-new-gemini-ethernet-regression-v3-0-a96b4374bfe8@linaro.org > > Changes in v3: > - Fix a whitespace bug in the first patch. > - Add generic accessors to obtain the raw ethertype of an > ethernet frame. VLAN already have the right accessors. > - Link to v2: https://lore.kernel.org/r/20231216-new-gemini-ethernet-regression-v2-0-64c269413dfa@linaro.org > > Changes in v2: > - Drop the TSO and length checks altogether, this was never > working properly. > - Plan to make a proper TSO implementation in the next kernel > cycle. > - Link to v1: https://lore.kernel.org/r/20231215-new-gemini-ethernet-regression-v1-0-93033544be23@linaro.org > --- > drivers/net/ethernet/cortina/gemini.c | 15 ++------------- > 1 file changed, 2 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c > index 78287cfcbf63..705c3eb19cd3 100644 > --- a/drivers/net/ethernet/cortina/gemini.c > +++ b/drivers/net/ethernet/cortina/gemini.c > @@ -79,8 +79,7 @@ MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)"); > #define GMAC0_IRQ4_8 (GMAC0_MIB_INT_BIT | GMAC0_RX_OVERRUN_INT_BIT) > > #define GMAC_OFFLOAD_FEATURES (NETIF_F_SG | NETIF_F_IP_CSUM | \ > - NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM | \ > - NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6) > + NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM) > > /** > * struct gmac_queue_page - page buffer per-page info > @@ -1143,23 +1142,13 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, > struct gmac_txdesc *txd; > skb_frag_t *skb_frag; > dma_addr_t mapping; > - unsigned short mtu; > void *buffer; > int ret; > > - mtu = ETH_HLEN; > - mtu += netdev->mtu; > - if (skb->protocol == htons(ETH_P_8021Q)) > - mtu += VLAN_HLEN; > - > + /* TODO: implement proper TSO using MTU in word3 */ Okay, but you still kept this wrong comment. MTU refers to the device MTU, which is very often bigger than the MSS of the flow. Hopefully the comment will be removed soon when TSO is properly implemented. Reviewed-by: Eric Dumazet <edumazet@google.com>
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Sat, 06 Jan 2024 01:12:22 +0100 you wrote: > The recent change to allow large frames without hardware checksumming > slotted in software checksumming in the driver if hardware could not > do it. > > This will however upset TSO (TCP Segment Offloading). Typical > error dumps includes this: > > [...] Here is the summary with links: - [net,v6] net: ethernet: cortina: Drop TSO support https://git.kernel.org/netdev/net/c/ac631873c9e7 You are awesome, thank you!
diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c index 78287cfcbf63..705c3eb19cd3 100644 --- a/drivers/net/ethernet/cortina/gemini.c +++ b/drivers/net/ethernet/cortina/gemini.c @@ -79,8 +79,7 @@ MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)"); #define GMAC0_IRQ4_8 (GMAC0_MIB_INT_BIT | GMAC0_RX_OVERRUN_INT_BIT) #define GMAC_OFFLOAD_FEATURES (NETIF_F_SG | NETIF_F_IP_CSUM | \ - NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM | \ - NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6) + NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM) /** * struct gmac_queue_page - page buffer per-page info @@ -1143,23 +1142,13 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb, struct gmac_txdesc *txd; skb_frag_t *skb_frag; dma_addr_t mapping; - unsigned short mtu; void *buffer; int ret; - mtu = ETH_HLEN; - mtu += netdev->mtu; - if (skb->protocol == htons(ETH_P_8021Q)) - mtu += VLAN_HLEN; - + /* TODO: implement proper TSO using MTU in word3 */ word1 = skb->len; word3 = SOF_BIT; - if (word1 > mtu) { - word1 |= TSS_MTU_ENABLE_BIT; - word3 |= mtu; - } - 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
The recent change to allow large frames without hardware checksumming slotted in software checksumming in the driver if hardware could not do it. This will however upset TSO (TCP Segment Offloading). Typical error dumps includes this: skb len=2961 headroom=222 headlen=66 tailroom=0 (...) WARNING: CPU: 0 PID: 956 at net/core/dev.c:3259 skb_warn_bad_offload+0x7c/0x108 gemini-ethernet-port: caps=(0x0000010000154813, 0x00002007ffdd7889) And the packets do not go through. The TSO implementation is bogus: a TSO enabled driver must propagate the skb_shinfo(skb)->gso_size value to the TSO engine on the NIC. Drop the size check and TSO offloading features for now: this needs to be fixed up properly. After this ethernet works fine on Gemini devices with a direct connected PHY such as D-Link DNS-313. Also tested to still be working with a DSA switch using the Gemini ethernet as conduit interface. Link: https://lore.kernel.org/netdev/CANn89iJLfxng1sYL5Zk0mknXpyYQPCp83m3KgD2KJ2_hKCpEUg@mail.gmail.com/ Suggested-by: Eric Dumazet <edumazet@google.com> Fixes: d4d0c5b4d279 ("net: ethernet: cortina: Handle large frames") Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- This fix was developed on top of the earlier fixes. Finding the right solution is hard because the Gemini checksumming engine is completely undocumented in the datasheets. --- Changes in v6: - Keep the software checksum on larger frames, just drop the TSO support which is bogus anyway. - Drop the heuristics in the second patch. Just dropping TSO makes everything work right. - Drop adding the length in word3. - Link to v5: https://lore.kernel.org/r/20240102-new-gemini-ethernet-regression-v5-0-cf61ab3aa8cd@linaro.org Changes in v5: - Drop the patch re-implementing eth_header_parse_protocol() - Link to v4: https://lore.kernel.org/r/20231222-new-gemini-ethernet-regression-v4-0-a36e71b0f32b@linaro.org Changes in v4: - Properly drop all MTU/TSO muckery in the TX function, the whole approach is bogus. - Make the raw etherype retrieveal return __be16, it is the callers job to deal with endianness (as per the pattern from if_vlan.h) - Use __vlan_get_protocol() instead of vlan_get_protocol() - Only actively bypass the TSS if the frame is over a certain size. - Drop comment that no longer applies. - Link to v3: https://lore.kernel.org/r/20231221-new-gemini-ethernet-regression-v3-0-a96b4374bfe8@linaro.org Changes in v3: - Fix a whitespace bug in the first patch. - Add generic accessors to obtain the raw ethertype of an ethernet frame. VLAN already have the right accessors. - Link to v2: https://lore.kernel.org/r/20231216-new-gemini-ethernet-regression-v2-0-64c269413dfa@linaro.org Changes in v2: - Drop the TSO and length checks altogether, this was never working properly. - Plan to make a proper TSO implementation in the next kernel cycle. - Link to v1: https://lore.kernel.org/r/20231215-new-gemini-ethernet-regression-v1-0-93033544be23@linaro.org --- drivers/net/ethernet/cortina/gemini.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) --- base-commit: 33cc938e65a98f1d29d0a18403dbbee050dcad9a change-id: 20231203-new-gemini-ethernet-regression-3c672de9cfd9 Best regards,