[net,v2,4/4] net: ethernet: cortina: Handle large frames

  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
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
<linux/skbuff.h>.

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 <olteanv@gmail.com>
Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/ethernet/cortina/gemini.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
  

On Sun, Nov 05, 2023 at 09:57:26PM +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
> packets start to appear on the target such as in this tcpdump
> resulting from ping -s 1474:

A bit related: what is gmac_fix_features() supposed to do? I see it
unsets GMAC_OFFLOAD_FEATURES when the MTU goes over a certain limit,
and that also includes NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM. Is that
limit correct, or is it supposed to kick in sooner, to allow
validate_xmit_skb() -> skb_csum_hwoffload_help() do the software
checksuum for you? I'm not sure whether that was the intention.

> 
> 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
> <linux/skbuff.h>.
> 
> 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 <olteanv@gmail.com>

To be clear, I didn't suggest any of this. I just pointed towards the gemini.c
driver as being the problem. Please remove my Suggested-by tag.

> Fixes: 4d5ae32f5e1e ("net: ethernet: Add a driver for Gemini gigabit ethernet")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/net/ethernet/cortina/gemini.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 576174a862a9..84295c1b87e6 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;
> @@ -1165,7 +1166,19 @@ 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. Perhaps the buffer is only 1518
> +		 * bytes fitting a normal frame and a checksum?
> +		 * 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;
>  
>  		if (skb->protocol == htons(ETH_P_IP)) {
> 
> -- 
> 2.34.1
> 

[ context: tag_rtl4_a.c is a "category 2", aka "Ethertype", tagger ]

We say this in Documentation/networking/dsa/dsa.rst:

Checksum offload should work with category 1 and 2 taggers when the DSA conduit
driver declares NETIF_F_HW_CSUM in vlan_features and looks at csum_start and
csum_offset. For those cases, DSA will shift the checksum start and offset by
the tag size. If the DSA conduit driver still uses the legacy NETIF_F_IP_CSUM
or NETIF_F_IPV6_CSUM in vlan_features, the offload might only work if the
offload hardware already expects that specific tag (perhaps due to matching
vendors). DSA user ports inherit those flags from the conduit, and it is up to
the driver to correctly fall back to software checksum when the IP header is not
where the hardware expects. If that check is ineffective, the packets might go
to the network without a proper checksum (the checksum field will have the
pseudo IP header sum).

Shouldn't "word1 |= TSS_BYPASS_BIT;" be done depending on skb->protocol,
rather than depending on skb->len?!

		if (skb->protocol == htons(ETH_P_IP)) {
			word1 |= TSS_IP_CHKSUM_BIT;
			tcp = ip_hdr(skb)->protocol == IPPROTO_TCP;
		} else { /* IPv6 */
			word1 |= TSS_IPV6_ENABLE_BIT;
			tcp = ipv6_hdr(skb)->nexthdr == IPPROTO_TCP;
		} // here
			word1 |= TSS_BYPASS_BIT;

Gemini should never attempt to provide checksums for DSA-tagged packets
unless it is able to take skb->csum_start into consideration, otherwise
it will get it wrong.

This is somewhat independent of the other problem you've found, which
seems to be that large non-DSA packets get truncated anyway. But not
bypassing TX checksum offload truncates a packet? Hmm, strange.
  

On Mon, Nov 06, 2023 at 03:26:26PM +0200, Vladimir Oltean wrote:
> Gemini should never attempt to provide checksums for DSA-tagged packets
> unless it is able to take skb->csum_start into consideration, otherwise
> it will get it wrong.

Additionally, since Gemini does not put NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM
in vlan_features, DSA won't inherit (thus won't have) them. So,
validate_xmit_skb() should perform the skb checksum during the xmit on
the user port, which is earlier compared to the xmit on the conduit.
So, I guess skb->ip_summed should already be CHECKSUM_NONE here?
I think the only problem for DSA is the lack of the TSS_BYPASS_BIT. The
rest is unrelated.
  

On Mon, Nov 6, 2023 at 2:26 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> On Sun, Nov 05, 2023 at 09:57:26PM +0100, Linus Walleij wrote:

> > If we start sending bigger frames (after first bumping up the MTU
> > on both interfaces sending and receiveing the frames), truncated
> > packets start to appear on the target such as in this tcpdump
> > resulting from ping -s 1474:
>
> A bit related: what is gmac_fix_features() supposed to do? I see it
> unsets GMAC_OFFLOAD_FEATURES when the MTU goes over a certain limit,
> and that also includes NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM. Is that
> limit correct, or is it supposed to kick in sooner, to allow
> validate_xmit_skb() -> skb_csum_hwoffload_help() do the software
> checksuum for you? I'm not sure whether that was the intention.

That indeed seems like the intention. But it's a bit suboptimal, because it
disables hardware checksum just because the MTU goes over a
certain level, and stops using the hardware checksum also for all
packets smaller than the MTU :(

I'll delete this and make the driver slot in the SW fallback per-packet
instead, I think it is fair to assume that most packets will be < MTU
and it is really just a question of where the fallback gets called.

> > Suggested-by: Vladimir Oltean <olteanv@gmail.com>
>
> To be clear, I didn't suggest any of this. I just pointed towards the gemini.c
> driver as being the problem. Please remove my Suggested-by tag.

OK sorry, it was just my way of trying to provide credit where
credit is due, because you helped so much with this bug.

> > -     if (skb->ip_summed != CHECKSUM_NONE) {
> > +     if (skb->len >= ETH_FRAME_LEN) {
> > +             /* Hardware offloaded checksumming isn't working on frames
> > +              * bigger than 1514 bytes. Perhaps the buffer is only 1518
> > +              * bytes fitting a normal frame and a checksum?
> > +              * 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;
> >
> >               if (skb->protocol == htons(ETH_P_IP)) {
>
> [ context: tag_rtl4_a.c is a "category 2", aka "Ethertype", tagger ]
>
> We say this in Documentation/networking/dsa/dsa.rst:
>
> Checksum offload should work with category 1 and 2 taggers when the DSA conduit
> driver declares NETIF_F_HW_CSUM in vlan_features
> and looks at csum_start and csum_offset.
> For those cases, DSA will shift the checksum start and offset by
> the tag size. If the DSA conduit driver still uses the legacy NETIF_F_IP_CSUM
> or NETIF_F_IPV6_CSUM in vlan_features, the offload might only work if the
> offload hardware already expects that specific tag (perhaps due to matching
> vendors).

Since things work smoothly I can only assume that the Gemini
checksum engine actually knows about the Realtek ethertype (0x8899)
and the protocol (0xa) and takes action on that, since the switch works.

OR: it has some heuristic on for how to handle it. (Such as looking for
a valid TCP or UDP header to figure out where to put the checksum.)

But I have no idea how it does it. It doesn't have a firmware AFAIK.

Examples listed were ICMP so just IP checksums but I tried for example
SSH, and HTTP and packets look like this:

22:51:35.457191 9a:ec:30:5a:46:96 (oui Unknown) > bc:ae:c5:6b:a8:3d
(oui Unknown),
ethertype IPv4 (0x0800), length 434: (tos 0x48, ttl 64, id 8221,
offset 0, flags [DF], proto TCP (6), length 420)
    _gateway.48102 > fedora.ssh: Flags [P.], cksum 0xcf1b (correct),
seq 811:1179, ack 2310,
   win 2054, options [nop,nop,TS val 74858741 ecr 1981407207], length 368

Checksum correct. So...

> DSA user ports inherit those flags from the conduit, and it is up to
> the driver to correctly fall back to software checksum when the IP header is not
> where the hardware expects. If that check is ineffective, the packets might go
> to the network without a proper checksum (the checksum field will have the
> pseudo IP header sum).

It definately does not contain the pseudo IP header sum because
it would be the same all the time but tcpdump is happy:
cksum 0xcf1b (correct)
cksum 0x0655 (correct)
cksum 0xd247 (correct)
cksum 0x06b1 (correct)

> Shouldn't "word1 |= TSS_BYPASS_BIT;" be done depending on skb->protocol,
> rather than depending on skb->len?!
>
>                 if (skb->protocol == htons(ETH_P_IP)) {
>                         word1 |= TSS_IP_CHKSUM_BIT;
>                         tcp = ip_hdr(skb)->protocol == IPPROTO_TCP;
>                 } else { /* IPv6 */
>                         word1 |= TSS_IPV6_ENABLE_BIT;
>                         tcp = ipv6_hdr(skb)->nexthdr == IPPROTO_TCP;
>                 } // here
>                         word1 |= TSS_BYPASS_BIT;

Oddly it assumes everything is either TCP or UDP on
IPv4 or IPv6. And yet things such as ICMP work just fine.

I think the checksum engine can contain some various
heuristics, such as if it cannot recognize what is coming
in as the selected TCP or UDP, it will pass right through.

> Gemini should never attempt to provide checksums for DSA-tagged packets
> unless it is able to take skb->csum_start into consideration, otherwise
> it will get it wrong.
>
> This is somewhat independent of the other problem you've found, which
> seems to be that large non-DSA packets get truncated anyway. But not
> bypassing TX checksum offload truncates a packet? Hmm, strange.

I have a theory about that in the comment, which is that when they
engineered the hardware they only put in a hardware buffer for
1518 bytes in the checksum engine. If you try to put in any more
it gets truncated. It's a reasonable guess.

If you do not set the checkumming engine to "bypass" it will try
to fit the incoming paket into the checksumming buffer, and then
it will look to see if it can find the right TCP or UDP headers.
If it can't it will pass the packet out from the buffer without doing
any changes. But the buffer is just 1518 bytes, which means that
no matter what kind of package it is, it will get truncated if it
does not fit into the checksumming buffer.

This would give exactly the behaviour we're seeing.

Yours,
Linus Walleij
  

On Mon, Nov 06, 2023 at 11:44:37PM +0100, Linus Walleij wrote:
> That indeed seems like the intention. But it's a bit suboptimal, because it
> disables hardware checksum just because the MTU goes over a
> certain level, and stops using the hardware checksum also for all
> packets smaller than the MTU :(
> 
> I'll delete this and make the driver slot in the SW fallback per-packet
> instead, I think it is fair to assume that most packets will be < MTU
> and it is really just a question of where the fallback gets called.

Performing the software checksum per packet instead of doing it for all
packets seems more sensible. I haven't looked at the other offload bits
from GMAC_OFFLOAD_FEATURES to determine if changing those is going to be
problematic.

> > > Suggested-by: Vladimir Oltean <olteanv@gmail.com>
> >
> > To be clear, I didn't suggest any of this. I just pointed towards the gemini.c
> > driver as being the problem. Please remove my Suggested-by tag.
> 
> OK sorry, it was just my way of trying to provide credit where
> credit is due, because you helped so much with this bug.

Ok, but asserting the validity of the DSA code and vaguely pointing
towards an unrelated driver and not even specifying what might be wrong
with its xmit procedure still does not count as worth a Suggested-by tag
to me. So still, please remove it.

> > We say this in Documentation/networking/dsa/dsa.rst:
> >
> > Checksum offload should work with category 1 and 2 taggers when the DSA conduit
> > driver declares NETIF_F_HW_CSUM in vlan_features
> > and looks at csum_start and csum_offset.
> > For those cases, DSA will shift the checksum start and offset by
> > the tag size. If the DSA conduit driver still uses the legacy NETIF_F_IP_CSUM
> > or NETIF_F_IPV6_CSUM in vlan_features, the offload might only work if the
> > offload hardware already expects that specific tag (perhaps due to matching
> > vendors).
> 
> Since things work smoothly I can only assume that the Gemini
> checksum engine actually knows about the Realtek ethertype (0x8899)
> and the protocol (0xa) and takes action on that, since the switch works.
> 
> OR: it has some heuristic on for how to handle it. (Such as looking for
> a valid TCP or UDP header to figure out where to put the checksum.)

The second option seems more plausible to me. It seems unlikely that a
non-Realtek controller would have information about a Realtek
proprietary protocol, that even Linux people have trouble finding a lot
of information about, when explicitly trying to do so.

> But I have no idea how it does it. It doesn't have a firmware AFAIK.
> 
> Examples listed were ICMP so just IP checksums but I tried for example
> SSH, and HTTP and packets look like this:
> 
> 22:51:35.457191 9a:ec:30:5a:46:96 (oui Unknown) > bc:ae:c5:6b:a8:3d
> (oui Unknown),
> ethertype IPv4 (0x0800), length 434: (tos 0x48, ttl 64, id 8221,
> offset 0, flags [DF], proto TCP (6), length 420)
>     _gateway.48102 > fedora.ssh: Flags [P.], cksum 0xcf1b (correct),
> seq 811:1179, ack 2310,
>    win 2054, options [nop,nop,TS val 74858741 ecr 1981407207], length 368
> 
> Checksum correct. So...

Well, the checksum is correct because it's done in software by the
network stack, prior to dsa_user_xmit() being even called, and thus
prior to the DSA tag being added. "ethtool -k lan0 | grep sum" will tell
you as much (compare it to "ethtool -k eth0" which will list some
offload bits set).

> > Shouldn't "word1 |= TSS_BYPASS_BIT;" be done depending on skb->protocol,
> > rather than depending on skb->len?!
> >
> >                 if (skb->protocol == htons(ETH_P_IP)) {
> >                         word1 |= TSS_IP_CHKSUM_BIT;
> >                         tcp = ip_hdr(skb)->protocol == IPPROTO_TCP;
> >                 } else { /* IPv6 */
> >                         word1 |= TSS_IPV6_ENABLE_BIT;
> >                         tcp = ipv6_hdr(skb)->nexthdr == IPPROTO_TCP;
> >                 } // here
> >                         word1 |= TSS_BYPASS_BIT;
> 
> Oddly it assumes everything is either TCP or UDP on
> IPv4 or IPv6. And yet things such as ICMP work just fine.
> 
> I think the checksum engine can contain some various
> heuristics, such as if it cannot recognize what is coming
> in as the selected TCP or UDP, it will pass right through.
> 
> > Gemini should never attempt to provide checksums for DSA-tagged packets
> > unless it is able to take skb->csum_start into consideration, otherwise
> > it will get it wrong.
> >
> > This is somewhat independent of the other problem you've found, which
> > seems to be that large non-DSA packets get truncated anyway. But not
> > bypassing TX checksum offload truncates a packet? Hmm, strange.
> 
> I have a theory about that in the comment, which is that when they
> engineered the hardware they only put in a hardware buffer for
> 1518 bytes in the checksum engine. If you try to put in any more
> it gets truncated. It's a reasonable guess.
> 
> If you do not set the checkumming engine to "bypass" it will try
> to fit the incoming paket into the checksumming buffer, and then
> it will look to see if it can find the right TCP or UDP headers.
> If it can't it will pass the packet out from the buffer without doing
> any changes. But the buffer is just 1518 bytes, which means that
> no matter what kind of package it is, it will get truncated if it
> does not fit into the checksumming buffer.
> 
> This would give exactly the behaviour we're seeing.

The evidence you've presented seems to support your position and not
mine. The controller does not attempt to provide checksums for
DSA-tagged packets, that is not a problem because those have checksums
already, but the packet size is the actual problem, irrespective of
whether the packets are DSA-tagged or not.

But in that case I have some comments on your latest version of the
patch set.