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

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

[Russell King (Oracle)]

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

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

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

Thanks.

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

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

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

[Andrew Lunn]

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

Hi Arınç

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

says:

  don't repost your patches within one 24h period

  2.6.6. Resending after review¶

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

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

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

     Andrew

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

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

[Arınç ÜNAL]

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

Understood, thank you both for the kind warning.

Arınç

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

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

[Russell King (Oracle)]

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

I think:

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

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

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

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

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

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

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

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

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

[Russell King (Oracle)]

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

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

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

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

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

[Arınç ÜNAL]

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

Sounds good to me.

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

Ok.

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

Will do.

Arınç

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

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

[Russell King (Oracle)]

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

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

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

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

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

[Arınç ÜNAL]

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

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

Arınç

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

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

[Russell King (Oracle)]

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

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

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

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

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

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

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

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

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

[Arınç ÜNAL]

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

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

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

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

Arınç

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

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

[Vladimir Oltean]

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

so far so good

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

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

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

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

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

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

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

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

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

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

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

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

[Arınç ÜNAL]

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

Ok, will remove.

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

Will remove.

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

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

Arınç

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

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

[Bartel Eerdekens]

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

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

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

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


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

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


Bartel

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

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

[Arınç ÜNAL]

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

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

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

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

Arınç

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

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

[Arınç ÜNAL]

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

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

Arınç

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

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

[Vladimir Oltean]

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

Would you agree that this is just preparatory work for change "net: dsa:
introduce preferred_default_local_cpu_port and use on MT7530" and not a
fix to an existing problem in the code base?

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

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

[Arınç ÜNAL]

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

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

So how do I change the patch to reflect this?

Arınç

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

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

[Vladimir Oltean]

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

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

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

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

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

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

Note that there is no Fixes tag

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

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

[Arınç ÜNAL]

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

Thanks a lot for making it easier for me.

Arınç

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

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

[Russell King (Oracle)]

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

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

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

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

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

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

[Arınç ÜNAL]

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

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

Arınç

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

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

[Vladimir Oltean]

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

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

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

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

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

[Arınç ÜNAL]

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

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

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

Arınç

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

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

[Vladimir Oltean]

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

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

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

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

[Vladimir Oltean]

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

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

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

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

[Arınç ÜNAL]

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

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

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

Arınç

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

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

[Vladimir Oltean]

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

I mean to say that I looked at

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

and I did not find this register.

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

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

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

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

[Arınç ÜNAL]

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

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

Arınç

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

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

[Vladimir Oltean]

On Mon, Jun 12, 2023 at 10:59:44AM +0300, arinc9.unal@gmail.com wrote:
> From: Vladimir Oltean <olteanv@gmail.com>
> 
> Since the introduction of the OF bindings, DSA has always had a policy that
> in case multiple CPU ports are present in the device tree, the numerically
> smallest one is always chosen.
> 
> The MT7530 switch family, except the switch on the MT7988 SoC, has 2 CPU
> ports, 5 and 6, where port 6 is preferable on the MT7531BE switch because
> it has higher bandwidth.
> 
> The MT7530 driver developers had 3 options:
> - to modify DSA when the MT7531 switch support was introduced, such as to
>   prefer the better port
> - to declare both CPU ports in device trees as CPU ports, and live with the
>   sub-optimal performance resulting from not preferring the better port
> - to declare just port 6 in the device tree as a CPU port
> 
> Of course they chose the path of least resistance (3rd option), kicking the
> can down the road. The hardware description in the device tree is supposed
> to be stable - developers are not supposed to adopt the strategy of
> piecemeal hardware description, where the device tree is updated in
> lockstep with the features that the kernel currently supports.
> 
> Now, as a result of the fact that they did that, any attempts to modify the
> device tree and describe both CPU ports as CPU ports would make DSA change
> its default selection from port 6 to 5, effectively resulting in a
> performance degradation visible to users with the MT7531BE switch as can be
> seen below.
> 
> Without preferring port 6:
> 
> [ ID][Role] Interval           Transfer     Bitrate         Retr
> [  5][TX-C]   0.00-20.00  sec   374 MBytes   157 Mbits/sec  734    sender
> [  5][TX-C]   0.00-20.00  sec   373 MBytes   156 Mbits/sec    receiver
> [  7][RX-C]   0.00-20.00  sec  1.81 GBytes   778 Mbits/sec    0    sender
> [  7][RX-C]   0.00-20.00  sec  1.81 GBytes   777 Mbits/sec    receiver
> 
> With preferring port 6:
> 
> [ ID][Role] Interval           Transfer     Bitrate         Retr
> [  5][TX-C]   0.00-20.00  sec  1.99 GBytes   856 Mbits/sec  273    sender
> [  5][TX-C]   0.00-20.00  sec  1.99 GBytes   855 Mbits/sec    receiver
> [  7][RX-C]   0.00-20.00  sec  1.72 GBytes   737 Mbits/sec   15    sender
> [  7][RX-C]   0.00-20.00  sec  1.71 GBytes   736 Mbits/sec    receiver
> 
> Using one port for WAN and the other ports for LAN is a very popular use
> case which is what this test emulates.
> 
> As such, this change proposes that we retroactively modify stable kernels

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

You can add, right here, after "stable kernels":

"(which don't support the modification of the CPU port assignments, so
as to let user space fix the problem and restore the throughput)"

> to keep the mt7530 driver preferring port 6 even with device trees where
> the hardware is more fully described.
> 
> Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
> Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---

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