Re: MT7530 bug, forward broadcast and unknown frames to the correct CPU port

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

(The netdev mailing list didn't like the big attachments so I'm sending
this again with links to the documents instead.)

I'm answering to myself now that I know better.

On 28.04.2023 16:31, Arınç ÜNAL wrote:
> On 26.04.2023 23:54, Vladimir Oltean wrote:
>> On Sun, Apr 23, 2023 at 06:22:41PM +0300, Arınç ÜNAL wrote:
>>> Hey there folks,
>>>
>>> On mt753x_cpu_port_enable() there's code [0] that sets which port to 
>>> forward
>>> the broadcast, unknown multicast, and unknown unicast frames to. Since
>>> mt753x_cpu_port_enable() runs twice when both CPU ports are enabled, 
>>> port 6
>>> becomes the port to forward the frames to. But port 5 is the active 
>>> port, so
>>> no broadcast frames received from the user ports will be forwarded to 
>>> port
>>> 5. This breaks network connectivity when multiple ports are being 
>>> used as
>>> CPU ports.
>>>
>>> My testing shows that only after receiving a broadcast ARP frame from 
>>> port 5
>>> then forwarding it to the user port, the unicast frames received from 
>>> that
>>> user port will be forwarded to port 5. I tested this with ping.
>>>
>>> Forwarding broadcast and unknown unicast frames to the CPU port was done
>>> with commit 5a30833b9a16 ("net: dsa: mt7530: support MDB and bridge flag
>>> operations"). I suppose forwarding the broadcast frames only to the 
>>> CPU port
>>> is what "disable flooding" here means.
>>
>> Flooding means forwarding a packet that does not have a precise 
>> destination
>> (its MAC DA is not present in the FDB or MDB). Flooding is done towards
>> the ports that have flooding enabled.
>>
>>>
>>> It’s a mystery to me how the switch classifies multicast and unicast 
>>> frames
>>> as unknown. Bartel's testing showed LLDP frames fall under this 
>>> category.
>>
>> What is mysterious exactly? What's not in the FDB/MDB is unknown. And
>> DSA, unless the requirements from dsa_switch_supports_uc_filtering() and
>> dsa_switch_supports_mc_filtering() are satisfied, will not program MAC
>> addresses for host RX filtering to the CPU port(s).
>>
>> This switch apparently has the option to automatically learn from the 
>> MAC SA
>> of packets injected by software. That option is automatically enabled
>> unless MTK_HDR_XMIT_SA_DIS is set (which currently it never is).
>>
>> So when software sends a broadcast ARP frame from port 5, the switch
>> learns the MAC SA of this packet (which is the software MAC address of
>> the user port) and it associates it with port 5. So future traffic
>> destined to the user port's software MAC address now reaches port 5, the
>> active CPU port (and the real CPU port from DSA's perspective).
>>
>> Wait 5 minutes for the learned FDB entry to expire, and the problem will
>> probably be back.
> 
> Understood, thank you.
> 
>>
>> LLDP frames should not obey the same rules. They are sent to the MAC DA
>> of 01:80:c2:00:00:0e, which is in the link-local multicast address space
>> (hence the "LL" in the name), and which according to IEEE 802.1Q-2018 is
>> the "Nearest Bridge group address":
>>
>> | The Nearest Bridge group address is an address that no conformant TPMR
>> | component, S-VLAN component, C-VLAN component, or MAC Bridge can
>> | forward. PDUs transmitted using this destination address, or any of the
>> | other addresses that appear in all three tables, can therefore 
>> travel no
>> | further than those stations that can be reached via a single individual
>> | LAN from the originating station. Hence the Nearest Bridge group 
>> address
>> | is also known as the Individual LAN Scope group address.
>>
>> Removing a packet from the forwarding data plane and delivering it only
>> to the CPU is known as "trapping", and thus, it is not "flooding".
>>
>> The MAC SA learning trick will not make port 5 see LLDP frames, since
>> those are not targeted towards a unicast MAC address which could be
>> learned.
> 
> Got it. Bartel has already wrote code to trap the LLDP frames with :03 
> and :0E MAC DA to the CPU port by utilising the MT753X_RGAC2 (0x2C) 
> register.
> 
> I will make sure the code gets in after the issues with multiple CPUs 
> are dealt with.
> 
>>
>>>
>>> Until the driver supports changing the DSA conduit, unknown frames 
>>> should be
>>> forwarded to the active CPU port, not the numerically greater one. 
>>> Any ideas
>>> how to address this and the "disable flooding" case?
>>
>> I think I also signaled the reverse problem in the other thread:
>> https://lore.kernel.org/netdev/20230222193951.rjxgxmopyatyv2t7@skbuf/
> 
> /* BPDU to CPU port */
> dsa_switch_for_each_cpu_port(cpu_dp, ds) {
>      mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK,
>              BIT(cpu_dp->index));
>      break;
> }
> 
> I don't really understand this. This doesn't seem to be related to 
> processing BPDUs. This looks something extra for MT7531, on top of 
> MT7530_MFC.
> 
> If both CPU ports are enabled, should BIT(5) and BIT(6) on 
> MT7531_CPU_PMAP_MASK be set to 1?

Yes, the MT7531 reference manual [0] points to that. Looks like the
MT7531 switch looks at the CPU ports set on the CPU_PMAP bits for the
BPDU_PORT_FW configuration on the MT753X_BPC register to work.

MT7531 has got the MT7530_MFC register too but it's slightly different.
The last 8 bits on the register are for IGMP/MLD Query Frame Flooding
Ports, instead of CPU_EN, CPU_PORT, MIRROR_EN, MIRROR_PORT on MT7530 [1].

Mirroring and CPU port configuration is handled on MT7531_CFC for
MT7531, so it makes sense.

> 
>>
>> Well, the most important step in fixing the problem would be to
>> politically decide which port should be the active CPU port in the case
>> of multiple choices, then to start fixing up the bits in the driver that
>> disagree with that. Having half the code think it's 5 and the other half
>> think it's 6 surely isn't any good.
>>
>> There was a discussion in the other thread with Frank that port 6 would
>> be somehow preferable if both are available, but I haven't seen 
>> convincing
>> enough arguments yet.
> 
> It seems to be fine for MT7530, but MT7531BE has got RGMII on port 5. It 
> would be much better to use port 6 which has got a 2.5G SGMII link.
> 
> The preferred_default_local_cpu_port operation should work properly 
> after we figure out the issue above and below.
> 
>>
>>>
>>> There's also this "set the CPU number" code that runs only for 
>>> MT7621. I'm
>>> not sure why this is needed or why it's only needed for MT7621. Greg, 
>>> could
>>> you shed some light on this since you added this code with commit
>>> ddda1ac116c8 ("net: dsa: mt7530: support the 7530 switch on the Mediatek
>>> MT7621 SoC")?
>>>
>>> There're more things to discuss after supporting changing the DSA 
>>> conduit,
>>> such as which CPU port to forward the unknown frames to, when user ports
>>> under different conduits receive unknown frames. What makes sense to 
>>> me is,
>>> if there are multiple CPU ports being used, forward the unknown 
>>> frames to
>>> port 6. This is already the case except the code runs twice. If not, 
>>> set it
>>> to whatever 'int port' is, which is the default behaviour already.
>>>
>>> [0] 
>>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c#n1005
>>
>> I suspect you may not have run sufficient tests. When there are 2 CPU
>> ports, both of them should be candidates for flooding unknown traffic.
>> Don't worry, software won't see duplicates, because the user <-> CPU port
>> affinity setting should restrict forwarding of the flooded frames to a
>> single CPU port.
>>
>> You might be confused by several things about this:
>>
>>     /* Disable flooding by default */
>>     mt7530_rmw(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK | 
>> UNU_FFP_MASK,
>>            BC_FFP(BIT(port)) | UNM_FFP(BIT(port)) | UNU_FFP(BIT(port)));
>>
>> First, the comment. It means to say: "disable flooding on all ports
>> except for this CPU port".
>>
>> Then, the fact that it runs twice, unsetting flooding for the first CPU
>> port (5) and setting it for the second one (6). There should be no
>> hardware limitation there. Both BIT(5) and BIT(6) could be part of the
>> flood mask without any problem.
>>
>> Perhaps the issue is that MT7530_MFC should have been written to all
>> zeroes as a first step, and then, every mt753x_cpu_port_enable() call
>> enables flooding to the "int port" argument.
> 
> Thanks a lot! I'm just learning about the process of bit masking. If I 
> understand correctly, masks for BC, UNM, and UNU are defined to have 8 
> bits.
> 
> UNU_FFP(~0) means that all bits are set to 1. I am supposed to first 
> clear them, then set bit 5 and 6 to 1.
> 
> Before mt753x_cpu_port_enable():
> 
> mt7530_rmw(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK | UNU_FFP_MASK, 0);
> 
> For every mt753x_cpu_port_enable():
> 
> mt7530_rmw(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK | UNU_FFP_MASK,
>         (BC_FFP(BIT(port))) & BC_FFP_MASK | (UNM_FFP(BIT(port))) &
>         UNM_FFP_MASK | (UNU_FFP(BIT(port))) & UNU_FFP_MASK);

This is the final diff I'm going to submit to net.

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 4d5c5820e461..cc5fa641b026 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1008,9 +1008,9 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
  	mt7530_write(priv, MT7530_PVC_P(port),
  		     PORT_SPEC_TAG);
  
-	/* Disable flooding by default */
-	mt7530_rmw(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK | UNU_FFP_MASK,
-		   BC_FFP(BIT(port)) | UNM_FFP(BIT(port)) | UNU_FFP(BIT(port)));
+	/* Enable flooding on the CPU port */
+	mt7530_set(priv, MT7530_MFC, BC_FFP(BIT(port)) | UNM_FFP(BIT(port)) |
+		   UNU_FFP(BIT(port)));
  
  	/* Set CPU port number */
  	if (priv->id == ID_MT7621)
@@ -2225,6 +2225,10 @@ mt7530_setup(struct dsa_switch *ds)
  		/* Disable learning by default on all ports */
  		mt7530_set(priv, MT7530_PSC_P(i), SA_DIS);
  
+		/* Disable flooding on all ports */
+		mt7530_clear(priv, MT7530_MFC, BC_FFP(BIT(i)) | UNM_FFP(BIT(i)) |
+			     UNU_FFP(BIT(i)));
+
  		if (dsa_is_cpu_port(ds, i)) {
  			ret = mt753x_cpu_port_enable(ds, i);
  			if (ret)
@@ -2412,6 +2416,10 @@ mt7531_setup(struct dsa_switch *ds)
  
  		mt7530_set(priv, MT7531_DBG_CNT(i), MT7531_DIS_CLR);
  
+		/* Disable flooding on all ports */
+		mt7530_clear(priv, MT7530_MFC, BC_FFP(BIT(i)) | UNM_FFP(BIT(i)) |
+			     UNU_FFP(BIT(i)));
+
  		if (dsa_is_cpu_port(ds, i)) {
  			ret = mt753x_cpu_port_enable(ds, i);
  			if (ret)

[0]
https://drive.google.com/file/d/1aVdQz3rbKWjkvdga8-LQ-VFXjmHR8yf9/view?usp=sharing
[1]
https://github.com/vschagen/documents/blob/main/MT7621_ProgrammingGuide_GSW_v0_3.pdf

Arınç