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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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


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

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


Bartel

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

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

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

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

Arınç

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

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

Arınç

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

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

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

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

Arınç

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

so far so good

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

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

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

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

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

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

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

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

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

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

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

Ok, will remove.

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

Will remove.

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

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

Arınç

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

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

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

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

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

Arınç