| Message ID | 20210731191023.1329446-4-dqfext@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mt7530 software fallback bridging fix | expand |
On Sun, Aug 01, 2021 at 03:10:21AM +0800, DENG Qingfang wrote: > As filter ID 1 is used, set STP state also on it. > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > --- > drivers/net/dsa/mt7530.c | 3 ++- > drivers/net/dsa/mt7530.h | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index 3876e265f844..38d6ce37d692 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -1147,7 +1147,8 @@ mt7530_stp_state_set(struct dsa_switch *ds, int port, u8 state) > break; > } > > - mt7530_rmw(priv, MT7530_SSP_P(port), FID_PST_MASK, stp_state); > + mt7530_rmw(priv, MT7530_SSP_P(port), FID_PST_MASK, > + FID_PST(stp_state)); > } > > static int > diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h > index a308886fdebc..294ff1cbd9e0 100644 > --- a/drivers/net/dsa/mt7530.h > +++ b/drivers/net/dsa/mt7530.h > @@ -181,7 +181,7 @@ enum mt7530_vlan_egress_attr { > > /* Register for port STP state control */ > #define MT7530_SSP_P(x) (0x2000 + ((x) * 0x100)) > -#define FID_PST(x) ((x) & 0x3) Shouldn't these macros have _two_ arguments, the FID and the port state? > +#define FID_PST(x) (((x) & 0x3) * 0x5) "* 5": explanation? > #define FID_PST_MASK FID_PST(0x3) > > enum mt7530_stp_state { > -- > 2.25.1 > I don't exactly understand how this patch works, sorry. Are you altering port state only on bridged ports, or also on standalone ports after this patch? Are standalone ports in the proper STP state (FORWARDING)?
On Mon, Aug 02, 2021 at 04:43:36PM +0300, Vladimir Oltean wrote: > On Sun, Aug 01, 2021 at 03:10:21AM +0800, DENG Qingfang wrote: > > --- a/drivers/net/dsa/mt7530.h > > +++ b/drivers/net/dsa/mt7530.h > > @@ -181,7 +181,7 @@ enum mt7530_vlan_egress_attr { > > > > /* Register for port STP state control */ > > #define MT7530_SSP_P(x) (0x2000 + ((x) * 0x100)) > > -#define FID_PST(x) ((x) & 0x3) > > Shouldn't these macros have _two_ arguments, the FID and the port state? > > > +#define FID_PST(x) (((x) & 0x3) * 0x5) > > "* 5": explanation? > > > #define FID_PST_MASK FID_PST(0x3) > > > > enum mt7530_stp_state { > > -- > > 2.25.1 > > > > I don't exactly understand how this patch works, sorry. > Are you altering port state only on bridged ports, or also on standalone > ports after this patch? Are standalone ports in the proper STP state > (FORWARDING)? The current code only sets FID 0's STP state. This patch sets both 0's and 1's states. The *5 part is binary magic. [1:0] is FID 0's state, [3:2] is FID 1's state and so on. Since 5 == 4'b0101, the value in [1:0] is copied to [3:2] after the multiplication. Perhaps I should only change FID 1's state.
On Mon, Aug 02, 2021 at 11:31:29PM +0800, DENG Qingfang wrote: > On Mon, Aug 02, 2021 at 04:43:36PM +0300, Vladimir Oltean wrote: > > On Sun, Aug 01, 2021 at 03:10:21AM +0800, DENG Qingfang wrote: > > > --- a/drivers/net/dsa/mt7530.h > > > +++ b/drivers/net/dsa/mt7530.h > > > @@ -181,7 +181,7 @@ enum mt7530_vlan_egress_attr { > > > > > > /* Register for port STP state control */ > > > #define MT7530_SSP_P(x) (0x2000 + ((x) * 0x100)) > > > -#define FID_PST(x) ((x) & 0x3) > > > > Shouldn't these macros have _two_ arguments, the FID and the port state? > > > > > +#define FID_PST(x) (((x) & 0x3) * 0x5) > > > > "* 5": explanation? > > > > > #define FID_PST_MASK FID_PST(0x3) > > > > > > enum mt7530_stp_state { > > > -- > > > 2.25.1 > > > > > > > I don't exactly understand how this patch works, sorry. > > Are you altering port state only on bridged ports, or also on standalone > > ports after this patch? Are standalone ports in the proper STP state > > (FORWARDING)? > > The current code only sets FID 0's STP state. This patch sets both 0's and > 1's states. > > The *5 part is binary magic. [1:0] is FID 0's state, [3:2] is FID 1's state > and so on. Since 5 == 4'b0101, the value in [1:0] is copied to [3:2] after > the multiplication. > > Perhaps I should only change FID 1's state. Keep the patches dumb for us mortals please. If you only change FID 1's state, I am concerned that the driver no longer initializes FID 0's port state, and might leave that to the default set by other pre-kernel initialization stage (bootloader?). So even if you might assume that standalone ports are FORWARDING, they might not be.
On Mon, Aug 02, 2021 at 06:42:26PM +0300, Vladimir Oltean wrote: > On Mon, Aug 02, 2021 at 11:31:29PM +0800, DENG Qingfang wrote: > > The current code only sets FID 0's STP state. This patch sets both 0's and > > 1's states. > > > > The *5 part is binary magic. [1:0] is FID 0's state, [3:2] is FID 1's state > > and so on. Since 5 == 4'b0101, the value in [1:0] is copied to [3:2] after > > the multiplication. > > > > Perhaps I should only change FID 1's state. > > Keep the patches dumb for us mortals please. > If you only change FID 1's state, I am concerned that the driver no > longer initializes FID 0's port state, and might leave that to the > default set by other pre-kernel initialization stage (bootloader?). > So even if you might assume that standalone ports are FORWARDING, they > might not be. The default value is forwarding, and the switch is reset by the driver so any pre-kernel initialization stage is no more.
On Mon, Aug 02, 2021 at 11:58:10PM +0800, DENG Qingfang wrote: > On Mon, Aug 02, 2021 at 06:42:26PM +0300, Vladimir Oltean wrote: > > On Mon, Aug 02, 2021 at 11:31:29PM +0800, DENG Qingfang wrote: > > > The current code only sets FID 0's STP state. This patch sets both 0's and > > > 1's states. > > > > > > The *5 part is binary magic. [1:0] is FID 0's state, [3:2] is FID 1's state > > > and so on. Since 5 == 4'b0101, the value in [1:0] is copied to [3:2] after > > > the multiplication. > > > > > > Perhaps I should only change FID 1's state. > > > > Keep the patches dumb for us mortals please. > > If you only change FID 1's state, I am concerned that the driver no > > longer initializes FID 0's port state, and might leave that to the > > default set by other pre-kernel initialization stage (bootloader?). > > So even if you might assume that standalone ports are FORWARDING, they > > might not be. > > The default value is forwarding, and the switch is reset by the driver > so any pre-kernel initialization stage is no more. So then change the port STP state only for FID 1 and resend. Any other reason why this patch series is marked RFC? It looked okay to me otherwise.
On Tue, Aug 03, 2021 at 12:00:06AM +0300, Vladimir Oltean wrote: > > So then change the port STP state only for FID 1 and resend. Any other > reason why this patch series is marked RFC? It looked okay to me otherwise. Okay, will resend with that change and without RFC. By the way, if I were to implement .port_fast_age, should I only flush dynamically learned FDB entries? What about MDB entries?
On Tue, Aug 03, 2021 at 04:23:16PM +0800, DENG Qingfang wrote: > On Tue, Aug 03, 2021 at 12:00:06AM +0300, Vladimir Oltean wrote: > > > > So then change the port STP state only for FID 1 and resend. Any other > > reason why this patch series is marked RFC? It looked okay to me otherwise. > > Okay, will resend with that change and without RFC. > > By the way, if I were to implement .port_fast_age, should I only flush > dynamically learned FDB entries? What about MDB entries? Yes, only dynamically learned entries. That should also answer the question about MDB entries, since a multicast address should never be a source MAC address so they should never be dynamically learned. The bridge should handle the deletion of static entries when needed.
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 3876e265f844..38d6ce37d692 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -1147,7 +1147,8 @@ mt7530_stp_state_set(struct dsa_switch *ds, int port, u8 state) break; } - mt7530_rmw(priv, MT7530_SSP_P(port), FID_PST_MASK, stp_state); + mt7530_rmw(priv, MT7530_SSP_P(port), FID_PST_MASK, + FID_PST(stp_state)); } static int diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h index a308886fdebc..294ff1cbd9e0 100644 --- a/drivers/net/dsa/mt7530.h +++ b/drivers/net/dsa/mt7530.h @@ -181,7 +181,7 @@ enum mt7530_vlan_egress_attr { /* Register for port STP state control */ #define MT7530_SSP_P(x) (0x2000 + ((x) * 0x100)) -#define FID_PST(x) ((x) & 0x3) +#define FID_PST(x) (((x) & 0x3) * 0x5) #define FID_PST_MASK FID_PST(0x3) enum mt7530_stp_state {
As filter ID 1 is used, set STP state also on it. Signed-off-by: DENG Qingfang <dqfext@gmail.com> --- drivers/net/dsa/mt7530.c | 3 ++- drivers/net/dsa/mt7530.h | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-)