Date:   Sat, 29 Apr 2023 16:03:57 +0300
From:   Arınç ÜNAL <arinc.unal@...nc9.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     DENG Qingfang <dqfext@...il.com>, Greg Ungerer <gerg@...nel.org>,
        Daniel Golle <daniel@...rotopia.org>,
        Richard van Schagen <richard@...terhints.com>,
        Richard van Schagen <vschagen@...com>,
        Frank Wunderlich <frank-w@...lic-files.de>,
        mithat.guner@...ont.com, erkin.bozoglu@...ont.com,
        bartel.eerdekens@...stell8.be, netdev <netdev@...r.kernel.org>
Subject: Re: MT7530 bug, forward broadcast and unknown frames to the correct
 CPU port

(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ç

Powered by blists - more mailing lists