[PATCH net v4 0/7] net: dsa: mt7530: fix multiple CPU ports, BPDU and LLDP handling

[PATCH net v4 0/7] net: dsa: mt7530: fix multiple CPU ports, BPDU and LLDP handling

[arinc9.unal]

Hi.

This patch series fixes hopefully all 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ç

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 (7):
  net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7531
  net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530
  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 | 75 ++++++++++++++++++++++++++++++++++++-------
 drivers/net/dsa/mt7530.h | 26 +++++++++------
 include/net/dsa.h        |  8 +++++
 net/dsa/dsa.c            | 24 +++++++++++++-
 5 files changed, 115 insertions(+), 23 deletions(-)

[PATCH net v4 1/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7531

[arinc9.unal]

From: Arınç ÜNAL <arinc.unal@arinc9.com>

Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
SoC represents a CPU port to trap frames to. These switches trap frames
received from a user port to the CPU port that is affine to the user port
from which the frames are received.

Currently, only the bit that corresponds to the first found CPU port is set
on the bitmap. When multiple CPU ports are being used, the trapped frames
from the user ports not affine to the first CPU port will be dropped as the
other CPU port is not set on the bitmap. The switch on the MT7988 SoC is
not affected as there's only one port to be used as a CPU port.

To fix this, introduce the MT7531_CPU_PMAP macro to individually set the
bits of the CPU port bitmap. Set the CPU port bitmap for MT7531 and the
switch on the MT7988 SoC on mt753x_cpu_port_enable() which runs on a loop
for each CPU port.

Add a comment to explain frame trapping for these switches.

According to the document MT7531 Reference Manual for Development Board
v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
beforehand. Since there's currently no public document for the switch on
the MT7988 SoC, I assume this is also the case for this switch.

Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 16 +++++++++-------
 drivers/net/dsa/mt7530.h |  1 +
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 9bc54e1348cb..b1657679e69d 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1010,6 +1010,14 @@ 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));
 
+	/* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
+	 * the MT7988 SoC. Frames received from a user port which are set for
+	 * trapping to CPU port will be trapped to the CPU port that is affine
+	 * to the user port from which the frames are received.
+	 */
+	if (priv->id == ID_MT7531 || priv->id == ID_MT7988)
+		mt7530_set(priv, MT7531_CFC, MT7531_CPU_PMAP(BIT(port)));
+
 	/* CPU port gets connected to all user ports of
 	 * the switch.
 	 */
@@ -2352,15 +2360,9 @@ static int
 mt7531_setup_common(struct dsa_switch *ds)
 {
 	struct mt7530_priv *priv = ds->priv;
-	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;
-	}
+	/* Trap BPDUs to the CPU port(s) */
 	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
 		   MT753X_BPDU_CPU_ONLY);
 
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 5084f48a8869..e590cf43f3ae 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -54,6 +54,7 @@ enum mt753x_id {
 #define  MT7531_MIRROR_PORT_GET(x)	(((x) >> 16) & MIRROR_MASK)
 #define  MT7531_MIRROR_PORT_SET(x)	(((x) & MIRROR_MASK) << 16)
 #define  MT7531_CPU_PMAP_MASK		GENMASK(7, 0)
+#define  MT7531_CPU_PMAP(x)		FIELD_PREP(MT7531_CPU_PMAP_MASK, x)
 
 #define MT753X_MIRROR_REG(id)		((((id) == ID_MT7531) || ((id) == ID_MT7988)) ?	\
 					 MT7531_CFC : MT7530_MFC)
-- 
2.39.2

[PATCH net v4 2/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530

[arinc9.unal]

From: Arınç ÜNAL <arinc.unal@arinc9.com>

The CPU_PORT bits represent the CPU port to trap frames to for the MT7530
switch. This switch traps frames received from a user port to the CPU port
set on the CPU_PORT bits, regardless of the affinity of the user port from
which the frames are received.

When multiple CPU ports are being used, the trapped frames won't be
received when the DSA conduit interface, which the frames are supposed to
be trapped to, is down because it's not affine to any user port. This
requires the DSA conduit interface to be manually set up for the trapped
frames to be received.

To fix this, implement ds->ops->master_state_change() on this subdriver and
set the CPU_PORT bits to the CPU port which the DSA conduit interface its
affine to is up. Introduce the active_cpu_ports field to store the
information of the active CPU ports. Correct the macros, CPU_PORT is bits 4
through 6 of the register.

Add a comment to explain frame trapping for this switch.

Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Suggested-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 32 ++++++++++++++++++++++++++++----
 drivers/net/dsa/mt7530.h |  6 ++++--
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index b1657679e69d..ef8879087932 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1006,10 +1006,6 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int 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)
-		mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
-
 	/* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
 	 * the MT7988 SoC. Frames received from a user port which are set for
 	 * trapping to CPU port will be trapped to the CPU port that is affine
@@ -3063,6 +3059,33 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static void
+mt753x_master_state_change(struct dsa_switch *ds,
+			   const struct net_device *master,
+			   bool operational)
+{
+	struct mt7530_priv *priv = ds->priv;
+	struct dsa_port *cpu_dp = master->dsa_ptr;
+
+	/* Set the CPU port to trap frames to for MT7530. There can be only one
+	 * CPU port due to CPU_PORT having only 3 bits. Frames received from a
+	 * user port which are set for trapping to CPU port will be trapped to
+	 * the numerically smallest CPU port which is affine to the DSA conduit
+	 * interface that is up.
+	 */
+	if (priv->id != ID_MT7621)
+		return;
+
+	if (operational)
+		priv->active_cpu_ports |= BIT(cpu_dp->index);
+	else
+		priv->active_cpu_ports &= ~BIT(cpu_dp->index);
+
+	if (priv->active_cpu_ports)
+		mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, CPU_EN |
+			   CPU_PORT(__ffs(priv->active_cpu_ports)));
+}
+
 static int mt7988_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
 {
 	return 0;
@@ -3117,6 +3140,7 @@ const struct dsa_switch_ops mt7530_switch_ops = {
 	.phylink_mac_link_up	= mt753x_phylink_mac_link_up,
 	.get_mac_eee		= mt753x_get_mac_eee,
 	.set_mac_eee		= mt753x_set_mac_eee,
+	.master_state_change	= mt753x_master_state_change,
 };
 EXPORT_SYMBOL_GPL(mt7530_switch_ops);
 
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index e590cf43f3ae..28dbd131a535 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -41,8 +41,8 @@ enum mt753x_id {
 #define  UNU_FFP(x)			(((x) & 0xff) << 8)
 #define  UNU_FFP_MASK			UNU_FFP(~0)
 #define  CPU_EN				BIT(7)
-#define  CPU_PORT(x)			((x) << 4)
-#define  CPU_MASK			(0xf << 4)
+#define  CPU_PORT_MASK			GENMASK(6, 4)
+#define  CPU_PORT(x)			FIELD_PREP(CPU_PORT_MASK, x)
 #define  MIRROR_EN			BIT(3)
 #define  MIRROR_PORT(x)			((x) & 0x7)
 #define  MIRROR_MASK			0x7
@@ -753,6 +753,7 @@ struct mt753x_info {
  * @irq_domain:		IRQ domain of the switch irq_chip
  * @irq_enable:		IRQ enable bits, synced to SYS_INT_EN
  * @create_sgmii:	Pointer to function creating SGMII PCS instance(s)
+ * @active_cpu_ports:	Holding the active CPU ports
  */
 struct mt7530_priv {
 	struct device		*dev;
@@ -779,6 +780,7 @@ struct mt7530_priv {
 	struct irq_domain *irq_domain;
 	u32 irq_enable;
 	int (*create_sgmii)(struct mt7530_priv *priv, bool dual_sgmii);
+	unsigned long active_cpu_ports;
 };
 
 struct mt7530_hw_vlan_entry {
-- 
2.39.2

[PATCH net v4 3/7] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch

[arinc9.unal]

From: Arınç ÜNAL <arinc.unal@arinc9.com>

The check for setting the CPU_PORT bits must include the non-MT7621 SoC
MT7530 switch variants to trap frames. Expand the check to include them.

Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index ef8879087932..2bde2fdb5fba 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -3073,7 +3073,7 @@ mt753x_master_state_change(struct dsa_switch *ds,
 	 * the numerically smallest CPU port which is affine to the DSA conduit
 	 * interface that is up.
 	 */
-	if (priv->id != ID_MT7621)
+	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
 		return;
 
 	if (operational)
-- 
2.39.2

[PATCH net v4 4/7] net: dsa: mt7530: fix handling of BPDUs on MT7530 switch

[arinc9.unal]

From: Arınç ÜNAL <arinc.unal@arinc9.com>

BPDUs are link-local frames, therefore they must be trapped to the CPU
port. Currently, the MT7530 switch treats BPDUs as regular multicast
frames, therefore flooding them to user ports. To fix this, set BPDUs to be
trapped to the CPU port.

BPDUs received from a user port will be trapped to the numerically smallest
CPU port which is affine to the DSA conduit interface that is up.

Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 2bde2fdb5fba..e4c169843f2e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2259,6 +2259,10 @@ mt7530_setup(struct dsa_switch *ds)
 
 	priv->p6_interface = PHY_INTERFACE_MODE_NA;
 
+	/* Trap BPDUs to the CPU port */
+	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
+		   MT753X_BPDU_CPU_ONLY);
+
 	/* Enable and reset MIB counters */
 	mt7530_mib_reset(ds);
 
-- 
2.39.2

[PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[arinc9.unal]

From: Arınç ÜNAL <arinc.unal@arinc9.com>

LLDP frames are link-local frames, therefore they must be trapped to the
CPU port. Currently, the MT753X switches treat LLDP frames as regular
multicast frames, therefore flooding them to user ports. To fix this, set
LLDP frames to be trapped to the CPU port(s).

The mt753x_bpdu_port_fw enum is universally used for trapping frames,
therefore rename it and the values in it to mt753x_port_fw.

For MT7530, LLDP frames received from a user port will be trapped to the
numerically smallest CPU port which is affine to the DSA conduit interface
that is up.

For MT7531 and the switch on the MT7988 SoC, LLDP frames received from a
user port will be trapped to the CPU port that is affine to the user port
from which the frames are received.

The bit for R0E_MANG_FR is 27. When set, the switch regards the frames with
:0E MAC DA as management (LLDP) frames. This bit is set to 1 after reset on
MT7530 and MT7531 according to the documents MT7620 Programming Guide v1.0
and MT7531 Reference Manual for Development Board v1.0, so there's no need
to deal with this bit. Since there's currently no public document for the
switch on the MT7988 SoC, I assume this is also the case for this switch.

Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 12 ++++++++++--
 drivers/net/dsa/mt7530.h | 19 ++++++++++++-------
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index e4c169843f2e..8388b058fbe4 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2261,7 +2261,11 @@ mt7530_setup(struct dsa_switch *ds)
 
 	/* Trap BPDUs to the CPU port */
 	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
-		   MT753X_BPDU_CPU_ONLY);
+		   MT753X_PORT_FW_CPU_ONLY);
+
+	/* Trap LLDP frames with :0E MAC DA to the CPU port */
+	mt7530_rmw(priv, MT753X_RGAC2, MT753X_R0E_PORT_FW_MASK,
+		   MT753X_R0E_PORT_FW(MT753X_PORT_FW_CPU_ONLY));
 
 	/* Enable and reset MIB counters */
 	mt7530_mib_reset(ds);
@@ -2364,7 +2368,11 @@ mt7531_setup_common(struct dsa_switch *ds)
 
 	/* Trap BPDUs to the CPU port(s) */
 	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
-		   MT753X_BPDU_CPU_ONLY);
+		   MT753X_PORT_FW_CPU_ONLY);
+
+	/* Trap LLDP frames with :0E MAC DA to the CPU port(s) */
+	mt7530_rmw(priv, MT753X_RGAC2, MT753X_R0E_PORT_FW_MASK,
+		   MT753X_R0E_PORT_FW(MT753X_PORT_FW_CPU_ONLY));
 
 	/* Enable and reset MIB counters */
 	mt7530_mib_reset(ds);
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 28dbd131a535..5f048af2d89f 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -63,16 +63,21 @@ enum mt753x_id {
 #define MT753X_MIRROR_MASK(id)		((((id) == ID_MT7531) || ((id) == ID_MT7988)) ?	\
 					 MT7531_MIRROR_MASK : MIRROR_MASK)
 
-/* Registers for BPDU and PAE frame control*/
+/* Register for BPDU and PAE frame control */
 #define MT753X_BPC			0x24
 #define  MT753X_BPDU_PORT_FW_MASK	GENMASK(2, 0)
 
-enum mt753x_bpdu_port_fw {
-	MT753X_BPDU_FOLLOW_MFC,
-	MT753X_BPDU_CPU_EXCLUDE = 4,
-	MT753X_BPDU_CPU_INCLUDE = 5,
-	MT753X_BPDU_CPU_ONLY = 6,
-	MT753X_BPDU_DROP = 7,
+/* Register for :03 and :0E MAC DA frame control */
+#define MT753X_RGAC2			0x2c
+#define  MT753X_R0E_PORT_FW_MASK	GENMASK(18, 16)
+#define  MT753X_R0E_PORT_FW(x)		FIELD_PREP(MT753X_R0E_PORT_FW_MASK, x)
+
+enum mt753x_port_fw {
+	MT753X_PORT_FW_FOLLOW_MFC,
+	MT753X_PORT_FW_CPU_EXCLUDE = 4,
+	MT753X_PORT_FW_CPU_INCLUDE = 5,
+	MT753X_PORT_FW_CPU_ONLY = 6,
+	MT753X_PORT_FW_DROP = 7,
 };
 
 /* Registers for address table access */
-- 
2.39.2

[PATCH net v4 6/7] net: dsa: introduce preferred_default_local_cpu_port and use on MT7530

[arinc9.unal]

From: Vladimir Oltean <olteanv@gmail.com>

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
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 <olteanv@gmail.com>
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 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 8388b058fbe4..4c44fc664419 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)
@@ -3122,6 +3136,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

[PATCH net v4 7/7] MAINTAINERS: add me as maintainer of MEDIATEK SWITCH DRIVER

[arinc9.unal]

From: Arınç ÜNAL <arinc.unal@arinc9.com>

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 <arinc.unal@arinc9.com>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
---
 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 <sean.wang@mediatek.com>
+M:	Arınç ÜNAL <arinc.unal@arinc9.com>
+M:	Daniel Golle <daniel@makrotopia.org>
 M:	Landen Chao <Landen.Chao@mediatek.com>
 M:	DENG Qingfang <dqfext@gmail.com>
-M:	Daniel Golle <daniel@makrotopia.org>
+M:	Sean Wang <sean.wang@mediatek.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/dsa/mt7530-mdio.c
-- 
2.39.2

Re: [PATCH net v4 0/7] net: dsa: mt7530: fix multiple CPU ports, BPDU and LLDP handling

[Russell King (Oracle)]

Hi,

Please slow down your rate of patch submission - I haven't had a chance
to review the other patches yet (and I suspect no one else has.) Always
allow a bit of time for discussion.

Just because you receive one comment doesn't mean you need to rush to
get a new series out. Give it at least a few days because there may be
further discussion of the points raised.

Sending new versions quickly after previous comments significantly
increases reviewer workload.

Thanks.

On Mon, Jun 12, 2023 at 10:59:38AM +0300, arinc9.unal@gmail.com wrote:
> Hi.
> 
> This patch series fixes hopefully all 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ç
> 
> 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 (7):
>   net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7531
>   net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530
>   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 | 75 ++++++++++++++++++++++++++++++++++++-------
>  drivers/net/dsa/mt7530.h | 26 +++++++++------
>  include/net/dsa.h        |  8 +++++
>  net/dsa/dsa.c            | 24 +++++++++++++-
>  5 files changed, 115 insertions(+), 23 deletions(-)
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v4 0/7] net: dsa: mt7530: fix multiple CPU ports, BPDU and LLDP handling

[Russell King (Oracle)]

On Mon, Jun 12, 2023 at 12:37:29PM +0100, Russell King (Oracle) wrote:
> Hi,
> 
> Please slow down your rate of patch submission - I haven't had a chance
> to review the other patches yet (and I suspect no one else has.) Always
> allow a bit of time for discussion.
> 
> Just because you receive one comment doesn't mean you need to rush to
> get a new series out. Give it at least a few days because there may be
> further discussion of the points raised.
> 
> Sending new versions quickly after previous comments significantly
> increases reviewer workload.

And a very illustratory point is that I responded with a follow up to
your reply on v2, hadn't noticed that you'd sent v4, and the comments
I subsequently made on v2 apply to v4... and I haven't even looked at
v3 yet.

This is precisely why you need to stop "I've received an email, I've
made changes. Quick! Post the next version!" No, don't do that. Wait
a while for further feedback before posting the next version,
particularly if you've replied to reviewer comments - give the
reviewer some time to respond before posting the next version.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v4 0/7] net: dsa: mt7530: fix multiple CPU ports, BPDU and LLDP handling

[Russell King (Oracle)]

On Mon, Jun 12, 2023 at 12:37:29PM +0100, Russell King (Oracle) wrote:
> Hi,
> 
> Please slow down your rate of patch submission - I haven't had a chance
> to review the other patches yet (and I suspect no one else has.) Always
> allow a bit of time for discussion.
> 
> Just because you receive one comment doesn't mean you need to rush to
> get a new series out. Give it at least a few days because there may be
> further discussion of the points raised.
> 
> Sending new versions quickly after previous comments significantly
> increases reviewer workload.

And a very illustratory point is that I responded with a follow up to
your reply on v2, hadn't noticed that you'd sent v4, and the comments
I subsequently made on v2 apply to v4... and I haven't even looked at
v3 yet.

This is precisely why you need to stop "I've received an email, I've
made changes. Quick! Post the next version!" No, don't do that. Wait
a while for further feedback before posting the next version,
particularly if you've replied to reviewer comments - give the
reviewer some time to respond before posting the next version.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 2/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530

[Russell King (Oracle)]

On Mon, Jun 12, 2023 at 10:59:40AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> The CPU_PORT bits represent the CPU port to trap frames to for the MT7530
> switch. This switch traps frames received from a user port to the CPU port
> set on the CPU_PORT bits, regardless of the affinity of the user port from
> which the frames are received.

I think:

"On the MT7530, the CPU_PORT() field indicates which CPU port to trap
frames to, regardless of the affinity of the inbound user port."

covers everything necessary in the first paragraph? Sorry to be a pain
about this, but commit logs should be understandable.

> When multiple CPU ports are being used, the trapped frames won't be
> received when the DSA conduit interface, which the frames are supposed to
> be trapped to, is down because it's not affine to any user port. This
> requires the DSA conduit interface to be manually set up for the trapped
> frames to be received.

"When multiple CPU ports are in use, if the DSA conduit interface is
down, trapped frames won't be passed to the conduit interface."

> To fix this, implement ds->ops->master_state_change() on this subdriver and
> set the CPU_PORT bits to the CPU port which the DSA conduit interface its

... "to the first CPU port" - isn't that what the code is doing with
__ffs(priv->active_cpu_ports)? You're giving priority to the lowest
numbered port, and I think that should be stated in the commit message.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 2/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530

[Russell King (Oracle)]

On Mon, Jun 12, 2023 at 10:59:40AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> The CPU_PORT bits represent the CPU port to trap frames to for the MT7530
> switch. This switch traps frames received from a user port to the CPU port
> set on the CPU_PORT bits, regardless of the affinity of the user port from
> which the frames are received.

I think:

"On the MT7530, the CPU_PORT() field indicates which CPU port to trap
frames to, regardless of the affinity of the inbound user port."

covers everything necessary in the first paragraph? Sorry to be a pain
about this, but commit logs should be understandable.

> When multiple CPU ports are being used, the trapped frames won't be
> received when the DSA conduit interface, which the frames are supposed to
> be trapped to, is down because it's not affine to any user port. This
> requires the DSA conduit interface to be manually set up for the trapped
> frames to be received.

"When multiple CPU ports are in use, if the DSA conduit interface is
down, trapped frames won't be passed to the conduit interface."

> To fix this, implement ds->ops->master_state_change() on this subdriver and
> set the CPU_PORT bits to the CPU port which the DSA conduit interface its

... "to the first CPU port" - isn't that what the code is doing with
__ffs(priv->active_cpu_ports)? You're giving priority to the lowest
numbered port, and I think that should be stated in the commit message.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v4 2/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530

[Russell King (Oracle)]

On Mon, Jun 12, 2023 at 10:09:45PM +0100, Russell King (Oracle) wrote:
> On Mon, Jun 12, 2023 at 10:59:40AM +0300, arinc9.unal@gmail.com wrote:
> > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > 
> > The CPU_PORT bits represent the CPU port to trap frames to for the MT7530
> > switch. This switch traps frames received from a user port to the CPU port
> > set on the CPU_PORT bits, regardless of the affinity of the user port from
> > which the frames are received.
> 
> I think:
> 
> "On the MT7530, the CPU_PORT() field indicates which CPU port to trap
> frames to, regardless of the affinity of the inbound user port."

also, is it really the MT7530, or is it MT7621? The code only does
something for the MT7621.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 2/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530

[Russell King (Oracle)]

On Mon, Jun 12, 2023 at 10:09:45PM +0100, Russell King (Oracle) wrote:
> On Mon, Jun 12, 2023 at 10:59:40AM +0300, arinc9.unal@gmail.com wrote:
> > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > 
> > The CPU_PORT bits represent the CPU port to trap frames to for the MT7530
> > switch. This switch traps frames received from a user port to the CPU port
> > set on the CPU_PORT bits, regardless of the affinity of the user port from
> > which the frames are received.
> 
> I think:
> 
> "On the MT7530, the CPU_PORT() field indicates which CPU port to trap
> frames to, regardless of the affinity of the inbound user port."

also, is it really the MT7530, or is it MT7621? The code only does
something for the MT7621.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v4 3/7] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch

[Russell King (Oracle)]

On Mon, Jun 12, 2023 at 10:59:41AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> The check for setting the CPU_PORT bits must include the non-MT7621 SoC
> MT7530 switch variants to trap frames. Expand the check to include them.

... and now you add support for this to the MT7530, which is what
alerted me to what seems to be a mistake in the previous patch.

"The setup of CPU_PORT() needs to be done for the MT7530 switch variants
as well as the MT7621."

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v4 3/7] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch

[Russell King (Oracle)]

On Mon, Jun 12, 2023 at 10:59:41AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> The check for setting the CPU_PORT bits must include the non-MT7621 SoC
> MT7530 switch variants to trap frames. Expand the check to include them.

... and now you add support for this to the MT7530, which is what
alerted me to what seems to be a mistake in the previous patch.

"The setup of CPU_PORT() needs to be done for the MT7530 switch variants
as well as the MT7621."

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 0/7] net: dsa: mt7530: fix multiple CPU ports, BPDU and LLDP handling

[Andrew Lunn]

On Mon, Jun 12, 2023 at 09:52:29PM +0100, Russell King (Oracle) wrote:
> On Mon, Jun 12, 2023 at 12:37:29PM +0100, Russell King (Oracle) wrote:
> > Hi,
> > 
> > Please slow down your rate of patch submission - I haven't had a chance
> > to review the other patches yet (and I suspect no one else has.) Always
> > allow a bit of time for discussion.
> > 
> > Just because you receive one comment doesn't mean you need to rush to
> > get a new series out. Give it at least a few days because there may be
> > further discussion of the points raised.
> > 
> > Sending new versions quickly after previous comments significantly
> > increases reviewer workload.
> 
> And a very illustratory point is that I responded with a follow up to
> your reply on v2, hadn't noticed that you'd sent v4, and the comments
> I subsequently made on v2 apply to v4... and I haven't even looked at
> v3 yet.

Hi Arınç

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#netdev-faq

says:

  don't repost your patches within one 24h period

  2.6.6. Resending after review¶

  Allow at least 24 hours to pass between postings. This will ensure
  reviewers from all geographical locations have a chance to chime
  in. Do not wait too long (weeks) between postings either as it will
  make it harder for reviewers to recall all the context.
 
  Make sure you address all the feedback in your new posting. Do not
  post a new version of the code if the discussion about the previous
  version is still ongoing, unless directly instructed by a reviewer.

During a weekend, i would say 24 hours is way too short, and 3 days is
more like it, given that for a lot of people being a Maintainer is a
day job, 9-5 week days.

You should also try to gauge how fast Maintainers are reacting. 24
hours is often too fast. You know Russell is interested in these
patches, so don't send a new version until you actually get feedback
from him, and the discussion has come to a conclusion.

     Andrew

Re: [PATCH net v4 0/7] net: dsa: mt7530: fix multiple CPU ports, BPDU and LLDP handling

[Andrew Lunn]

On Mon, Jun 12, 2023 at 09:52:29PM +0100, Russell King (Oracle) wrote:
> On Mon, Jun 12, 2023 at 12:37:29PM +0100, Russell King (Oracle) wrote:
> > Hi,
> > 
> > Please slow down your rate of patch submission - I haven't had a chance
> > to review the other patches yet (and I suspect no one else has.) Always
> > allow a bit of time for discussion.
> > 
> > Just because you receive one comment doesn't mean you need to rush to
> > get a new series out. Give it at least a few days because there may be
> > further discussion of the points raised.
> > 
> > Sending new versions quickly after previous comments significantly
> > increases reviewer workload.
> 
> And a very illustratory point is that I responded with a follow up to
> your reply on v2, hadn't noticed that you'd sent v4, and the comments
> I subsequently made on v2 apply to v4... and I haven't even looked at
> v3 yet.

Hi Arınç

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#netdev-faq

says:

  don't repost your patches within one 24h period

  2.6.6. Resending after review¶

  Allow at least 24 hours to pass between postings. This will ensure
  reviewers from all geographical locations have a chance to chime
  in. Do not wait too long (weeks) between postings either as it will
  make it harder for reviewers to recall all the context.
 
  Make sure you address all the feedback in your new posting. Do not
  post a new version of the code if the discussion about the previous
  version is still ongoing, unless directly instructed by a reviewer.

During a weekend, i would say 24 hours is way too short, and 3 days is
more like it, given that for a lot of people being a Maintainer is a
day job, 9-5 week days.

You should also try to gauge how fast Maintainers are reacting. 24
hours is often too fast. You know Russell is interested in these
patches, so don't send a new version until you actually get feedback
from him, and the discussion has come to a conclusion.

     Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 0/7] net: dsa: mt7530: fix multiple CPU ports, BPDU and LLDP handling

[Arınç ÜNAL]

On 13.06.2023 00:30, Andrew Lunn wrote:
> On Mon, Jun 12, 2023 at 09:52:29PM +0100, Russell King (Oracle) wrote:
>> On Mon, Jun 12, 2023 at 12:37:29PM +0100, Russell King (Oracle) wrote:
>>> Hi,
>>>
>>> Please slow down your rate of patch submission - I haven't had a chance
>>> to review the other patches yet (and I suspect no one else has.) Always
>>> allow a bit of time for discussion.
>>>
>>> Just because you receive one comment doesn't mean you need to rush to
>>> get a new series out. Give it at least a few days because there may be
>>> further discussion of the points raised.
>>>
>>> Sending new versions quickly after previous comments significantly
>>> increases reviewer workload.
>>
>> And a very illustratory point is that I responded with a follow up to
>> your reply on v2, hadn't noticed that you'd sent v4, and the comments
>> I subsequently made on v2 apply to v4... and I haven't even looked at
>> v3 yet.
> 
> Hi Arınç
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#netdev-faq
> 
> says:
> 
>    don't repost your patches within one 24h period
> 
>    2.6.6. Resending after review¶
> 
>    Allow at least 24 hours to pass between postings. This will ensure
>    reviewers from all geographical locations have a chance to chime
>    in. Do not wait too long (weeks) between postings either as it will
>    make it harder for reviewers to recall all the context.
>   
>    Make sure you address all the feedback in your new posting. Do not
>    post a new version of the code if the discussion about the previous
>    version is still ongoing, unless directly instructed by a reviewer.
> 
> During a weekend, i would say 24 hours is way too short, and 3 days is
> more like it, given that for a lot of people being a Maintainer is a
> day job, 9-5 week days.
> 
> You should also try to gauge how fast Maintainers are reacting. 24
> hours is often too fast. You know Russell is interested in these
> patches, so don't send a new version until you actually get feedback
> from him, and the discussion has come to a conclusion.

Understood, thank you both for the kind warning.

Arınç

Re: [PATCH net v4 0/7] net: dsa: mt7530: fix multiple CPU ports, BPDU and LLDP handling

[Arınç ÜNAL]

On 13.06.2023 00:30, Andrew Lunn wrote:
> On Mon, Jun 12, 2023 at 09:52:29PM +0100, Russell King (Oracle) wrote:
>> On Mon, Jun 12, 2023 at 12:37:29PM +0100, Russell King (Oracle) wrote:
>>> Hi,
>>>
>>> Please slow down your rate of patch submission - I haven't had a chance
>>> to review the other patches yet (and I suspect no one else has.) Always
>>> allow a bit of time for discussion.
>>>
>>> Just because you receive one comment doesn't mean you need to rush to
>>> get a new series out. Give it at least a few days because there may be
>>> further discussion of the points raised.
>>>
>>> Sending new versions quickly after previous comments significantly
>>> increases reviewer workload.
>>
>> And a very illustratory point is that I responded with a follow up to
>> your reply on v2, hadn't noticed that you'd sent v4, and the comments
>> I subsequently made on v2 apply to v4... and I haven't even looked at
>> v3 yet.
> 
> Hi Arınç
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#netdev-faq
> 
> says:
> 
>    don't repost your patches within one 24h period
> 
>    2.6.6. Resending after review¶
> 
>    Allow at least 24 hours to pass between postings. This will ensure
>    reviewers from all geographical locations have a chance to chime
>    in. Do not wait too long (weeks) between postings either as it will
>    make it harder for reviewers to recall all the context.
>   
>    Make sure you address all the feedback in your new posting. Do not
>    post a new version of the code if the discussion about the previous
>    version is still ongoing, unless directly instructed by a reviewer.
> 
> During a weekend, i would say 24 hours is way too short, and 3 days is
> more like it, given that for a lot of people being a Maintainer is a
> day job, 9-5 week days.
> 
> You should also try to gauge how fast Maintainers are reacting. 24
> hours is often too fast. You know Russell is interested in these
> patches, so don't send a new version until you actually get feedback
> from him, and the discussion has come to a conclusion.

Understood, thank you both for the kind warning.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 2/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530

[Arınç ÜNAL]

On 13.06.2023 00:09, Russell King (Oracle) wrote:
> On Mon, Jun 12, 2023 at 10:59:40AM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> The CPU_PORT bits represent the CPU port to trap frames to for the MT7530
>> switch. This switch traps frames received from a user port to the CPU port
>> set on the CPU_PORT bits, regardless of the affinity of the user port from
>> which the frames are received.
> 
> I think:
> 
> "On the MT7530, the CPU_PORT() field indicates which CPU port to trap
> frames to, regardless of the affinity of the inbound user port."
> 
> covers everything necessary in the first paragraph? Sorry to be a pain
> about this, but commit logs should be understandable.

Sounds good to me.

> 
>> When multiple CPU ports are being used, the trapped frames won't be
>> received when the DSA conduit interface, which the frames are supposed to
>> be trapped to, is down because it's not affine to any user port. This
>> requires the DSA conduit interface to be manually set up for the trapped
>> frames to be received.
> 
> "When multiple CPU ports are in use, if the DSA conduit interface is
> down, trapped frames won't be passed to the conduit interface."

Ok.

> 
>> To fix this, implement ds->ops->master_state_change() on this subdriver and
>> set the CPU_PORT bits to the CPU port which the DSA conduit interface its
> 
> ... "to the first CPU port" - isn't that what the code is doing with
> __ffs(priv->active_cpu_ports)? You're giving priority to the lowest
> numbered port, and I think that should be stated in the commit message.

Will do.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 2/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530

[Arınç ÜNAL]

On 13.06.2023 00:09, Russell King (Oracle) wrote:
> On Mon, Jun 12, 2023 at 10:59:40AM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> The CPU_PORT bits represent the CPU port to trap frames to for the MT7530
>> switch. This switch traps frames received from a user port to the CPU port
>> set on the CPU_PORT bits, regardless of the affinity of the user port from
>> which the frames are received.
> 
> I think:
> 
> "On the MT7530, the CPU_PORT() field indicates which CPU port to trap
> frames to, regardless of the affinity of the inbound user port."
> 
> covers everything necessary in the first paragraph? Sorry to be a pain
> about this, but commit logs should be understandable.

Sounds good to me.

> 
>> When multiple CPU ports are being used, the trapped frames won't be
>> received when the DSA conduit interface, which the frames are supposed to
>> be trapped to, is down because it's not affine to any user port. This
>> requires the DSA conduit interface to be manually set up for the trapped
>> frames to be received.
> 
> "When multiple CPU ports are in use, if the DSA conduit interface is
> down, trapped frames won't be passed to the conduit interface."

Ok.

> 
>> To fix this, implement ds->ops->master_state_change() on this subdriver and
>> set the CPU_PORT bits to the CPU port which the DSA conduit interface its
> 
> ... "to the first CPU port" - isn't that what the code is doing with
> __ffs(priv->active_cpu_ports)? You're giving priority to the lowest
> numbered port, and I think that should be stated in the commit message.

Will do.

Arınç

Re: [PATCH net v4 3/7] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch

[Arınç ÜNAL]

On 13.06.2023 00:14, Russell King (Oracle) wrote:
> On Mon, Jun 12, 2023 at 10:59:41AM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> The check for setting the CPU_PORT bits must include the non-MT7621 SoC
>> MT7530 switch variants to trap frames. Expand the check to include them.
> 
> ... and now you add support for this to the MT7530, which is what
> alerted me to what seems to be a mistake in the previous patch.
> 
> "The setup of CPU_PORT() needs to be done for the MT7530 switch variants
> as well as the MT7621."

No. ID_MT7621 represents the multi-chip module MT7530 switch in certain 
MT7621 SoCs. So saying "the MT7530 switch variants" already covers the 
switch on the MT7621 SoCs.

Arınç

Re: [PATCH net v4 3/7] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch

[Arınç ÜNAL]

On 13.06.2023 00:14, Russell King (Oracle) wrote:
> On Mon, Jun 12, 2023 at 10:59:41AM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> The check for setting the CPU_PORT bits must include the non-MT7621 SoC
>> MT7530 switch variants to trap frames. Expand the check to include them.
> 
> ... and now you add support for this to the MT7530, which is what
> alerted me to what seems to be a mistake in the previous patch.
> 
> "The setup of CPU_PORT() needs to be done for the MT7530 switch variants
> as well as the MT7621."

No. ID_MT7621 represents the multi-chip module MT7530 switch in certain 
MT7621 SoCs. So saying "the MT7530 switch variants" already covers the 
switch on the MT7621 SoCs.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Russell King (Oracle)]

On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index e4c169843f2e..8388b058fbe4 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2261,7 +2261,11 @@ mt7530_setup(struct dsa_switch *ds)
>  
>  	/* Trap BPDUs to the CPU port */
>  	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> -		   MT753X_BPDU_CPU_ONLY);
> +		   MT753X_PORT_FW_CPU_ONLY);
> +
> +	/* Trap LLDP frames with :0E MAC DA to the CPU port */
> +	mt7530_rmw(priv, MT753X_RGAC2, MT753X_R0E_PORT_FW_MASK,
> +		   MT753X_R0E_PORT_FW(MT753X_PORT_FW_CPU_ONLY));
>  
>  	/* Enable and reset MIB counters */
>  	mt7530_mib_reset(ds);
> @@ -2364,7 +2368,11 @@ mt7531_setup_common(struct dsa_switch *ds)
>  
>  	/* Trap BPDUs to the CPU port(s) */
>  	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> -		   MT753X_BPDU_CPU_ONLY);
> +		   MT753X_PORT_FW_CPU_ONLY);
> +
> +	/* Trap LLDP frames with :0E MAC DA to the CPU port(s) */
> +	mt7530_rmw(priv, MT753X_RGAC2, MT753X_R0E_PORT_FW_MASK,
> +		   MT753X_R0E_PORT_FW(MT753X_PORT_FW_CPU_ONLY));

Looking at the above two hunks, they look 100% identical. Given that
they are both setting up trapping to the CPU port, maybe they should
be moved into their own common function called from both setup()
functions?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Russell King (Oracle)]

On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index e4c169843f2e..8388b058fbe4 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2261,7 +2261,11 @@ mt7530_setup(struct dsa_switch *ds)
>  
>  	/* Trap BPDUs to the CPU port */
>  	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> -		   MT753X_BPDU_CPU_ONLY);
> +		   MT753X_PORT_FW_CPU_ONLY);
> +
> +	/* Trap LLDP frames with :0E MAC DA to the CPU port */
> +	mt7530_rmw(priv, MT753X_RGAC2, MT753X_R0E_PORT_FW_MASK,
> +		   MT753X_R0E_PORT_FW(MT753X_PORT_FW_CPU_ONLY));
>  
>  	/* Enable and reset MIB counters */
>  	mt7530_mib_reset(ds);
> @@ -2364,7 +2368,11 @@ mt7531_setup_common(struct dsa_switch *ds)
>  
>  	/* Trap BPDUs to the CPU port(s) */
>  	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> -		   MT753X_BPDU_CPU_ONLY);
> +		   MT753X_PORT_FW_CPU_ONLY);
> +
> +	/* Trap LLDP frames with :0E MAC DA to the CPU port(s) */
> +	mt7530_rmw(priv, MT753X_RGAC2, MT753X_R0E_PORT_FW_MASK,
> +		   MT753X_R0E_PORT_FW(MT753X_PORT_FW_CPU_ONLY));

Looking at the above two hunks, they look 100% identical. Given that
they are both setting up trapping to the CPU port, maybe they should
be moved into their own common function called from both setup()
functions?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Russell King (Oracle)]

On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> LLDP frames are link-local frames, therefore they must be trapped to the
> CPU port. Currently, the MT753X switches treat LLDP frames as regular
> multicast frames, therefore flooding them to user ports. To fix this, set
> LLDP frames to be trapped to the CPU port(s).
> 
> The mt753x_bpdu_port_fw enum is universally used for trapping frames,
> therefore rename it and the values in it to mt753x_port_fw.
> 
> For MT7530, LLDP frames received from a user port will be trapped to the
> numerically smallest CPU port which is affine to the DSA conduit interface
> that is up.
> 
> For MT7531 and the switch on the MT7988 SoC, LLDP frames received from a
> user port will be trapped to the CPU port that is affine to the user port
> from which the frames are received.
> 
> The bit for R0E_MANG_FR is 27. When set, the switch regards the frames with
> :0E MAC DA as management (LLDP) frames. This bit is set to 1 after reset on
> MT7530 and MT7531 according to the documents MT7620 Programming Guide v1.0
> and MT7531 Reference Manual for Development Board v1.0, so there's no need
> to deal with this bit. Since there's currently no public document for the
> switch on the MT7988 SoC, I assume this is also the case for this switch.
> 
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")

Patch 4 claims to be a fix for this commit, and introduces one of these
modifications to MT753X_BPC, which this patch then changes.

On the face of it, it seems this patch is actually a fix to patch 4 as
well as the original patch, so does that mean that patch 4 only half
fixes a problem?

Bah, I give up with this. IMHO it's just too much of a mess trying to
do any sane review of it. No, I'm not going to give any acks or
reviewed-bys to it because nothing here makes much sense to me.

And I just can't be bothered trying to parse the commit messages
anymore.

Sorry but no, I'm going to be ignoring these patch sets from now on.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Russell King (Oracle)]

On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> LLDP frames are link-local frames, therefore they must be trapped to the
> CPU port. Currently, the MT753X switches treat LLDP frames as regular
> multicast frames, therefore flooding them to user ports. To fix this, set
> LLDP frames to be trapped to the CPU port(s).
> 
> The mt753x_bpdu_port_fw enum is universally used for trapping frames,
> therefore rename it and the values in it to mt753x_port_fw.
> 
> For MT7530, LLDP frames received from a user port will be trapped to the
> numerically smallest CPU port which is affine to the DSA conduit interface
> that is up.
> 
> For MT7531 and the switch on the MT7988 SoC, LLDP frames received from a
> user port will be trapped to the CPU port that is affine to the user port
> from which the frames are received.
> 
> The bit for R0E_MANG_FR is 27. When set, the switch regards the frames with
> :0E MAC DA as management (LLDP) frames. This bit is set to 1 after reset on
> MT7530 and MT7531 according to the documents MT7620 Programming Guide v1.0
> and MT7531 Reference Manual for Development Board v1.0, so there's no need
> to deal with this bit. Since there's currently no public document for the
> switch on the MT7988 SoC, I assume this is also the case for this switch.
> 
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")

Patch 4 claims to be a fix for this commit, and introduces one of these
modifications to MT753X_BPC, which this patch then changes.

On the face of it, it seems this patch is actually a fix to patch 4 as
well as the original patch, so does that mean that patch 4 only half
fixes a problem?

Bah, I give up with this. IMHO it's just too much of a mess trying to
do any sane review of it. No, I'm not going to give any acks or
reviewed-bys to it because nothing here makes much sense to me.

And I just can't be bothered trying to parse the commit messages
anymore.

Sorry but no, I'm going to be ignoring these patch sets from now on.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 1/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7531

[Vladimir Oltean]

On Mon, Jun 12, 2023 at 10:59:39AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
> SoC represents a CPU port to trap frames to. These switches trap frames
> received from a user port to the CPU port that is affine to the user port
> from which the frames are received.
> 
> Currently, only the bit that corresponds to the first found CPU port is set
> on the bitmap. When multiple CPU ports are being used, the trapped frames
> from the user ports not affine to the first CPU port will be dropped as the
> other CPU port is not set on the bitmap. The switch on the MT7988 SoC is
> not affected as there's only one port to be used as a CPU port.
> 
> To fix this, introduce the MT7531_CPU_PMAP macro to individually set the
> bits of the CPU port bitmap. Set the CPU port bitmap for MT7531 and the
> switch on the MT7988 SoC on mt753x_cpu_port_enable() which runs on a loop
> for each CPU port.
> 
> Add a comment to explain frame trapping for these switches.
> 
> According to the document MT7531 Reference Manual for Development Board
> v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
> beforehand. Since there's currently no public document for the switch on
> the MT7988 SoC, I assume this is also the case for this switch.
> 
> Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---

Would you agree that this 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 in the code base?

Re: [PATCH net v4 1/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7531

[Vladimir Oltean]

On Mon, Jun 12, 2023 at 10:59:39AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
> SoC represents a CPU port to trap frames to. These switches trap frames
> received from a user port to the CPU port that is affine to the user port
> from which the frames are received.
> 
> Currently, only the bit that corresponds to the first found CPU port is set
> on the bitmap. When multiple CPU ports are being used, the trapped frames
> from the user ports not affine to the first CPU port will be dropped as the
> other CPU port is not set on the bitmap. The switch on the MT7988 SoC is
> not affected as there's only one port to be used as a CPU port.
> 
> To fix this, introduce the MT7531_CPU_PMAP macro to individually set the
> bits of the CPU port bitmap. Set the CPU port bitmap for MT7531 and the
> switch on the MT7988 SoC on mt753x_cpu_port_enable() which runs on a loop
> for each CPU port.
> 
> Add a comment to explain frame trapping for these switches.
> 
> According to the document MT7531 Reference Manual for Development Board
> v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
> beforehand. Since there's currently no public document for the switch on
> the MT7988 SoC, I assume this is also the case for this switch.
> 
> Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---

Would you agree that this 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 in the code base?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 3/7] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch

[Vladimir Oltean]

On Mon, Jun 12, 2023 at 10:59:41AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> The check for setting the CPU_PORT bits must include the non-MT7621 SoC
> MT7530 switch variants to trap frames. Expand the check to include them.
> 
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  drivers/net/dsa/mt7530.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index ef8879087932..2bde2fdb5fba 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -3073,7 +3073,7 @@ mt753x_master_state_change(struct dsa_switch *ds,
>  	 * the numerically smallest CPU port which is affine to the DSA conduit
>  	 * interface that is up.
>  	 */
> -	if (priv->id != ID_MT7621)
> +	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
>  		return;

This patch and 2/7 should probably be reversed, since 2/7 is not going to net.

>  
>  	if (operational)
> -- 
> 2.39.2
> 

Re: [PATCH net v4 3/7] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch

[Vladimir Oltean]

On Mon, Jun 12, 2023 at 10:59:41AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> The check for setting the CPU_PORT bits must include the non-MT7621 SoC
> MT7530 switch variants to trap frames. Expand the check to include them.
> 
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  drivers/net/dsa/mt7530.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index ef8879087932..2bde2fdb5fba 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -3073,7 +3073,7 @@ mt753x_master_state_change(struct dsa_switch *ds,
>  	 * the numerically smallest CPU port which is affine to the DSA conduit
>  	 * interface that is up.
>  	 */
> -	if (priv->id != ID_MT7621)
> +	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
>  		return;

This patch and 2/7 should probably be reversed, since 2/7 is not going to net.

>  
>  	if (operational)
> -- 
> 2.39.2
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Arınç ÜNAL]

On 14.06.2023 19:35, Russell King (Oracle) wrote:
> On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index e4c169843f2e..8388b058fbe4 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2261,7 +2261,11 @@ mt7530_setup(struct dsa_switch *ds)
>>   
>>   	/* Trap BPDUs to the CPU port */
>>   	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
>> -		   MT753X_BPDU_CPU_ONLY);
>> +		   MT753X_PORT_FW_CPU_ONLY);
>> +
>> +	/* Trap LLDP frames with :0E MAC DA to the CPU port */
>> +	mt7530_rmw(priv, MT753X_RGAC2, MT753X_R0E_PORT_FW_MASK,
>> +		   MT753X_R0E_PORT_FW(MT753X_PORT_FW_CPU_ONLY));
>>   
>>   	/* Enable and reset MIB counters */
>>   	mt7530_mib_reset(ds);
>> @@ -2364,7 +2368,11 @@ mt7531_setup_common(struct dsa_switch *ds)
>>   
>>   	/* Trap BPDUs to the CPU port(s) */
>>   	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
>> -		   MT753X_BPDU_CPU_ONLY);
>> +		   MT753X_PORT_FW_CPU_ONLY);
>> +
>> +	/* Trap LLDP frames with :0E MAC DA to the CPU port(s) */
>> +	mt7530_rmw(priv, MT753X_RGAC2, MT753X_R0E_PORT_FW_MASK,
>> +		   MT753X_R0E_PORT_FW(MT753X_PORT_FW_CPU_ONLY));
> 
> Looking at the above two hunks, they look 100% identical. Given that
> they are both setting up trapping to the CPU port, maybe they should
> be moved into their own common function called from both setup()
> functions?

Good idea, I shall make a function called something like 
mt753x_trap_frames() on my net-next series. For this series which is for 
net, I'd like my patches to fix the issue with as less structural 
changes as possible.

Arınç

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Arınç ÜNAL]

On 14.06.2023 19:35, Russell King (Oracle) wrote:
> On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index e4c169843f2e..8388b058fbe4 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2261,7 +2261,11 @@ mt7530_setup(struct dsa_switch *ds)
>>   
>>   	/* Trap BPDUs to the CPU port */
>>   	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
>> -		   MT753X_BPDU_CPU_ONLY);
>> +		   MT753X_PORT_FW_CPU_ONLY);
>> +
>> +	/* Trap LLDP frames with :0E MAC DA to the CPU port */
>> +	mt7530_rmw(priv, MT753X_RGAC2, MT753X_R0E_PORT_FW_MASK,
>> +		   MT753X_R0E_PORT_FW(MT753X_PORT_FW_CPU_ONLY));
>>   
>>   	/* Enable and reset MIB counters */
>>   	mt7530_mib_reset(ds);
>> @@ -2364,7 +2368,11 @@ mt7531_setup_common(struct dsa_switch *ds)
>>   
>>   	/* Trap BPDUs to the CPU port(s) */
>>   	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
>> -		   MT753X_BPDU_CPU_ONLY);
>> +		   MT753X_PORT_FW_CPU_ONLY);
>> +
>> +	/* Trap LLDP frames with :0E MAC DA to the CPU port(s) */
>> +	mt7530_rmw(priv, MT753X_RGAC2, MT753X_R0E_PORT_FW_MASK,
>> +		   MT753X_R0E_PORT_FW(MT753X_PORT_FW_CPU_ONLY));
> 
> Looking at the above two hunks, they look 100% identical. Given that
> they are both setting up trapping to the CPU port, maybe they should
> be moved into their own common function called from both setup()
> functions?

Good idea, I shall make a function called something like 
mt753x_trap_frames() on my net-next series. For this series which is for 
net, I'd like my patches to fix the issue with as less structural 
changes as possible.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 4/7] net: dsa: mt7530: fix handling of BPDUs on MT7530 switch

[Vladimir Oltean]

On Mon, Jun 12, 2023 at 10:59:42AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> BPDUs are link-local frames, therefore they must be trapped to the CPU
> port. Currently, the MT7530 switch treats BPDUs as regular multicast
> frames, therefore flooding them to user ports. To fix this, set BPDUs to be
> trapped to the CPU port.
> 
> BPDUs received from a user port will be trapped to the numerically smallest
> CPU port which is affine to the DSA conduit interface that is up.
> 
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  drivers/net/dsa/mt7530.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 2bde2fdb5fba..e4c169843f2e 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2259,6 +2259,10 @@ mt7530_setup(struct dsa_switch *ds)
>  
>  	priv->p6_interface = PHY_INTERFACE_MODE_NA;
>  
> +	/* Trap BPDUs to the CPU port */
> +	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> +		   MT753X_BPDU_CPU_ONLY);
> +
>  	/* Enable and reset MIB counters */
>  	mt7530_mib_reset(ds);
>  
> -- 
> 2.39.2
> 

Where have you seen the BPC register in the memory map of MT7530 or MT7621?

Re: [PATCH net v4 4/7] net: dsa: mt7530: fix handling of BPDUs on MT7530 switch

[Vladimir Oltean]

On Mon, Jun 12, 2023 at 10:59:42AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> BPDUs are link-local frames, therefore they must be trapped to the CPU
> port. Currently, the MT7530 switch treats BPDUs as regular multicast
> frames, therefore flooding them to user ports. To fix this, set BPDUs to be
> trapped to the CPU port.
> 
> BPDUs received from a user port will be trapped to the numerically smallest
> CPU port which is affine to the DSA conduit interface that is up.
> 
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  drivers/net/dsa/mt7530.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 2bde2fdb5fba..e4c169843f2e 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2259,6 +2259,10 @@ mt7530_setup(struct dsa_switch *ds)
>  
>  	priv->p6_interface = PHY_INTERFACE_MODE_NA;
>  
> +	/* Trap BPDUs to the CPU port */
> +	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> +		   MT753X_BPDU_CPU_ONLY);
> +
>  	/* Enable and reset MIB counters */
>  	mt7530_mib_reset(ds);
>  
> -- 
> 2.39.2
> 

Where have you seen the BPC register in the memory map of MT7530 or MT7621?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Arınç ÜNAL]

On 14.06.2023 19:42, Russell King (Oracle) wrote:
> On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> LLDP frames are link-local frames, therefore they must be trapped to the
>> CPU port. Currently, the MT753X switches treat LLDP frames as regular
>> multicast frames, therefore flooding them to user ports. To fix this, set
>> LLDP frames to be trapped to the CPU port(s).
>>
>> The mt753x_bpdu_port_fw enum is universally used for trapping frames,
>> therefore rename it and the values in it to mt753x_port_fw.
>>
>> For MT7530, LLDP frames received from a user port will be trapped to the
>> numerically smallest CPU port which is affine to the DSA conduit interface
>> that is up.
>>
>> For MT7531 and the switch on the MT7988 SoC, LLDP frames received from a
>> user port will be trapped to the CPU port that is affine to the user port
>> from which the frames are received.
>>
>> The bit for R0E_MANG_FR is 27. When set, the switch regards the frames with
>> :0E MAC DA as management (LLDP) frames. This bit is set to 1 after reset on
>> MT7530 and MT7531 according to the documents MT7620 Programming Guide v1.0
>> and MT7531 Reference Manual for Development Board v1.0, so there's no need
>> to deal with this bit. Since there's currently no public document for the
>> switch on the MT7988 SoC, I assume this is also the case for this switch.
>>
>> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> 
> Patch 4 claims to be a fix for this commit, and introduces one of these
> modifications to MT753X_BPC, which this patch then changes.
> 
> On the face of it, it seems this patch is actually a fix to patch 4 as
> well as the original patch, so does that mean that patch 4 only half
> fixes a problem?

I should do the enum renaming on my net-next series instead, as it's not 
useful to what this patch fixes at all.

> 
> Bah, I give up with this. IMHO it's just too much of a mess trying to
> do any sane review of it. No, I'm not going to give any acks or
> reviewed-bys to it because nothing here makes much sense to me.
> 
> And I just can't be bothered trying to parse the commit messages
> anymore.
> 
> Sorry but no, I'm going to be ignoring these patch sets from now on.

... okay. I listen to your reviews and change my patches accordingly. If 
that's not enough, I don't know what is.

Arınç

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Arınç ÜNAL]

On 14.06.2023 19:42, Russell King (Oracle) wrote:
> On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> LLDP frames are link-local frames, therefore they must be trapped to the
>> CPU port. Currently, the MT753X switches treat LLDP frames as regular
>> multicast frames, therefore flooding them to user ports. To fix this, set
>> LLDP frames to be trapped to the CPU port(s).
>>
>> The mt753x_bpdu_port_fw enum is universally used for trapping frames,
>> therefore rename it and the values in it to mt753x_port_fw.
>>
>> For MT7530, LLDP frames received from a user port will be trapped to the
>> numerically smallest CPU port which is affine to the DSA conduit interface
>> that is up.
>>
>> For MT7531 and the switch on the MT7988 SoC, LLDP frames received from a
>> user port will be trapped to the CPU port that is affine to the user port
>> from which the frames are received.
>>
>> The bit for R0E_MANG_FR is 27. When set, the switch regards the frames with
>> :0E MAC DA as management (LLDP) frames. This bit is set to 1 after reset on
>> MT7530 and MT7531 according to the documents MT7620 Programming Guide v1.0
>> and MT7531 Reference Manual for Development Board v1.0, so there's no need
>> to deal with this bit. Since there's currently no public document for the
>> switch on the MT7988 SoC, I assume this is also the case for this switch.
>>
>> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> 
> Patch 4 claims to be a fix for this commit, and introduces one of these
> modifications to MT753X_BPC, which this patch then changes.
> 
> On the face of it, it seems this patch is actually a fix to patch 4 as
> well as the original patch, so does that mean that patch 4 only half
> fixes a problem?

I should do the enum renaming on my net-next series instead, as it's not 
useful to what this patch fixes at all.

> 
> Bah, I give up with this. IMHO it's just too much of a mess trying to
> do any sane review of it. No, I'm not going to give any acks or
> reviewed-bys to it because nothing here makes much sense to me.
> 
> And I just can't be bothered trying to parse the commit messages
> anymore.
> 
> Sorry but no, I'm going to be ignoring these patch sets from now on.

... okay. I listen to your reviews and change my patches accordingly. If 
that's not enough, I don't know what is.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 1/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7531

[Arınç ÜNAL]

On 14.06.2023 22:43, Vladimir Oltean wrote:
> On Mon, Jun 12, 2023 at 10:59:39AM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
>> SoC represents a CPU port to trap frames to. These switches trap frames
>> received from a user port to the CPU port that is affine to the user port
>> from which the frames are received.
>>
>> Currently, only the bit that corresponds to the first found CPU port is set
>> on the bitmap. When multiple CPU ports are being used, the trapped frames
>> from the user ports not affine to the first CPU port will be dropped as the
>> other CPU port is not set on the bitmap. The switch on the MT7988 SoC is
>> not affected as there's only one port to be used as a CPU port.
>>
>> To fix this, introduce the MT7531_CPU_PMAP macro to individually set the
>> bits of the CPU port bitmap. Set the CPU port bitmap for MT7531 and the
>> switch on the MT7988 SoC on mt753x_cpu_port_enable() which runs on a loop
>> for each CPU port.
>>
>> Add a comment to explain frame trapping for these switches.
>>
>> According to the document MT7531 Reference Manual for Development Board
>> v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
>> beforehand. Since there's currently no public document for the switch on
>> the MT7988 SoC, I assume this is also the case for this switch.
>>
>> Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
> 
> Would you agree that this 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 in the code base?

Makes sense. Pre-preferred_default_local_cpu_port patch, there isn't a 
case where there's a user port affine to a CPU port that is not enabled 
on the CPU port bitmap. So yeah, this is just preparatory work for "net: 
dsa: introduce preferred_default_local_cpu_port and use on MT7530".

So how do I change the patch to reflect this?

Arınç

Re: [PATCH net v4 1/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7531

[Arınç ÜNAL]

On 14.06.2023 22:43, Vladimir Oltean wrote:
> On Mon, Jun 12, 2023 at 10:59:39AM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
>> SoC represents a CPU port to trap frames to. These switches trap frames
>> received from a user port to the CPU port that is affine to the user port
>> from which the frames are received.
>>
>> Currently, only the bit that corresponds to the first found CPU port is set
>> on the bitmap. When multiple CPU ports are being used, the trapped frames
>> from the user ports not affine to the first CPU port will be dropped as the
>> other CPU port is not set on the bitmap. The switch on the MT7988 SoC is
>> not affected as there's only one port to be used as a CPU port.
>>
>> To fix this, introduce the MT7531_CPU_PMAP macro to individually set the
>> bits of the CPU port bitmap. Set the CPU port bitmap for MT7531 and the
>> switch on the MT7988 SoC on mt753x_cpu_port_enable() which runs on a loop
>> for each CPU port.
>>
>> Add a comment to explain frame trapping for these switches.
>>
>> According to the document MT7531 Reference Manual for Development Board
>> v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
>> beforehand. Since there's currently no public document for the switch on
>> the MT7988 SoC, I assume this is also the case for this switch.
>>
>> Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
> 
> Would you agree that this 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 in the code base?

Makes sense. Pre-preferred_default_local_cpu_port patch, there isn't a 
case where there's a user port affine to a CPU port that is not enabled 
on the CPU port bitmap. So yeah, this is just preparatory work for "net: 
dsa: introduce preferred_default_local_cpu_port and use on MT7530".

So how do I change the patch to reflect this?

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 3/7] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch

[Arınç ÜNAL]

On 14.06.2023 23:13, Vladimir Oltean wrote:
> On Mon, Jun 12, 2023 at 10:59:41AM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> The check for setting the CPU_PORT bits must include the non-MT7621 SoC
>> MT7530 switch variants to trap frames. Expand the check to include them.
>>
>> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>   drivers/net/dsa/mt7530.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index ef8879087932..2bde2fdb5fba 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -3073,7 +3073,7 @@ mt753x_master_state_change(struct dsa_switch *ds,
>>   	 * the numerically smallest CPU port which is affine to the DSA conduit
>>   	 * interface that is up.
>>   	 */
>> -	if (priv->id != ID_MT7621)
>> +	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
>>   		return;
> 
> This patch and 2/7 should probably be reversed, since 2/7 is not going to net.

This patch is still necessary. It'll just modify the other location 
instead of here.

https://github.com/arinc9/linux/commit/4c8b983f7a95ba637799ccd1b700ee054b030729

Arınç

Re: [PATCH net v4 3/7] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch

[Arınç ÜNAL]

On 14.06.2023 23:13, Vladimir Oltean wrote:
> On Mon, Jun 12, 2023 at 10:59:41AM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> The check for setting the CPU_PORT bits must include the non-MT7621 SoC
>> MT7530 switch variants to trap frames. Expand the check to include them.
>>
>> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>   drivers/net/dsa/mt7530.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index ef8879087932..2bde2fdb5fba 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -3073,7 +3073,7 @@ mt753x_master_state_change(struct dsa_switch *ds,
>>   	 * the numerically smallest CPU port which is affine to the DSA conduit
>>   	 * interface that is up.
>>   	 */
>> -	if (priv->id != ID_MT7621)
>> +	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
>>   		return;
> 
> This patch and 2/7 should probably be reversed, since 2/7 is not going to net.

This patch is still necessary. It'll just modify the other location 
instead of here.

https://github.com/arinc9/linux/commit/4c8b983f7a95ba637799ccd1b700ee054b030729

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 4/7] net: dsa: mt7530: fix handling of BPDUs on MT7530 switch

[Arınç ÜNAL]

On 14.06.2023 23:50, Vladimir Oltean wrote:
> On Mon, Jun 12, 2023 at 10:59:42AM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> BPDUs are link-local frames, therefore they must be trapped to the CPU
>> port. Currently, the MT7530 switch treats BPDUs as regular multicast
>> frames, therefore flooding them to user ports. To fix this, set BPDUs to be
>> trapped to the CPU port.
>>
>> BPDUs received from a user port will be trapped to the numerically smallest
>> CPU port which is affine to the DSA conduit interface that is up.
>>
>> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>   drivers/net/dsa/mt7530.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 2bde2fdb5fba..e4c169843f2e 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2259,6 +2259,10 @@ mt7530_setup(struct dsa_switch *ds)
>>   
>>   	priv->p6_interface = PHY_INTERFACE_MODE_NA;
>>   
>> +	/* Trap BPDUs to the CPU port */
>> +	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
>> +		   MT753X_BPDU_CPU_ONLY);
>> +
>>   	/* Enable and reset MIB counters */
>>   	mt7530_mib_reset(ds);
>>   
>> -- 
>> 2.39.2
>>
> 
> Where have you seen the BPC register in the memory map of MT7530 or MT7621?

I did not somehow dump the memory map of the switch hardware and confirm 
the BPC register is there, if that's what you're asking.

However, I can confirm the register is there and identical across all 
MT7530 variants. I have tested the function of the register on the MCM 
MT7530 on the MT7621 SoC and the standalone MT7530. The register is also 
described on the document MT7620 Programming Guide v1.0, page 262.

Arınç

Re: [PATCH net v4 4/7] net: dsa: mt7530: fix handling of BPDUs on MT7530 switch

[Arınç ÜNAL]

On 14.06.2023 23:50, Vladimir Oltean wrote:
> On Mon, Jun 12, 2023 at 10:59:42AM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> BPDUs are link-local frames, therefore they must be trapped to the CPU
>> port. Currently, the MT7530 switch treats BPDUs as regular multicast
>> frames, therefore flooding them to user ports. To fix this, set BPDUs to be
>> trapped to the CPU port.
>>
>> BPDUs received from a user port will be trapped to the numerically smallest
>> CPU port which is affine to the DSA conduit interface that is up.
>>
>> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>   drivers/net/dsa/mt7530.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 2bde2fdb5fba..e4c169843f2e 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2259,6 +2259,10 @@ mt7530_setup(struct dsa_switch *ds)
>>   
>>   	priv->p6_interface = PHY_INTERFACE_MODE_NA;
>>   
>> +	/* Trap BPDUs to the CPU port */
>> +	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
>> +		   MT753X_BPDU_CPU_ONLY);
>> +
>>   	/* Enable and reset MIB counters */
>>   	mt7530_mib_reset(ds);
>>   
>> -- 
>> 2.39.2
>>
> 
> Where have you seen the BPC register in the memory map of MT7530 or MT7621?

I did not somehow dump the memory map of the switch hardware and confirm 
the BPC register is there, if that's what you're asking.

However, I can confirm the register is there and identical across all 
MT7530 variants. I have tested the function of the register on the MCM 
MT7530 on the MT7621 SoC and the standalone MT7530. The register is also 
described on the document MT7620 Programming Guide v1.0, page 262.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 1/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7531

[Vladimir Oltean]

On Wed, Jun 14, 2023 at 11:56:44PM +0300, Arınç ÜNAL wrote:
> On 14.06.2023 22:43, Vladimir Oltean wrote:
> > On Mon, Jun 12, 2023 at 10:59:39AM +0300, arinc9.unal@gmail.com wrote:
> > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > 
> > > Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
> > > SoC represents a CPU port to trap frames to. These switches trap frames
> > > received from a user port to the CPU port that is affine to the user port
> > > from which the frames are received.
> > > 
> > > Currently, only the bit that corresponds to the first found CPU port is set
> > > on the bitmap. When multiple CPU ports are being used, the trapped frames
> > > from the user ports not affine to the first CPU port will be dropped as the
> > > other CPU port is not set on the bitmap. The switch on the MT7988 SoC is
> > > not affected as there's only one port to be used as a CPU port.
> > > 
> > > To fix this, introduce the MT7531_CPU_PMAP macro to individually set the
> > > bits of the CPU port bitmap. Set the CPU port bitmap for MT7531 and the
> > > switch on the MT7988 SoC on mt753x_cpu_port_enable() which runs on a loop
> > > for each CPU port.
> > > 
> > > Add a comment to explain frame trapping for these switches.
> > > 
> > > According to the document MT7531 Reference Manual for Development Board
> > > v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
> > > beforehand. Since there's currently no public document for the switch on
> > > the MT7988 SoC, I assume this is also the case for this switch.
> > > 
> > > Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
> > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > ---
> > 
> > Would you agree that this 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 in the code base?
> 
> Makes sense. Pre-preferred_default_local_cpu_port patch, there isn't a case
> where there's a user port affine to a CPU port that is not enabled on the
> CPU port bitmap. So yeah, this is just preparatory work for "net: dsa:
> introduce preferred_default_local_cpu_port and use on MT7530".
> 
> So how do I change the patch to reflect this?
> 
> Arınç

net: dsa: mt7530: set all CPU ports in MT7531_CPU_PMAP

MT7531_CPU_PMAP represents the destination port mask for trapped-to-CPU
frames (further restricted by PCR_MATRIX).

Currently the driver sets the first CPU port as the single port in this
bit mask, which works fine regardless of whether the device tree defines
port 5, 6 or 5+6 as CPU ports. This is because the logic coincides with
DSA's logic of picking the first CPU port as the CPU port that all user
ports are affine to, by default.

An upcoming change would like to influence DSA's selection of the
default CPU port to no longer be the first one, and in that case, this
logic needs adaptation.

Since there is no observed leakage or duplication of frames if all CPU
ports are defined in this bit mask, simply include them all.

Note that there is no Fixes tag

Re: [PATCH net v4 1/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7531

[Vladimir Oltean]

On Wed, Jun 14, 2023 at 11:56:44PM +0300, Arınç ÜNAL wrote:
> On 14.06.2023 22:43, Vladimir Oltean wrote:
> > On Mon, Jun 12, 2023 at 10:59:39AM +0300, arinc9.unal@gmail.com wrote:
> > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > 
> > > Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
> > > SoC represents a CPU port to trap frames to. These switches trap frames
> > > received from a user port to the CPU port that is affine to the user port
> > > from which the frames are received.
> > > 
> > > Currently, only the bit that corresponds to the first found CPU port is set
> > > on the bitmap. When multiple CPU ports are being used, the trapped frames
> > > from the user ports not affine to the first CPU port will be dropped as the
> > > other CPU port is not set on the bitmap. The switch on the MT7988 SoC is
> > > not affected as there's only one port to be used as a CPU port.
> > > 
> > > To fix this, introduce the MT7531_CPU_PMAP macro to individually set the
> > > bits of the CPU port bitmap. Set the CPU port bitmap for MT7531 and the
> > > switch on the MT7988 SoC on mt753x_cpu_port_enable() which runs on a loop
> > > for each CPU port.
> > > 
> > > Add a comment to explain frame trapping for these switches.
> > > 
> > > According to the document MT7531 Reference Manual for Development Board
> > > v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
> > > beforehand. Since there's currently no public document for the switch on
> > > the MT7988 SoC, I assume this is also the case for this switch.
> > > 
> > > Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
> > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > ---
> > 
> > Would you agree that this 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 in the code base?
> 
> Makes sense. Pre-preferred_default_local_cpu_port patch, there isn't a case
> where there's a user port affine to a CPU port that is not enabled on the
> CPU port bitmap. So yeah, this is just preparatory work for "net: dsa:
> introduce preferred_default_local_cpu_port and use on MT7530".
> 
> So how do I change the patch to reflect this?
> 
> Arınç

net: dsa: mt7530: set all CPU ports in MT7531_CPU_PMAP

MT7531_CPU_PMAP represents the destination port mask for trapped-to-CPU
frames (further restricted by PCR_MATRIX).

Currently the driver sets the first CPU port as the single port in this
bit mask, which works fine regardless of whether the device tree defines
port 5, 6 or 5+6 as CPU ports. This is because the logic coincides with
DSA's logic of picking the first CPU port as the CPU port that all user
ports are affine to, by default.

An upcoming change would like to influence DSA's selection of the
default CPU port to no longer be the first one, and in that case, this
logic needs adaptation.

Since there is no observed leakage or duplication of frames if all CPU
ports are defined in this bit mask, simply include them all.

Note that there is no Fixes tag

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 4/7] net: dsa: mt7530: fix handling of BPDUs on MT7530 switch

[Vladimir Oltean]

On Thu, Jun 15, 2023 at 12:05:44AM +0300, Arınç ÜNAL wrote:
> On 14.06.2023 23:50, Vladimir Oltean wrote:
> > Where have you seen the BPC register in the memory map of MT7530 or MT7621?
> 
> I did not somehow dump the memory map of the switch hardware and confirm the
> BPC register is there, if that's what you're asking.

I mean to say that I looked at

MT7530 Giga Switch programming guide.pdf
MT7621 Giga Switch Programming Guide.pdf
MT7621_ProgrammingGuide_GSW_v01.pdf

and I did not find this register.

> However, I can confirm the register is there and identical across all MT7530
> variants. I have tested the function of the register on the MCM MT7530 on
> the MT7621 SoC and the standalone MT7530. The register is also described on
> the document MT7620 Programming Guide v1.0, page 262.

Interesting. I did not have that one. Hard to keep up.

Re: [PATCH net v4 4/7] net: dsa: mt7530: fix handling of BPDUs on MT7530 switch

[Vladimir Oltean]

On Thu, Jun 15, 2023 at 12:05:44AM +0300, Arınç ÜNAL wrote:
> On 14.06.2023 23:50, Vladimir Oltean wrote:
> > Where have you seen the BPC register in the memory map of MT7530 or MT7621?
> 
> I did not somehow dump the memory map of the switch hardware and confirm the
> BPC register is there, if that's what you're asking.

I mean to say that I looked at

MT7530 Giga Switch programming guide.pdf
MT7621 Giga Switch Programming Guide.pdf
MT7621_ProgrammingGuide_GSW_v01.pdf

and I did not find this register.

> However, I can confirm the register is there and identical across all MT7530
> variants. I have tested the function of the register on the MCM MT7530 on
> the MT7621 SoC and the standalone MT7530. The register is also described on
> the document MT7620 Programming Guide v1.0, page 262.

Interesting. I did not have that one. Hard to keep up.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 3/7] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch

[Vladimir Oltean]

On Wed, Jun 14, 2023 at 11:59:33PM +0300, Arınç ÜNAL wrote:
> On 14.06.2023 23:13, Vladimir Oltean wrote:
> > On Mon, Jun 12, 2023 at 10:59:41AM +0300, arinc9.unal@gmail.com wrote:
> > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > 
> > > The check for setting the CPU_PORT bits must include the non-MT7621 SoC
> > > MT7530 switch variants to trap frames. Expand the check to include them.
> > > 
> > > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > ---
> > >   drivers/net/dsa/mt7530.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > index ef8879087932..2bde2fdb5fba 100644
> > > --- a/drivers/net/dsa/mt7530.c
> > > +++ b/drivers/net/dsa/mt7530.c
> > > @@ -3073,7 +3073,7 @@ mt753x_master_state_change(struct dsa_switch *ds,
> > >   	 * the numerically smallest CPU port which is affine to the DSA conduit
> > >   	 * interface that is up.
> > >   	 */
> > > -	if (priv->id != ID_MT7621)
> > > +	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
> > >   		return;
> > 
> > This patch and 2/7 should probably be reversed, since 2/7 is not going to net.
> 
> This patch is still necessary. It'll just modify the other location instead
> of here.
> 
> https://github.com/arinc9/linux/commit/4c8b983f7a95ba637799ccd1b700ee054b030729
> 
> Arınç

That's basically what I said, sorry if I wasn't clear.

Re: [PATCH net v4 3/7] net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch

[Vladimir Oltean]

On Wed, Jun 14, 2023 at 11:59:33PM +0300, Arınç ÜNAL wrote:
> On 14.06.2023 23:13, Vladimir Oltean wrote:
> > On Mon, Jun 12, 2023 at 10:59:41AM +0300, arinc9.unal@gmail.com wrote:
> > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > 
> > > The check for setting the CPU_PORT bits must include the non-MT7621 SoC
> > > MT7530 switch variants to trap frames. Expand the check to include them.
> > > 
> > > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > ---
> > >   drivers/net/dsa/mt7530.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > index ef8879087932..2bde2fdb5fba 100644
> > > --- a/drivers/net/dsa/mt7530.c
> > > +++ b/drivers/net/dsa/mt7530.c
> > > @@ -3073,7 +3073,7 @@ mt753x_master_state_change(struct dsa_switch *ds,
> > >   	 * the numerically smallest CPU port which is affine to the DSA conduit
> > >   	 * interface that is up.
> > >   	 */
> > > -	if (priv->id != ID_MT7621)
> > > +	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
> > >   		return;
> > 
> > This patch and 2/7 should probably be reversed, since 2/7 is not going to net.
> 
> This patch is still necessary. It'll just modify the other location instead
> of here.
> 
> https://github.com/arinc9/linux/commit/4c8b983f7a95ba637799ccd1b700ee054b030729
> 
> Arınç

That's basically what I said, sorry if I wasn't clear.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 1/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7531

[Arınç ÜNAL]

On 15 June 2023 00:13:52 EEST, Vladimir Oltean <olteanv@gmail.com> wrote:
>On Wed, Jun 14, 2023 at 11:56:44PM +0300, Arınç ÜNAL wrote:
>> On 14.06.2023 22:43, Vladimir Oltean wrote:
>> > On Mon, Jun 12, 2023 at 10:59:39AM +0300, arinc9.unal@gmail.com wrote:
>> > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
>> > > 
>> > > Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
>> > > SoC represents a CPU port to trap frames to. These switches trap frames
>> > > received from a user port to the CPU port that is affine to the user port
>> > > from which the frames are received.
>> > > 
>> > > Currently, only the bit that corresponds to the first found CPU port is set
>> > > on the bitmap. When multiple CPU ports are being used, the trapped frames
>> > > from the user ports not affine to the first CPU port will be dropped as the
>> > > other CPU port is not set on the bitmap. The switch on the MT7988 SoC is
>> > > not affected as there's only one port to be used as a CPU port.
>> > > 
>> > > To fix this, introduce the MT7531_CPU_PMAP macro to individually set the
>> > > bits of the CPU port bitmap. Set the CPU port bitmap for MT7531 and the
>> > > switch on the MT7988 SoC on mt753x_cpu_port_enable() which runs on a loop
>> > > for each CPU port.
>> > > 
>> > > Add a comment to explain frame trapping for these switches.
>> > > 
>> > > According to the document MT7531 Reference Manual for Development Board
>> > > v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
>> > > beforehand. Since there's currently no public document for the switch on
>> > > the MT7988 SoC, I assume this is also the case for this switch.
>> > > 
>> > > Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
>> > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> > > ---
>> > 
>> > Would you agree that this 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 in the code base?
>> 
>> Makes sense. Pre-preferred_default_local_cpu_port patch, there isn't a case
>> where there's a user port affine to a CPU port that is not enabled on the
>> CPU port bitmap. So yeah, this is just preparatory work for "net: dsa:
>> introduce preferred_default_local_cpu_port and use on MT7530".
>> 
>> So how do I change the patch to reflect this?
>> 
>> Arınç
>
>net: dsa: mt7530: set all CPU ports in MT7531_CPU_PMAP
>
>MT7531_CPU_PMAP represents the destination port mask for trapped-to-CPU
>frames (further restricted by PCR_MATRIX).
>
>Currently the driver sets the first CPU port as the single port in this
>bit mask, which works fine regardless of whether the device tree defines
>port 5, 6 or 5+6 as CPU ports. This is because the logic coincides with
>DSA's logic of picking the first CPU port as the CPU port that all user
>ports are affine to, by default.
>
>An upcoming change would like to influence DSA's selection of the
>default CPU port to no longer be the first one, and in that case, this
>logic needs adaptation.
>
>Since there is no observed leakage or duplication of frames if all CPU
>ports are defined in this bit mask, simply include them all.
>
>Note that there is no Fixes tag

Thanks a lot for making it easier for me.

Arınç

Re: [PATCH net v4 1/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7531

[Arınç ÜNAL]

On 15 June 2023 00:13:52 EEST, Vladimir Oltean <olteanv@gmail.com> wrote:
>On Wed, Jun 14, 2023 at 11:56:44PM +0300, Arınç ÜNAL wrote:
>> On 14.06.2023 22:43, Vladimir Oltean wrote:
>> > On Mon, Jun 12, 2023 at 10:59:39AM +0300, arinc9.unal@gmail.com wrote:
>> > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
>> > > 
>> > > Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
>> > > SoC represents a CPU port to trap frames to. These switches trap frames
>> > > received from a user port to the CPU port that is affine to the user port
>> > > from which the frames are received.
>> > > 
>> > > Currently, only the bit that corresponds to the first found CPU port is set
>> > > on the bitmap. When multiple CPU ports are being used, the trapped frames
>> > > from the user ports not affine to the first CPU port will be dropped as the
>> > > other CPU port is not set on the bitmap. The switch on the MT7988 SoC is
>> > > not affected as there's only one port to be used as a CPU port.
>> > > 
>> > > To fix this, introduce the MT7531_CPU_PMAP macro to individually set the
>> > > bits of the CPU port bitmap. Set the CPU port bitmap for MT7531 and the
>> > > switch on the MT7988 SoC on mt753x_cpu_port_enable() which runs on a loop
>> > > for each CPU port.
>> > > 
>> > > Add a comment to explain frame trapping for these switches.
>> > > 
>> > > According to the document MT7531 Reference Manual for Development Board
>> > > v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
>> > > beforehand. Since there's currently no public document for the switch on
>> > > the MT7988 SoC, I assume this is also the case for this switch.
>> > > 
>> > > Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
>> > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> > > ---
>> > 
>> > Would you agree that this 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 in the code base?
>> 
>> Makes sense. Pre-preferred_default_local_cpu_port patch, there isn't a case
>> where there's a user port affine to a CPU port that is not enabled on the
>> CPU port bitmap. So yeah, this is just preparatory work for "net: dsa:
>> introduce preferred_default_local_cpu_port and use on MT7530".
>> 
>> So how do I change the patch to reflect this?
>> 
>> Arınç
>
>net: dsa: mt7530: set all CPU ports in MT7531_CPU_PMAP
>
>MT7531_CPU_PMAP represents the destination port mask for trapped-to-CPU
>frames (further restricted by PCR_MATRIX).
>
>Currently the driver sets the first CPU port as the single port in this
>bit mask, which works fine regardless of whether the device tree defines
>port 5, 6 or 5+6 as CPU ports. This is because the logic coincides with
>DSA's logic of picking the first CPU port as the CPU port that all user
>ports are affine to, by default.
>
>An upcoming change would like to influence DSA's selection of the
>default CPU port to no longer be the first one, and in that case, this
>logic needs adaptation.
>
>Since there is no observed leakage or duplication of frames if all CPU
>ports are defined in this bit mask, simply include them all.
>
>Note that there is no Fixes tag

Thanks a lot for making it easier for me.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Vladimir Oltean]

On Wed, Jun 14, 2023 at 11:52:24PM +0300, Arınç ÜNAL wrote:
> On 14.06.2023 19:42, Russell King (Oracle) wrote:
> > On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
> > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > 
> > > LLDP frames are link-local frames, therefore they must be trapped to the
> > > CPU port. Currently, the MT753X switches treat LLDP frames as regular
> > > multicast frames, therefore flooding them to user ports. To fix this, set
> > > LLDP frames to be trapped to the CPU port(s).

so far so good

> > > 
> > > The mt753x_bpdu_port_fw enum is universally used for trapping frames,
> > > therefore rename it and the values in it to mt753x_port_fw.

yeah, this part of the patch is not useful at all [ here ]

> > > 
> > > For MT7530, LLDP frames received from a user port will be trapped to the
> > > numerically smallest CPU port which is affine to the DSA conduit interface
> > > that is up.
> > > 
> > > For MT7531 and the switch on the MT7988 SoC, LLDP frames received from a
> > > user port will be trapped to the CPU port that is affine to the user port
> > > from which the frames are received.

redundant and useless information here - what's important here is that
they're trapped, not where

> > > The bit for R0E_MANG_FR is 27. When set, the switch regards the frames with
> > > :0E MAC DA as management (LLDP) frames. This bit is set to 1 after reset on
> > > MT7530 and MT7531 according to the documents MT7620 Programming Guide v1.0
> > > and MT7531 Reference Manual for Development Board v1.0, so there's no need
> > > to deal with this bit. Since there's currently no public document for the
> > > switch on the MT7988 SoC, I assume this is also the case for this switch.

I guess that the reader who isn't familiar with the hardware will never
get to ask himself "is the unrelated R0E_MANG_FR bit set ok?", and the
familiar reader can just look that up in the programming guides that are
available, and see the default value and that the driver doesn't change it.

So I just don't see how this bit of information is relevant in this
patch. Sure, by all means, provide all context that helps the reader to
understand the change, but at the same time: less is more.

> > > 
> > > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> > 
> > Patch 4 claims to be a fix for this commit, and introduces one of these
> > modifications to MT753X_BPC, which this patch then changes.
> > 
> > On the face of it, it seems this patch is actually a fix to patch 4 as
> > well as the original patch, so does that mean that patch 4 only half
> > fixes a problem?
> 
> I should do the enum renaming on my net-next series instead, as it's not
> useful to what this patch fixes at all.

please do so (assuming that the enum really has to be changed).

also, if you're not really sure that this behavior has impacted any user
(including yourself), I suppose there's also the option of fixing this in
net-next as one of the earliest patches, independent from any other rework,
so that in case there's a request to backport it to stable, it's possible.
I remember having suggested this once already.

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Vladimir Oltean]

On Wed, Jun 14, 2023 at 11:52:24PM +0300, Arınç ÜNAL wrote:
> On 14.06.2023 19:42, Russell King (Oracle) wrote:
> > On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
> > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > 
> > > LLDP frames are link-local frames, therefore they must be trapped to the
> > > CPU port. Currently, the MT753X switches treat LLDP frames as regular
> > > multicast frames, therefore flooding them to user ports. To fix this, set
> > > LLDP frames to be trapped to the CPU port(s).

so far so good

> > > 
> > > The mt753x_bpdu_port_fw enum is universally used for trapping frames,
> > > therefore rename it and the values in it to mt753x_port_fw.

yeah, this part of the patch is not useful at all [ here ]

> > > 
> > > For MT7530, LLDP frames received from a user port will be trapped to the
> > > numerically smallest CPU port which is affine to the DSA conduit interface
> > > that is up.
> > > 
> > > For MT7531 and the switch on the MT7988 SoC, LLDP frames received from a
> > > user port will be trapped to the CPU port that is affine to the user port
> > > from which the frames are received.

redundant and useless information here - what's important here is that
they're trapped, not where

> > > The bit for R0E_MANG_FR is 27. When set, the switch regards the frames with
> > > :0E MAC DA as management (LLDP) frames. This bit is set to 1 after reset on
> > > MT7530 and MT7531 according to the documents MT7620 Programming Guide v1.0
> > > and MT7531 Reference Manual for Development Board v1.0, so there's no need
> > > to deal with this bit. Since there's currently no public document for the
> > > switch on the MT7988 SoC, I assume this is also the case for this switch.

I guess that the reader who isn't familiar with the hardware will never
get to ask himself "is the unrelated R0E_MANG_FR bit set ok?", and the
familiar reader can just look that up in the programming guides that are
available, and see the default value and that the driver doesn't change it.

So I just don't see how this bit of information is relevant in this
patch. Sure, by all means, provide all context that helps the reader to
understand the change, but at the same time: less is more.

> > > 
> > > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> > 
> > Patch 4 claims to be a fix for this commit, and introduces one of these
> > modifications to MT753X_BPC, which this patch then changes.
> > 
> > On the face of it, it seems this patch is actually a fix to patch 4 as
> > well as the original patch, so does that mean that patch 4 only half
> > fixes a problem?
> 
> I should do the enum renaming on my net-next series instead, as it's not
> useful to what this patch fixes at all.

please do so (assuming that the enum really has to be changed).

also, if you're not really sure that this behavior has impacted any user
(including yourself), I suppose there's also the option of fixing this in
net-next as one of the earliest patches, independent from any other rework,
so that in case there's a request to backport it to stable, it's possible.
I remember having suggested this once already.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 4/7] net: dsa: mt7530: fix handling of BPDUs on MT7530 switch

[Arınç ÜNAL]

On 15 June 2023 00:16:57 EEST, Vladimir Oltean <olteanv@gmail.com> wrote:
>On Thu, Jun 15, 2023 at 12:05:44AM +0300, Arınç ÜNAL wrote:
>> On 14.06.2023 23:50, Vladimir Oltean wrote:
>> > Where have you seen the BPC register in the memory map of MT7530 or MT7621?
>> 
>> I did not somehow dump the memory map of the switch hardware and confirm the
>> BPC register is there, if that's what you're asking.
>
>I mean to say that I looked at
>
>MT7530 Giga Switch programming guide.pdf
>MT7621 Giga Switch Programming Guide.pdf
>MT7621_ProgrammingGuide_GSW_v01.pdf
>
>and I did not find this register.
>
>> However, I can confirm the register is there and identical across all MT7530
>> variants. I have tested the function of the register on the MCM MT7530 on
>> the MT7621 SoC and the standalone MT7530. The register is also described on
>> the document MT7620 Programming Guide v1.0, page 262.
>
>Interesting. I did not have that one. Hard to keep up.

I know the pain. The MT7530 registers are scattered throughout these four documents. TRGMII stuff is on MT7530 Giga Switch programming guide.pdf, etc. Good thing MT7531 Reference Manual for Development Board v1.0 has it all for MT7531.

Arınç

Re: [PATCH net v4 4/7] net: dsa: mt7530: fix handling of BPDUs on MT7530 switch

[Arınç ÜNAL]

On 15 June 2023 00:16:57 EEST, Vladimir Oltean <olteanv@gmail.com> wrote:
>On Thu, Jun 15, 2023 at 12:05:44AM +0300, Arınç ÜNAL wrote:
>> On 14.06.2023 23:50, Vladimir Oltean wrote:
>> > Where have you seen the BPC register in the memory map of MT7530 or MT7621?
>> 
>> I did not somehow dump the memory map of the switch hardware and confirm the
>> BPC register is there, if that's what you're asking.
>
>I mean to say that I looked at
>
>MT7530 Giga Switch programming guide.pdf
>MT7621 Giga Switch Programming Guide.pdf
>MT7621_ProgrammingGuide_GSW_v01.pdf
>
>and I did not find this register.
>
>> However, I can confirm the register is there and identical across all MT7530
>> variants. I have tested the function of the register on the MCM MT7530 on
>> the MT7621 SoC and the standalone MT7530. The register is also described on
>> the document MT7620 Programming Guide v1.0, page 262.
>
>Interesting. I did not have that one. Hard to keep up.

I know the pain. The MT7530 registers are scattered throughout these four documents. TRGMII stuff is on MT7530 Giga Switch programming guide.pdf, etc. Good thing MT7531 Reference Manual for Development Board v1.0 has it all for MT7531.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Arınç ÜNAL]

On 15 June 2023 00:43:13 EEST, Vladimir Oltean <olteanv@gmail.com> wrote:
>On Wed, Jun 14, 2023 at 11:52:24PM +0300, Arınç ÜNAL wrote:
>> On 14.06.2023 19:42, Russell King (Oracle) wrote:
>> > On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
>> > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
>> > > 
>> > > LLDP frames are link-local frames, therefore they must be trapped to the
>> > > CPU port. Currently, the MT753X switches treat LLDP frames as regular
>> > > multicast frames, therefore flooding them to user ports. To fix this, set
>> > > LLDP frames to be trapped to the CPU port(s).
>
>so far so good
>
>> > > 
>> > > The mt753x_bpdu_port_fw enum is universally used for trapping frames,
>> > > therefore rename it and the values in it to mt753x_port_fw.
>
>yeah, this part of the patch is not useful at all [ here ]
>
>> > > 
>> > > For MT7530, LLDP frames received from a user port will be trapped to the
>> > > numerically smallest CPU port which is affine to the DSA conduit interface
>> > > that is up.
>> > > 
>> > > For MT7531 and the switch on the MT7988 SoC, LLDP frames received from a
>> > > user port will be trapped to the CPU port that is affine to the user port
>> > > from which the frames are received.
>
>redundant and useless information here - what's important here is that
>they're trapped, not where

Ok, will remove.

>
>> > > The bit for R0E_MANG_FR is 27. When set, the switch regards the frames with
>> > > :0E MAC DA as management (LLDP) frames. This bit is set to 1 after reset on
>> > > MT7530 and MT7531 according to the documents MT7620 Programming Guide v1.0
>> > > and MT7531 Reference Manual for Development Board v1.0, so there's no need
>> > > to deal with this bit. Since there's currently no public document for the
>> > > switch on the MT7988 SoC, I assume this is also the case for this switch.
>
>I guess that the reader who isn't familiar with the hardware will never
>get to ask himself "is the unrelated R0E_MANG_FR bit set ok?", and the
>familiar reader can just look that up in the programming guides that are
>available, and see the default value and that the driver doesn't change it.
>
>So I just don't see how this bit of information is relevant in this
>patch. Sure, by all means, provide all context that helps the reader to
>understand the change, but at the same time: less is more.

Will remove.

>
>> > > 
>> > > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>> > 
>> > Patch 4 claims to be a fix for this commit, and introduces one of these
>> > modifications to MT753X_BPC, which this patch then changes.
>> > 
>> > On the face of it, it seems this patch is actually a fix to patch 4 as
>> > well as the original patch, so does that mean that patch 4 only half
>> > fixes a problem?
>> 
>> I should do the enum renaming on my net-next series instead, as it's not
>> useful to what this patch fixes at all.
>
>please do so (assuming that the enum really has to be changed).
>
>also, if you're not really sure that this behavior has impacted any user
>(including yourself), I suppose there's also the option of fixing this in
>net-next as one of the earliest patches, independent from any other rework,
>so that in case there's a request to backport it to stable, it's possible.
>I remember having suggested this once already.

This impacts the devices of the company I work with and Bartel's, so I would like this on the stable kernels immediately.

Arınç

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Arınç ÜNAL]

On 15 June 2023 00:43:13 EEST, Vladimir Oltean <olteanv@gmail.com> wrote:
>On Wed, Jun 14, 2023 at 11:52:24PM +0300, Arınç ÜNAL wrote:
>> On 14.06.2023 19:42, Russell King (Oracle) wrote:
>> > On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
>> > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
>> > > 
>> > > LLDP frames are link-local frames, therefore they must be trapped to the
>> > > CPU port. Currently, the MT753X switches treat LLDP frames as regular
>> > > multicast frames, therefore flooding them to user ports. To fix this, set
>> > > LLDP frames to be trapped to the CPU port(s).
>
>so far so good
>
>> > > 
>> > > The mt753x_bpdu_port_fw enum is universally used for trapping frames,
>> > > therefore rename it and the values in it to mt753x_port_fw.
>
>yeah, this part of the patch is not useful at all [ here ]
>
>> > > 
>> > > For MT7530, LLDP frames received from a user port will be trapped to the
>> > > numerically smallest CPU port which is affine to the DSA conduit interface
>> > > that is up.
>> > > 
>> > > For MT7531 and the switch on the MT7988 SoC, LLDP frames received from a
>> > > user port will be trapped to the CPU port that is affine to the user port
>> > > from which the frames are received.
>
>redundant and useless information here - what's important here is that
>they're trapped, not where

Ok, will remove.

>
>> > > The bit for R0E_MANG_FR is 27. When set, the switch regards the frames with
>> > > :0E MAC DA as management (LLDP) frames. This bit is set to 1 after reset on
>> > > MT7530 and MT7531 according to the documents MT7620 Programming Guide v1.0
>> > > and MT7531 Reference Manual for Development Board v1.0, so there's no need
>> > > to deal with this bit. Since there's currently no public document for the
>> > > switch on the MT7988 SoC, I assume this is also the case for this switch.
>
>I guess that the reader who isn't familiar with the hardware will never
>get to ask himself "is the unrelated R0E_MANG_FR bit set ok?", and the
>familiar reader can just look that up in the programming guides that are
>available, and see the default value and that the driver doesn't change it.
>
>So I just don't see how this bit of information is relevant in this
>patch. Sure, by all means, provide all context that helps the reader to
>understand the change, but at the same time: less is more.

Will remove.

>
>> > > 
>> > > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>> > 
>> > Patch 4 claims to be a fix for this commit, and introduces one of these
>> > modifications to MT753X_BPC, which this patch then changes.
>> > 
>> > On the face of it, it seems this patch is actually a fix to patch 4 as
>> > well as the original patch, so does that mean that patch 4 only half
>> > fixes a problem?
>> 
>> I should do the enum renaming on my net-next series instead, as it's not
>> useful to what this patch fixes at all.
>
>please do so (assuming that the enum really has to be changed).
>
>also, if you're not really sure that this behavior has impacted any user
>(including yourself), I suppose there's also the option of fixing this in
>net-next as one of the earliest patches, independent from any other rework,
>so that in case there's a request to backport it to stable, it's possible.
>I remember having suggested this once already.

This impacts the devices of the company I work with and Bartel's, so I would like this on the stable kernels immediately.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 6/7] net: dsa: introduce preferred_default_local_cpu_port and use on MT7530

[Vladimir Oltean]

On Mon, Jun 12, 2023 at 10:59:44AM +0300, arinc9.unal@gmail.com wrote:
> From: Vladimir Oltean <olteanv@gmail.com>
> 
> 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

I keep mentally objecting to this patch and then I need to remind myself
why this decision was taken. I believe that a key element influencing
that decision is not sufficiently highlighted.

You can add, right here, after "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 <olteanv@gmail.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---

Re: [PATCH net v4 6/7] net: dsa: introduce preferred_default_local_cpu_port and use on MT7530

[Vladimir Oltean]

On Mon, Jun 12, 2023 at 10:59:44AM +0300, arinc9.unal@gmail.com wrote:
> From: Vladimir Oltean <olteanv@gmail.com>
> 
> 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

I keep mentally objecting to this patch and then I need to remind myself
why this decision was taken. I believe that a key element influencing
that decision is not sufficiently highlighted.

You can add, right here, after "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 <olteanv@gmail.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Bartel Eerdekens]

On Wed, Jun 14, 2023 at 6:42 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
> > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> >
> > LLDP frames are link-local frames, therefore they must be trapped to the
> > CPU port. Currently, the MT753X switches treat LLDP frames as regular
> > multicast frames, therefore flooding them to user ports. To fix this, set
> > LLDP frames to be trapped to the CPU port(s).
> >
> > The mt753x_bpdu_port_fw enum is universally used for trapping frames,
> > therefore rename it and the values in it to mt753x_port_fw.
> >
> > For MT7530, LLDP frames received from a user port will be trapped to the
> > numerically smallest CPU port which is affine to the DSA conduit interface
> > that is up.
> >
> > For MT7531 and the switch on the MT7988 SoC, LLDP frames received from a
> > user port will be trapped to the CPU port that is affine to the user port
> > from which the frames are received.
> >
> > The bit for R0E_MANG_FR is 27. When set, the switch regards the frames with
> > :0E MAC DA as management (LLDP) frames. This bit is set to 1 after reset on
> > MT7530 and MT7531 according to the documents MT7620 Programming Guide v1.0
> > and MT7531 Reference Manual for Development Board v1.0, so there's no need
> > to deal with this bit. Since there's currently no public document for the
> > switch on the MT7988 SoC, I assume this is also the case for this switch.
> >
> > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>
>
> Patch 4 claims to be a fix for this commit, and introduces one of these
> modifications to MT753X_BPC, which this patch then changes.

Let me chime in on this one, as mentioned by Arinç, I am one of the
requesters of having this patch (and patch 4).
Patch 4 enables the trapping of BPDU's to the CPU, being STP (Spanning
Tree) frames. Maybe that should be mentioned, to be clear.

>
> On the face of it, it seems this patch is actually a fix to patch 4 as
> well as the original patch, so does that mean that patch 4 only half
> fixes a problem?

This patch then also adds trapping for LLDP frames (Link Layer
Discovery Protocol) which is a completely different protocol.
But both rely on trapping frames, instead of forwarding them. So
that's why the enum was changed, to be named generic.


> And I just can't be bothered trying to parse the commit messages
> anymore.
>

I do agree some parts of the commit message could be omitted.
Especially the part of the R0E_MANG_FR, as it just describes a default
reset state of a register.
But it adds confusion mentioning it, as it is not even used in the patch itself.


Bartel

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Bartel Eerdekens]

On Wed, Jun 14, 2023 at 6:42 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
> > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> >
> > LLDP frames are link-local frames, therefore they must be trapped to the
> > CPU port. Currently, the MT753X switches treat LLDP frames as regular
> > multicast frames, therefore flooding them to user ports. To fix this, set
> > LLDP frames to be trapped to the CPU port(s).
> >
> > The mt753x_bpdu_port_fw enum is universally used for trapping frames,
> > therefore rename it and the values in it to mt753x_port_fw.
> >
> > For MT7530, LLDP frames received from a user port will be trapped to the
> > numerically smallest CPU port which is affine to the DSA conduit interface
> > that is up.
> >
> > For MT7531 and the switch on the MT7988 SoC, LLDP frames received from a
> > user port will be trapped to the CPU port that is affine to the user port
> > from which the frames are received.
> >
> > The bit for R0E_MANG_FR is 27. When set, the switch regards the frames with
> > :0E MAC DA as management (LLDP) frames. This bit is set to 1 after reset on
> > MT7530 and MT7531 according to the documents MT7620 Programming Guide v1.0
> > and MT7531 Reference Manual for Development Board v1.0, so there's no need
> > to deal with this bit. Since there's currently no public document for the
> > switch on the MT7988 SoC, I assume this is also the case for this switch.
> >
> > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>
>
> Patch 4 claims to be a fix for this commit, and introduces one of these
> modifications to MT753X_BPC, which this patch then changes.

Let me chime in on this one, as mentioned by Arinç, I am one of the
requesters of having this patch (and patch 4).
Patch 4 enables the trapping of BPDU's to the CPU, being STP (Spanning
Tree) frames. Maybe that should be mentioned, to be clear.

>
> On the face of it, it seems this patch is actually a fix to patch 4 as
> well as the original patch, so does that mean that patch 4 only half
> fixes a problem?

This patch then also adds trapping for LLDP frames (Link Layer
Discovery Protocol) which is a completely different protocol.
But both rely on trapping frames, instead of forwarding them. So
that's why the enum was changed, to be named generic.


> And I just can't be bothered trying to parse the commit messages
> anymore.
>

I do agree some parts of the commit message could be omitted.
Especially the part of the R0E_MANG_FR, as it just describes a default
reset state of a register.
But it adds confusion mentioning it, as it is not even used in the patch itself.


Bartel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Arınç ÜNAL]

On 15.06.2023 15:45, Bartel Eerdekens wrote:
> On Wed, Jun 14, 2023 at 6:42 PM Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
>>
>> On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
>>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>
>>> LLDP frames are link-local frames, therefore they must be trapped to the
>>> CPU port. Currently, the MT753X switches treat LLDP frames as regular
>>> multicast frames, therefore flooding them to user ports. To fix this, set
>>> LLDP frames to be trapped to the CPU port(s).
>>>
>>> The mt753x_bpdu_port_fw enum is universally used for trapping frames,
>>> therefore rename it and the values in it to mt753x_port_fw.
>>>
>>> For MT7530, LLDP frames received from a user port will be trapped to the
>>> numerically smallest CPU port which is affine to the DSA conduit interface
>>> that is up.
>>>
>>> For MT7531 and the switch on the MT7988 SoC, LLDP frames received from a
>>> user port will be trapped to the CPU port that is affine to the user port
>>> from which the frames are received.
>>>
>>> The bit for R0E_MANG_FR is 27. When set, the switch regards the frames with
>>> :0E MAC DA as management (LLDP) frames. This bit is set to 1 after reset on
>>> MT7530 and MT7531 according to the documents MT7620 Programming Guide v1.0
>>> and MT7531 Reference Manual for Development Board v1.0, so there's no need
>>> to deal with this bit. Since there's currently no public document for the
>>> switch on the MT7988 SoC, I assume this is also the case for this switch.
>>>
>>> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>>
>>
>> Patch 4 claims to be a fix for this commit, and introduces one of these
>> modifications to MT753X_BPC, which this patch then changes.
> 
> Let me chime in on this one, as mentioned by Arinç, I am one of the
> requesters of having this patch (and patch 4).
> Patch 4 enables the trapping of BPDU's to the CPU, being STP (Spanning
> Tree) frames. Maybe that should be mentioned, to be clear.

Sure, I can quote the first sentence on the wikipedia page "Bridge 
protocol data unit".

> 
>>
>> On the face of it, it seems this patch is actually a fix to patch 4 as
>> well as the original patch, so does that mean that patch 4 only half
>> fixes a problem?
> 
> This patch then also adds trapping for LLDP frames (Link Layer
> Discovery Protocol) which is a completely different protocol.
> But both rely on trapping frames, instead of forwarding them.

Flooding is a better term. "Trapped" frames are still forwarded, the 
difference is they are forwarded only to the CPU port.

Arınç

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Arınç ÜNAL]

On 15.06.2023 15:45, Bartel Eerdekens wrote:
> On Wed, Jun 14, 2023 at 6:42 PM Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
>>
>> On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
>>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>
>>> LLDP frames are link-local frames, therefore they must be trapped to the
>>> CPU port. Currently, the MT753X switches treat LLDP frames as regular
>>> multicast frames, therefore flooding them to user ports. To fix this, set
>>> LLDP frames to be trapped to the CPU port(s).
>>>
>>> The mt753x_bpdu_port_fw enum is universally used for trapping frames,
>>> therefore rename it and the values in it to mt753x_port_fw.
>>>
>>> For MT7530, LLDP frames received from a user port will be trapped to the
>>> numerically smallest CPU port which is affine to the DSA conduit interface
>>> that is up.
>>>
>>> For MT7531 and the switch on the MT7988 SoC, LLDP frames received from a
>>> user port will be trapped to the CPU port that is affine to the user port
>>> from which the frames are received.
>>>
>>> The bit for R0E_MANG_FR is 27. When set, the switch regards the frames with
>>> :0E MAC DA as management (LLDP) frames. This bit is set to 1 after reset on
>>> MT7530 and MT7531 according to the documents MT7620 Programming Guide v1.0
>>> and MT7531 Reference Manual for Development Board v1.0, so there's no need
>>> to deal with this bit. Since there's currently no public document for the
>>> switch on the MT7988 SoC, I assume this is also the case for this switch.
>>>
>>> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>>
>>
>> Patch 4 claims to be a fix for this commit, and introduces one of these
>> modifications to MT753X_BPC, which this patch then changes.
> 
> Let me chime in on this one, as mentioned by Arinç, I am one of the
> requesters of having this patch (and patch 4).
> Patch 4 enables the trapping of BPDU's to the CPU, being STP (Spanning
> Tree) frames. Maybe that should be mentioned, to be clear.

Sure, I can quote the first sentence on the wikipedia page "Bridge 
protocol data unit".

> 
>>
>> On the face of it, it seems this patch is actually a fix to patch 4 as
>> well as the original patch, so does that mean that patch 4 only half
>> fixes a problem?
> 
> This patch then also adds trapping for LLDP frames (Link Layer
> Discovery Protocol) which is a completely different protocol.
> But both rely on trapping frames, instead of forwarding them.

Flooding is a better term. "Trapped" frames are still forwarded, the 
difference is they are forwarded only to the CPU port.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Arınç ÜNAL]

On 16.06.2023 04:53, Arınç ÜNAL wrote:
> On 15.06.2023 15:45, Bartel Eerdekens wrote:
>> On Wed, Jun 14, 2023 at 6:42 PM Russell King (Oracle)
>> <linux@armlinux.org.uk> wrote:
>>>
>>> On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
>>>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>>
>>>> LLDP frames are link-local frames, therefore they must be trapped to 
>>>> the
>>>> CPU port. Currently, the MT753X switches treat LLDP frames as regular
>>>> multicast frames, therefore flooding them to user ports. To fix 
>>>> this, set
>>>> LLDP frames to be trapped to the CPU port(s).
>>>>
>>>> The mt753x_bpdu_port_fw enum is universally used for trapping frames,
>>>> therefore rename it and the values in it to mt753x_port_fw.
>>>>
>>>> For MT7530, LLDP frames received from a user port will be trapped to 
>>>> the
>>>> numerically smallest CPU port which is affine to the DSA conduit 
>>>> interface
>>>> that is up.
>>>>
>>>> For MT7531 and the switch on the MT7988 SoC, LLDP frames received 
>>>> from a
>>>> user port will be trapped to the CPU port that is affine to the user 
>>>> port
>>>> from which the frames are received.
>>>>
>>>> The bit for R0E_MANG_FR is 27. When set, the switch regards the 
>>>> frames with
>>>> :0E MAC DA as management (LLDP) frames. This bit is set to 1 after 
>>>> reset on
>>>> MT7530 and MT7531 according to the documents MT7620 Programming 
>>>> Guide v1.0
>>>> and MT7531 Reference Manual for Development Board v1.0, so there's 
>>>> no need
>>>> to deal with this bit. Since there's currently no public document 
>>>> for the
>>>> switch on the MT7988 SoC, I assume this is also the case for this 
>>>> switch.
>>>>
>>>> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek 
>>>> MT7530 switch")
>>>
>>>
>>> Patch 4 claims to be a fix for this commit, and introduces one of these
>>> modifications to MT753X_BPC, which this patch then changes.
>>
>> Let me chime in on this one, as mentioned by Arinç, I am one of the
>> requesters of having this patch (and patch 4).
>> Patch 4 enables the trapping of BPDU's to the CPU, being STP (Spanning
>> Tree) frames. Maybe that should be mentioned, to be clear.
> 
> Sure, I can quote the first sentence on the wikipedia page "Bridge 
> protocol data unit".

I changed my mind. There's no reason to describe BPDUs. The familiar 
reader will know, the unfamiliar reader should just look it up. Like 
Vladimir said, context helps but at the same time, less is more.

Arınç

Re: [PATCH net v4 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Arınç ÜNAL]

On 16.06.2023 04:53, Arınç ÜNAL wrote:
> On 15.06.2023 15:45, Bartel Eerdekens wrote:
>> On Wed, Jun 14, 2023 at 6:42 PM Russell King (Oracle)
>> <linux@armlinux.org.uk> wrote:
>>>
>>> On Mon, Jun 12, 2023 at 10:59:43AM +0300, arinc9.unal@gmail.com wrote:
>>>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>>
>>>> LLDP frames are link-local frames, therefore they must be trapped to 
>>>> the
>>>> CPU port. Currently, the MT753X switches treat LLDP frames as regular
>>>> multicast frames, therefore flooding them to user ports. To fix 
>>>> this, set
>>>> LLDP frames to be trapped to the CPU port(s).
>>>>
>>>> The mt753x_bpdu_port_fw enum is universally used for trapping frames,
>>>> therefore rename it and the values in it to mt753x_port_fw.
>>>>
>>>> For MT7530, LLDP frames received from a user port will be trapped to 
>>>> the
>>>> numerically smallest CPU port which is affine to the DSA conduit 
>>>> interface
>>>> that is up.
>>>>
>>>> For MT7531 and the switch on the MT7988 SoC, LLDP frames received 
>>>> from a
>>>> user port will be trapped to the CPU port that is affine to the user 
>>>> port
>>>> from which the frames are received.
>>>>
>>>> The bit for R0E_MANG_FR is 27. When set, the switch regards the 
>>>> frames with
>>>> :0E MAC DA as management (LLDP) frames. This bit is set to 1 after 
>>>> reset on
>>>> MT7530 and MT7531 according to the documents MT7620 Programming 
>>>> Guide v1.0
>>>> and MT7531 Reference Manual for Development Board v1.0, so there's 
>>>> no need
>>>> to deal with this bit. Since there's currently no public document 
>>>> for the
>>>> switch on the MT7988 SoC, I assume this is also the case for this 
>>>> switch.
>>>>
>>>> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek 
>>>> MT7530 switch")
>>>
>>>
>>> Patch 4 claims to be a fix for this commit, and introduces one of these
>>> modifications to MT753X_BPC, which this patch then changes.
>>
>> Let me chime in on this one, as mentioned by Arinç, I am one of the
>> requesters of having this patch (and patch 4).
>> Patch 4 enables the trapping of BPDU's to the CPU, being STP (Spanning
>> Tree) frames. Maybe that should be mentioned, to be clear.
> 
> Sure, I can quote the first sentence on the wikipedia page "Bridge 
> protocol data unit".

I changed my mind. There's no reason to describe BPDUs. The familiar 
reader will know, the unfamiliar reader should just look it up. Like 
Vladimir said, context helps but at the same time, less is more.

Arınç

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel