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

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

[Arınç ÜNAL]

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 to
the CPU port the user port, which the frames are received from, is affine
to.

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

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 comments 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..8ab4718abb06 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. Any frames set for trapping to CPU port will be
+	 * trapped to the CPU port the user port, which the frames are received
+	 * from, is affine to.
+	 */
+	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 v2 2/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530

[Arınç ÜNAL]

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

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 comments 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 8ab4718abb06..da75f9b312bc 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. Any frames set for trapping to CPU port will be
 	 * trapped to the CPU port the user port, which the frames are received
@@ -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. Any 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

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

[Vladimir Oltean]

On Sun, Jun 11, 2023 at 11:15:42AM +0300, Arınç ÜNAL wrote:
> The CPU_PORT bits represent the CPU port to trap frames to for the MT7530
> switch. This switch traps frames to the CPU port set on the CPU_PORT bits,
> regardless of the affinity of the user port which the frames are received
> from.
> 
> 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 comments 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>
> ---

My only concern with this patch is that it depends upon functionality
that was introduced in kernel v5.18 - commit 295ab96f478d ("net: dsa:
provide switch operations for tracking the master state"). But otherwise
it is correct, does not require subsequent net-next rework, and
relatively clean, at least I think it's cleaner than checking which of
the multiple CPU ports is the active CPU port - the other will have no
user port dp->cpu_dp pointing to it. But strictly, the master_state_change()
logic is not needed when you can't change the CPU port assignment.

It might also be that your patch "net: dsa: introduce
preferred_default_local_cpu_port and use on MT7530" gets backported
to stable kernels that this patch doesn't get backported to, and then,
we have a problem, because that will cause even more breakage.

I wonder if there's a way to specify a dependency from this to that
other patch, to ensure that at least that does not happen?

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

[Arınç ÜNAL]

On 13.06.2023 18:08, Vladimir Oltean wrote:
> On Sun, Jun 11, 2023 at 11:15:42AM +0300, Arınç ÜNAL wrote:
>> The CPU_PORT bits represent the CPU port to trap frames to for the MT7530
>> switch. This switch traps frames to the CPU port set on the CPU_PORT bits,
>> regardless of the affinity of the user port which the frames are received
>> from.
>>
>> 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 comments 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>
>> ---
> 
> My only concern with this patch is that it depends upon functionality
> that was introduced in kernel v5.18 - commit 295ab96f478d ("net: dsa:
> provide switch operations for tracking the master state"). But otherwise
> it is correct, does not require subsequent net-next rework, and
> relatively clean, at least I think it's cleaner than checking which of
> the multiple CPU ports is the active CPU port - the other will have no
> user port dp->cpu_dp pointing to it. But strictly, the master_state_change()
> logic is not needed when you can't change the CPU port assignment.
> 
> It might also be that your patch "net: dsa: introduce
> preferred_default_local_cpu_port and use on MT7530" gets backported
> to stable kernels that this patch doesn't get backported to, and then,
> we have a problem, because that will cause even more breakage.
> 
> I wonder if there's a way to specify a dependency from this to that
> other patch, to ensure that at least that does not happen?

Actually, having only "net: dsa: introduce 
preferred_default_local_cpu_port and use on MT7530" backported is an 
enough solution for the current stable kernels.

When multiple CPU ports are defined on the devicetree, the CPU_PORT bits 
will be set to port 6. The active CPU port will also be port 6.

This would only become an issue with the changing the DSA conduit 
support. But that's never going to happen as this patch will always be 
on the kernels that support changing the DSA conduit.

Arınç

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

[Vladimir Oltean]

On Tue, Jun 13, 2023 at 08:14:35PM +0300, Arınç ÜNAL wrote:
> Actually, having only "net: dsa: introduce preferred_default_local_cpu_port
> and use on MT7530" backported is an enough solution for the current stable
> kernels.
> 
> When multiple CPU ports are defined on the devicetree, the CPU_PORT bits
> will be set to port 6. The active CPU port will also be port 6.
> 
> This would only become an issue with the changing the DSA conduit support.
> But that's never going to happen as this patch will always be on the kernels
> that support changing the DSA conduit.

Aha, ok. I thought that device trees with CPU port 5 exclusively defined
also exist in the wild. If not, and this patch fixes a theoretical only
issue, then it is net-next material.

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

[Vladimir Oltean]

On Tue, Jun 13, 2023 at 08:18:58PM +0300, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 08:14:35PM +0300, Arınç ÜNAL wrote:
> > Actually, having only "net: dsa: introduce preferred_default_local_cpu_port
> > and use on MT7530" backported is an enough solution for the current stable
> > kernels.
> > 
> > When multiple CPU ports are defined on the devicetree, the CPU_PORT bits
> > will be set to port 6. The active CPU port will also be port 6.
> > 
> > This would only become an issue with the changing the DSA conduit support.
> > But that's never going to happen as this patch will always be on the kernels
> > that support changing the DSA conduit.
> 
> Aha, ok. I thought that device trees with CPU port 5 exclusively defined
> also exist in the wild. If not, and this patch fixes a theoretical only
> issue, then it is net-next material.

On second thought, compatibility with future device trees is the reason
for this patch set, so that should equally be a reason for keeping this
patch in a "net" series.

If I understand you correctly, port 5 should have worked since commit
c8b8a3c601f2 ("net: dsa: mt7530: permit port 5 to work without port 6 on
MT7621 SoC"), and it did, except for trapping, right?

So how about settling on that as a more modest Fixes: tag, and
explaining clearly in the commit message what's affected?

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

[Arınç ÜNAL]

On 13.06.2023 20:24, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 08:18:58PM +0300, Vladimir Oltean wrote:
>> On Tue, Jun 13, 2023 at 08:14:35PM +0300, Arınç ÜNAL wrote:
>>> Actually, having only "net: dsa: introduce preferred_default_local_cpu_port
>>> and use on MT7530" backported is an enough solution for the current stable
>>> kernels.
>>>
>>> When multiple CPU ports are defined on the devicetree, the CPU_PORT bits
>>> will be set to port 6. The active CPU port will also be port 6.
>>>
>>> This would only become an issue with the changing the DSA conduit support.
>>> But that's never going to happen as this patch will always be on the kernels
>>> that support changing the DSA conduit.
>>
>> Aha, ok. I thought that device trees with CPU port 5 exclusively defined
>> also exist in the wild. If not, and this patch fixes a theoretical only
>> issue, then it is net-next material.
> 
> On second thought, compatibility with future device trees is the reason
> for this patch set, so that should equally be a reason for keeping this
> patch in a "net" series.
> 
> If I understand you correctly, port 5 should have worked since commit
> c8b8a3c601f2 ("net: dsa: mt7530: permit port 5 to work without port 6 on
> MT7621 SoC"), and it did, except for trapping, right?

That fixes port 5 on certain variants of the MT7530 switch, as it was 
already working on the other variants, which, in conclusion, fixes port 
5 on all MT7530 variants.

And no, trapping works. Having only CPU port 5 defined on the devicetree 
will cause the CPU_PORT bits to be set to port 5. There's only a problem 
when multiple CPU ports are defined.

> 
> So how about settling on that as a more modest Fixes: tag, and
> explaining clearly in the commit message what's affected?

I don't see anything to change in the patch log except addressing 
Russell's comments.

Arınç

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

[Vladimir Oltean]

On Tue, Jun 13, 2023 at 08:30:28PM +0300, Arınç ÜNAL wrote:
> That fixes port 5 on certain variants of the MT7530 switch, as it was
> already working on the other variants, which, in conclusion, fixes port 5 on
> all MT7530 variants.

Ok. I didn't pay enough attention to the commit message.

> And no, trapping works. Having only CPU port 5 defined on the devicetree
> will cause the CPU_PORT bits to be set to port 5. There's only a problem
> when multiple CPU ports are defined.

Got it. Then this is really not a problem, and the commit message frames
it incorrectly.

> > So how about settling on that as a more modest Fixes: tag, and
> > explaining clearly in the commit message what's affected?
> 
> I don't see anything to change in the patch log except addressing Russell's
> comments.

Ok. I see Russell has commented on v4, though I don't see that he particularly
pointed out that this fixes a problem which isn't yet a problem. I got lost in
all the versions. v2 and v3 are out of my inbox now :)

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

[Arınç ÜNAL]

On 13.06.2023 20:39, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 08:30:28PM +0300, Arınç ÜNAL wrote:
>> That fixes port 5 on certain variants of the MT7530 switch, as it was
>> already working on the other variants, which, in conclusion, fixes port 5 on
>> all MT7530 variants.
> 
> Ok. I didn't pay enough attention to the commit message.
> 
>> And no, trapping works. Having only CPU port 5 defined on the devicetree
>> will cause the CPU_PORT bits to be set to port 5. There's only a problem
>> when multiple CPU ports are defined.
> 
> Got it. Then this is really not a problem, and the commit message frames
> it incorrectly.

Actually this patch fixes the issue it describes. At the state of this 
patch, when multiple CPU ports are defined, port 5 is the active CPU 
port, CPU_PORT bits are set to port 6.

Once "the patch that prefers port 6, I could easily find the exact name 
but your mail snipping makes it hard" is applied, this issue becomes 
redundant.

> 
>>> So how about settling on that as a more modest Fixes: tag, and
>>> explaining clearly in the commit message what's affected?
>>
>> I don't see anything to change in the patch log except addressing Russell's
>> comments.
> 
> Ok. I see Russell has commented on v4, though I don't see that he particularly
> pointed out that this fixes a problem which isn't yet a problem. I got lost in
> all the versions. v2 and v3 are out of my inbox now :)

All good, I had to quickly roll v3 as v2 had wrong author information 
and I couldn't risk getting v2 applied.

Arınç

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

[Jakub Kicinski]

On Tue, 13 Jun 2023 20:58:33 +0300 Arınç ÜNAL wrote:
> > Ok. I see Russell has commented on v4, though I don't see that he particularly
> > pointed out that this fixes a problem which isn't yet a problem. I got lost in
> > all the versions. v2 and v3 are out of my inbox now :)  
> 
> All good, I had to quickly roll v3 as v2 had wrong author information 
> and I couldn't risk getting v2 applied.

FWIW you can reply with pw-bot: changes-requested to your own patches
and the bot should discard them from patchwork.

https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#updating-patch-status

It's a new capability that nobody has used, yet, so YMMV :)

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

[Arınç ÜNAL]

On 13.06.2023 21:12, Jakub Kicinski wrote:
> On Tue, 13 Jun 2023 20:58:33 +0300 Arınç ÜNAL wrote:
>>> Ok. I see Russell has commented on v4, though I don't see that he particularly
>>> pointed out that this fixes a problem which isn't yet a problem. I got lost in
>>> all the versions. v2 and v3 are out of my inbox now :)
>>
>> All good, I had to quickly roll v3 as v2 had wrong author information
>> and I couldn't risk getting v2 applied.
> 
> FWIW you can reply with pw-bot: changes-requested to your own patches
> and the bot should discard them from patchwork.
> 
> https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#updating-patch-status
> 
> It's a new capability that nobody has used, yet, so YMMV :)

Interesting, I've got some questions regarding this.

> For example to mark a series as Changes Requested one needs to send the following line anywhere in the email thread:
> 
> pw-bot: changes-requested

I suppose a reply to the cover letter or under the cover letter thread 
applies?

> The use of the bot is restricted to authors of the patches (the From: header on patch submission and command must match!)

So, for example, if this patch series was new, Vladimir would reply to 
the patch they're the author of with 'pw-bot: changes-requested', and 
the patchworks would mark the whole patch series as changes requested?

Arınç

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

[Arınç ÜNAL]

On 13.06.2023 22:03, Arınç ÜNAL wrote:
> On 13.06.2023 21:12, Jakub Kicinski wrote:
>> On Tue, 13 Jun 2023 20:58:33 +0300 Arınç ÜNAL wrote:
>>>> Ok. I see Russell has commented on v4, though I don't see that he 
>>>> particularly
>>>> pointed out that this fixes a problem which isn't yet a problem. I 
>>>> got lost in
>>>> all the versions. v2 and v3 are out of my inbox now :)
>>>
>>> All good, I had to quickly roll v3 as v2 had wrong author information
>>> and I couldn't risk getting v2 applied.
>>
>> FWIW you can reply with pw-bot: changes-requested to your own patches
>> and the bot should discard them from patchwork.
>>
>> https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#updating-patch-status
>>
>> It's a new capability that nobody has used, yet, so YMMV :)
> 
> Interesting, I've got some questions regarding this.
> 
>> For example to mark a series as Changes Requested one needs to send 
>> the following line anywhere in the email thread:
>>
>> pw-bot: changes-requested
> 
> I suppose a reply to the cover letter or under the cover letter thread 
> applies?
> 
>> The use of the bot is restricted to authors of the patches (the From: 
>> header on patch submission and command must match!)
> 
> So, for example, if this patch series was new, Vladimir would reply to 
> the patch they're the author of with 'pw-bot: changes-requested', and 
> the patchworks would mark the whole patch series as changes requested?

Also, replying with 'pw-bot: changes-requested' on an already accepted 
patch series as the author won't change the state from accepted to 
changes requested, correct?

Arınç

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

[Jakub Kicinski]

On Tue, 13 Jun 2023 22:09:02 +0300 Arınç ÜNAL wrote:
> >> It's a new capability that nobody has used, yet, so YMMV :)  
> > 
> > Interesting, I've got some questions regarding this.
> >   
> >> For example to mark a series as Changes Requested one needs to send 
> >> the following line anywhere in the email thread:
> >>
> >> pw-bot: changes-requested  
> > 
> > I suppose a reply to the cover letter or under the cover letter thread 
> > applies?

Anywhere in the thread, the bot should walk references.

> >> The use of the bot is restricted to authors of the patches (the From: 
> >> header on patch submission and command must match!)  
> > 
> > So, for example, if this patch series was new, Vladimir would reply to 
> > the patch they're the author of with 'pw-bot: changes-requested', and 
> > the patchworks would mark the whole patch series as changes requested?  

Yes.

> Also, replying with 'pw-bot: changes-requested' on an already accepted 
> patch series as the author won't change the state from accepted to 
> changes requested, correct?

Yes, authors can only changes status from active to inactive. Accepted
is an inactive state.

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

[Russell King (Oracle)]

On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
> On 13.06.2023 20:39, Vladimir Oltean wrote:
> > On Tue, Jun 13, 2023 at 08:30:28PM +0300, Arınç ÜNAL wrote:
> > > That fixes port 5 on certain variants of the MT7530 switch, as it was
> > > already working on the other variants, which, in conclusion, fixes port 5 on
> > > all MT7530 variants.
> > 
> > Ok. I didn't pay enough attention to the commit message.
> > 
> > > And no, trapping works. Having only CPU port 5 defined on the devicetree
> > > will cause the CPU_PORT bits to be set to port 5. There's only a problem
> > > when multiple CPU ports are defined.
> > 
> > Got it. Then this is really not a problem, and the commit message frames
> > it incorrectly.
> 
> Actually this patch fixes the issue it describes. At the state of this
> patch, when multiple CPU ports are defined, port 5 is the active CPU port,
> CPU_PORT bits are set to port 6.

Maybe it's just me being dumb, but I keep finding things difficult to
understand, such as the above paragraph.

It sounds like you're saying that _before_ this patch, port 5 is the
active CPU port, but the CPU_PORT *FIELD* NOT BITS are set such that
port 6 is the active CPU port. Therefore, things are broken, and this
patch fixes it.

Or are you saying that after this patch is applied, port 5 is the
active CPU port, but the CPU_PORT *FIELD* is set to port 6. If that's
true, then I've no idea what the hell is going on here because it
seems to be senseless.

I think at this point I just give up trying to understand what the
hell these patches are trying to do - in my opinion, the commit
messages are worded attrociously and incomprehensively.

-- 
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 v2 2/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530

[Arınç ÜNAL]

On 13.06.2023 21:29, Russell King (Oracle) wrote:
> On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
>> On 13.06.2023 20:39, Vladimir Oltean wrote:
>>> On Tue, Jun 13, 2023 at 08:30:28PM +0300, Arınç ÜNAL wrote:
>>>> That fixes port 5 on certain variants of the MT7530 switch, as it was
>>>> already working on the other variants, which, in conclusion, fixes port 5 on
>>>> all MT7530 variants.
>>>
>>> Ok. I didn't pay enough attention to the commit message.
>>>
>>>> And no, trapping works. Having only CPU port 5 defined on the devicetree
>>>> will cause the CPU_PORT bits to be set to port 5. There's only a problem
>>>> when multiple CPU ports are defined.
>>>
>>> Got it. Then this is really not a problem, and the commit message frames
>>> it incorrectly.
>>
>> Actually this patch fixes the issue it describes. At the state of this
>> patch, when multiple CPU ports are defined, port 5 is the active CPU port,
>> CPU_PORT bits are set to port 6.
> 
> Maybe it's just me being dumb, but I keep finding things difficult to
> understand, such as the above paragraph.
> 
> It sounds like you're saying that _before_ this patch, port 5 is the
> active CPU port, but the CPU_PORT *FIELD* NOT BITS are set such that
> port 6 is the active CPU port. Therefore, things are broken, and this
> patch fixes it.

Yes, CPU_PORT field, and yes this patch fixes the issue by setting the 
field to 5 when multiple CPU ports are used. Because before this patch, 
the active CPU port is 5. The CPU_PORT field must match this.

> 
> Or are you saying that after this patch is applied, port 5 is the
> active CPU port, but the CPU_PORT *FIELD* is set to port 6. If that's
> true, then I've no idea what the hell is going on here because it
> seems to be senseless.

No, when the "prefer port 6" patch is applied, the active CPU port will 
be port 6.

The CPU_PORT field will always be set to 6, regardless of "net: dsa: 
mt7530: fix trapping frames with multiple CPU ports on MT7530". 
Therefore, the "prefer port 6" patch makes "net: dsa: mt7530: fix 
trapping frames with multiple CPU ports on MT7530" redundant.

"net: dsa: mt7530: fix trapping frames with multiple CPU ports on 
MT7530" becomes important after the changing the DSA conduit support is 
introduced.

> 
> I think at this point I just give up trying to understand what the
> hell these patches are trying to do - in my opinion, the commit
> messages are worded attrociously and incomprehensively.

If that's how you feel, you can tune in to v5 where I will address the 
patch logs.

Arınç

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

[Vladimir Oltean]

On Tue, Jun 13, 2023 at 07:29:18PM +0100, Russell King (Oracle) wrote:
> Maybe it's just me being dumb, but I keep finding things difficult to
> understand, such as the above paragraph.
> 
> It sounds like you're saying that _before_ this patch, port 5 is the
> active CPU port, but the CPU_PORT *FIELD* NOT BITS are set such that
> port 6 is the active CPU port. Therefore, things are broken, and this
> patch fixes it.
> 
> Or are you saying that after this patch is applied, port 5 is the
> active CPU port, but the CPU_PORT *FIELD* is set to port 6. If that's
> true, then I've no idea what the hell is going on here because it
> seems to be senseless.

There are 2 distinct patches at play. I'll be showing some tables below.
Their legend is that patch (1) affects only the second column and patch
(2) affects only the third column.

Patch (1): net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530
----------------------------------------------------------------------------------

What you need to know looking at the current code in net-next is that
mt753x_cpu_port_enable() always overwrites the CPU_MASK field of MT7530_MFC,
because that contains a single port. So when operating on a device tree
with multiple CPU ports defined, only the last CPU port will be recorded
in that register, and this will affect the destination port for
trapped-to-CPU frames.

However, DSA, when operating on a DT with multiple CPU ports, makes the
dp->cpu_dp pointer of all user ports equal to the first CPU port. That
affects which DSA master is automatically brought up when user ports are
brought up. Minor issue, TBH, because it is sufficient for the user to
manually bring up the DSA master corresponding to the second CPU port,
and then trapped frames would be processed just fine. Prior to ~2021/v5.12,
that facility wasn't even a thing - the user always had to bring that
interface up manually.

That means that without patch (1) we have:

CPU ports in the    Trapping CPU port        Default CPU port
device tree         (MT7530_MFC:CPU_MASK)    (all dp->cpu_dp point to it)
-------------------------------------------------------------------------
5                   5                        5
6                   6                        6
5 and 6             6                        5

The semi-problem is that the DSA master of the trapping port (6) is not
automatically brought up by the dsa_slave_open() logic, because no slave
has port 6 (the trapping port) as a master.

All that this patch is doing is that it moves around the trapping CPU
port towards one of the DSA masters that is up, so that the user doesn't
really need to care. The table becomes:

CPU ports in the    Trapping CPU port        Default CPU port
device tree         (MT7530_MFC:CPU_MASK)    (all dp->cpu_dp point to it)
--------------------------------------------------------------------------
5                   5                        5
6                   6                        6
5 and 6             5                        5


Patch (2) net: dsa: introduce preferred_default_local_cpu_port and use on MT7530
--------------------------------------------------------------------------------

This patch influences the choice that DSA makes when it comes to the
dp->cpu_dp assignments of user ports, when fed a device tree with
multiple CPU ports.

The preference of the mt7530 driver is: if port 6 is defined in the DT
as a CPU port, choose that. Otherwise don't care (which implicitly means:
let DSA pick the first CPU port that is defined there, be it 5 or 6).

The effect of this patch taken in isolation is (showing only "after",
because "before" is the same as the "before" of patch 1):

CPU ports in the    Trapping CPU port        Default CPU port
device tree         (MT7530_MFC:CPU_MASK)    (all dp->cpu_dp point to it)
-------------------------------------------------------------------------
5                   5                        5
6                   6                        6
5 and 6             6                        6

As you can see, patch (2) resolves the same semi-problem even in
isolation because it makes the trapping port coincide with the default
CPU port in a different way.

> 
> I think at this point I just give up trying to understand what the
> hell these patches are trying to do - in my opinion, the commit
> messages are worded attrociously and incomprehensively.

+1, could have been better..

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

[Vladimir Oltean]

On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
> On 13.06.2023 20:39, Vladimir Oltean wrote:
> > Got it. Then this is really not a problem, and the commit message frames
> > it incorrectly.
> 
> Actually this patch fixes the issue it describes. At the state of this
> patch, when multiple CPU ports are defined, port 5 is the active CPU port,
> CPU_PORT bits are set to port 6.
> 
> Once "the patch that prefers port 6, I could easily find the exact name but
> your mail snipping makes it hard" is applied, this issue becomes redundant.

Ok. Well, you don't get bonus points for fixing a problem in 2 different
ways, once is enough :)

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

[Arınç ÜNAL]

On 13.06.2023 23:18, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
>> On 13.06.2023 20:39, Vladimir Oltean wrote:
>>> Got it. Then this is really not a problem, and the commit message frames
>>> it incorrectly.
>>
>> Actually this patch fixes the issue it describes. At the state of this
>> patch, when multiple CPU ports are defined, port 5 is the active CPU port,
>> CPU_PORT bits are set to port 6.
>>
>> Once "the patch that prefers port 6, I could easily find the exact name but
>> your mail snipping makes it hard" is applied, this issue becomes redundant.
> 
> Ok. Well, you don't get bonus points for fixing a problem in 2 different
> ways, once is enough :)

This is not the case here though.

This patch fixes an issue that can be stumbled upon in two ways. This is 
for when multiple CPU ports are defined on the devicetree.

As I explained to Russell, the first is the CPU_PORT field not matching 
the active CPU port.

The second is when port 5 becomes the only active CPU port. This can 
only happen with the changing the DSA conduit support.

The "prefer port 6" patch only prevents the first way from happening. 
The latter still can happen. But this feature doesn't exist yet. Hence 
why I think we should apply this series as-is (after some patch log 
changes) and backport it without this patch on kernels older than 5.18.

Arınç

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

[Vladimir Oltean]

On Tue, Jun 13, 2023 at 11:35:08PM +0300, Arınç ÜNAL wrote:
> On 13.06.2023 23:18, Vladimir Oltean wrote:
> > On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
> > > On 13.06.2023 20:39, Vladimir Oltean wrote:
> > > > Got it. Then this is really not a problem, and the commit message frames
> > > > it incorrectly.
> > > 
> > > Actually this patch fixes the issue it describes. At the state of this
> > > patch, when multiple CPU ports are defined, port 5 is the active CPU port,
> > > CPU_PORT bits are set to port 6.
> > > 
> > > Once "the patch that prefers port 6, I could easily find the exact name but
> > > your mail snipping makes it hard" is applied, this issue becomes redundant.
> > 
> > Ok. Well, you don't get bonus points for fixing a problem in 2 different
> > ways, once is enough :)
> 
> This is not the case here though.
> 
> This patch fixes an issue that can be stumbled upon in two ways. This is for
> when multiple CPU ports are defined on the devicetree.
> 
> As I explained to Russell, the first is the CPU_PORT field not matching the
> active CPU port.
> 
> The second is when port 5 becomes the only active CPU port. This can only
> happen with the changing the DSA conduit support.
> 
> The "prefer port 6" patch only prevents the first way from happening. The
> latter still can happen. But this feature doesn't exist yet. Hence why I
> think we should apply this series as-is (after some patch log changes) and
> backport it without this patch on kernels older than 5.18.
> 
> Arınç

I was following you until the last phrase. Why should we apply this series
as-is [ to net.git ], if this patch fixes a problem (the *only* problem in
lack of .port_change_master() support, aka for stable kernels) that is
already masked by a different patch targeted to net.git?

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

[Arınç ÜNAL]

On 13.06.2023 23:59, Vladimir Oltean wrote:
> On Tue, Jun 13, 2023 at 11:35:08PM +0300, Arınç ÜNAL wrote:
>> On 13.06.2023 23:18, Vladimir Oltean wrote:
>>> On Tue, Jun 13, 2023 at 08:58:33PM +0300, Arınç ÜNAL wrote:
>>>> On 13.06.2023 20:39, Vladimir Oltean wrote:
>>>>> Got it. Then this is really not a problem, and the commit message frames
>>>>> it incorrectly.
>>>>
>>>> Actually this patch fixes the issue it describes. At the state of this
>>>> patch, when multiple CPU ports are defined, port 5 is the active CPU port,
>>>> CPU_PORT bits are set to port 6.
>>>>
>>>> Once "the patch that prefers port 6, I could easily find the exact name but
>>>> your mail snipping makes it hard" is applied, this issue becomes redundant.
>>>
>>> Ok. Well, you don't get bonus points for fixing a problem in 2 different
>>> ways, once is enough :)
>>
>> This is not the case here though.
>>
>> This patch fixes an issue that can be stumbled upon in two ways. This is for
>> when multiple CPU ports are defined on the devicetree.
>>
>> As I explained to Russell, the first is the CPU_PORT field not matching the
>> active CPU port.
>>
>> The second is when port 5 becomes the only active CPU port. This can only
>> happen with the changing the DSA conduit support.
>>
>> The "prefer port 6" patch only prevents the first way from happening. The
>> latter still can happen. But this feature doesn't exist yet. Hence why I
>> think we should apply this series as-is (after some patch log changes) and
>> backport it without this patch on kernels older than 5.18.
>>
>> Arınç
> 
> I was following you until the last phrase. Why should we apply this series
> as-is [ to net.git ], if this patch fixes a problem (the *only* problem in
> lack of .port_change_master() support, aka for stable kernels) that is
> already masked by a different patch targeted to net.git?

Because I don't see the latter patch as a fix. It treats the symptom, 
not the cause.

Anyway, I'm fine with taking this patch from this series and put it on 
my series for net-next instead.

Arınç

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

[Vladimir Oltean]

On Wed, Jun 14, 2023 at 12:04:10AM +0300, Arınç ÜNAL wrote:
> Because I don't see the latter patch as a fix. It treats the symptom, not
> the cause.
> 
> Anyway, I'm fine with taking this patch from this series and put it on my
> series for net-next instead.

Right, but what seems to have been the case during the net.git
(and linux-stable.git) triage so far is that user impact matters.
A configuration that works by coincidence and not by intention, but
otherwise works reliably, still works, at the end of the day.

If you read the weekly net.git pull requests sent to Linus Torvalds,
you'll see that maintainers try to make a summary of what had to be
changed and why. There isn't really a strong reason why this patch *has*
to be in those pull requests. That's kind of the mindset of what makes
"stable" "stable".

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

[Arınç ÜNAL]

On 14.06.2023 00:14, Vladimir Oltean wrote:
> On Wed, Jun 14, 2023 at 12:04:10AM +0300, Arınç ÜNAL wrote:
>> Because I don't see the latter patch as a fix. It treats the symptom, not
>> the cause.
>>
>> Anyway, I'm fine with taking this patch from this series and put it on my
>> series for net-next instead.
> 
> Right, but what seems to have been the case during the net.git
> (and linux-stable.git) triage so far is that user impact matters.
> A configuration that works by coincidence and not by intention, but
> otherwise works reliably, still works, at the end of the day.
> 
> If you read the weekly net.git pull requests sent to Linus Torvalds,
> you'll see that maintainers try to make a summary of what had to be
> changed and why. There isn't really a strong reason why this patch *has*
> to be in those pull requests. That's kind of the mindset of what makes
> "stable" "stable".

Makes sense. I have prepared v5 that addresses everything so far, should 
I send it today now that Russell has reviewed v4?

Arınç

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

[Vladimir Oltean]

On Wed, Jun 14, 2023 at 10:03:04AM +0300, Arınç ÜNAL wrote:
> Makes sense. I have prepared v5 that addresses everything so far, should I
> send it today now that Russell has reviewed v4?
> 
> Arınç

Let's wait for Russell to ack that all discussions on v2-v4 are closed
and that there aren't any follow-up questions there.

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

[Russell King (Oracle)]

On Tue, Jun 13, 2023 at 08:39:08PM +0300, Vladimir Oltean wrote:
> Ok. I see Russell has commented on v4, though I don't see that he particularly
> pointed out that this fixes a problem which isn't yet a problem. I got lost in
> all the versions. v2 and v3 are out of my inbox now :)

I didn't really get to that level of detail (which is why I haven't
given any r-b's) - I was more focused on trying to understand what
each commit was doing, and trying to get easier to parse commit
messages.

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

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

[Frank Wunderlich]

Hi

> Gesendet: Dienstag, 13. Juni 2023 um 19:18 Uhr
> Von: "Vladimir Oltean" <olteanv@gmail.com>

> On Tue, Jun 13, 2023 at 08:14:35PM +0300, Arınç ÜNAL wrote:
> > Actually, having only "net: dsa: introduce preferred_default_local_cpu_port
> > and use on MT7530" backported is an enough solution for the current stable
> > kernels.
> > 
> > When multiple CPU ports are defined on the devicetree, the CPU_PORT bits
> > will be set to port 6. The active CPU port will also be port 6.
> > 
> > This would only become an issue with the changing the DSA conduit support.
> > But that's never going to happen as this patch will always be on the kernels
> > that support changing the DSA conduit.
> 
> Aha, ok. I thought that device trees with CPU port 5 exclusively defined
> also exist in the wild. If not, and this patch fixes a theoretical only
> issue, then it is net-next material.

BananaPi R2Pro (Rockchip rk3568 arm64) has currently port5 only. And there only port5
is connected to the SoC (so port6 is not added there).

Most boards using port6 at the moment.

regards Frank

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

[Arınç ÜNAL]

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 da75f9b312bc..df2626f72367 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 v2 4/7] net: dsa: mt7530: fix handling of BPDUs on MT7530 switch

[Arınç ÜNAL]

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

v2: Add this patch.

---
 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 df2626f72367..c2af23f2bc5d 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 v2 5/7] net: dsa: mt7530: fix handling of LLDP frames

[Arınç ÜNAL]

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 the user port is affine to.

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

v2: Add this patch.

---
 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 c2af23f2bc5d..97f389f8d6ea 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 v2 6/7] net: dsa: introduce preferred_default_local_cpu_port and use on MT7530

[Arınç ÜNAL]

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 97f389f8d6ea..1ec047e552d2 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 v2 7/7] MAINTAINERS: add me as maintainer of MEDIATEK SWITCH DRIVER

[Arınç ÜNAL]

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 v2 1/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7531

[Russell King (Oracle)]

On Sun, Jun 11, 2023 at 11:15:41AM +0300, Arınç ÜNAL wrote:
> 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 to
> the CPU port the user port, which the frames are received from, is affine
> to.

I think you need to reword that, because at least I went "err what" -
especially the second sentence!

> Currently, only the bit that corresponds to the first found CPU port is set
> on the bitmap.

Ok.

> When multiple CPU ports are being used, frames from the user
> ports affine to the other CPU port which are set to be trapped will be
> dropped as the affine CPU port is not set on the bitmap.

Hmm. I think this is trying to say:

"When multiple CPU ports are being used, trapped frames from user ports
not affine to the first CPU port will be dropped we do not set these
ports as being affine to the second CPU port."

> Only the MT7531
> switch is affected as there's only one port to be used as a CPU port on the
> switch on the MT7988 SoC.

Erm, hang on. The previous bit indicated there was a problem when there
are multiple CPU ports, but here you're saying that only one switch is
affected - and that switch has only one CPU port. This at the very least
raises eyebrows, because it's just contradicted the first part
explaining when there's a problem.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 9bc54e1348cb..8ab4718abb06 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. Any frames set for trapping to CPU port will be
> +	 * trapped to the CPU port the user port, which the frames are received
> +	 * from, is affine to.

Please reword the second sentence.

-- 
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 v2 1/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7531

[Arınç ÜNAL]

On 11.06.2023 19:04, Russell King (Oracle) wrote:
> On Sun, Jun 11, 2023 at 11:15:41AM +0300, Arınç ÜNAL wrote:
>> 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 to
>> the CPU port the user port, which the frames are received from, is affine
>> to.
> 
> I think you need to reword that, because at least I went "err what" -
> especially the second sentence!

Sure, how does this sound:

These switches trap frames 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.
> 
> Ok.
> 
>> When multiple CPU ports are being used, frames from the user
>> ports affine to the other CPU port which are set to be trapped will be
>> dropped as the affine CPU port is not set on the bitmap.
> 
> Hmm. I think this is trying to say:
> 
> "When multiple CPU ports are being used, trapped frames from user ports
> not affine to the first CPU port will be dropped we do not set these
> ports as being affine to the second CPU port."

Yes but it's not the affinity we set here. It's to enable the CPU port 
for trapping.

> 
>> Only the MT7531
>> switch is affected as there's only one port to be used as a CPU port on the
>> switch on the MT7988 SoC.
> 
> Erm, hang on. The previous bit indicated there was a problem when there
> are multiple CPU ports, but here you're saying that only one switch is
> affected - and that switch has only one CPU port. This at the very least
> raises eyebrows, because it's just contradicted the first part
> explaining when there's a problem.

I meant to say, since I already explained at the start of the patch log 
that this patch changes the bits of the CPU port bitmap for MT7531 and 
the switch on the MT7988 SoC, only MT7531 is affected as there's only a 
single CPU port on the switch on the MT7988 SoC. So the switch on the 
MT7988 SoC cannot be affected.

> 
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 9bc54e1348cb..8ab4718abb06 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. Any frames set for trapping to CPU port will be
>> +	 * trapped to the CPU port the user port, which the frames are received
>> +	 * from, is affine to.
> 
> Please reword the second sentence.

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

Arınç

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

[Russell King (Oracle)]

On Mon, Jun 12, 2023 at 09:40:45AM +0300, Arınç ÜNAL wrote:
> On 11.06.2023 19:04, Russell King (Oracle) wrote:
> > On Sun, Jun 11, 2023 at 11:15:41AM +0300, Arınç ÜNAL wrote:
> > > 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 to
> > > the CPU port the user port, which the frames are received from, is affine
> > > to.
> > 
> > I think you need to reword that, because at least I went "err what" -
> > especially the second sentence!
> 
> Sure, how does this sound:
> 
> These switches trap frames to the CPU port that is affine to the user port
> from which the frames are received.

"... to the inbound user port." I think that's a better way to describe
"user port from which the frames are received."

However, I'm still struggling to understand what the overall message for
this entire commit log actually is.

The actual affinity of the user ports seems to be not relevant, but this
commit is more about telling the switch which of its ports are CPU
ports.

So, if the problem is that we only end up with a single port set as a
CPU port when there are multiple, isn't it going to be better to say
something like:

"For MT7531 and the switch on MT7988, we are not correctly indicating
which ports are CPU ports when we have more than one CPU port. In order
to solve this, we need to set multiple bits in the XYZ register so the
switch will trap frames to the appropriate CPU port for frames received
on the inbound user port.

> > > Currently, only the bit that corresponds to the first found CPU port is set
> > > on the bitmap.
> > 
> > Ok.
> > 
> > > When multiple CPU ports are being used, frames from the user
> > > ports affine to the other CPU port which are set to be trapped will be
> > > dropped as the affine CPU port is not set on the bitmap.
> > 
> > Hmm. I think this is trying to say:
> > 
> > "When multiple CPU ports are being used, trapped frames from user ports
> > not affine to the first CPU port will be dropped we do not set these
> > ports as being affine to the second CPU port."
> 
> Yes but it's not the affinity we set here. It's to enable the CPU port for
> trapping.

In light of that, is the problem that we only enable one CPU port to
receive trapped frames from their affine user ports?

> > > Only the MT7531
> > > switch is affected as there's only one port to be used as a CPU port on the
> > > switch on the MT7988 SoC.
> > 
> > Erm, hang on. The previous bit indicated there was a problem when there
> > are multiple CPU ports, but here you're saying that only one switch is
> > affected - and that switch has only one CPU port. This at the very least
> > raises eyebrows, because it's just contradicted the first part
> > explaining when there's a problem.
> 
> I meant to say, since I already explained at the start of the patch log that
> this patch changes the bits of the CPU port bitmap for MT7531 and the switch
> on the MT7988 SoC, only MT7531 is affected as there's only a single CPU port
> on the switch on the MT7988 SoC. So the switch on the MT7988 SoC cannot be
> affected.



> 
> > 
> > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > index 9bc54e1348cb..8ab4718abb06 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. Any frames set for trapping to CPU port will be
> > > +	 * trapped to the CPU port the user port, which the frames are received
> > > +	 * from, is affine to.
> > 
> > Please reword the second sentence.
> 
> Any frames 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.

Too many "port"s. Would:

"Add this port to the CPU port bitmap for the MT7531 and switch on the
MT7988. Trapped frames will be sent to the CPU port that is affine to
the inbound user port."

explain it better?

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 v2 1/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7531

[Vladimir Oltean]

On Mon, Jun 12, 2023 at 11:09:10AM +0100, Russell King (Oracle) wrote:
> > Yes but it's not the affinity we set here. It's to enable the CPU port for
> > trapping.
> 
> In light of that, is the problem that we only enable one CPU port to
> receive trapped frames from their affine user ports?

The badly explained problem is that this driver is not coded up to handle
device trees with multiple CPU ports in the way that is desirable for Arınç.

Namely, when both CPU ports 5 and 6 are described in the device tree,
DSA currently chooses port 5 as the active and unchangeable CPU port.
That works, however it is not desirable for Arınç for performance reasons,
as explained in commit "net: dsa: introduce preferred_default_local_cpu_port
and use on MT7530" from this series.

So that change makes DSA choose port 6 as the active and unchangeable
CPU port. But as a preliminary change for that to work, one would need
to remove the current built-in assumption of the mt7530 driver: that the
active and unchangeable CPU port is also the first CPU port.

This change builds on the observation that there is no problem when all
CPU ports described in the device tree are set in the CPU port bitmap,
regardless of whether they are active or not. This is because packet
trapping on these switch sub-families follows the user to CPU port
affinity, and inactive CPU ports have no user ports affine to them.

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

[Arınç ÜNAL]

On 12.06.2023 13:09, Russell King (Oracle) wrote:
> On Mon, Jun 12, 2023 at 09:40:45AM +0300, Arınç ÜNAL wrote:
>> On 11.06.2023 19:04, Russell King (Oracle) wrote:
>>> On Sun, Jun 11, 2023 at 11:15:41AM +0300, Arınç ÜNAL wrote:
>>>> 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 to
>>>> the CPU port the user port, which the frames are received from, is affine
>>>> to.
>>>
>>> I think you need to reword that, because at least I went "err what" -
>>> especially the second sentence!
>>
>> Sure, how does this sound:
>>
>> These switches trap frames to the CPU port that is affine to the user port
>> from which the frames are received.
> 
> "... to the inbound user port." I think that's a better way to describe
> "user port from which the frames are received."

Sounds good to me.

> 
> However, I'm still struggling to understand what the overall message for
> this entire commit log actually is.
> 
> The actual affinity of the user ports seems to be not relevant, but this
> commit is more about telling the switch which of its ports are CPU
> ports.

Yes, I also add a comment to explain how frame trapping works. The user 
port - CPU port affinity is only relevant there.

> 
> So, if the problem is that we only end up with a single port set as a
> CPU port when there are multiple, isn't it going to be better to say
> something like:
> 
> "For MT7531 and the switch on MT7988, we are not correctly indicating
> which ports are CPU ports when we have more than one CPU port. In order
> to solve this, we need to set multiple bits in the XYZ register so the
> switch will trap frames to the appropriate CPU port for frames received
> on the inbound user port.

Yes, I'll replace this with the second paragraph.

> 
>>>> Currently, only the bit that corresponds to the first found CPU port is set
>>>> on the bitmap.
>>>
>>> Ok.
>>>
>>>> When multiple CPU ports are being used, frames from the user
>>>> ports affine to the other CPU port which are set to be trapped will be
>>>> dropped as the affine CPU port is not set on the bitmap.
>>>
>>> Hmm. I think this is trying to say:
>>>
>>> "When multiple CPU ports are being used, trapped frames from user ports
>>> not affine to the first CPU port will be dropped we do not set these
>>> ports as being affine to the second CPU port."
>>
>> Yes but it's not the affinity we set here. It's to enable the CPU port for
>> trapping.
> 
> In light of that, is the problem that we only enable one CPU port to
> receive trapped frames from their affine user ports?

Yes.

> 
>>>> Only the MT7531
>>>> switch is affected as there's only one port to be used as a CPU port on the
>>>> switch on the MT7988 SoC.
>>>
>>> Erm, hang on. The previous bit indicated there was a problem when there
>>> are multiple CPU ports, but here you're saying that only one switch is
>>> affected - and that switch has only one CPU port. This at the very least
>>> raises eyebrows, because it's just contradicted the first part
>>> explaining when there's a problem.
>>
>> I meant to say, since I already explained at the start of the patch log that
>> this patch changes the bits of the CPU port bitmap for MT7531 and the switch
>> on the MT7988 SoC, only MT7531 is affected as there's only a single CPU port
>> on the switch on the MT7988 SoC. So the switch on the MT7988 SoC cannot be
>> affected.
> 
> 
> 
>>
>>>
>>>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>>>> index 9bc54e1348cb..8ab4718abb06 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. Any frames set for trapping to CPU port will be
>>>> +	 * trapped to the CPU port the user port, which the frames are received
>>>> +	 * from, is affine to.
>>>
>>> Please reword the second sentence.
>>
>> Any frames 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.
> 
> Too many "port"s. Would:
> 
> "Add this port to the CPU port bitmap for the MT7531 and switch on the
> MT7988. Trapped frames will be sent to the CPU port that is affine to
> the inbound user port."
> 
> explain it better?

Sounds good.

Arınç