[PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"

[PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"

[Sven Van Asbroeck]

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;
-- 
2.17.1

Re: [PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"

[George McCollister]

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

Re: [PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"

[Sven Van Asbroeck]

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

Re: [PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"

[Heiner Kallweit]

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?

Re: [PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"

[Sven Van Asbroeck]

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.

Re: [PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"

[George McCollister]

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

Re: [PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"

[Heiner Kallweit]

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

Re: [PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"

[Sven Van Asbroeck]

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 :)

Re: [PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"

[Sven Van Asbroeck]

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;

Re: [PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"

[Heiner Kallweit]

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;
> 

Re: [PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"

[George McCollister]

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;

Re: [PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"

[Sven Van Asbroeck]

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?

Re: [PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"

[Sven Van Asbroeck]

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.

RE: [PATCH net v1] Revert "lan743x: trim all 4 bytes of the FCS; not just 2"

[David Laight]

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)