[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20231221180457.GE1202958@kernel.org>
Date: Thu, 21 Dec 2023 19:04:57 +0100
From: Simon Horman <horms@...nel.org>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Hans Ulli Kroll <ulli.kroll@...glemail.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
netdev@...r.kernel.org
Subject: Re: [PATCH net v3 3/3] net: ethernet: cortina: Bypass checksumming
engine of alien ethertypes
On Thu, Dec 21, 2023 at 09:50:41AM +0100, Eric Dumazet wrote:
> On Thu, Dec 21, 2023 at 1:02 AM Linus Walleij <linus.walleij@...aro.org> wrote:
> >
> > We had workarounds were the ethernet checksumming engine would be bypassed
> > for larger frames, this fixed devices using DSA, but regressed devices
> > where the ethernet was connected directly to a PHY.
> >
> > The devices with a PHY connected directly can't handle large frames
> > either way, with or without bypass. Looking at the size of the frame
> > is probably just wrong.
> >
> > Rework the workaround such that we just bypass the checksumming engine if
> > the ethertype inside the actual frame is something else than 0x0800
> > (IPv4) or 0x86dd (IPv6). These are the only frames the checksumming engine
> > can actually handle. VLAN framing (0x8100) also works fine.
> >
> > We can't inspect skb->protocol because DSA frames will sometimes have a
> > custom ethertype despite skb->protocol is e.g. 0x0800.
> >
> > After this both devices with direct ethernet attached such as D-Link
> > DNS-313 and devices with a DSA switch with a custom ethertype such as
> > D-Link DIR-685 work fine.
> >
> > Fixes: d4d0c5b4d279 ("net: ethernet: cortina: Handle large frames")
> > Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> > ---
> > drivers/net/ethernet/cortina/gemini.c | 21 ++++++++++++++++++++-
> > 1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> > index ecc247acac39..6d153eba8e81 100644
> > --- a/drivers/net/ethernet/cortina/gemini.c
> > +++ b/drivers/net/ethernet/cortina/gemini.c
> > @@ -29,6 +29,7 @@
> > #include <linux/of_net.h>
> > #include <linux/of_platform.h>
> > #include <linux/etherdevice.h>
> > +#include <linux/if_ether.h>
> > #include <linux/if_vlan.h>
> > #include <linux/skbuff.h>
> > #include <linux/phy.h>
> > @@ -1143,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;
> > + u16 ethertype;
> > void *buffer;
> >
> > mtu = ETH_HLEN;
> > @@ -1158,7 +1160,24 @@ static int gmac_map_tx_bufs(struct net_device *netdev, struct sk_buff *skb,
> > word3 |= mtu;
> > }
> >
> > - if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > + /* Dig out the the ethertype actually in the buffer and not what the
> > + * protocol claims to be. This is the raw data that the checksumming
> > + * offload engine will have to deal with.
> > + */
> > + ethertype = skb_eth_raw_ethertype(skb);
> > + /* This is the only VLAN type supported by this hardware so check for
> > + * that: the checksumming engine can handle IP and IPv6 inside 802.1Q.
> > + */
> > + if (ethertype == ETH_P_8021Q)
> > + ethertype = vlan_get_protocol(skb);
>
> You meant : ethertype = __vlan_get_protocol(skb, ethertype, NULL);
>
> Otherwise skb->protocol could be something unexpected, according to
> your comments.
Also, __vlan_get_protocol() deals with big endian Ether Types,
so I think you need something like:
ethertype = be16_to_cpu(__vlan_get_protocol(skb, cpu_to_be16(ethertype),
NULL));
Which is a bit ugly, so perhaps it is nicer to switch
things around so ethertype is __be16. And perhaps the
return value of skb_eth_raw_ethertype is __be16 too.
>
> > +
> > + if (ethertype != ETH_P_IP && ethertype != ETH_P_IPV6) {
> > + /* Hardware offloaded checksumming isn't working on non-IP frames.
> > + * This happens for example on some DSA switches using a custom
> > + * ethertype. Just bypass the engine for those.
> > + */
> > + 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
> >
> > --
> > 2.34.1
> >
>
Powered by blists - more mailing lists