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

["Arınç ÜNAL" <arinc.unal@arinc9.com>]

Finally I got time. It's been a seismically active day where I'm from.

On 6.02.2023 02:50, Vladimir Oltean wrote:
> 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);
>   	}
>   

# ping 192.168.2.2
PING 192.168.2.2[   39.508013] mtk_soc_eth 1b100000.ethernet eth1: 
dsa_switch_rcv: there is no metadata dst attached to skb 0xc2dfecc0
  (192.168.2.2): 56 data bytes
[   40.558253] mtk_soc_eth 1b100000.ethernet eth1: dsa_switch_rcv: there 
is no metadata dst attached to skb 0xc2dfed80
^C
--- 192.168.2.2 ping statistics ---
2 packets transmitted, 0 packets received, 100% packet loss
# [   41.598312] mtk_soc_eth 1b100000.ethernet eth1: dsa_switch_rcv: 
there is no metadata dst attached to skb 0xc2dfee40
[   55.432363] mtk_soc_eth 1b100000.ethernet eth1: dsa_switch_rcv: there 
is no metadata dst attached to skb 0xc2dfef00
[   56.442233] mtk_soc_eth 1b100000.ethernet eth1: dsa_switch_rcv: there 
is no metadata dst attached to skb 0xc2dfef00
[   57.466253] mtk_soc_eth 1b100000.ethernet eth1: dsa_switch_rcv: there 
is no metadata dst attached to skb 0xc2dfef00
[   60.538211] mtk_soc_eth 1b100000.ethernet eth1: dsa_switch_rcv: there 
is no metadata dst attached to skb 0xc2dfef00
[   61.562191] mtk_soc_eth 1b100000.ethernet eth1: dsa_switch_rcv: there 
is no metadata dst attached to skb 0xc2dfec00
[   62.586190] mtk_soc_eth 1b100000.ethernet eth1: dsa_switch_rcv: there 
is no metadata dst attached to skb 0xc2dfeb40

On a working port:

[  113.278462] mt7530 mdio-bus:1f wan: Link is Down
[  113.283214] br0: port 1(wan) entered disabled state
[  115.438955] mt7530 mdio-bus:1f lan0: Link is Up - 1Gbps/Full - flow 
control off
[  115.446332] br0: port 2(lan0) entered blocking state
[  115.451346] br0: port 2(lan0) entered forwarding state
[  118.007199] mtk_soc_eth 1b100000.ethernet eth1: dsa_switch_rcv: skb 
c2dfeb40 metadata dst contains port id 1 attached
[  118.018209] mtk_soc_eth 1b100000.ethernet eth1: dsa_switch_rcv: skb 
c2dfeb40 metadata dst contains port id 1 attached
[  119.009252] mtk_soc_eth 1b100000.ethernet eth1: dsa_switch_rcv: skb 
c2dfed80 metadata dst contains port id 1 attached
[  120.010470] mtk_soc_eth 1b100000.ethernet eth1: dsa_switch_rcv: skb 
c2dfed80 metadata dst contains port id 1 attached
[  123.038246] mtk_soc_eth 1b100000.ethernet eth1: dsa_switch_rcv: skb 
c2dfe900 metadata dst contains port id 1 attached

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

# ping 192.168.2.2
PING 192.168.2.2[   22.674182] mtk_soc_eth 1b100000.ethernet eth1: 
mtk_poll_rx: received skb 0xc2d67840 without VLAN/DSA tag present
  (192.168.2.2): 56 data bytes
[   23.678336] mtk_soc_eth 1b100000.ethernet eth1: mtk_poll_rx: received 
skb 0xc2d67840 without VLAN/DSA tag present
[   24.718355] mtk_soc_eth 1b100000.ethernet eth1: mtk_poll_rx: received 
skb 0xc2d67840 without VLAN/DSA tag present
^C
--- 192.168.2.2 ping statistics ---
4 packets transmitted, 0 packets received, 100% packet loss
# [   28.757693] mtk_soc_eth 1b100000.ethernet eth1: mtk_poll_rx: 
received skb 0xc2d67840 without VLAN/DSA tag present
[   29.758347] mtk_soc_eth 1b100000.ethernet eth1: mtk_poll_rx: received 
skb 0xc2d67840 without VLAN/DSA tag present
[   30.782404] mtk_soc_eth 1b100000.ethernet eth1: mtk_poll_rx: received 
skb 0xc2d67840 without VLAN/DSA tag present
[   33.854281] mtk_soc_eth 1b100000.ethernet eth1: mtk_poll_rx: received 
skb 0xc2d67840 without VLAN/DSA tag present

On a working port:

[   48.798419] mt7530 mdio-bus:1f wan: Link is Down
[   48.803166] br0: port 1(wan) entered disabled state
[   50.958903] mt7530 mdio-bus:1f lan0: Link is Up - 1Gbps/Full - flow 
control off
[   50.966282] br0: port 2(lan0) entered blocking state
[   50.971300] br0: port 2(lan0) entered forwarding state
[   54.261846] mtk_soc_eth 1b100000.ethernet eth1: mtk_poll_rx: 
skb->vlan_proto 0x1 port 1
[   54.269905] mtk_soc_eth 1b100000.ethernet eth1: mtk_poll_rx: 
attaching metadata dst with port 1 to skb 0xc2d67840
[   54.280412] mtk_soc_eth 1b100000.ethernet eth1: mtk_poll_rx: 
skb->vlan_proto 0x1 port 1
[   54.288460] mtk_soc_eth 1b100000.ethernet eth1: mtk_poll_rx: 
attaching metadata dst with port 1 to skb 0xc2d67840
[   55.263241] mtk_soc_eth 1b100000.ethernet eth1: mtk_poll_rx: 
skb->vlan_proto 0x1 port 1
[   55.271292] mtk_soc_eth 1b100000.ethernet eth1: mtk_poll_rx: 
attaching metadata dst with port 1 to skb 0xc2d67840
[   59.358317] mtk_soc_eth 1b100000.ethernet eth1: mtk_poll_rx: 
skb->vlan_proto 0x1 port 1
[   59.366361] mtk_soc_eth 1b100000.ethernet eth1: mtk_poll_rx: 
attaching metadata dst with port 1 to skb 0xc2d67a80

Arınç