[PATCH net] net: dsa: microchip: fix initial port flush problem

[PATCH net] net: dsa: microchip: fix initial port flush problem

[Tristram.Ha]

From: Tristram Ha <tristram.ha@microchip.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.

Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index f8ad7833f5d9..7cc92b90ffea 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -356,8 +356,7 @@ int ksz9477_reset_switch(struct ksz_device *dev)
 
 	/* default configuration */
 	ksz_read8(dev, REG_SW_LUE_CTRL_1, &data8);
-	data8 = SW_AGING_ENABLE | SW_LINK_AUTO_AGING |
-	      SW_SRC_ADDR_FILTER | SW_FLUSH_STP_TABLE | SW_FLUSH_MSTP_TABLE;
+	data8 |= SW_AGING_ENABLE | SW_LINK_AUTO_AGING | SW_SRC_ADDR_FILTER;
 	ksz_write8(dev, REG_SW_LUE_CTRL_1, data8);
 
 	/* disable interrupts */
-- 
2.34.1

Re: [PATCH net] net: dsa: microchip: fix initial port flush problem

[Simon Horman]

On Tue, May 28, 2024 at 02:35:45PM -0700, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <tristram.ha@microchip.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

> 
> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
> Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
> ---
>  drivers/net/dsa/microchip/ksz9477.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index f8ad7833f5d9..7cc92b90ffea 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -356,8 +356,7 @@ int ksz9477_reset_switch(struct ksz_device *dev)
>  
>  	/* default configuration */
>  	ksz_read8(dev, REG_SW_LUE_CTRL_1, &data8);
> -	data8 = SW_AGING_ENABLE | SW_LINK_AUTO_AGING |
> -	      SW_SRC_ADDR_FILTER | SW_FLUSH_STP_TABLE | SW_FLUSH_MSTP_TABLE;
> +	data8 |= SW_AGING_ENABLE | SW_LINK_AUTO_AGING | SW_SRC_ADDR_FILTER;
>  	ksz_write8(dev, REG_SW_LUE_CTRL_1, data8);
>  
>  	/* disable interrupts */
> -- 
> 2.34.1
> 
> 

RE: [PATCH net] net: dsa: microchip: fix initial port flush problem

[Tristram.Ha]

> 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@microchip.com wrote:
> > From: Tristram Ha <tristram.ha@microchip.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.

> >
> > Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
> > Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
> > ---
> >  drivers/net/dsa/microchip/ksz9477.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/dsa/microchip/ksz9477.c
> b/drivers/net/dsa/microchip/ksz9477.c
> > index f8ad7833f5d9..7cc92b90ffea 100644
> > --- a/drivers/net/dsa/microchip/ksz9477.c
> > +++ b/drivers/net/dsa/microchip/ksz9477.c
> > @@ -356,8 +356,7 @@ int ksz9477_reset_switch(struct ksz_device *dev)
> >
> >       /* default configuration */
> >       ksz_read8(dev, REG_SW_LUE_CTRL_1, &data8);
> > -     data8 = SW_AGING_ENABLE | SW_LINK_AUTO_AGING |
> > -           SW_SRC_ADDR_FILTER | SW_FLUSH_STP_TABLE | SW_FLUSH_MSTP_TABLE;
> > +     data8 |= SW_AGING_ENABLE | SW_LINK_AUTO_AGING |
> SW_SRC_ADDR_FILTER;
> >       ksz_write8(dev, REG_SW_LUE_CTRL_1, data8);
> >
> >       /* disable interrupts */
> > --
> > 2.34.1
> >
> >

Re: [PATCH net] net: dsa: microchip: fix initial port flush problem

[Simon Horman]

On Fri, May 31, 2024 at 07:19:54PM +0000, Tristram.Ha@microchip.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@microchip.com wrote:
> > > From: Tristram Ha <tristram.ha@microchip.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.

> 
> > >
> > > Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
> > > Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
> > > ---
> > >  drivers/net/dsa/microchip/ksz9477.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/dsa/microchip/ksz9477.c
> > b/drivers/net/dsa/microchip/ksz9477.c
> > > index f8ad7833f5d9..7cc92b90ffea 100644
> > > --- a/drivers/net/dsa/microchip/ksz9477.c
> > > +++ b/drivers/net/dsa/microchip/ksz9477.c
> > > @@ -356,8 +356,7 @@ int ksz9477_reset_switch(struct ksz_device *dev)
> > >
> > >       /* default configuration */
> > >       ksz_read8(dev, REG_SW_LUE_CTRL_1, &data8);
> > > -     data8 = SW_AGING_ENABLE | SW_LINK_AUTO_AGING |
> > > -           SW_SRC_ADDR_FILTER | SW_FLUSH_STP_TABLE | SW_FLUSH_MSTP_TABLE;
> > > +     data8 |= SW_AGING_ENABLE | SW_LINK_AUTO_AGING |
> > SW_SRC_ADDR_FILTER;
> > >       ksz_write8(dev, REG_SW_LUE_CTRL_1, data8);
> > >
> > >       /* disable interrupts */
> > > --
> > > 2.34.1
> > >
> > >
> 

Re: [PATCH net] net: dsa: microchip: fix initial port flush problem

[Vladimir Oltean]

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@microchip.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@microchip.com wrote:
> > > > From: Tristram Ha <tristram.ha@microchip.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.

Re: [PATCH net] net: dsa: microchip: fix initial port flush problem

[Simon Horman]

On Sun, Jun 02, 2024 at 05:01:18PM +0300, Vladimir Oltean wrote:
> 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@microchip.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@microchip.com wrote:
> > > > > From: Tristram Ha <tristram.ha@microchip.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

Thanks, that makes things a lot clearer to me.

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

FWIIW, I thought about that too. But I was concerned that perhaps the read
has a side effect, because I don't know the hw well enough to say otherwise.