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

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

[Arınç ÜNAL]

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.

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.

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?

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

Arınç

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

[Vladimir Oltean]

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.

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.

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

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.

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

That being said, with trapped packets, software can end up processing
traffic on a conduit interface that didn't originate from a user port
affine to it. Software (DSA) should be fine with it, as long as the
hardware is fine with it too.

The only thing to keep in mind is that the designated CPU port for
trapped packets should always be reselected based on which conduit
interface is up. Maybe lan0's conduit interface is eth0, which is up,
but its trapped packets are handled by eth1 through some previous
configuration, and eth1 went down. You don't want lan0 to lose packets.

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

[Arınç ÜNAL]

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?

> 
> 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);

> 
> That being said, with trapped packets, software can end up processing
> traffic on a conduit interface that didn't originate from a user port
> affine to it. Software (DSA) should be fine with it, as long as the
> hardware is fine with it too.
> 
> The only thing to keep in mind is that the designated CPU port for
> trapped packets should always be reselected based on which conduit
> interface is up. Maybe lan0's conduit interface is eth0, which is up,
> but its trapped packets are handled by eth1 through some previous
> configuration, and eth1 went down. You don't want lan0 to lose packets.

If I understand correctly, this should be done on the port_change_master 
member whilst support for changing the DSA conduit is being introduced. 
I'm taking a note for when I get to this, thanks.

Arınç

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

[Arınç ÜNAL]

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

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

[Vladimir Oltean]

On Sat, Apr 29, 2023 at 04:03:57PM +0300, Arınç ÜNAL wrote:
> 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)

Looks ok, but considering that the register is the same for all ports,
then instead of accessing the hardware one by one for each port, you
could issue a single:

	mt7530_clear(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK | UNU_FFP_MASK);

before the per-port for loop.

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

[Arınç ÜNAL]

On 29.04.2023 20:35, Vladimir Oltean wrote:
> On Sat, Apr 29, 2023 at 04:03:57PM +0300, Arınç ÜNAL wrote:
>> 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)
> 
> Looks ok, but considering that the register is the same for all ports,
> then instead of accessing the hardware one by one for each port, you
> could issue a single:
> 
> 	mt7530_clear(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK | UNU_FFP_MASK);
> 
> before the per-port for loop.

Will do, thanks.

The preferred port operation should be in the clear after this diff:

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index cb0f138d39eb..3a69ef68ceae 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -967,6 +967,10 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
  	if (priv->id == ID_MT7621)
  		mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
  
+	/* Set the CPU port for MT7531 and switch on MT7988 SoC */
+	if (priv->id == ID_MT7531 || priv->id == ID_MT7988)
+		mt7530_set(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK, BIT(port));
+
  	/* CPU port gets connected to all user ports of
  	 * the switch.
  	 */
@@ -2321,15 +2325,6 @@ mt7531_setup_common(struct dsa_switch *ds)
  	struct dsa_port *cpu_dp;
  	int ret, i;
  
-	/* 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;
-	}
-	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
-		   MT753X_BPDU_CPU_ONLY);
-
  	/* Enable and reset MIB counters */
  	mt7530_mib_reset(ds);
  
@@ -2360,6 +2355,10 @@ mt7531_setup_common(struct dsa_switch *ds)
  			   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
  	}
  
+	/* Trap BPDUs to the CPU port */
+	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
+		   MT753X_BPDU_CPU_ONLY);
+
  	/* Flush the FDB table */
  	ret = mt7530_fdb_cmd(priv, MT7530_FDB_FLUSH, NULL);
  	if (ret < 0)

The MT7531 manual states that the CPU_PMAP bits are unset after reset so
no need to clear it beforehand.

Are you fine with the preferred port patch now that I mentioned port 6
would be preferred for MT7531BE since it's got 2.5G whilst port 5 has
got 1G? Would you like to submit it or leave it to me to send the diff
above and this?

Arınç

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

[Arınç ÜNAL]

On 29.04.2023 21:39, Arınç ÜNAL wrote:
> On 29.04.2023 20:35, Vladimir Oltean wrote:
>> On Sat, Apr 29, 2023 at 04:03:57PM +0300, Arınç ÜNAL wrote:
>>> 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)
>>
>> Looks ok, but considering that the register is the same for all ports,
>> then instead of accessing the hardware one by one for each port, you
>> could issue a single:
>>
>>     mt7530_clear(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK | 
>> UNU_FFP_MASK);
>>
>> before the per-port for loop.
> 
> Will do, thanks.
> 
> The preferred port operation should be in the clear after this diff:
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index cb0f138d39eb..3a69ef68ceae 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -967,6 +967,10 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int 
> port)
>       if (priv->id == ID_MT7621)
>           mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
> 
> +    /* Set the CPU port for MT7531 and switch on MT7988 SoC */
> +    if (priv->id == ID_MT7531 || priv->id == ID_MT7988)
> +        mt7530_set(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK, BIT(port));

Should be 'mt7530_set(priv, MT7531_CFC, 
MT7531_CPU_PMAP_MASK(BIT(port)));' instead.

> +
>       /* CPU port gets connected to all user ports of
>        * the switch.
>        */
> @@ -2321,15 +2325,6 @@ mt7531_setup_common(struct dsa_switch *ds)
>       struct dsa_port *cpu_dp;
>       int ret, i;
> 
> -    /* 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;
> -    }
> -    mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> -           MT753X_BPDU_CPU_ONLY);
> -
>       /* Enable and reset MIB counters */
>       mt7530_mib_reset(ds);
> 
> @@ -2360,6 +2355,10 @@ mt7531_setup_common(struct dsa_switch *ds)
>                  PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
>       }
> 
> +    /* Trap BPDUs to the CPU port */
> +    mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> +           MT753X_BPDU_CPU_ONLY);
> +
>       /* Flush the FDB table */
>       ret = mt7530_fdb_cmd(priv, MT7530_FDB_FLUSH, NULL);
>       if (ret < 0)
> 
> The MT7531 manual states that the CPU_PMAP bits are unset after reset so
> no need to clear it beforehand.
> 
> Are you fine with the preferred port patch now that I mentioned port 6
> would be preferred for MT7531BE since it's got 2.5G whilst port 5 has
> got 1G? Would you like to submit it or leave it to me to send the diff
> above and this?
> 
> Arınç

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

[Vladimir Oltean]

On Sat, Apr 29, 2023 at 09:39:41PM +0300, Arınç ÜNAL wrote:
> Are you fine with the preferred port patch now that I mentioned port 6
> would be preferred for MT7531BE since it's got 2.5G whilst port 5 has
> got 1G? Would you like to submit it or leave it to me to send the diff
> above and this?

No, please tell me: what real life difference would it make to a user
who doesn't care to analyze which CPU port is used?

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

[Arınç ÜNAL]

On 29.04.2023 21:56, Vladimir Oltean wrote:
> On Sat, Apr 29, 2023 at 09:39:41PM +0300, Arınç ÜNAL wrote:
>> Are you fine with the preferred port patch now that I mentioned port 6
>> would be preferred for MT7531BE since it's got 2.5G whilst port 5 has
>> got 1G? Would you like to submit it or leave it to me to send the diff
>> above and this?
> 
> No, please tell me: what real life difference would it make to a user
> who doesn't care to analyze which CPU port is used?

They would get 2.5 Gbps download/upload bandwidth in total to the CPU, 
instead of 1 Gbps. 3 computers connected to 3 switch ports would each 
get 833 Mbps download/upload speed to/from the CPU instead of 333 Mbps.

Arınç

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

[Vladimir Oltean]

On Sat, Apr 29, 2023 at 10:52:12PM +0300, Arınç ÜNAL wrote:
> On 29.04.2023 21:56, Vladimir Oltean wrote:
> > On Sat, Apr 29, 2023 at 09:39:41PM +0300, Arınç ÜNAL wrote:
> > > Are you fine with the preferred port patch now that I mentioned port 6
> > > would be preferred for MT7531BE since it's got 2.5G whilst port 5 has
> > > got 1G? Would you like to submit it or leave it to me to send the diff
> > > above and this?
> > 
> > No, please tell me: what real life difference would it make to a user
> > who doesn't care to analyze which CPU port is used?
> 
> They would get 2.5 Gbps download/upload bandwidth in total to the CPU,
> instead of 1 Gbps. 3 computers connected to 3 switch ports would each get
> 833 Mbps download/upload speed to/from the CPU instead of 333 Mbps.

In theory, theory and practice are the same. In practice, they aren't.
Are you able to obtain 833 Mbps concurrently over 3 user ports?

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

[Daniel Golle]

On Mon, May 01, 2023 at 01:09:30PM +0300, Vladimir Oltean wrote:
> On Sat, Apr 29, 2023 at 10:52:12PM +0300, Arınç ÜNAL wrote:
> > On 29.04.2023 21:56, Vladimir Oltean wrote:
> > > On Sat, Apr 29, 2023 at 09:39:41PM +0300, Arınç ÜNAL wrote:
> > > > Are you fine with the preferred port patch now that I mentioned port 6
> > > > would be preferred for MT7531BE since it's got 2.5G whilst port 5 has
> > > > got 1G? Would you like to submit it or leave it to me to send the diff
> > > > above and this?
> > > 
> > > No, please tell me: what real life difference would it make to a user
> > > who doesn't care to analyze which CPU port is used?
> > 
> > They would get 2.5 Gbps download/upload bandwidth in total to the CPU,
> > instead of 1 Gbps. 3 computers connected to 3 switch ports would each get
> > 833 Mbps download/upload speed to/from the CPU instead of 333 Mbps.
> 
> In theory, theory and practice are the same. In practice, they aren't.
> Are you able to obtain 833 Mbps concurrently over 3 user ports?

Probably the 2.5 GBit/s won't saturate, but I do manage to get more
than 1 Gbit/s total (using the hardware flow offloading capability to
NAT-route WAN<->LAN and simultanously have a WiFi client access a NAS
device which also connects to a LAN port. I use MT7915E+MT7975D mPCIe
module with BPi-R2)

Using PHY muxing to directly map the WAN port to GMAC2 is also an
option, but would be limiting the bandwidth for those users who just
want all 5 ports to be bridged. Hence I do agree with Arınç that the
best would be to use the TRGMII link on GMAC1 for the 4 WAN ports and
prefer using RGMII link on GMAC2 for the WAN port, but keep using DSA.

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

[Arınç ÜNAL]

On 1.05.2023 13:31, Daniel Golle wrote:
> On Mon, May 01, 2023 at 01:09:30PM +0300, Vladimir Oltean wrote:
>> On Sat, Apr 29, 2023 at 10:52:12PM +0300, Arınç ÜNAL wrote:
>>> On 29.04.2023 21:56, Vladimir Oltean wrote:
>>>> On Sat, Apr 29, 2023 at 09:39:41PM +0300, Arınç ÜNAL wrote:
>>>>> Are you fine with the preferred port patch now that I mentioned port 6
>>>>> would be preferred for MT7531BE since it's got 2.5G whilst port 5 has
>>>>> got 1G? Would you like to submit it or leave it to me to send the diff
>>>>> above and this?
>>>>
>>>> No, please tell me: what real life difference would it make to a user
>>>> who doesn't care to analyze which CPU port is used?
>>>
>>> They would get 2.5 Gbps download/upload bandwidth in total to the CPU,
>>> instead of 1 Gbps. 3 computers connected to 3 switch ports would each get
>>> 833 Mbps download/upload speed to/from the CPU instead of 333 Mbps.
>>
>> In theory, theory and practice are the same. In practice, they aren't.
>> Are you able to obtain 833 Mbps concurrently over 3 user ports?
> 
> Probably the 2.5 GBit/s won't saturate, but I do manage to get more
> than 1 Gbit/s total (using the hardware flow offloading capability to
> NAT-route WAN<->LAN and simultanously have a WiFi client access a NAS
> device which also connects to a LAN port. I use MT7915E+MT7975D mPCIe
> module with BPi-R2)
> 
> Using PHY muxing to directly map the WAN port to GMAC2 is also an
> option, but would be limiting the bandwidth for those users who just
> want all 5 ports to be bridged. Hence I do agree with Arınç that the
> best would be to use the TRGMII link on GMAC1 for the 4 WAN ports and
> prefer using RGMII link on GMAC2 for the WAN port, but keep using DSA.

You seem to be rather talking about MT7530 while I think preferring port 6
would benefit MT7531BE the most.

Can you test the actual speed with SGMII on MT7531? Route between two ports and
do a bidirectional iperf3 speed test.

SGMII should at least provide you with 2 Gbps bandwidth in total in a
router-on-a-stick scenario which is the current situation until the changing
DSA conduit support is added.

If we were to use port 5, download and upload speed would be capped at 500
Mbps. With SGMII you should get 1000 Mbps on each.

Arınç

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

[Arınç ÜNAL]

On 1.05.2023 12:43, Arınç ÜNAL wrote:
> On 1.05.2023 13:31, Daniel Golle wrote:
>> On Mon, May 01, 2023 at 01:09:30PM +0300, Vladimir Oltean wrote:
>>> On Sat, Apr 29, 2023 at 10:52:12PM +0300, Arınç ÜNAL wrote:
>>>> On 29.04.2023 21:56, Vladimir Oltean wrote:
>>>>> On Sat, Apr 29, 2023 at 09:39:41PM +0300, Arınç ÜNAL wrote:
>>>>>> Are you fine with the preferred port patch now that I mentioned 
>>>>>> port 6
>>>>>> would be preferred for MT7531BE since it's got 2.5G whilst port 5 has
>>>>>> got 1G? Would you like to submit it or leave it to me to send the 
>>>>>> diff
>>>>>> above and this?
>>>>>
>>>>> No, please tell me: what real life difference would it make to a user
>>>>> who doesn't care to analyze which CPU port is used?
>>>>
>>>> They would get 2.5 Gbps download/upload bandwidth in total to the CPU,
>>>> instead of 1 Gbps. 3 computers connected to 3 switch ports would 
>>>> each get
>>>> 833 Mbps download/upload speed to/from the CPU instead of 333 Mbps.
>>>
>>> In theory, theory and practice are the same. In practice, they aren't.
>>> Are you able to obtain 833 Mbps concurrently over 3 user ports?
>>
>> Probably the 2.5 GBit/s won't saturate, but I do manage to get more
>> than 1 Gbit/s total (using the hardware flow offloading capability to
>> NAT-route WAN<->LAN and simultanously have a WiFi client access a NAS
>> device which also connects to a LAN port. I use MT7915E+MT7975D mPCIe
>> module with BPi-R2)
>>
>> Using PHY muxing to directly map the WAN port to GMAC2 is also an
>> option, but would be limiting the bandwidth for those users who just
>> want all 5 ports to be bridged. Hence I do agree with Arınç that the
>> best would be to use the TRGMII link on GMAC1 for the 4 WAN ports and
>> prefer using RGMII link on GMAC2 for the WAN port, but keep using DSA.
> 
> You seem to be rather talking about MT7530 while I think preferring port 6
> would benefit MT7531BE the most.
> 
> Can you test the actual speed with SGMII on MT7531? Route between two 
> ports and
> do a bidirectional iperf3 speed test.
> 
> SGMII should at least provide you with 2 Gbps bandwidth in total in a
> router-on-a-stick scenario which is the current situation until the 
> changing
> DSA conduit support is added.
> 
> If we were to use port 5, download and upload speed would be capped at 500
> Mbps. With SGMII you should get 1000 Mbps on each.

I tested this on Daniel's Banana Pi BPI-R3 which has got an MT7531AE 
switch. I can confirm I get more than 500 Mbps for RX and TX on a 
bidirectional speed test.

[SUM][RX-S]   0.00-18.00  sec  1.50 GBytes   715 Mbits/sec 
    receiver

[SUM][TX-S]   0.00-18.00  sec  1.55 GBytes   742 Mbits/sec  6996 
     sender

The test was run between two computers on different networks, 
192.168.1.0/24 and 192.168.2.0/24, both computers had static routes to 
reach each other. I tried iperf3 as the server and client on both 
computers with similar results.

This concludes preferring port 6 is practically beneficial for MT7531BE.

Arınç

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

[Vladimir Oltean]

On Wed, May 10, 2023 at 10:59:36AM +0200, Arınç ÜNAL wrote:
> > You seem to be rather talking about MT7530 while I think preferring port 6
> > would benefit MT7531BE the most.
> > 
> > Can you test the actual speed with SGMII on MT7531? Route between two ports and
> > do a bidirectional iperf3 speed test.
> > 
> > SGMII should at least provide you with 2 Gbps bandwidth in total in a
> > router-on-a-stick scenario which is the current situation until the changing
> > DSA conduit support is added.
> > 
> > If we were to use port 5, download and upload speed would be capped at 500
> > Mbps. With SGMII you should get 1000 Mbps on each.
> 
> I tested this on Daniel's Banana Pi BPI-R3 which has got an MT7531AE switch.
> I can confirm I get more than 500 Mbps for RX and TX on a bidirectional
> speed test.
> 
> [SUM][RX-S]   0.00-18.00  sec  1.50 GBytes   715 Mbits/sec    receiver
> 
> [SUM][TX-S]   0.00-18.00  sec  1.55 GBytes   742 Mbits/sec  6996     sender
> 
> The test was run between two computers on different networks, 192.168.1.0/24
> and 192.168.2.0/24, both computers had static routes to reach each other. I
> tried iperf3 as the server and client on both computers with similar
> results.
> 
> This concludes preferring port 6 is practically beneficial for MT7531BE.

One thing you seem to not realize is that "1 Gbit/sec full duplex" means
that there is 1Gbps of bandwidth in the TX direction and 1 Gbps of
bandwidth of throughput in the RX direction. So, I don't see how your
test proves anything, since a single SGMII full duplex link to the CPU
should be able to absorb your 715 RX + 742 TX traffic just fine.

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

[Arınç ÜNAL]

On 10.05.2023 17:02, Vladimir Oltean wrote:
> On Wed, May 10, 2023 at 10:59:36AM +0200, Arınç ÜNAL wrote:
>>> You seem to be rather talking about MT7530 while I think preferring port 6
>>> would benefit MT7531BE the most.
>>>
>>> Can you test the actual speed with SGMII on MT7531? Route between two ports and
>>> do a bidirectional iperf3 speed test.
>>>
>>> SGMII should at least provide you with 2 Gbps bandwidth in total in a
>>> router-on-a-stick scenario which is the current situation until the changing
>>> DSA conduit support is added.
>>>
>>> If we were to use port 5, download and upload speed would be capped at 500
>>> Mbps. With SGMII you should get 1000 Mbps on each.
>>
>> I tested this on Daniel's Banana Pi BPI-R3 which has got an MT7531AE switch.
>> I can confirm I get more than 500 Mbps for RX and TX on a bidirectional
>> speed test.
>>
>> [SUM][RX-S]   0.00-18.00  sec  1.50 GBytes   715 Mbits/sec    receiver
>>
>> [SUM][TX-S]   0.00-18.00  sec  1.55 GBytes   742 Mbits/sec  6996     sender
>>
>> The test was run between two computers on different networks, 192.168.1.0/24
>> and 192.168.2.0/24, both computers had static routes to reach each other. I
>> tried iperf3 as the server and client on both computers with similar
>> results.
>>
>> This concludes preferring port 6 is practically beneficial for MT7531BE.
> 
> One thing you seem to not realize is that "1 Gbit/sec full duplex" means
> that there is 1Gbps of bandwidth in the TX direction and 1 Gbps of
> bandwidth of throughput in the RX direction. So, I don't see how your
> test proves anything, since a single SGMII full duplex link to the CPU
> should be able to absorb your 715 RX + 742 TX traffic just fine.

I'm talking about 1 Gbps TX and RX (bidirectional) traffic between two 
DSA user ports. We're doing routing so bridge offloading is out of 
question. In this case, the SoC MAC would have to do 2 Gbps TX and 2 
Gbps RX.

I have a BPI-R64 with an MT7531BE at home now, thanks to Daniel's folks. 
Here's the iperf3 bidirectional test result:

Without preferring port 6:

[ ID][Role] Interval           Transfer     Bitrate         Retr
[  5][TX-C]   0.00-20.00  sec   374 MBytes   157 Mbits/sec  734 
    sender
[  5][TX-C]   0.00-20.00  sec   373 MBytes   156 Mbits/sec 
    receiver
[  7][RX-C]   0.00-20.00  sec  1.81 GBytes   778 Mbits/sec    0 
    sender
[  7][RX-C]   0.00-20.00  sec  1.81 GBytes   777 Mbits/sec 
    receiver

With preferring port 6:

[ ID][Role] Interval           Transfer     Bitrate         Retr
[  5][TX-C]   0.00-20.00  sec  1.99 GBytes   856 Mbits/sec  273 
    sender
[  5][TX-C]   0.00-20.00  sec  1.99 GBytes   855 Mbits/sec 
    receiver
[  7][RX-C]   0.00-20.00  sec  1.72 GBytes   737 Mbits/sec   15 
    sender
[  7][RX-C]   0.00-20.00  sec  1.71 GBytes   736 Mbits/sec 
    receiver

This scenario is quite popular as you would see a lot of people using 
one port for WAN and the other ports for LAN.

Therefore, the "prefer local CPU port" operation would be useful. If you 
can introduce the operation to the DSA subsystem, I will make a patch to 
start using it on the MT7530 DSA subdriver.

Arınç

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

[Vladimir Oltean]

On Tue, May 16, 2023 at 11:01:02PM +0300, Arınç ÜNAL wrote:
> Without preferring port 6:
> 
> [ ID][Role] Interval           Transfer     Bitrate         Retr
> [  5][TX-C]   0.00-20.00  sec   374 MBytes   157 Mbits/sec  734    sender
> [  5][TX-C]   0.00-20.00  sec   373 MBytes   156 Mbits/sec    receiver
> [  7][RX-C]   0.00-20.00  sec  1.81 GBytes   778 Mbits/sec    0    sender
> [  7][RX-C]   0.00-20.00  sec  1.81 GBytes   777 Mbits/sec    receiver
> 
> With preferring port 6:
> 
> [ ID][Role] Interval           Transfer     Bitrate         Retr
> [  5][TX-C]   0.00-20.00  sec  1.99 GBytes   856 Mbits/sec  273    sender
> [  5][TX-C]   0.00-20.00  sec  1.99 GBytes   855 Mbits/sec    receiver
> [  7][RX-C]   0.00-20.00  sec  1.72 GBytes   737 Mbits/sec   15    sender
> [  7][RX-C]   0.00-20.00  sec  1.71 GBytes   736 Mbits/sec    receiver
> 
> This scenario is quite popular as you would see a lot of people using one
> port for WAN and the other ports for LAN.
> 
> Therefore, the "prefer local CPU port" operation would be useful. If you can
> introduce the operation to the DSA subsystem, I will make a patch to start
> using it on the MT7530 DSA subdriver.

Patches adding infrastructure will not be accepted without one user of
that infra, so if you could pick up my patch from the ML, keep my author
and signed-off-by tag, add yours afterwards, give it a compelling commit
message backed up by data such as this, and submit it along your mt7530
user, that would probably be good.