diff mbox series

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

Message ID 1716932145-3486-1-git-send-email-Tristram.Ha@microchip.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net] net: dsa: microchip: fix initial port flush problem | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 45 this patch: 45
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang fail Errors and warnings before: 36 this patch: 36
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn fail Errors and warnings before: 45 this patch: 45
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 9 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Tristram.Ha@microchip.com May 28, 2024, 9:35 p.m. UTC 
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(-)

Comments

Simon Horman May 31, 2024, 7:02 p.m. UTC | #1 
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
> 
>
Tristram.Ha@microchip.com May 31, 2024, 7:19 p.m. UTC | #2 
> 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
> >
> >
Simon Horman June 1, 2024, 12:05 p.m. UTC | #3 
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
> > >
> > >
>
Vladimir Oltean June 2, 2024, 2:01 p.m. UTC | #4 
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.
Simon Horman June 3, 2024, 7:40 a.m. UTC | #5 
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.
diff mbox series

Patch

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 */