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@xxxxxxxxx wrote:
> > > From: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
> > > 
> > > 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.