rtl8168e-vl dropping tftp ack

On 19.04.2013 09:49, hayeswang wrote:

>> I don't get it: arp aside, the normal trace in the capture 
>> file exhibits no
>> sub-60 bytes packet. Could you reformulate ?
>>
> 
> In brief, when the packet < 60, that is skb->len < 60, the hw should pad the
> packet to 60 bytes automatically. However, in my memory, the rtl8168e-vl
> wouldn't do this, and the packet wouldn't be sent. Therefore, the patch would be
> similar with the followings.
> 
> --- r8169.c.org 2013-04-19 22:35:40.785759473 +0800
> +++ r8169.c     2013-04-19 22:38:24.227189535 +0800
> @@ -5760,12 +5760,29 @@ static inline void rtl8169_tso_csum(stru
>         } else if (skb->ip_summed == CHECKSUM_PARTIAL) {
>                 const struct iphdr *ip = ip_hdr(skb);
> 
> +               if (unlikely(skb->len < ETH_ZLEN &&
> +                   (tp->mac_version == RTL_GIGA_MAC_VER_34))) {
> +                       if (skb_padto(skb, ETH_ZLEN))
> +                               return false;
> +                       skb_checksum_help(skb);
> +                       skb_put(skb, ETH_ZLEN - skb->len);
> +                       return true;
> +               }
> +
>                 if (ip->protocol == IPPROTO_TCP)
>                         opts[offset] |= info->checksum.tcp;
>                 else if (ip->protocol == IPPROTO_UDP)
>                         opts[offset] |= info->checksum.udp;
>                 else
>                         WARN_ON_ONCE(1);
> +       } else {
> +               if (unlikely(skb->len < ETH_ZLEN &&
> +                   (tp->mac_version == RTL_GIGA_MAC_VER_34))) {
> +                       if (skb_padto(skb, ETH_ZLEN))
> +                               return false;
> +                       skb_put(skb, ETH_ZLEN - skb->len);
> +                       return true;

Hardware is now back and I ran a couple of tests. First, the crash I ran into
previously was because I accidentally slipped in a skp_checksum_help into the
else case that Hayes proposed. Sorry.

So with the right version and both hunks in rtl8169_tso_csum, I see the problem
go away. I do observe the else case being hit on other occasions (not only while
doing pxe boots) from time to time. So that may happen more often than it causes
visible problems. I never saw the checksum case being hit.

Naively assuming that the checksum path never gets executed (which may be wrong)
I condensed the changes to:


This variant also seems to work (and does not drop the tftp packet). Whichever
path looks more suitable to you, you could add my tested-by to both.
We should also flag whichever change results from this as stable material as I
could see this happen even in v3.2 (just did not try further back).

Thanks,
Stefan

> +               }
>         }
>  }
>

Stefan Bader <stefan.bader@canonical.com> :
[...]
> This variant also seems to work (and does not drop the tftp packet). Whichever
> path looks more suitable to you, you could add my tested-by to both.

It's a bit late today. I'll submit it tomorrow.

> We should also flag whichever change results from this as stable material as I
> could see this happen even in v3.2 (just did not try further back).

Davem usually takes care of the last stable. I'll forward it to every
8168evl-supporting stable kernel once it gets included in mainline.

Thanks.