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

From: Kurt Kanzenbach <kurt@linutronix.de>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	netdev@vger.kernel.org
Subject: Re: [PATCH net-next] net: dsa: hellcreek: Print warning only once
Date: Wed, 31 Aug 2022 21:34:22 +0200	[thread overview]
Message-ID: <87v8q8jjgh.fsf@kurt> (raw)
In-Reply-To: <20220831152628.um4ktfj4upcz7zwq@skbuf>

[-- 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 --]
