[PATCH net-next] net: dsa: hellcreek: Print warning only once

[PATCH net-next] net: dsa: hellcreek: Print warning only once

[Kurt Kanzenbach]

In case the source port cannot be decoded, print the warning only once. This
still brings attention to the user and does not spam the logs at the same time.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 net/dsa/tag_hellcreek.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/dsa/tag_hellcreek.c b/net/dsa/tag_hellcreek.c
index eb204ad36eee..846588c0070a 100644
--- a/net/dsa/tag_hellcreek.c
+++ b/net/dsa/tag_hellcreek.c
@@ -45,7 +45,7 @@ static struct sk_buff *hellcreek_rcv(struct sk_buff *skb,
 
 	skb->dev = dsa_master_find_slave(dev, 0, port);
 	if (!skb->dev) {
-		netdev_warn(dev, "Failed to get source port: %d\n", port);
+		netdev_warn_once(dev, "Failed to get source port: %d\n", port);
 		return NULL;
 	}
 
-- 
2.30.2

Re: [PATCH net-next] net: dsa: hellcreek: Print warning only once

[Andrew Lunn]

On Tue, Aug 30, 2022 at 06:34:48PM +0200, Kurt Kanzenbach wrote:
> In case the source port cannot be decoded, print the warning only once. This
> still brings attention to the user and does not spam the logs at the same time.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next] net: dsa: hellcreek: Print warning only once

[Vladimir Oltean]

On Tue, Aug 30, 2022 at 06:34:48PM +0200, Kurt Kanzenbach wrote:
> In case the source port cannot be decoded, print the warning only once. This
> still brings attention to the user and does not spam the logs at the same time.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Out of curiosity, how did this happen?

Re: [PATCH net-next] net: dsa: hellcreek: Print warning only once

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 3587 bytes --]

On Wed Aug 31 2022, Vladimir Oltean wrote:
> On Tue, Aug 30, 2022 at 06:34:48PM +0200, Kurt Kanzenbach wrote:
>> In case the source port cannot be decoded, print the warning only once. This
>> still brings attention to the user and does not spam the logs at the same time.
>> 
>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>> ---
>
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
>
> Out of curiosity, how did this happen?

Well, I'm still looking into it...

I've plugged my hellcreek test device into a Cisco switch and saw these
messages flying by. It said it failed to get source port 0 at a constant
rate. Turns out the Cisco switch does RSTP by default. Every STP frame
received has source port 0 which doesn't make any sense. The switch adds
a tail tag to every frame towards the CPU. All STP frames have their
tail tag not really at the end of the frame. It's off by exactly four
bytes. So, maybe it's the FCS.

The DSA master is a older stmmac. It has this code here:

|drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
|	/* Clear ACS bit because Ethernet switch tagging formats such as
|	 * Broadcom tags can look like invalid LLC/SNAP packets and cause the
|	 * hardware to truncate packets on reception.
|	 */
|	if (netdev_uses_dsa(dev) || !priv->plat->enh_desc)
|		value &= ~GMAC_CONTROL_ACS;
|

Actually, this has to be done. Without disabling the ACS bit the STP
frames are truncated and the trailer tag is gone.

Then, there is

|drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
|		/* ACS is set; GMAC core strips PAD/FCS for IEEE 802.3
|		 * Type frames (LLC/LLC-SNAP)
|		 *
|		 * llc_snap is never checked in GMAC >= 4, so this ACS
|		 * feature is always disabled and packets need to be
|		 * stripped manually.
|		 */
|		if (likely(!(status & rx_not_ls)) &&
|		    (likely(priv->synopsys_id >= DWMAC_CORE_4_00) ||
|		     unlikely(status != llc_snap))) {
|			if (buf2_len)
|				buf2_len -= ETH_FCS_LEN;
|			else
|				buf1_len -= ETH_FCS_LEN;
|
|			len -= ETH_FCS_LEN;
|		}

Great. Unfortunately the stmmac used here is a dwmac-3.70. So, the FCS
doesn't seem to be stripped for the STP frames.

Now, I'm currently testing the patch below and it seems to work:

|root@tsn:~# dmesg | grep -i 'Failed to get source port'
|root@tsn:~# tcpdump -i lan0 stp
|tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
|listening on lan0, link-type EN10MB (Ethernet), snapshot length 262144 bytes
|19:25:17.031699 STP 802.1w, Rapid STP, Flags [Learn, Forward, Agreement], bridge-id 8000.2c:1a:05:28:06:c1.8006, length 36

Thanks,
Kurt

Patch:

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 9c1e19ea6fcd..74f348e27005 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -41,6 +41,7 @@
 #include <linux/bpf_trace.h>
 #include <net/pkt_cls.h>
 #include <net/xdp_sock_drv.h>
+#include <net/dsa.h>
 #include "stmmac_ptp.h"
 #include "stmmac.h"
 #include "stmmac_xdp.h"
@@ -5209,6 +5210,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
                 */
                if (likely(!(status & rx_not_ls)) &&
                    (likely(priv->synopsys_id >= DWMAC_CORE_4_00) ||
+                    unlikely(netdev_uses_dsa(priv->dev)) ||
                     unlikely(status != llc_snap))) {
                        if (buf2_len)
                                buf2_len -= ETH_FCS_LEN;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net-next] net: dsa: hellcreek: Print warning only once

[Vladimir Oltean]

On Wed, Aug 31, 2022 at 09:34:22PM +0200, Kurt Kanzenbach wrote:
> I've plugged my hellcreek test device into a Cisco switch and saw these
> messages flying by. It said it failed to get source port 0 at a constant
> rate. Turns out the Cisco switch does RSTP by default. Every STP frame
> received has source port 0 which doesn't make any sense. The switch adds
> a tail tag to every frame towards the CPU. All STP frames have their
> tail tag not really at the end of the frame. It's off by exactly four
> bytes. So, maybe it's the FCS.

Hmm, I'm not sure I'm awake enough to be looking at this. So AFAIU, some
DWMAC versions can be configured to strip the padding and FCS from
"Length" frames (EtherType <= 0x600) via the ACS bit in the MAC
configuration register, and some others can also be configured to strip
the FCS from "Type" frames (EtherType > 0x800) via the CST bit of the
same register. Ok, I'll go with that, although I'm confused as to why
some DWMACs would have ACS but not CST available.

The stmmac driver does not support NETIF_F_RXFCS, so it wants frames
passed up the stack to never have the FCS. That is good. Except that the
frames passed via the RX buffer descriptors may sometimes have RX FCS,
and sometimes may not.

This means that the driver needs to make a determination of whether the
hardware has already stripped the FCS or not, before attempting to strip
the FCS by itself.

So we may have:

(a) FCS stripped by HW, left alone by SW => frame looks ok
(b) FCS stripped by HW and also by SW => frame is truncated by 4 bytes
(c) FCS left alone by HW, left alone by SW => frame has 4 bytes too many (the FCS)
(d) FCS left alone by HW, stripped by SW => frame looks ok

It seems from what you're saying that you're under circumstance (c).

> The DSA master is a older stmmac. It has this code here:
> 
> |drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> |	/* Clear ACS bit because Ethernet switch tagging formats such as
> |	 * Broadcom tags can look like invalid LLC/SNAP packets and cause the
> |	 * hardware to truncate packets on reception.
> |	 */
> |	if (netdev_uses_dsa(dev) || !priv->plat->enh_desc)
> |		value &= ~GMAC_CONTROL_ACS;
> |
> 
> Actually, this has to be done. Without disabling the ACS bit the STP
> frames are truncated and the trailer tag is gone.

So from Florian's comment above, he was under case (b), different than yours.
I don't understand why you say that when ACS is set, "the STP frames are
truncated and the trailer tag is gone". Simply altering the ACS bit
isn't going to change the determination made by stmmac_rx(). My guess
based on the current input I have is that it would work fine for you
(but possibly not for Florian).

> Then, there is
> 
> |drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> |		/* ACS is set; GMAC core strips PAD/FCS for IEEE 802.3
> |		 * Type frames (LLC/LLC-SNAP)
> |		 *
> |		 * llc_snap is never checked in GMAC >= 4, so this ACS
> |		 * feature is always disabled and packets need to be
> |		 * stripped manually.
> |		 */
> |		if (likely(!(status & rx_not_ls)) &&
> |		    (likely(priv->synopsys_id >= DWMAC_CORE_4_00) ||
> |		     unlikely(status != llc_snap))) {
> |			if (buf2_len)
> |				buf2_len -= ETH_FCS_LEN;
> |			else
> |				buf1_len -= ETH_FCS_LEN;
> |
> |			len -= ETH_FCS_LEN;
> |		}
> 
> Great. Unfortunately the stmmac used here is a dwmac-3.70. So, the FCS
> doesn't seem to be stripped for the STP frames.

Yes, neither by hardware nor by software.

It isn't stripped by hardware due to the ACS setting. And it isn't
stripped by software because, although the synopsys_id is smaller than 4.00,
there's only a single stmmac_desc_ops :: rx_status implementation which
will ever return "llc_snap" as parse result for a frame, and that isn't
yours.

> Now, I'm currently testing the patch below and it seems to work:
> 
> |root@tsn:~# dmesg | grep -i 'Failed to get source port'
> |root@tsn:~# tcpdump -i lan0 stp
> |tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
> |listening on lan0, link-type EN10MB (Ethernet), snapshot length 262144 bytes
> |19:25:17.031699 STP 802.1w, Rapid STP, Flags [Learn, Forward, Agreement], bridge-id 8000.2c:1a:05:28:06:c1.8006, length 36
> 
> Thanks,
> Kurt
> 
> Patch:
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 9c1e19ea6fcd..74f348e27005 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -41,6 +41,7 @@
>  #include <linux/bpf_trace.h>
>  #include <net/pkt_cls.h>
>  #include <net/xdp_sock_drv.h>
> +#include <net/dsa.h>
>  #include "stmmac_ptp.h"
>  #include "stmmac.h"
>  #include "stmmac_xdp.h"
> @@ -5209,6 +5210,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>                  */
>                 if (likely(!(status & rx_not_ls)) &&
>                     (likely(priv->synopsys_id >= DWMAC_CORE_4_00) ||
> +                    unlikely(netdev_uses_dsa(priv->dev)) ||

This change forces FCS stripping in software for all DWMACs used as DSA
masters.

Florian solved his problem by moving from case (b) to (d), so the change
should continue to work for him as well.

>                      unlikely(status != llc_snap))) {
>                         if (buf2_len)
>                                 buf2_len -= ETH_FCS_LEN;

My 2 cents on this topic are:

1. The software determination is way too complicated and hard to reason
   about, there are too many tests in the fast path, and you are adding
   one more (actually two, if you look at the implementation of netdev_uses_dsa).
   Additionally, someone will probably need to modify the zerocopy rx
   procedure as well, in a not so distant future. It must be taken into
   consideration how much worse would the performance be if the driver
   would just configure the hardware to never selectively strip the FCS
   (i.e. for some packets but not all; in practice this means never
   strip the FCS. Ideally it would *always* strip the FCS, but I'm not
   sure whether this is possible with other hardware pre-4.0).
   Considering that for the common case of IP traffic the FCS is already
   stripped in software, I think there will be a net gain by simplifying
   stmmac_rx() and leaving just the "BD really is last" check.

2. The FCS stripping logic actually looks wrong to me.

			if (buf2_len) {
				buf2_len -= ETH_FCS_LEN;
				len -= ETH_FCS_LEN;
			} else if (buf1_len) {
				buf1_len -= ETH_FCS_LEN;
				len -= ETH_FCS_LEN;
			}

   The "if (x != 0) x -= 4" idiom seems classically wrong, in that x may
   still be < 4, and this will result in a negative buf2_len, or buf1_len.

   Applied to reality, this would mean a scatter/gather frame where the FCS
   is split between 2 buffers. I don't think the driver handles this case
   well at all.

How large can you configure the hellcreek switch to send packets towards
the DSA master? Could you force a packet size (including hellcreek tail tag)
comparable to dma_conf->dma_buf_sz? Or if not, perhaps you could hack
the way in which stmmac_set_bfsize() works, or one of the constants?

Re: [PATCH net-next] net: dsa: hellcreek: Print warning only once

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 8317 bytes --]

On Thu Sep 01 2022, Vladimir Oltean wrote:
> On Wed, Aug 31, 2022 at 09:34:22PM +0200, Kurt Kanzenbach wrote:
>> I've plugged my hellcreek test device into a Cisco switch and saw these
>> messages flying by. It said it failed to get source port 0 at a constant
>> rate. Turns out the Cisco switch does RSTP by default. Every STP frame
>> received has source port 0 which doesn't make any sense. The switch adds
>> a tail tag to every frame towards the CPU. All STP frames have their
>> tail tag not really at the end of the frame. It's off by exactly four
>> bytes. So, maybe it's the FCS.
>
> Hmm, I'm not sure I'm awake enough to be looking at this. So AFAIU, some
> DWMAC versions can be configured to strip the padding and FCS from
> "Length" frames (EtherType <= 0x600) via the ACS bit in the MAC
> configuration register, and some others can also be configured to strip
> the FCS from "Type" frames (EtherType > 0x800) via the CST bit of the
> same register. Ok, I'll go with that, although I'm confused as to why
> some DWMACs would have ACS but not CST available.
>
> The stmmac driver does not support NETIF_F_RXFCS, so it wants frames
> passed up the stack to never have the FCS. That is good. Except that the
> frames passed via the RX buffer descriptors may sometimes have RX FCS,
> and sometimes may not.
>
> This means that the driver needs to make a determination of whether the
> hardware has already stripped the FCS or not, before attempting to strip
> the FCS by itself.
>
> So we may have:
>
> (a) FCS stripped by HW, left alone by SW => frame looks ok
> (b) FCS stripped by HW and also by SW => frame is truncated by 4 bytes
> (c) FCS left alone by HW, left alone by SW => frame has 4 bytes too many (the FCS)
> (d) FCS left alone by HW, stripped by SW => frame looks ok
>
> It seems from what you're saying that you're under circumstance (c).

Yes, exactly.

>
>> The DSA master is a older stmmac. It has this code here:
>> 
>> |drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
>> |	/* Clear ACS bit because Ethernet switch tagging formats such as
>> |	 * Broadcom tags can look like invalid LLC/SNAP packets and cause the
>> |	 * hardware to truncate packets on reception.
>> |	 */
>> |	if (netdev_uses_dsa(dev) || !priv->plat->enh_desc)
>> |		value &= ~GMAC_CONTROL_ACS;
>> |
>> 
>> Actually, this has to be done. Without disabling the ACS bit the STP
>> frames are truncated and the trailer tag is gone.
>
> So from Florian's comment above, he was under case (b), different than yours.
> I don't understand why you say that when ACS is set, "the STP frames are
> truncated and the trailer tag is gone". Simply altering the ACS bit
> isn't going to change the determination made by stmmac_rx(). My guess
> based on the current input I have is that it would work fine for you
> (but possibly not for Florian).

I thought so too. However, altering the ACS Bit didn't help at all.

>
>> Then, there is
>> 
>> |drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> |		/* ACS is set; GMAC core strips PAD/FCS for IEEE 802.3
>> |		 * Type frames (LLC/LLC-SNAP)
>> |		 *
>> |		 * llc_snap is never checked in GMAC >= 4, so this ACS
>> |		 * feature is always disabled and packets need to be
>> |		 * stripped manually.
>> |		 */
>> |		if (likely(!(status & rx_not_ls)) &&
>> |		    (likely(priv->synopsys_id >= DWMAC_CORE_4_00) ||
>> |		     unlikely(status != llc_snap))) {
>> |			if (buf2_len)
>> |				buf2_len -= ETH_FCS_LEN;
>> |			else
>> |				buf1_len -= ETH_FCS_LEN;
>> |
>> |			len -= ETH_FCS_LEN;
>> |		}
>> 
>> Great. Unfortunately the stmmac used here is a dwmac-3.70. So, the FCS
>> doesn't seem to be stripped for the STP frames.
>
> Yes, neither by hardware nor by software.
>
> It isn't stripped by hardware due to the ACS setting. And it isn't
> stripped by software because, although the synopsys_id is smaller than 4.00,
> there's only a single stmmac_desc_ops :: rx_status implementation which
> will ever return "llc_snap" as parse result for a frame, and that isn't
> yours.
>
>> Now, I'm currently testing the patch below and it seems to work:
>> 
>> |root@tsn:~# dmesg | grep -i 'Failed to get source port'
>> |root@tsn:~# tcpdump -i lan0 stp
>> |tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
>> |listening on lan0, link-type EN10MB (Ethernet), snapshot length 262144 bytes
>> |19:25:17.031699 STP 802.1w, Rapid STP, Flags [Learn, Forward, Agreement], bridge-id 8000.2c:1a:05:28:06:c1.8006, length 36
>> 
>> Thanks,
>> Kurt
>> 
>> Patch:
>> 
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 9c1e19ea6fcd..74f348e27005 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -41,6 +41,7 @@
>>  #include <linux/bpf_trace.h>
>>  #include <net/pkt_cls.h>
>>  #include <net/xdp_sock_drv.h>
>> +#include <net/dsa.h>
>>  #include "stmmac_ptp.h"
>>  #include "stmmac.h"
>>  #include "stmmac_xdp.h"
>> @@ -5209,6 +5210,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>>                  */
>>                 if (likely(!(status & rx_not_ls)) &&
>>                     (likely(priv->synopsys_id >= DWMAC_CORE_4_00) ||
>> +                    unlikely(netdev_uses_dsa(priv->dev)) ||
>
> This change forces FCS stripping in software for all DWMACs used as DSA
> masters.
>
> Florian solved his problem by moving from case (b) to (d), so the change
> should continue to work for him as well.
>
>>                      unlikely(status != llc_snap))) {
>>                         if (buf2_len)
>>                                 buf2_len -= ETH_FCS_LEN;
>
> My 2 cents on this topic are:
>
> 1. The software determination is way too complicated and hard to reason
>    about, there are too many tests in the fast path, and you are adding
>    one more (actually two, if you look at the implementation of netdev_uses_dsa).

Yes, exactly. That's why I dislike the extra check.

>    Additionally, someone will probably need to modify the zerocopy rx
>    procedure as well, in a not so distant future.

Yes, i know.

>    It must be taken into consideration how much worse would the
>    performance be if the driver would just configure the hardware to
>    never selectively strip the FCS (i.e. for some packets but not all;
>    in practice this means never strip the FCS. Ideally it would
>    *always* strip the FCS, but I'm not sure whether this is possible
>    with other hardware pre-4.0).  Considering that for the common case
>    of IP traffic the FCS is already stripped in software, I think
>    there will be a net gain by simplifying stmmac_rx() and leaving
>    just the "BD really is last" check.

I was thinking exactly the same. The FCS strip determination logic seems
complicated and is performed in the fast path.

From what I see, the stmmac strips the FCS in hardware only for IEEE
802.3 type frames and older versions. Meaning in most cases today the
FCS is stripped in software anyway.

We could do some measurements e.g., with perf to determine whether
removing the FCS logic has positive or negative effects?

>
> 2. The FCS stripping logic actually looks wrong to me.
>
> 			if (buf2_len) {
> 				buf2_len -= ETH_FCS_LEN;
> 				len -= ETH_FCS_LEN;
> 			} else if (buf1_len) {
> 				buf1_len -= ETH_FCS_LEN;
> 				len -= ETH_FCS_LEN;
> 			}
>
>    The "if (x != 0) x -= 4" idiom seems classically wrong, in that x may
>    still be < 4, and this will result in a negative buf2_len, or buf1_len.
>
>    Applied to reality, this would mean a scatter/gather frame where the FCS
>    is split between 2 buffers. I don't think the driver handles this case
>    well at all.

Oh, well.

>
> How large can you configure the hellcreek switch to send packets towards
> the DSA master? Could you force a packet size (including hellcreek tail tag)
> comparable to dma_conf->dma_buf_sz?

I don't think so.

> Or if not, perhaps you could hack the way in which stmmac_set_bfsize()
> works, or one of the constants?

I'm not sure if i follow you here.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net-next] net: dsa: hellcreek: Print warning only once

[Vladimir Oltean]

On Thu, Sep 01, 2022 at 08:21:33AM +0200, Kurt Kanzenbach wrote:
> > So from Florian's comment above, he was under case (b), different than yours.
> > I don't understand why you say that when ACS is set, "the STP frames are
> > truncated and the trailer tag is gone". Simply altering the ACS bit
> > isn't going to change the determination made by stmmac_rx(). My guess
> > based on the current input I have is that it would work fine for you
> > (but possibly not for Florian).
> 
> I thought so too. However, altering the ACS Bit didn't help at all.

This is curious. Could you dump the Length/Type Field (LT bits 18:16)
from the RDES3 for these packets? If ACS does not take effect, it would
mean the DWMAC doesn't think they're Length packets I guess? Also, does
the Error Summary say anything? In principle, the length of this packet
is greater than the EtherType/Length would say, by the size of the tail
tag. Not sure how that affects the RX parser.

> We could do some measurements e.g., with perf to determine whether
> removing the FCS logic has positive or negative effects?

Yes, some IP forwarding of 60 byte frames at line rate gigabit or higher
should do the trick. Testing with MTU sized packets is probably not
going to show much of a difference.

> > How large can you configure the hellcreek switch to send packets towards
> > the DSA master? Could you force a packet size (including hellcreek tail tag)
> > comparable to dma_conf->dma_buf_sz?
> 
> I don't think so.
> 
> > Or if not, perhaps you could hack the way in which stmmac_set_bfsize()
> > works, or one of the constants?
> 
> I'm not sure if i follow you here.

I mean if you can't bring the MTU of the switch closer to the buffer
size of the DSA master, at least bring the buffer size closer to the MTU
of the switch. If this means, idk, hacking the DEFAULT_BUFSIZE macro to
a lower value such as to force splitting some otherwise "normal" packet
sizes into 2 buffers, fine.

Then, the next step would be to force this splitting to occur exactly
where the FCS lies in the buffers. Then I should be able to hear the
kaboom from over here.

Re: [PATCH net-next] net: dsa: hellcreek: Print warning only once

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 8639 bytes --]

On Thu Sep 01 2022, Vladimir Oltean wrote:
> On Thu, Sep 01, 2022 at 08:21:33AM +0200, Kurt Kanzenbach wrote:
>> > So from Florian's comment above, he was under case (b), different than yours.
>> > I don't understand why you say that when ACS is set, "the STP frames are
>> > truncated and the trailer tag is gone". Simply altering the ACS bit
>> > isn't going to change the determination made by stmmac_rx(). My guess
>> > based on the current input I have is that it would work fine for you
>> > (but possibly not for Florian).
>> 
>> I thought so too. However, altering the ACS Bit didn't help at all.
>
> This is curious. Could you dump the Length/Type Field (LT bits 18:16)
> from the RDES3 for these packets? If ACS does not take effect, it would
> mean the DWMAC doesn't think they're Length packets I guess? Also, does
> the Error Summary say anything? In principle, the length of this packet
> is greater than the EtherType/Length would say, by the size of the tail
> tag. Not sure how that affects the RX parser.

That's the point. The RX parser seems to be affected by the tail
tag. I'll have a look at the packets with ACS feature set.

>
>> We could do some measurements e.g., with perf to determine whether
>> removing the FCS logic has positive or negative effects?
>
> Yes, some IP forwarding of 60 byte frames at line rate gigabit or higher
> should do the trick. Testing with MTU sized packets is probably not
> going to show much of a difference.

Well, I don't see much of a difference. However, running iperf3 with
small packets is nowhere near line rate of 100Mbit/s. Nevertheless, I
like the following patch more, because it looks cleaner than adding more
checks to the receive path. It fixes my problem. Florian's use case
should also work.

From 5a54383167c624a90ba460531fccabbb87d40e51 Mon Sep 17 00:00:00 2001
From: Kurt Kanzenbach <kurt@linutronix.de>
Date: Fri, 2 Sep 2022 19:49:52 +0200
Subject: [PATCH] net: stmmac: Disable automatic FCS/Pad stripping

The stmmac has the possibility to automatically strip the padding/FCS for IEEE
802.3 type frames. This feature is enabled conditionally. Therefore, the stmmac
receive path has to have a determination logic whether the FCS has to be
stripped in software or not.

In fact, for DSA this ACS feature is disabled and the determination logic
doesn't check for it properly. For instance, when using DSA in combination with
an older stmmac (pre version 4), the FCS is not stripped by hardware or software
which is problematic.

So either add another check for DSA to the fast path or simply disable ACS
feature completely. The latter approach has been chosen, because most of the
time the FCS is stripped in software anyway and it removes conditionals from the
receive fast path.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 .../net/ethernet/stmicro/stmmac/dwmac100.h    |  2 +-
 .../net/ethernet/stmicro/stmmac/dwmac1000.h   |  2 +-
 .../ethernet/stmicro/stmmac/dwmac1000_core.c  |  9 -------
 .../ethernet/stmicro/stmmac/dwmac100_core.c   |  8 ------
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 26 +++----------------
 5 files changed, 6 insertions(+), 41 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac100.h b/drivers/net/ethernet/stmicro/stmmac/dwmac100.h
index 35ab8d0bdce7..7ab791c8d355 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac100.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac100.h
@@ -56,7 +56,7 @@
 #define MAC_CONTROL_TE		0x00000008	/* Transmitter Enable */
 #define MAC_CONTROL_RE		0x00000004	/* Receiver Enable */
 
-#define MAC_CORE_INIT (MAC_CONTROL_HBD | MAC_CONTROL_ASTP)
+#define MAC_CORE_INIT (MAC_CONTROL_HBD)
 
 /* MAC FLOW CTRL defines */
 #define MAC_FLOW_CTRL_PT_MASK	0xffff0000	/* Pause Time Mask */
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
index 3c73453725f9..4296ddda8aaa 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
@@ -126,7 +126,7 @@ enum inter_frame_gap {
 #define GMAC_CONTROL_TE		0x00000008	/* Transmitter Enable */
 #define GMAC_CONTROL_RE		0x00000004	/* Receiver Enable */
 
-#define GMAC_CORE_INIT (GMAC_CONTROL_JD | GMAC_CONTROL_PS | GMAC_CONTROL_ACS | \
+#define GMAC_CORE_INIT (GMAC_CONTROL_JD | GMAC_CONTROL_PS | \
 			GMAC_CONTROL_BE | GMAC_CONTROL_DCRS)
 
 /* GMAC Frame Filter defines */
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
index fc8759f146c7..35d7c1cb1cf1 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
@@ -15,7 +15,6 @@
 #include <linux/crc32.h>
 #include <linux/slab.h>
 #include <linux/ethtool.h>
-#include <net/dsa.h>
 #include <asm/io.h>
 #include "stmmac.h"
 #include "stmmac_pcs.h"
@@ -24,7 +23,6 @@
 static void dwmac1000_core_init(struct mac_device_info *hw,
 				struct net_device *dev)
 {
-	struct stmmac_priv *priv = netdev_priv(dev);
 	void __iomem *ioaddr = hw->pcsr;
 	u32 value = readl(ioaddr + GMAC_CONTROL);
 	int mtu = dev->mtu;
@@ -32,13 +30,6 @@ static void dwmac1000_core_init(struct mac_device_info *hw,
 	/* Configure GMAC core */
 	value |= GMAC_CORE_INIT;
 
-	/* Clear ACS bit because Ethernet switch tagging formats such as
-	 * Broadcom tags can look like invalid LLC/SNAP packets and cause the
-	 * hardware to truncate packets on reception.
-	 */
-	if (netdev_uses_dsa(dev) || !priv->plat->enh_desc)
-		value &= ~GMAC_CONTROL_ACS;
-
 	if (mtu > 1500)
 		value |= GMAC_CONTROL_2K;
 	if (mtu > 2000)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac100_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac100_core.c
index ebcad8dd99db..f799f8f824e8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac100_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac100_core.c
@@ -15,7 +15,6 @@
 *******************************************************************************/
 
 #include <linux/crc32.h>
-#include <net/dsa.h>
 #include <asm/io.h>
 #include "stmmac.h"
 #include "dwmac100.h"
@@ -28,13 +27,6 @@ static void dwmac100_core_init(struct mac_device_info *hw,
 
 	value |= MAC_CORE_INIT;
 
-	/* Clear ASTP bit because Ethernet switch tagging formats such as
-	 * Broadcom tags can look like invalid LLC/SNAP packets and cause the
-	 * hardware to truncate packets on reception.
-	 */
-	if (netdev_uses_dsa(dev))
-		value &= ~MAC_CONTROL_ASTP;
-
 	writel(value, ioaddr + MAC_CONTROL);
 
 #ifdef STMMAC_VLAN_TAG_USED
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 74f348e27005..0df248a5321e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -41,7 +41,6 @@
 #include <linux/bpf_trace.h>
 #include <net/pkt_cls.h>
 #include <net/xdp_sock_drv.h>
-#include <net/dsa.h>
 #include "stmmac_ptp.h"
 #include "stmmac.h"
 #include "stmmac_xdp.h"
@@ -5015,16 +5014,8 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 		buf1_len = stmmac_rx_buf1_len(priv, p, status, len);
 		len += buf1_len;
 
-		/* ACS is set; GMAC core strips PAD/FCS for IEEE 802.3
-		 * Type frames (LLC/LLC-SNAP)
-		 *
-		 * llc_snap is never checked in GMAC >= 4, so this ACS
-		 * feature is always disabled and packets need to be
-		 * stripped manually.
-		 */
-		if (likely(!(status & rx_not_ls)) &&
-		    (likely(priv->synopsys_id >= DWMAC_CORE_4_00) ||
-		     unlikely(status != llc_snap))) {
+		/* ACS is disabled; strip manually. */
+		if (likely(!(status & rx_not_ls))) {
 			buf1_len -= ETH_FCS_LEN;
 			len -= ETH_FCS_LEN;
 		}
@@ -5201,17 +5192,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		buf2_len = stmmac_rx_buf2_len(priv, p, status, len);
 		len += buf2_len;
 
-		/* ACS is set; GMAC core strips PAD/FCS for IEEE 802.3
-		 * Type frames (LLC/LLC-SNAP)
-		 *
-		 * llc_snap is never checked in GMAC >= 4, so this ACS
-		 * feature is always disabled and packets need to be
-		 * stripped manually.
-		 */
-		if (likely(!(status & rx_not_ls)) &&
-		    (likely(priv->synopsys_id >= DWMAC_CORE_4_00) ||
-		     unlikely(netdev_uses_dsa(priv->dev)) ||
-		     unlikely(status != llc_snap))) {
+		/* ACS is disabled; strip manually */
+		if (likely(!(status & rx_not_ls))) {
 			if (buf2_len)
 				buf2_len -= ETH_FCS_LEN;
 			else
-- 
2.30.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net-next] net: dsa: hellcreek: Print warning only once

[Vladimir Oltean]

On Sat, Sep 03, 2022 at 03:24:33PM +0200, Kurt Kanzenbach wrote:
> > Yes, some IP forwarding of 60 byte frames at line rate gigabit or higher
> > should do the trick. Testing with MTU sized packets is probably not
> > going to show much of a difference.
> 
> Well, I don't see much of a difference. However, running iperf3 with
> small packets is nowhere near line rate of 100Mbit/s.

Try something smarter than iperf3, like CONFIG_NET_PKTGEN=y on a link
partner as traffic generator. I had this command lying around, I forget
exactly what it does.

/root/git/net-next/samples/pktgen/pktgen_sample03_burst_single_flow.sh \
	-i eth1 -s 64 -m 00:04:9f:05:de:0a -d 20.0.0.2  -t 2 -f 13 -c 4 -p 400 -n 0 -b 4

Re: [PATCH net-next] net: dsa: hellcreek: Print warning only once

[Florian Fainelli]

On 9/3/2022 6:24 AM, Kurt Kanzenbach wrote:
> On Thu Sep 01 2022, Vladimir Oltean wrote:
>> On Thu, Sep 01, 2022 at 08:21:33AM +0200, Kurt Kanzenbach wrote:
>>>> So from Florian's comment above, he was under case (b), different than yours.
>>>> I don't understand why you say that when ACS is set, "the STP frames are
>>>> truncated and the trailer tag is gone". Simply altering the ACS bit
>>>> isn't going to change the determination made by stmmac_rx(). My guess
>>>> based on the current input I have is that it would work fine for you
>>>> (but possibly not for Florian).
>>>
>>> I thought so too. However, altering the ACS Bit didn't help at all.
>>
>> This is curious. Could you dump the Length/Type Field (LT bits 18:16)
>> from the RDES3 for these packets? If ACS does not take effect, it would
>> mean the DWMAC doesn't think they're Length packets I guess? Also, does
>> the Error Summary say anything? In principle, the length of this packet
>> is greater than the EtherType/Length would say, by the size of the tail
>> tag. Not sure how that affects the RX parser.
> 
> That's the point. The RX parser seems to be affected by the tail
> tag. I'll have a look at the packets with ACS feature set.
> 
>>
>>> We could do some measurements e.g., with perf to determine whether
>>> removing the FCS logic has positive or negative effects?
>>
>> Yes, some IP forwarding of 60 byte frames at line rate gigabit or higher
>> should do the trick. Testing with MTU sized packets is probably not
>> going to show much of a difference.
> 
> Well, I don't see much of a difference. However, running iperf3 with
> small packets is nowhere near line rate of 100Mbit/s. Nevertheless, I
> like the following patch more, because it looks cleaner than adding more
> checks to the receive path. It fixes my problem. Florian's use case
> should also work.

LGTM, some suggestions below.

> 
>  From 5a54383167c624a90ba460531fccabbb87d40e51 Mon Sep 17 00:00:00 2001
> From: Kurt Kanzenbach <kurt@linutronix.de>
> Date: Fri, 2 Sep 2022 19:49:52 +0200
> Subject: [PATCH] net: stmmac: Disable automatic FCS/Pad stripping
> 
> The stmmac has the possibility to automatically strip the padding/FCS for IEEE
> 802.3 type frames. This feature is enabled conditionally. Therefore, the stmmac
> receive path has to have a determination logic whether the FCS has to be
> stripped in software or not.
> 
> In fact, for DSA this ACS feature is disabled and the determination logic
> doesn't check for it properly. For instance, when using DSA in combination with
> an older stmmac (pre version 4), the FCS is not stripped by hardware or software
> which is problematic.
> 
> So either add another check for DSA to the fast path or simply disable ACS
> feature completely. The latter approach has been chosen, because most of the
> time the FCS is stripped in software anyway and it removes conditionals from the
> receive fast path.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>   .../net/ethernet/stmicro/stmmac/dwmac100.h    |  2 +-
>   .../net/ethernet/stmicro/stmmac/dwmac1000.h   |  2 +-
>   .../ethernet/stmicro/stmmac/dwmac1000_core.c  |  9 -------
>   .../ethernet/stmicro/stmmac/dwmac100_core.c   |  8 ------
>   .../net/ethernet/stmicro/stmmac/stmmac_main.c | 26 +++----------------
>   5 files changed, 6 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac100.h b/drivers/net/ethernet/stmicro/stmmac/dwmac100.h
> index 35ab8d0bdce7..7ab791c8d355 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac100.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac100.h
> @@ -56,7 +56,7 @@
>   #define MAC_CONTROL_TE		0x00000008	/* Transmitter Enable */
>   #define MAC_CONTROL_RE		0x00000004	/* Receiver Enable */
>   
> -#define MAC_CORE_INIT (MAC_CONTROL_HBD | MAC_CONTROL_ASTP)
> +#define MAC_CORE_INIT (MAC_CONTROL_HBD)
>   
>   /* MAC FLOW CTRL defines */
>   #define MAC_FLOW_CTRL_PT_MASK	0xffff0000	/* Pause Time Mask */
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
> index 3c73453725f9..4296ddda8aaa 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000.h
> @@ -126,7 +126,7 @@ enum inter_frame_gap {
>   #define GMAC_CONTROL_TE		0x00000008	/* Transmitter Enable */
>   #define GMAC_CONTROL_RE		0x00000004	/* Receiver Enable */
>   
> -#define GMAC_CORE_INIT (GMAC_CONTROL_JD | GMAC_CONTROL_PS | GMAC_CONTROL_ACS | \
> +#define GMAC_CORE_INIT (GMAC_CONTROL_JD | GMAC_CONTROL_PS | \
>   			GMAC_CONTROL_BE | GMAC_CONTROL_DCRS)
>   
>   /* GMAC Frame Filter defines */
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> index fc8759f146c7..35d7c1cb1cf1 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac1000_core.c
> @@ -15,7 +15,6 @@
>   #include <linux/crc32.h>
>   #include <linux/slab.h>
>   #include <linux/ethtool.h>
> -#include <net/dsa.h>
>   #include <asm/io.h>
>   #include "stmmac.h"
>   #include "stmmac_pcs.h"
> @@ -24,7 +23,6 @@
>   static void dwmac1000_core_init(struct mac_device_info *hw,
>   				struct net_device *dev)

You should be able to remove the net_device reference here since we do 
not use it anymore after the removal of netdev_uses_dsa() or 
de-referencing of priv.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next] net: dsa: hellcreek: Print warning only once

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]

>>   static void dwmac1000_core_init(struct mac_device_info *hw,
>>   				struct net_device *dev)
>
> You should be able to remove the net_device reference here since we do 
> not use it anymore after the removal of netdev_uses_dsa() or 
> de-referencing of priv.

That's true for dwmac1000_core_init(). However, dwmac4_core_init() is
using priv now for some timestamping waitqueue initialization.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH net-next] net: dsa: hellcreek: Print warning only once

[Jakub Kicinski]

On Tue, 30 Aug 2022 18:34:48 +0200 Kurt Kanzenbach wrote:
> In case the source port cannot be decoded, print the warning only once. This
> still brings attention to the user and does not spam the logs at the same time.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

The ongoing convo is tangential AFAIU, so applying.
Applying to net, 'cause printing warnings based on packet
contents without a rate limit is a bad idea.

Re: [PATCH net-next] net: dsa: hellcreek: Print warning only once

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 30 Aug 2022 18:34:48 +0200 you wrote:
> In case the source port cannot be decoded, print the warning only once. This
> still brings attention to the user and does not spam the logs at the same time.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  net/dsa/tag_hellcreek.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Here is the summary with links:
  - [net-next] net: dsa: hellcreek: Print warning only once
    https://git.kernel.org/netdev/net/c/52267ce25f60

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html