Hi.
This patch series fixes all non-theoretical issues regarding multiple CPU
ports and the handling of LLDP frames and BPDUs.
I am adding me as a maintainer, I've got some code improvements on the way.
I will keep an eye on this driver and the patches submitted for it in the
future.
Arınç
v6:
- Change a small portion of the comment in the diff on "net: dsa: mt7530:
set all CPU ports in MT7531_CPU_PMAP" with Russell's suggestion.
- Change the patch log of "net: dsa: mt7530: fix trapping frames on
non-MT7621 SoC MT7530 switch" with Vladimir's suggestion.
- Group the code for trapping frames into a common function and call that.
- Add Vladimir and Russell's reviewed-by tags to where they're given.
v5:
- Change the comment in the diff on the first patch with Russell's words.
- Change the patch log of the first patch to state that the patch is just
preparatory work for change "net: dsa: introduce
preferred_default_local_cpu_port and use on MT7530" and not a fix to an
existing problem on the code base.
- Remove the "net: dsa: mt7530: fix trapping frames with multiple CPU ports
on MT7530" patch. It fixes a theoretical issue, therefore it is net-next
material.
- Remove unnecessary information from the patch logs. Remove the enum
renaming change.
- Strengthen the point of the "net: dsa: introduce
preferred_default_local_cpu_port and use on MT7530" patch.
v4: Make the patch logs and my comments in the code easier to understand.
v3: Fix the from header on the patches. Write a cover letter.
v2: Add patches to fix the handling of LLDP frames and BPDUs.
Arınç ÜNAL (6):
net: dsa: mt7530: set all CPU ports in MT7531_CPU_PMAP
net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch
net: dsa: mt7530: fix handling of BPDUs on MT7530 switch
net: dsa: mt7530: fix handling of LLDP frames
net: dsa: introduce preferred_default_local_cpu_port and use on MT7530
MAINTAINERS: add me as maintainer of MEDIATEK SWITCH DRIVER
MAINTAINERS | 5 +++--
drivers/net/dsa/mt7530.c | 48 ++++++++++++++++++++++++++++++++++---------
drivers/net/dsa/mt7530.h | 6 ++++++
include/net/dsa.h | 8 ++++++++
net/dsa/dsa.c | 24 +++++++++++++++++++++-
5 files changed, 78 insertions(+), 13 deletions(-)
From: Arınç ÜNAL <[email protected]>
Add me as a maintainer of the MediaTek MT7530 DSA subdriver.
List maintainers in alphabetical order by first name.
Signed-off-by: Arınç ÜNAL <[email protected]>
Reviewed-by: Vladimir Oltean <[email protected]>
---
MAINTAINERS | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index a73e5a98503a..c58d7fbb40ed 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13259,10 +13259,11 @@ F: drivers/memory/mtk-smi.c
F: include/soc/mediatek/smi.h
MEDIATEK SWITCH DRIVER
-M: Sean Wang <[email protected]>
+M: Arınç ÜNAL <[email protected]>
+M: Daniel Golle <[email protected]>
M: Landen Chao <[email protected]>
M: DENG Qingfang <[email protected]>
-M: Daniel Golle <[email protected]>
+M: Sean Wang <[email protected]>
L: [email protected]
S: Maintained
F: drivers/net/dsa/mt7530-mdio.c
--
2.39.2
From: Vladimir Oltean <[email protected]>
Since the introduction of the OF bindings, DSA has always had a policy that
in case multiple CPU ports are present in the device tree, the numerically
smallest one is always chosen.
The MT7530 switch family, except the switch on the MT7988 SoC, has 2 CPU
ports, 5 and 6, where port 6 is preferable on the MT7531BE switch because
it has higher bandwidth.
The MT7530 driver developers had 3 options:
- to modify DSA when the MT7531 switch support was introduced, such as to
prefer the better port
- to declare both CPU ports in device trees as CPU ports, and live with the
sub-optimal performance resulting from not preferring the better port
- to declare just port 6 in the device tree as a CPU port
Of course they chose the path of least resistance (3rd option), kicking the
can down the road. The hardware description in the device tree is supposed
to be stable - developers are not supposed to adopt the strategy of
piecemeal hardware description, where the device tree is updated in
lockstep with the features that the kernel currently supports.
Now, as a result of the fact that they did that, any attempts to modify the
device tree and describe both CPU ports as CPU ports would make DSA change
its default selection from port 6 to 5, effectively resulting in a
performance degradation visible to users with the MT7531BE switch as can be
seen below.
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
Using one port for WAN and the other ports for LAN is a very popular use
case which is what this test emulates.
As such, this change proposes that we retroactively modify stable kernels
(which don't support the modification of the CPU port assignments, so as to
let user space fix the problem and restore the throughput) to keep the
mt7530 driver preferring port 6 even with device trees where the hardware
is more fully described.
Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 15 +++++++++++++++
include/net/dsa.h | 8 ++++++++
net/dsa/dsa.c | 24 +++++++++++++++++++++++-
3 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 6d6ff293900c..7e773c4ba046 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -399,6 +399,20 @@ static void mt7530_pll_setup(struct mt7530_priv *priv)
core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
}
+/* If port 6 is available as a CPU port, always prefer that as the default,
+ * otherwise don't care.
+ */
+static struct dsa_port *
+mt753x_preferred_default_local_cpu_port(struct dsa_switch *ds)
+{
+ struct dsa_port *cpu_dp = dsa_to_port(ds, 6);
+
+ if (dsa_port_is_cpu(cpu_dp))
+ return cpu_dp;
+
+ return NULL;
+}
+
/* Setup port 6 interface mode and TRGMII TX circuit */
static int
mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
@@ -3098,6 +3112,7 @@ static int mt7988_setup(struct dsa_switch *ds)
const struct dsa_switch_ops mt7530_switch_ops = {
.get_tag_protocol = mtk_get_tag_protocol,
.setup = mt753x_setup,
+ .preferred_default_local_cpu_port = mt753x_preferred_default_local_cpu_port,
.get_strings = mt7530_get_strings,
.get_ethtool_stats = mt7530_get_ethtool_stats,
.get_sset_count = mt7530_get_sset_count,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 8903053fa5aa..ab0f0a5b0860 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -958,6 +958,14 @@ struct dsa_switch_ops {
struct phy_device *phy);
void (*port_disable)(struct dsa_switch *ds, int port);
+ /*
+ * Compatibility between device trees defining multiple CPU ports and
+ * drivers which are not OK to use by default the numerically smallest
+ * CPU port of a switch for its local ports. This can return NULL,
+ * meaning "don't know/don't care".
+ */
+ struct dsa_port *(*preferred_default_local_cpu_port)(struct dsa_switch *ds);
+
/*
* Port's MAC EEE settings
*/
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index ab1afe67fd18..1afed89e03c0 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -403,6 +403,24 @@ static int dsa_tree_setup_default_cpu(struct dsa_switch_tree *dst)
return 0;
}
+static struct dsa_port *
+dsa_switch_preferred_default_local_cpu_port(struct dsa_switch *ds)
+{
+ struct dsa_port *cpu_dp;
+
+ if (!ds->ops->preferred_default_local_cpu_port)
+ return NULL;
+
+ cpu_dp = ds->ops->preferred_default_local_cpu_port(ds);
+ if (!cpu_dp)
+ return NULL;
+
+ if (WARN_ON(!dsa_port_is_cpu(cpu_dp) || cpu_dp->ds != ds))
+ return NULL;
+
+ return cpu_dp;
+}
+
/* Perform initial assignment of CPU ports to user ports and DSA links in the
* fabric, giving preference to CPU ports local to each switch. Default to
* using the first CPU port in the switch tree if the port does not have a CPU
@@ -410,12 +428,16 @@ static int dsa_tree_setup_default_cpu(struct dsa_switch_tree *dst)
*/
static int dsa_tree_setup_cpu_ports(struct dsa_switch_tree *dst)
{
- struct dsa_port *cpu_dp, *dp;
+ struct dsa_port *preferred_cpu_dp, *cpu_dp, *dp;
list_for_each_entry(cpu_dp, &dst->ports, list) {
if (!dsa_port_is_cpu(cpu_dp))
continue;
+ preferred_cpu_dp = dsa_switch_preferred_default_local_cpu_port(cpu_dp->ds);
+ if (preferred_cpu_dp && preferred_cpu_dp != cpu_dp)
+ continue;
+
/* Prefer a local CPU port */
dsa_switch_for_each_port(dp, cpu_dp->ds) {
/* Prefer the first local CPU port found */
--
2.39.2
On Sat, Jun 17, 2023 at 09:26:48AM +0300, [email protected] wrote:
> From: Vladimir Oltean <[email protected]>
>
> Since the introduction of the OF bindings, DSA has always had a policy that
> in case multiple CPU ports are present in the device tree, the numerically
> smallest one is always chosen.
>
> The MT7530 switch family, except the switch on the MT7988 SoC, has 2 CPU
> ports, 5 and 6, where port 6 is preferable on the MT7531BE switch because
> it has higher bandwidth.
>
> The MT7530 driver developers had 3 options:
> - to modify DSA when the MT7531 switch support was introduced, such as to
> prefer the better port
> - to declare both CPU ports in device trees as CPU ports, and live with the
> sub-optimal performance resulting from not preferring the better port
> - to declare just port 6 in the device tree as a CPU port
>
> Of course they chose the path of least resistance (3rd option), kicking the
> can down the road. The hardware description in the device tree is supposed
> to be stable - developers are not supposed to adopt the strategy of
> piecemeal hardware description, where the device tree is updated in
> lockstep with the features that the kernel currently supports.
>
> Now, as a result of the fact that they did that, any attempts to modify the
> device tree and describe both CPU ports as CPU ports would make DSA change
> its default selection from port 6 to 5, effectively resulting in a
> performance degradation visible to users with the MT7531BE switch as can be
> seen below.
>
> 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
>
> Using one port for WAN and the other ports for LAN is a very popular use
> case which is what this test emulates.
>
> As such, this change proposes that we retroactively modify stable kernels
> (which don't support the modification of the CPU port assignments, so as to
> let user space fix the problem and restore the throughput) to keep the
> mt7530 driver preferring port 6 even with device trees where the hardware
> is more fully described.
>
> Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
> Signed-off-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
Reviewed-by: Russell King (Oracle) <[email protected]>
Thanks!
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On 17/06/2023 08:26, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> Add me as a maintainer of the MediaTek MT7530 DSA subdriver.
>
> List maintainers in alphabetical order by first name.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> Reviewed-by: Vladimir Oltean <[email protected]>
Thanks for all the work you are doing!
Reviewed-by: Matthias Brugger <[email protected]>
> ---
> MAINTAINERS | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a73e5a98503a..c58d7fbb40ed 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13259,10 +13259,11 @@ F: drivers/memory/mtk-smi.c
> F: include/soc/mediatek/smi.h
>
> MEDIATEK SWITCH DRIVER
> -M: Sean Wang <[email protected]>
> +M: Arınç ÜNAL <[email protected]>
> +M: Daniel Golle <[email protected]>
> M: Landen Chao <[email protected]>
> M: DENG Qingfang <[email protected]>
> -M: Daniel Golle <[email protected]>
> +M: Sean Wang <[email protected]>
> L: [email protected]
> S: Maintained
> F: drivers/net/dsa/mt7530-mdio.c
On 17/06/2023 08:26, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> Add me as a maintainer of the MediaTek MT7530 DSA subdriver.
>
> List maintainers in alphabetical order by first name.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> Reviewed-by: Vladimir Oltean <[email protected]>
Reviewed-by: Matthias Brugger <[email protected]>
> ---
> MAINTAINERS | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a73e5a98503a..c58d7fbb40ed 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13259,10 +13259,11 @@ F: drivers/memory/mtk-smi.c
> F: include/soc/mediatek/smi.h
>
> MEDIATEK SWITCH DRIVER
> -M: Sean Wang <[email protected]>
> +M: Arınç ÜNAL <[email protected]>
> +M: Daniel Golle <[email protected]>
> M: Landen Chao <[email protected]>
> M: DENG Qingfang <[email protected]>
> -M: Daniel Golle <[email protected]>
> +M: Sean Wang <[email protected]>
> L: [email protected]
> S: Maintained
> F: drivers/net/dsa/mt7530-mdio.c
On 6/17/2023 7:26 AM, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> Add me as a maintainer of the MediaTek MT7530 DSA subdriver.
>
> List maintainers in alphabetical order by first name.
>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> Reviewed-by: Vladimir Oltean <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 6/17/2023 7:26 AM, [email protected] wrote:
> From: Vladimir Oltean <[email protected]>
>
> Since the introduction of the OF bindings, DSA has always had a policy that
> in case multiple CPU ports are present in the device tree, the numerically
> smallest one is always chosen.
>
> The MT7530 switch family, except the switch on the MT7988 SoC, has 2 CPU
> ports, 5 and 6, where port 6 is preferable on the MT7531BE switch because
> it has higher bandwidth.
>
> The MT7530 driver developers had 3 options:
> - to modify DSA when the MT7531 switch support was introduced, such as to
> prefer the better port
> - to declare both CPU ports in device trees as CPU ports, and live with the
> sub-optimal performance resulting from not preferring the better port
> - to declare just port 6 in the device tree as a CPU port
>
> Of course they chose the path of least resistance (3rd option), kicking the
> can down the road. The hardware description in the device tree is supposed
> to be stable - developers are not supposed to adopt the strategy of
> piecemeal hardware description, where the device tree is updated in
> lockstep with the features that the kernel currently supports.
>
> Now, as a result of the fact that they did that, any attempts to modify the
> device tree and describe both CPU ports as CPU ports would make DSA change
> its default selection from port 6 to 5, effectively resulting in a
> performance degradation visible to users with the MT7531BE switch as can be
> seen below.
>
> 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
>
> Using one port for WAN and the other ports for LAN is a very popular use
> case which is what this test emulates.
>
> As such, this change proposes that we retroactively modify stable kernels
> (which don't support the modification of the CPU port assignments, so as to
> let user space fix the problem and restore the throughput) to keep the
> mt7530 driver preferring port 6 even with device trees where the hardware
> is more fully described.
>
> Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
> Signed-off-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
Hello:
This series was applied to netdev/net.git (main)
by David S. Miller <[email protected]>:
On Sat, 17 Jun 2023 09:26:43 +0300 you wrote:
> Hi.
>
> This patch series fixes all non-theoretical issues regarding multiple CPU
> ports and the handling of LLDP frames and BPDUs.
>
> I am adding me as a maintainer, I've got some code improvements on the way.
> I will keep an eye on this driver and the patches submitted for it in the
> future.
>
> [...]
Here is the summary with links:
- [net,v6,1/6] net: dsa: mt7530: set all CPU ports in MT7531_CPU_PMAP
https://git.kernel.org/netdev/net/c/ff221029a51f
- [net,v6,2/6] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch
https://git.kernel.org/netdev/net/c/4ae90f90e490
- [net,v6,3/6] net: dsa: mt7530: fix handling of BPDUs on MT7530 switch
https://git.kernel.org/netdev/net/c/d7c660735593
- [net,v6,4/6] net: dsa: mt7530: fix handling of LLDP frames
https://git.kernel.org/netdev/net/c/8332cf6fd7c7
- [net,v6,5/6] net: dsa: introduce preferred_default_local_cpu_port and use on MT7530
https://git.kernel.org/netdev/net/c/b79d7c14f480
- [net,v6,6/6] MAINTAINERS: add me as maintainer of MEDIATEK SWITCH DRIVER
https://git.kernel.org/netdev/net/c/94d12d88b4a8
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html