Re: [PATCH net] net: dsa: mt7530: don't change PVC_EG_TAG when CPU port becomes VLAN-aware

From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: "Arınç ÜNAL" <arinc.unal@arinc9.com>
Cc: Andrew Lunn <andrew@lunn.ch>,
	Landen Chao <Landen.Chao@mediatek.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	netdev@vger.kernel.org, Sean Wang <sean.wang@mediatek.com>,
	linux-kernel@vger.kernel.org, richard@routerhints.com,
	DENG Qingfang <dqfext@gmail.com>,
	Eric Dumazet <edumazet@google.com>,
	linux-mediatek@lists.infradead.org,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	erkin.bozoglu@xeront.com, "David S. Miller" <davem@davemloft.net>,
	linux-arm-kernel@lists.infradead.org,
	AngeloGioacchino Del Regno
	<angelogioacchino.delregno@collabora.com>
Subject: Re: [PATCH net] net: dsa: mt7530: don't change PVC_EG_TAG when CPU port becomes VLAN-aware
Date: Mon, 6 Feb 2023 01:50:53 +0200	[thread overview]
Message-ID: <20230205235053.g5cttegcdsvh7uk3@skbuf> (raw)
In-Reply-To: <b055e42f-ff0f-d05a-d462-961694b035c1@arinc9.com>

On Mon, Feb 06, 2023 at 02:02:48AM +0300, Arınç ÜNAL wrote:
> # ethtool -S eth1 | grep -v ': 0'
> NIC statistics:
>      tx_bytes: 6272
>      tx_packets: 81
>      rx_bytes: 9089
>      rx_packets: 136
>      p05_TxUnicast: 52
>      p05_TxMulticast: 3
>      p05_TxBroadcast: 81
>      p05_TxPktSz65To127: 136
>      p05_TxBytes: 9633
>      p05_RxFiltering: 11
>      p05_RxUnicast: 11
>      p05_RxMulticast: 26
>      p05_RxBroadcast: 44
>      p05_RxPktSz64: 47
>      p05_RxPktSz65To127: 34
>      p05_RxBytes: 6272
> # ethtool -S eth1 | grep -v ': 0'
> NIC statistics:
>      tx_bytes: 6784
>      tx_packets: 89
>      rx_bytes: 9601
>      rx_packets: 144
>      p05_TxUnicast: 60
>      p05_TxMulticast: 3
>      p05_TxBroadcast: 81
>      p05_TxPktSz65To127: 144
>      p05_TxBytes: 10177
>      p05_RxFiltering: 11
>      p05_RxUnicast: 11
>      p05_RxMulticast: 26
>      p05_RxBroadcast: 52
>      p05_RxPktSz64: 55
>      p05_RxPktSz65To127: 34
>      p05_RxBytes: 6784
> # ethtool -S eth1 | grep -v ': 0'
> NIC statistics:
>      tx_bytes: 7424
>      tx_packets: 99
>      rx_bytes: 10241
>      rx_packets: 154
>      p05_TxUnicast: 70
>      p05_TxMulticast: 3
>      p05_TxBroadcast: 81
>      p05_TxPktSz65To127: 154
>      p05_TxBytes: 10857
>      p05_RxFiltering: 11
>      p05_RxUnicast: 11
>      p05_RxMulticast: 26
>      p05_RxBroadcast: 62
>      p05_RxPktSz64: 65
>      p05_RxPktSz65To127: 34
>      p05_RxBytes: 7424

I see no signs of packet loss on the DSA master or the CPU port.
However my analysis of the packets shows:

> # tcpdump -i eth1 -e -n -Q in -XX
> tcpdump: verbose output suppressed, use -v[v]... for full protocol decode
> listening on eth1, link-type NULL (BSD loopback), snapshot length 262144 bytes
> 03:50:38.645568 AF Unknown (2459068999), length 60: 
> 	0x0000:  9292 6a47 1ac0 e0d5 5ea4 edcc 0806 0001  ..jG....^.......
                 ^              ^              ^
                 |              |              |
                 |              |              ETH_P_ARP
                 |              MAC SA:
                 |              e0:d5:5e:a4:ed:cc
                 MAC DA:
                 92:92:6a:47:1a:c0

> 	0x0010:  0800 0604 0002 e0d5 5ea4 edcc c0a8 0202  ........^.......
> 	0x0020:  9292 6a47 1ac0 c0a8 0201 0000 0000 0000  ..jG............
> 	0x0030:  0000 0000 0000 0000 0000 0000            ............

So you have no tag_mtk header in the EtherType position where it's
supposed to be. This means you must be making use of the hardware DSA
untagging feature that Felix Fietkau added.

Let's do some debugging. I'd like to know 2 things, in this order.
First, whether DSA sees the accelerated header (stripped by hardware, as
opposed to being present in the packet):

diff --git a/net/dsa/tag.c b/net/dsa/tag.c
index b2fba1a003ce..e64628cf7fc1 100644
--- a/net/dsa/tag.c
+++ b/net/dsa/tag.c
@@ -75,12 +75,17 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
 		if (!skb_has_extensions(skb))
 			skb->slow_gro = 0;
 
+		netdev_err(dev, "%s: skb %px metadata dst contains port id %d attached\n",
+			   __func__, skb, port);
+
 		skb->dev = dsa_master_find_slave(dev, 0, port);
 		if (likely(skb->dev)) {
 			dsa_default_offload_fwd_mark(skb);
 			nskb = skb;
 		}
 	} else {
+		netdev_err(dev, "%s: there is no metadata dst attached to skb 0x%px\n",
+			   __func__, skb);
 		nskb = cpu_dp->rcv(skb, dev);
 	}
 

And second, which is what does the DSA master actually see, and put in
the skb metadata dst field:

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index f1cb1efc94cf..e7ff569959b4 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -2077,11 +2077,23 @@ static int mtk_poll_rx(struct napi_struct *napi, int budget,
 		if (skb_vlan_tag_present(skb) && netdev_uses_dsa(netdev)) {
 			unsigned int port = ntohs(skb->vlan_proto) & GENMASK(2, 0);
 
+			netdev_err(netdev, "%s: skb->vlan_proto 0x%x port %d\n", __func__,
+				   ntohs(skb->vlan_proto), port);
+
 			if (port < ARRAY_SIZE(eth->dsa_meta) &&
-			    eth->dsa_meta[port])
+			    eth->dsa_meta[port]) {
+				netdev_err(netdev, "%s: attaching metadata dst with port %d to skb 0x%px\n",
+					   __func__, port, skb);
 				skb_dst_set_noref(skb, &eth->dsa_meta[port]->dst);
+			} else {
+				netdev_err(netdev, "%s: not attaching any metadata dst to skb 0x%px\n",
+					   __func__, skb);
+			}
 
 			__vlan_hwaccel_clear_tag(skb);
+		} else if (netdev_uses_dsa(netdev)) {
+			netdev_err(netdev, "%s: received skb 0x%px without VLAN/DSA tag present\n",
+				   __func__, skb);
 		}
 
 		skb_record_rx_queue(skb, 0);

Be warned that there may be a considerable amount of output to the console,
so it would be best if you used a single switch port with small amounts
of traffic.

