| Message ID | 20210408172353.21143-1-TheSven73@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net,v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2" | expand |
On 08.04.2021 19:23, Sven Van Asbroeck wrote: > From: Sven Van Asbroeck <thesven73@gmail.com> > > This reverts commit 3e21a10fdea3c2e4e4d1b72cb9d720256461af40. > > The reverted patch completely breaks all network connectivity on the > lan7430. tcpdump indicates missing bytes when receiving ping > packets from an external host: > > host$ ping $lan7430_ip > lan7430$ tcpdump -v > IP truncated-ip - 2 bytes missing! (tos 0x0, ttl 64, id 21715, > offset 0, flags [DF], proto ICMP (1), length 84) > > Fixes: 3e21a10fdea3 ("lan743x: trim all 4 bytes of the FCS; not just 2") > Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com> > --- > > To: Bryan Whitehead <bryan.whitehead@microchip.com> > To: "David S. Miller" <davem@davemloft.net> > To: Jakub Kicinski <kuba@kernel.org> > To: George McCollister <george.mccollister@gmail.com> > Cc: UNGLinuxDriver@microchip.com > Cc: netdev@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > > drivers/net/ethernet/microchip/lan743x_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c > index 1c3e204d727c..dbdfabff3b00 100644 > --- a/drivers/net/ethernet/microchip/lan743x_main.c > +++ b/drivers/net/ethernet/microchip/lan743x_main.c > @@ -2040,7 +2040,7 @@ lan743x_rx_trim_skb(struct sk_buff *skb, int frame_length) > dev_kfree_skb_irq(skb); > return NULL; > } > - frame_length = max_t(int, 0, frame_length - RX_HEAD_PADDING - 4); > + frame_length = max_t(int, 0, frame_length - RX_HEAD_PADDING - 2); > if (skb->len > frame_length) { > skb->tail -= skb->len - frame_length; > skb->len = frame_length; > Can't we use frame_length - ETH_FCS_LEN direcctly here?
On Thu, Apr 8, 2021 at 12:46 PM Sven Van Asbroeck <thesven73@gmail.com> wrote: > > Hi George, > > On Thu, Apr 8, 2021 at 1:36 PM George McCollister > <george.mccollister@gmail.com> wrote: > > > > Can you explain the difference in behavior with what I was observing > > on the LAN7431? > > I'm not using DSA in my application, so I cannot test or replicate > what you were observing. It would be great if we could work together > and settle on a solution that is acceptable to both of us. Sounds good. > > > I'll retest but if this is reverted I'm going to start > > seeing 2 extra bytes on the end of frames and it's going to break DSA > > with the LAN7431 again. > > > > Seen from my point of view, your patch is a regression. But perhaps my > patch set is a regression for you? Catch 22... > > Would you be able to identify which patch broke your DSA behaviour? > Was it one of mine? Perhaps we can start from there. Yes, first I'm going to confirm that what is in the net branch still works (unlikely but perhaps something else could have broken it since last I tried it). Then I'll confirm the patch which I believe broke it actually did and report back. > > Sven
Hi Heiner, On Thu, Apr 8, 2021 at 2:22 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > Just an idea: > RX_HEAD_PADDING is an alias for NET_IP_ALIGN that can have two values: > 0 and 2 > The two systems you use may have different NET_IP_ALIGN values. > This could explain the behavior. Then what I proposed should work > for both of you: frame_length - ETH_FCS_LEN Yes, good point! I was thinking the exact same thing just now. Subtracting 2 + RX_HEAD_PADDING from the frame length made no sense. George, I will send a patch for you to try shortly. Except if you're already ahead :)
Hi George, On Thu, Apr 8, 2021 at 2:26 PM Sven Van Asbroeck <thesven73@gmail.com> wrote: > > George, I will send a patch for you to try shortly. Except if you're > already ahead :) Would this work for you? It does for me. diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c index dbdfabff3b00..7b6794aa8ea9 100644 --- a/drivers/net/ethernet/microchip/lan743x_main.c +++ b/drivers/net/ethernet/microchip/lan743x_main.c @@ -885,8 +885,8 @@ static int lan743x_mac_set_mtu(struct lan743x_adapter *adapter, int new_mtu) } mac_rx &= ~(MAC_RX_MAX_SIZE_MASK_); - mac_rx |= (((new_mtu + ETH_HLEN + 4) << MAC_RX_MAX_SIZE_SHIFT_) & - MAC_RX_MAX_SIZE_MASK_); + mac_rx |= (((new_mtu + ETH_HLEN + ETH_FCS_LEN) + << MAC_RX_MAX_SIZE_SHIFT_) & MAC_RX_MAX_SIZE_MASK_); lan743x_csr_write(adapter, MAC_RX, mac_rx); if (enabled) { @@ -1944,7 +1944,7 @@ static int lan743x_rx_init_ring_element(struct lan743x_rx *rx, int index) struct sk_buff *skb; dma_addr_t dma_ptr; - buffer_length = netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING; + buffer_length = netdev->mtu + ETH_HLEN + ETH_FCS_LEN + RX_HEAD_PADDING; descriptor = &rx->ring_cpu_ptr[index]; buffer_info = &rx->buffer_info[index]; @@ -2040,7 +2040,7 @@ lan743x_rx_trim_skb(struct sk_buff *skb, int frame_length) dev_kfree_skb_irq(skb); return NULL; } - frame_length = max_t(int, 0, frame_length - RX_HEAD_PADDING - 2); + frame_length = max_t(int, 0, frame_length - ETH_FCS_LEN); if (skb->len > frame_length) { skb->tail -= skb->len - frame_length; skb->len = frame_length;
On 08.04.2021 20:35, Sven Van Asbroeck wrote: > Hi George, > > On Thu, Apr 8, 2021 at 2:26 PM Sven Van Asbroeck <thesven73@gmail.com> wrote: >> >> George, I will send a patch for you to try shortly. Except if you're >> already ahead :) > > Would this work for you? It does for me. > > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c > b/drivers/net/ethernet/microchip/lan743x_main.c > index dbdfabff3b00..7b6794aa8ea9 100644 > --- a/drivers/net/ethernet/microchip/lan743x_main.c > +++ b/drivers/net/ethernet/microchip/lan743x_main.c > @@ -885,8 +885,8 @@ static int lan743x_mac_set_mtu(struct > lan743x_adapter *adapter, int new_mtu) > } > > mac_rx &= ~(MAC_RX_MAX_SIZE_MASK_); > - mac_rx |= (((new_mtu + ETH_HLEN + 4) << MAC_RX_MAX_SIZE_SHIFT_) & > - MAC_RX_MAX_SIZE_MASK_); > + mac_rx |= (((new_mtu + ETH_HLEN + ETH_FCS_LEN) > + << MAC_RX_MAX_SIZE_SHIFT_) & MAC_RX_MAX_SIZE_MASK_); > lan743x_csr_write(adapter, MAC_RX, mac_rx); > > if (enabled) { > @@ -1944,7 +1944,7 @@ static int lan743x_rx_init_ring_element(struct > lan743x_rx *rx, int index) > struct sk_buff *skb; > dma_addr_t dma_ptr; > > - buffer_length = netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING; > + buffer_length = netdev->mtu + ETH_HLEN + ETH_FCS_LEN + RX_HEAD_PADDING; > A completely unrelated question: How about VLAN packets with a 802.1Q tag? Should VLAN_ETH_HLEN be used? > descriptor = &rx->ring_cpu_ptr[index]; > buffer_info = &rx->buffer_info[index]; > @@ -2040,7 +2040,7 @@ lan743x_rx_trim_skb(struct sk_buff *skb, int frame_length) > dev_kfree_skb_irq(skb); > return NULL; > } > - frame_length = max_t(int, 0, frame_length - RX_HEAD_PADDING - 2); > + frame_length = max_t(int, 0, frame_length - ETH_FCS_LEN); > if (skb->len > frame_length) { > skb->tail -= skb->len - frame_length; > skb->len = frame_length; >
Hi George, On Thu, Apr 8, 2021 at 3:55 PM George McCollister <george.mccollister@gmail.com> wrote: > > Works for me too. Sounds good. I'll post a proper patch soon. Would you be able to review+test, and perhaps offer your Reviewed-by/Tested-by tags when everything looks ok?
From: Sven Van Asbroeck > Sent: 08 April 2021 19:35 ... > - buffer_length = netdev->mtu + ETH_HLEN + 4 + RX_HEAD_PADDING; > + buffer_length = netdev->mtu + ETH_HLEN + ETH_FCS_LEN + RX_HEAD_PADDING; I'd try to write the lengths in the order they happen, so: buffer_length = RX_HEAD_PADDING + ETH_HLEN + netdev->mtu + ETH_FCS_LEN; The compiler should add all the constants together, so the generated code ought to be the same. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c index 1c3e204d727c..dbdfabff3b00 100644 --- a/drivers/net/ethernet/microchip/lan743x_main.c +++ b/drivers/net/ethernet/microchip/lan743x_main.c @@ -2040,7 +2040,7 @@ lan743x_rx_trim_skb(struct sk_buff *skb, int frame_length) dev_kfree_skb_irq(skb); return NULL; } - frame_length = max_t(int, 0, frame_length - RX_HEAD_PADDING - 4); + frame_length = max_t(int, 0, frame_length - RX_HEAD_PADDING - 2); if (skb->len > frame_length) { skb->tail -= skb->len - frame_length; skb->len = frame_length;