| Message ID | 20210408172353.21143-1-TheSven73@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2" | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Thu, Apr 8, 2021 at 12:23 PM Sven Van Asbroeck <thesven73@gmail.com> 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: Can you explain the difference in behavior with what I was observing on the LAN7431? 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. > > 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; > -- > 2.17.1 > Regards, George
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. > 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. Sven
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?
Hi Heiner, On Thu, Apr 8, 2021 at 1:49 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > Can't we use frame_length - ETH_FCS_LEN direcctly here? If the hard-coded "4" refers to ETH_FCS_LEN, then yes, good point. I'd love to find out first why George and I need different patches to make the driver work in our use case, though.
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
On 08.04.2021 20:00, George McCollister wrote: > 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 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
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; >
On Thu, Apr 8, 2021 at 1:35 PM Sven Van Asbroeck <thesven73@gmail.com> 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. Works for me too. > > 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;
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?
Hi Heiner, On Thu, Apr 8, 2021 at 3:06 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > A completely unrelated question: > How about VLAN packets with a 802.1Q tag? Should VLAN_ETH_HLEN be used? That's a good question. My use-case does not involve 802.1Q though, so I'm unable to test. Thank you so much for your suggestion earlier, I'll put a proper attribution to you in the new patch's commit message.
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;