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ç
v5:
- Change the comment in the diff on the first patch with Russell's words.
- Change the patch log of the first patch to state that the patch is just
preparatory work for change "net: dsa: introduce
preferred_default_local_cpu_port and use on MT7530" and not a fix to an
existing problem on the code base.
- Remove the "net: dsa: mt7530: fix trapping frames with multiple CPU ports
on MT7530" patch. It fixes a theoretical issue, therefore it is net-next
material.
- Remove unnecessary information from the patch logs. Remove the enum
renaming change.
- Strengthen the point of the "net: dsa: introduce
preferred_default_local_cpu_port and use on MT7530" patch.
v4: Make the patch logs and my comments in the code easier to understand.
v3: Fix the from header on the patches. Write a cover letter.
v2: Add patches to fix the handling of LLDP frames and BPDUs.
Arınç ÜNAL (6):
net: dsa: mt7530: set all CPU ports in MT7531_CPU_PMAP
net: dsa: mt7530: fix trapping frames on non-MT7621 SoC MT7530 switch
net: dsa: mt7530: fix handling of BPDUs on MT7530 switch
net: dsa: mt7530: fix handling of LLDP frames
net: dsa: introduce preferred_default_local_cpu_port and use on MT7530
MAINTAINERS: add me as maintainer of MEDIATEK SWITCH DRIVER
MAINTAINERS | 5 +++--
drivers/net/dsa/mt7530.c | 44 +++++++++++++++++++++++++++++++++++--------
drivers/net/dsa/mt7530.h | 6 ++++++
include/net/dsa.h | 8 ++++++++
net/dsa/dsa.c | 24 ++++++++++++++++++++++-
5 files changed, 76 insertions(+), 11 deletions(-)
From: Vladimir Oltean <[email protected]>
Since the introduction of the OF bindings, DSA has always had a policy that
in case multiple CPU ports are present in the device tree, the numerically
smallest one is always chosen.
The MT7530 switch family, except the switch on the MT7988 SoC, has 2 CPU
ports, 5 and 6, where port 6 is preferable on the MT7531BE switch because
it has higher bandwidth.
The MT7530 driver developers had 3 options:
- to modify DSA when the MT7531 switch support was introduced, such as to
prefer the better port
- to declare both CPU ports in device trees as CPU ports, and live with the
sub-optimal performance resulting from not preferring the better port
- to declare just port 6 in the device tree as a CPU port
Of course they chose the path of least resistance (3rd option), kicking the
can down the road. The hardware description in the device tree is supposed
to be stable - developers are not supposed to adopt the strategy of
piecemeal hardware description, where the device tree is updated in
lockstep with the features that the kernel currently supports.
Now, as a result of the fact that they did that, any attempts to modify the
device tree and describe both CPU ports as CPU ports would make DSA change
its default selection from port 6 to 5, effectively resulting in a
performance degradation visible to users with the MT7531BE switch as can be
seen below.
Without preferring port 6:
[ ID][Role] Interval Transfer Bitrate Retr
[ 5][TX-C] 0.00-20.00 sec 374 MBytes 157 Mbits/sec 734 sender
[ 5][TX-C] 0.00-20.00 sec 373 MBytes 156 Mbits/sec receiver
[ 7][RX-C] 0.00-20.00 sec 1.81 GBytes 778 Mbits/sec 0 sender
[ 7][RX-C] 0.00-20.00 sec 1.81 GBytes 777 Mbits/sec receiver
With preferring port 6:
[ ID][Role] Interval Transfer Bitrate Retr
[ 5][TX-C] 0.00-20.00 sec 1.99 GBytes 856 Mbits/sec 273 sender
[ 5][TX-C] 0.00-20.00 sec 1.99 GBytes 855 Mbits/sec receiver
[ 7][RX-C] 0.00-20.00 sec 1.72 GBytes 737 Mbits/sec 15 sender
[ 7][RX-C] 0.00-20.00 sec 1.71 GBytes 736 Mbits/sec receiver
Using one port for WAN and the other ports for LAN is a very popular use
case which is what this test emulates.
As such, this change proposes that we retroactively modify stable kernels
(which don't support the modification of the CPU port assignments, so as to
let user space fix the problem and restore the throughput) to keep the
mt7530 driver preferring port 6 even with device trees where the hardware
is more fully described.
Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 15 +++++++++++++++
include/net/dsa.h | 8 ++++++++
net/dsa/dsa.c | 24 +++++++++++++++++++++++-
3 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index c85876fd9107..3f84bcb79b3a 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -399,6 +399,20 @@ static void mt7530_pll_setup(struct mt7530_priv *priv)
core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN);
}
+/* If port 6 is available as a CPU port, always prefer that as the default,
+ * otherwise don't care.
+ */
+static struct dsa_port *
+mt753x_preferred_default_local_cpu_port(struct dsa_switch *ds)
+{
+ struct dsa_port *cpu_dp = dsa_to_port(ds, 6);
+
+ if (dsa_port_is_cpu(cpu_dp))
+ return cpu_dp;
+
+ return NULL;
+}
+
/* Setup port 6 interface mode and TRGMII TX circuit */
static int
mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
@@ -3098,6 +3112,7 @@ static int mt7988_setup(struct dsa_switch *ds)
const struct dsa_switch_ops mt7530_switch_ops = {
.get_tag_protocol = mtk_get_tag_protocol,
.setup = mt753x_setup,
+ .preferred_default_local_cpu_port = mt753x_preferred_default_local_cpu_port,
.get_strings = mt7530_get_strings,
.get_ethtool_stats = mt7530_get_ethtool_stats,
.get_sset_count = mt7530_get_sset_count,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 8903053fa5aa..ab0f0a5b0860 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -958,6 +958,14 @@ struct dsa_switch_ops {
struct phy_device *phy);
void (*port_disable)(struct dsa_switch *ds, int port);
+ /*
+ * Compatibility between device trees defining multiple CPU ports and
+ * drivers which are not OK to use by default the numerically smallest
+ * CPU port of a switch for its local ports. This can return NULL,
+ * meaning "don't know/don't care".
+ */
+ struct dsa_port *(*preferred_default_local_cpu_port)(struct dsa_switch *ds);
+
/*
* Port's MAC EEE settings
*/
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index ab1afe67fd18..1afed89e03c0 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -403,6 +403,24 @@ static int dsa_tree_setup_default_cpu(struct dsa_switch_tree *dst)
return 0;
}
+static struct dsa_port *
+dsa_switch_preferred_default_local_cpu_port(struct dsa_switch *ds)
+{
+ struct dsa_port *cpu_dp;
+
+ if (!ds->ops->preferred_default_local_cpu_port)
+ return NULL;
+
+ cpu_dp = ds->ops->preferred_default_local_cpu_port(ds);
+ if (!cpu_dp)
+ return NULL;
+
+ if (WARN_ON(!dsa_port_is_cpu(cpu_dp) || cpu_dp->ds != ds))
+ return NULL;
+
+ return cpu_dp;
+}
+
/* Perform initial assignment of CPU ports to user ports and DSA links in the
* fabric, giving preference to CPU ports local to each switch. Default to
* using the first CPU port in the switch tree if the port does not have a CPU
@@ -410,12 +428,16 @@ static int dsa_tree_setup_default_cpu(struct dsa_switch_tree *dst)
*/
static int dsa_tree_setup_cpu_ports(struct dsa_switch_tree *dst)
{
- struct dsa_port *cpu_dp, *dp;
+ struct dsa_port *preferred_cpu_dp, *cpu_dp, *dp;
list_for_each_entry(cpu_dp, &dst->ports, list) {
if (!dsa_port_is_cpu(cpu_dp))
continue;
+ preferred_cpu_dp = dsa_switch_preferred_default_local_cpu_port(cpu_dp->ds);
+ if (preferred_cpu_dp && preferred_cpu_dp != cpu_dp)
+ continue;
+
/* Prefer a local CPU port */
dsa_switch_for_each_port(dp, cpu_dp->ds) {
/* Prefer the first local CPU port found */
--
2.39.2
From: Arınç ÜNAL <[email protected]>
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.
Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Signed-off-by: Arınç ÜNAL <[email protected]>
---
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 e9fbe7ae6c2c..7b72cf3a0e30 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2262,6 +2262,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
From: Arınç ÜNAL <[email protected]>
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.
Suggested-by: Russell King (Oracle) <[email protected]>
Suggested-by: Vladimir Oltean <[email protected]>
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 15 ++++++++-------
drivers/net/dsa/mt7530.h | 1 +
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 9bc54e1348cb..0a5237793209 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1010,6 +1010,13 @@ 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. Trapped frames will be trapped to the CPU port that
+ * is affine to the inbound user port.
+ */
+ 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 +2359,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
From: Arınç ÜNAL <[email protected]>
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 <[email protected]>
---
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 0a5237793209..e9fbe7ae6c2c 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1007,7 +1007,7 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
UNU_FFP(BIT(port)));
/* Set CPU port number */
- if (priv->id == ID_MT7621)
+ if (priv->id == ID_MT7530 || 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
--
2.39.2
From: Arınç ÜNAL <[email protected]>
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).
Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Signed-off-by: Arınç ÜNAL <[email protected]>
---
drivers/net/dsa/mt7530.c | 8 ++++++++
drivers/net/dsa/mt7530.h | 5 +++++
2 files changed, 13 insertions(+)
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 7b72cf3a0e30..c85876fd9107 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2266,6 +2266,10 @@ mt7530_setup(struct dsa_switch *ds)
mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
MT753X_BPDU_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_BPDU_CPU_ONLY));
+
/* Enable and reset MIB counters */
mt7530_mib_reset(ds);
@@ -2369,6 +2373,10 @@ mt7531_setup_common(struct dsa_switch *ds)
mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
MT753X_BPDU_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_BPDU_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 e590cf43f3ae..08045b035e6a 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -67,6 +67,11 @@ enum mt753x_id {
#define MT753X_BPC 0x24
#define MT753X_BPDU_PORT_FW_MASK GENMASK(2, 0)
+/* 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_bpdu_port_fw {
MT753X_BPDU_FOLLOW_MFC,
MT753X_BPDU_CPU_EXCLUDE = 4,
--
2.39.2
From: Arınç ÜNAL <[email protected]>
Add me as a maintainer of the MediaTek MT7530 DSA subdriver.
List maintainers in alphabetical order by first name.
Signed-off-by: Arınç ÜNAL <[email protected]>
Reviewed-by: Vladimir Oltean <[email protected]>
---
MAINTAINERS | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index a73e5a98503a..c58d7fbb40ed 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13259,10 +13259,11 @@ F: drivers/memory/mtk-smi.c
F: include/soc/mediatek/smi.h
MEDIATEK SWITCH DRIVER
-M: Sean Wang <[email protected]>
+M: Arınç ÜNAL <[email protected]>
+M: Daniel Golle <[email protected]>
M: Landen Chao <[email protected]>
M: DENG Qingfang <[email protected]>
-M: Daniel Golle <[email protected]>
+M: Sean Wang <[email protected]>
L: [email protected]
S: Maintained
F: drivers/net/dsa/mt7530-mdio.c
--
2.39.2
Am 16. Juni 2023 04:53:25 MESZ schrieb [email protected]:
>From: Arınç ÜNAL <[email protected]>
>
>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).
>
>Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>Signed-off-by: Arınç ÜNAL <[email protected]>
>---
> drivers/net/dsa/mt7530.c | 8 ++++++++
> drivers/net/dsa/mt7530.h | 5 +++++
> 2 files changed, 13 insertions(+)
>
>diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>index 7b72cf3a0e30..c85876fd9107 100644
>--- a/drivers/net/dsa/mt7530.c
>+++ b/drivers/net/dsa/mt7530.c
>@@ -2266,6 +2266,10 @@ mt7530_setup(struct dsa_switch *ds)
> mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> MT753X_BPDU_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_BPDU_CPU_ONLY));
>+
> /* Enable and reset MIB counters */
> mt7530_mib_reset(ds);
>
>@@ -2369,6 +2373,10 @@ mt7531_setup_common(struct dsa_switch *ds)
> mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> MT753X_BPDU_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_BPDU_CPU_ONLY));
>+
> /* Enable and reset MIB counters */
> mt7530_mib_reset(ds);
>
I though these 2 hunks should go into some common function as these are redundant or am i wrong?
Btw.thx for your work on this driver :)
regards Frank
On 16 June 2023 08:44:32 GMT+03:00, Frank Wunderlich <[email protected]> wrote:
>Am 16. Juni 2023 04:53:25 MESZ schrieb [email protected]:
>>From: Arınç ÜNAL <[email protected]>
>>
>>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).
>>
>>Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>>Signed-off-by: Arınç ÜNAL <[email protected]>
>>---
>> drivers/net/dsa/mt7530.c | 8 ++++++++
>> drivers/net/dsa/mt7530.h | 5 +++++
>> 2 files changed, 13 insertions(+)
>>
>>diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>>index 7b72cf3a0e30..c85876fd9107 100644
>>--- a/drivers/net/dsa/mt7530.c
>>+++ b/drivers/net/dsa/mt7530.c
>>@@ -2266,6 +2266,10 @@ mt7530_setup(struct dsa_switch *ds)
>> mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
>> MT753X_BPDU_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_BPDU_CPU_ONLY));
>>+
>> /* Enable and reset MIB counters */
>> mt7530_mib_reset(ds);
>>
>>@@ -2369,6 +2373,10 @@ mt7531_setup_common(struct dsa_switch *ds)
>> mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
>> MT753X_BPDU_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_BPDU_CPU_ONLY));
>>+
>> /* Enable and reset MIB counters */
>> mt7530_mib_reset(ds);
>>
>
>
>I though these 2 hunks should go into some common function
Like I said, I will do that on my net-next patch series.
Arınç
On Fri, Jun 16, 2023 at 09:21:40AM +0300, Arınç ÜNAL wrote:
> On 16 June 2023 08:44:32 GMT+03:00, Frank Wunderlich <[email protected]> wrote:
> >Am 16. Juni 2023 04:53:25 MESZ schrieb [email protected]:
> >I though these 2 hunks should go into some common function
>
> Like I said, I will do that on my net-next patch series.
>
> Arınç
Please create a common function now.
On Fri, Jun 16, 2023 at 05:53:22AM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> 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.
>
> Suggested-by: Russell King (Oracle) <[email protected]>
> Suggested-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
On Fri, Jun 16, 2023 at 05:53:23AM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> 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 <[email protected]>
> ---
why do you say non-MT7621 when the change specifically includes MT7621?
What is the affected SoC then?
> 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 0a5237793209..e9fbe7ae6c2c 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -1007,7 +1007,7 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
> UNU_FFP(BIT(port)));
>
> /* Set CPU port number */
> - if (priv->id == ID_MT7621)
> + if (priv->id == ID_MT7530 || 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
> --
> 2.39.2
>
On Fri, Jun 16, 2023 at 05:53:22AM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> 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.
Nice and clear commit message, thanks.
> + /* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
> + * the MT7988 SoC. Trapped frames will be trapped to the CPU port that
> + * is affine to the inbound user port.
As a general rule, English doesn't like repetition in sentences, which
means that having "trapped" twice (or more times) makes the sentence
awkward.
"Trapped frames will be forwarded to the CPU port that is affine to the
inbound user port."
reads much better.
Apart from that...
Reviewed-by: Russell King (Oracle) <[email protected]>
Thanks!
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Fri, Jun 16, 2023 at 01:03:14PM +0300, Vladimir Oltean wrote:
> On Fri, Jun 16, 2023 at 05:53:23AM +0300, [email protected] wrote:
> > From: Arınç ÜNAL <[email protected]>
> >
> > 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 <[email protected]>
> > ---
>
> why do you say non-MT7621 when the change specifically includes MT7621?
> What is the affected SoC then?
Thanks for falling into one of the issues that makes reviewing these
patches difficult. :/
> > - if (priv->id == ID_MT7621)
> > + if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
> > mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
I *think* what the commit message should be saying is that the setup
for the CPU port(*) is necessary not only for MT7621, but also for
MT7530 variants as well.
That can be construed from the commit message, but it doesn't easily
read that way.
* - in this case, it's the CPU port field and the CPU enable bit.
Note that CPU_MASK only covers CPU_PORT() and not CPU_EN, but this
doesn't matter for mt7530_rmw().
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Fri, Jun 16, 2023 at 05:53:24AM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> 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.
>
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
> 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 e9fbe7ae6c2c..7b72cf3a0e30 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2262,6 +2262,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
>
Ok, so this issue dates back to v4.12, but the patch won't apply that
far due to the difference in patch context.
Since the definition itself of the MT753X_BPC register was added as part
of commit c288575f7810 ("net: dsa: mt7530: Add the support of MT7531
switch") - dated v5.10 - then this patch cannot be practically be
backported beyond that.
So I see no possible objection to the request I'm about to make, which is:
please group this and the identical logic from mt7531_setup() into a
common function and call that.
On Fri, Jun 16, 2023 at 11:17:54AM +0100, Russell King (Oracle) wrote:
> On Fri, Jun 16, 2023 at 01:03:14PM +0300, Vladimir Oltean wrote:
> > On Fri, Jun 16, 2023 at 05:53:23AM +0300, [email protected] wrote:
> > > From: Arınç ÜNAL <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> >
> > why do you say non-MT7621 when the change specifically includes MT7621?
> > What is the affected SoC then?
>
> Thanks for falling into one of the issues that makes reviewing these
> patches difficult. :/
>
> > > - if (priv->id == ID_MT7621)
> > > + if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
> > > mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
>
> I *think* what the commit message should be saying is that the setup
> for the CPU port(*) is necessary not only for MT7621, but also for
> MT7530 variants as well.
>
> That can be construed from the commit message, but it doesn't easily
> read that way.
>
> * - in this case, it's the CPU port field and the CPU enable bit.
> Note that CPU_MASK only covers CPU_PORT() and not CPU_EN, but this
> doesn't matter for mt7530_rmw().
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Ah, no, I just misread the patch, because the new term was added to the
left of the existing one, and not to the right as I would have expected.
I though it was this:
> > > - if (priv->id == ID_MT7530)
> > > + if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
thus my confusion and my question.
I'm okay with the change now.
On Fri, Jun 16, 2023 at 05:53:23AM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> 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 <[email protected]>
> ---
Maybe this alternative commit message is an improvement:
All MT7530 switch IP variants share the MT7530_MFC register, but the
current driver only writes it for the switch variant that is integrated
in the MT7621 SoC. Modify the code to include all MT7530 derivatives.
On Fri, Jun 16, 2023 at 01:11:08PM +0300, Vladimir Oltean wrote:
> On Fri, Jun 16, 2023 at 05:53:24AM +0300, [email protected] wrote:
> > From: Arınç ÜNAL <[email protected]>
> >
> > 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.
> >
> > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> > Signed-off-by: Arınç ÜNAL <[email protected]>
> > ---
> > 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 e9fbe7ae6c2c..7b72cf3a0e30 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -2262,6 +2262,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
> >
>
> Ok, so this issue dates back to v4.12, but the patch won't apply that
> far due to the difference in patch context.
>
> Since the definition itself of the MT753X_BPC register was added as part
> of commit c288575f7810 ("net: dsa: mt7530: Add the support of MT7531
> switch") - dated v5.10 - then this patch cannot be practically be
> backported beyond that.
>
> So I see no possible objection to the request I'm about to make, which is:
> please group this and the identical logic from mt7531_setup() into a
> common function and call that.
I agree.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Fri, Jun 16, 2023 at 05:53:23AM +0300, [email protected] wrote:
> From: Arınç ÜNAL <[email protected]>
>
> The check for setting the CPU_PORT bits must include the non-MT7621 SoC
> MT7530 switch variants to trap frames.
I would add a simple "(identified by ID_MT7530)" to the commit message here.
> Expand the check to include them.
>
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Signed-off-by: Arınç ÜNAL <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
> 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 0a5237793209..e9fbe7ae6c2c 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -1007,7 +1007,7 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
> UNU_FFP(BIT(port)));
>
> /* Set CPU port number */
> - if (priv->id == ID_MT7621)
> + if (priv->id == ID_MT7530 || 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
> --
> 2.39.2
>