Message-ID: <20240602140118.nnlvydm4dp6wr4c3@skbuf>
Date: Sun, 2 Jun 2024 17:01:18 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Tristram.Ha@...rochip.com, Simon Horman <horms@...nel.org>
Cc: Woojung.Huh@...rochip.com, andrew@...n.ch, vivien.didelot@...il.com,
	f.fainelli@...il.com, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, UNGLinuxDriver@...rochip.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: dsa: microchip: fix initial port flush problem

On Sat, Jun 01, 2024 at 01:05:45PM +0100, Simon Horman wrote:
> On Fri, May 31, 2024 at 07:19:54PM +0000, Tristram.Ha@...rochip.com wrote:
> > > Subject: Re: [PATCH net] net: dsa: microchip: fix initial port flush problem
> > > 
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content
> > > is safe
> > > 
> > > On Tue, May 28, 2024 at 02:35:45PM -0700, Tristram.Ha@...rochip.com wrote:
> > > > From: Tristram Ha <tristram.ha@...rochip.com>
> > > >
> > > > The very first flush in any port will flush all learned addresses in all
> > > > ports.  This can be observed by unplugging a cable from one port while
> > > > additional ports are connected and dumping the fdb entries.
> > > >
> > > > This problem is caused by the initially wrong value programmed to the
> > > > register.  After the first flush the value is reset back to the normal so
> > > > the next port flush will not cause such problem again.
> > > 
> > > Hi Tristram,
> > > 
> > > I think it would be worth spelling out why it is correct to:
> > > 1. Not set SW_FLUSH_STP_TABLE or SW_FLUSH_MSTP_TABLE; and
> > > 2. Preserve the value of the other bits of REG_SW_LUE_CTRL_1
> > 
> > Setting SW_FLUSH_STP_TABLE and SW_FLUSH_MSTP_TABLE bits are wrong as they
> > are action bits.  The bit should be set only when doing an action like
> > flushing.
> 
> Understood, thanks. And I guess that only bits that are being configured
> should be changed, thus the values other bits are preserved with this
> change.
> 
> FWIIW, I do think it would be worth adding something about this to the
> patch description.

I agree the description is confusing and I had to look it up in the
datasheet to understand.

I would suggest something along the lines of:

Setting the SW_FLUSH_STP_TABLE | SW_FLUSH_MSTP_TABLE bits of
REG_SW_LUE_CTRL_1 does not do anything right away. They are
just one-shot modifiers of the upcoming flush action executed by
ksz9477_flush_dyn_mac_table().

It is wrong to set these bits at ksz9477_reset_switch() time, because
it makes ksz9477_flush_dyn_mac_table() have an unexpected and incorrect
behavior during its first run. When DSA calls ksz_port_fast_age() on a
single port for the first time, due to this modifier being set, the
entire FDB will be flushed of dynamically learned entries, across all
ports.

Additionally, there is another mistake in the original code, which is
that the value read from the REG_SW_LUE_CTRL_1 is immediately discarded,
rather than preserved. The relevant bit which is set by default in this
register (but we are mistakenly clearing) is:

Bit 3: Multicast Source Address Filtering
1 = Forward packets with a multicast source address
0 = Drop packets with a multicast source address

Tristram, now a question to you: why would we want to forward packets
with a multicast source address? It looks like clearing that field is
one of those things which were accidentally correct.

The cleanest way to not make a functional change where none is intended
is to simply delete the read.

Powered by blists - more mailing lists