On Tue, Jan 26, 2021 at 2:40 PM Heiner Kallweit <hkallwe...@gmail.com> wrote:
>
> It was reported that on RTL8125 network breaks under heavy UDP load,
> e.g. torrent traffic ([0], from comment 27). Realtek confirmed a hw bug
> and provided me with a test version of the r8125 driver including a
> workaround. Tests confirmed that the workaround fixes the issue.
> I modified the original version of the workaround to meet mainline
> code style.
>
> [0] https://bugzilla.kernel.org/show_bug.cgi?id=209839
>
> Fixes: f1bce4ad2f1c ("r8169: add support for RTL8125")
> Tested-by: xplo <xplo...@gmail.com>
> Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
> ---
> drivers/net/ethernet/realtek/r8169_main.c | 64 ++++++++++++++++++++---
> 1 file changed, 58 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c
> b/drivers/net/ethernet/realtek/r8169_main.c
> index fb67d8f79..90052033b 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -28,6 +28,7 @@
> #include <linux/bitfield.h>
> #include <linux/prefetch.h>
> #include <linux/ipv6.h>
> +#include <linux/ptp_classify.h>
> #include <asm/unaligned.h>
> #include <net/ip6_checksum.h>
>
> @@ -4007,17 +4008,64 @@ static int rtl8169_xmit_frags(struct rtl8169_private
> *tp, struct sk_buff *skb,
> return -EIO;
> }
>
> -static bool rtl_test_hw_pad_bug(struct rtl8169_private *tp)
> +static bool rtl_skb_is_udp(struct sk_buff *skb)
> {
> + switch (vlan_get_protocol(skb)) {
> + case htons(ETH_P_IP):
> + return ip_hdr(skb)->protocol == IPPROTO_UDP;
> + case htons(ETH_P_IPV6):
> + return ipv6_hdr(skb)->nexthdr == IPPROTO_UDP;
This trusts that an skb with given skb->protocol is well behaved. With
packet sockets/tun/virtio, that may be false.
> + default:
> + return false;
> + }
> +}
> +
> +#define RTL_MIN_PATCH_LEN 47
> +#define PTP_GEN_PORT 320
Why the two PTP ports? The report is not PTP specific. Also, what does
patch mean in this context?
> +
> +/* see rtl8125_get_patch_pad_len() in r8125 vendor driver */
> +static unsigned int rtl8125_quirk_udp_padto(struct rtl8169_private *tp,
> + struct sk_buff *skb)
> +{
> + unsigned int padto = 0, len = skb->len;
> +
> + if (rtl_is_8125(tp) && len < 175 && rtl_skb_is_udp(skb) &&
> + skb_transport_header_was_set(skb)) {
What is 175 here?
> + unsigned int trans_data_len = skb_tail_pointer(skb) -
> + skb_transport_header(skb);
> +
> + if (trans_data_len > 3 && trans_data_len < RTL_MIN_PATCH_LEN)
> {
And 3 here, instead of sizeof(struct udphdr)
> + u16 dest = ntohs(udp_hdr(skb)->dest);
> +
> + if (dest == PTP_EV_PORT || dest == PTP_GEN_PORT)
> + padto = len + RTL_MIN_PATCH_LEN -
> trans_data_len;
> + }
> +
> + if (trans_data_len < UDP_HLEN)
> + padto = max(padto, len + UDP_HLEN - trans_data_len);
> + }
> +
> + return padto;
> +}
> +
> +static unsigned int rtl_quirk_packet_padto(struct rtl8169_private *tp,
> + struct sk_buff *skb)
> +{
> + unsigned int padto;
> +
> + padto = rtl8125_quirk_udp_padto(tp, skb);
> +
> switch (tp->mac_version) {
> case RTL_GIGA_MAC_VER_34:
> case RTL_GIGA_MAC_VER_60:
> case RTL_GIGA_MAC_VER_61:
> case RTL_GIGA_MAC_VER_63:
> - return true;
> + padto = max_t(unsigned int, padto, ETH_ZLEN);
> default:
> - return false;
> + break;
> }
> +
> + return padto;
> }
>
> static void rtl8169_tso_csum_v1(struct sk_buff *skb, u32 *opts)
> @@ -4089,9 +4137,10 @@ static bool rtl8169_tso_csum_v2(struct rtl8169_private
> *tp,
>
> opts[1] |= transport_offset << TCPHO_SHIFT;
> } else {
> - if (unlikely(skb->len < ETH_ZLEN && rtl_test_hw_pad_bug(tp)))
> - /* eth_skb_pad would free the skb on error */
> - return !__skb_put_padto(skb, ETH_ZLEN, false);
> + unsigned int padto = rtl_quirk_packet_padto(tp, skb);
> +
> + /* skb_padto would free the skb on error */
> + return !__skb_put_padto(skb, padto, false);
> }
>
> return true;
> @@ -4268,6 +4317,9 @@ static netdev_features_t rtl8169_features_check(struct
> sk_buff *skb,
> if (skb->len < ETH_ZLEN)
> features &= ~NETIF_F_CSUM_MASK;
>
> + if (rtl_quirk_packet_padto(tp, skb))
> + features &= ~NETIF_F_CSUM_MASK;
> +
> if (transport_offset > TCPHO_MAX &&
> rtl_chip_supports_csum_v2(tp))
> features &= ~NETIF_F_CSUM_MASK;
> --
> 2.30.0
>
