From: Stefan Bader <stefan.bader@canonical•com>
To: hayeswang <hayeswang@realtek•com>
Cc: 'Francois Romieu' <romieu@fr•zoreil.com>,
netdev@vger•kernel.org, 'nic_swsd' <nic_swsd@realtek•com>
Subject: Re: rtl8168e-vl dropping tftp ack
Date: Thu, 25 Apr 2013 12:56:30 +0200 [thread overview]
Message-ID: <51790BDE.7090500@canonical.com> (raw)
In-Reply-To: <55FF74168B2A4152AD875C000535466F@realtek.com.tw>
[-- Attachment #1: Type: text/plain, Size: 4629 bytes --]
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:
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek
index 4ecbe64..1519a2f 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5790,12 +5790,23 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *sk
if (unlikely(le32_to_cpu(txd->opts1) & DescOwn))
goto err_stop_0;
+ /*
+ * 8168E-VL hardware does not automatically pad to minimum
+ * length.
+ */
+ if (unlikely(skb->len < ETH_ZLEN &&
+ (tp->mac_version == RTL_GIGA_MAC_VER_34))) {
+ if (skb_padto(skb, ETH_ZLEN))
+ goto err_update_stats_0;
+ skb_put(skb, ETH_ZLEN - skb->len);
+ }
+
len = skb_headlen(skb);
mapping = dma_map_single(d, skb->data, len, DMA_TO_DEVICE);
if (unlikely(dma_mapping_error(d, mapping))) {
if (net_ratelimit())
netif_err(tp, drv, dev, "Failed to map TX DMA!\n");
- goto err_dma_0;
+ goto err_free_skb_1;
}
tp->tx_skb[entry].len = len;
@@ -5808,7 +5819,7 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
frags = rtl8169_xmit_frags(tp, skb, opts);
if (frags < 0)
- goto err_dma_1;
+ goto err_unmap_dma_1;
else if (frags)
opts[0] |= FirstFrag;
else {
@@ -5854,10 +5865,11 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *sk
return NETDEV_TX_OK;
-err_dma_1:
+err_unmap_dma_1:
rtl8169_unmap_tx_skb(d, tp->tx_skb + entry, txd);
-err_dma_0:
+err_free_skb_1:
dev_kfree_skb(skb);
+err_update_stats_0:
dev->stats.tx_dropped++;
return NETDEV_TX_OK;
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
> + }
> }
> }
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]
next prev parent reply other threads:[~2013-04-25 10:56 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-17 14:31 rtl8168e-vl dropping tftp ack Stefan Bader
2013-04-17 21:32 ` Francois Romieu
2013-04-18 9:51 ` Stefan Bader
2013-04-18 21:55 ` Francois Romieu
2013-04-19 3:27 ` hayeswang
2013-04-19 6:14 ` Francois Romieu
2013-04-19 7:49 ` hayeswang
2013-04-25 10:56 ` Stefan Bader [this message]
2013-04-25 23:03 ` Francois Romieu
2013-04-19 12:54 ` Stefan Bader
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=51790BDE.7090500@canonical.com \
--to=stefan.bader@canonical$(echo .)com \
--cc=hayeswang@realtek$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nic_swsd@realtek$(echo .)com \
--cc=romieu@fr$(echo .)zoreil.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox