[PATCH v2 net] net: dsa: microchip: make learning configurable and keep it off while standalone

[PATCH v2 net] net: dsa: microchip: make learning configurable and keep it off while standalone

[Vladimir Oltean]

Address learning should initially be turned off by the driver for port
operation in standalone mode, then the DSA core handles changes to it
via ds->ops->port_bridge_flags().

Leaving address learning enabled while ports are standalone breaks any
kind of communication which involves port B receiving what port A has
sent. Notably it breaks the ksz9477 driver used with a (non offloaded,
ports act as if standalone) bonding interface in active-backup mode,
when the ports are connected together through external switches, for
redundancy purposes.

This fixes a major design flaw in the ksz9477 and ksz8795 drivers, which
unconditionally leave address learning enabled even while ports operate
as standalone.

Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
Link: https://lore.kernel.org/netdev/CAFZh4h-JVWt80CrQWkFji7tZJahMfOToUJQgKS5s0_=9zzpvYQ@mail.gmail.com/
Reported-by: Brian Hutchinson <b.hutchman@gmail.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v2: targeting the 6.0 release candidates as opposed to the 5.19 rc's
        from v1.

Again, this is compile-tested only, but the equivalent change was
confirmed by Brian as working on a 5.10 kernel.

@maintainers: when should I submit the backports to "stable", for older
trees?

 drivers/net/dsa/microchip/ksz_common.c | 45 +++++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz_common.h |  1 +
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index ed7d137cba99..b23241ccbc5a 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -962,6 +962,7 @@ static void ksz_update_port_member(struct ksz_device *dev, int port)
 static int ksz_setup(struct dsa_switch *ds)
 {
 	struct ksz_device *dev = ds->priv;
+	struct ksz_port *p;
 	const u16 *regs;
 	int ret;
 
@@ -1001,6 +1002,14 @@ static int ksz_setup(struct dsa_switch *ds)
 			return ret;
 	}
 
+	/* Start with learning disabled on standalone user ports, and enabled
+	 * on the CPU port. In lack of other finer mechanisms, learning on the
+	 * CPU port will avoid flooding bridge local addresses on the network
+	 * in some cases.
+	 */
+	p = &dev->ports[dev->cpu_port];
+	p->learning = true;
+
 	/* start switch */
 	regmap_update_bits(dev->regmap[0], regs[S_START_CTRL],
 			   SW_START, SW_START);
@@ -1277,6 +1286,8 @@ void ksz_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
 	ksz_pread8(dev, port, regs[P_STP_CTRL], &data);
 	data &= ~(PORT_TX_ENABLE | PORT_RX_ENABLE | PORT_LEARN_DISABLE);
 
+	p = &dev->ports[port];
+
 	switch (state) {
 	case BR_STATE_DISABLED:
 		data |= PORT_LEARN_DISABLE;
@@ -1286,9 +1297,13 @@ void ksz_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
 		break;
 	case BR_STATE_LEARNING:
 		data |= PORT_RX_ENABLE;
+		if (!p->learning)
+			data |= PORT_LEARN_DISABLE;
 		break;
 	case BR_STATE_FORWARDING:
 		data |= (PORT_TX_ENABLE | PORT_RX_ENABLE);
+		if (!p->learning)
+			data |= PORT_LEARN_DISABLE;
 		break;
 	case BR_STATE_BLOCKING:
 		data |= PORT_LEARN_DISABLE;
@@ -1300,12 +1315,38 @@ void ksz_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
 
 	ksz_pwrite8(dev, port, regs[P_STP_CTRL], data);
 
-	p = &dev->ports[port];
 	p->stp_state = state;
 
 	ksz_update_port_member(dev, port);
 }
 
+static int ksz_port_pre_bridge_flags(struct dsa_switch *ds, int port,
+				     struct switchdev_brport_flags flags,
+				     struct netlink_ext_ack *extack)
+{
+	if (flags.mask & ~BR_LEARNING)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ksz_port_bridge_flags(struct dsa_switch *ds, int port,
+				 struct switchdev_brport_flags flags,
+				 struct netlink_ext_ack *extack)
+{
+	struct ksz_device *dev = ds->priv;
+	struct ksz_port *p = &dev->ports[port];
+
+	if (flags.mask & BR_LEARNING) {
+		p->learning = !!(flags.val & BR_LEARNING);
+
+		/* Make the change take effect immediately */
+		ksz_port_stp_state_set(ds, port, p->stp_state);
+	}
+
+	return 0;
+}
+
 static enum dsa_tag_protocol ksz_get_tag_protocol(struct dsa_switch *ds,
 						  int port,
 						  enum dsa_tag_protocol mp)
@@ -1719,6 +1760,8 @@ static const struct dsa_switch_ops ksz_switch_ops = {
 	.port_bridge_join	= ksz_port_bridge_join,
 	.port_bridge_leave	= ksz_port_bridge_leave,
 	.port_stp_state_set	= ksz_port_stp_state_set,
+	.port_pre_bridge_flags	= ksz_port_pre_bridge_flags,
+	.port_bridge_flags	= ksz_port_bridge_flags,
 	.port_fast_age		= ksz_port_fast_age,
 	.port_vlan_filtering	= ksz_port_vlan_filtering,
 	.port_vlan_add		= ksz_port_vlan_add,
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 764ada3a0f42..0d9520dc6d2d 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -65,6 +65,7 @@ struct ksz_chip_data {
 
 struct ksz_port {
 	bool remove_tag;		/* Remove Tag flag set, for ksz8795 only */
+	bool learning;
 	int stp_state;
 	struct phy_device phydev;
 
-- 
2.34.1

Re: [PATCH v2 net] net: dsa: microchip: make learning configurable and keep it off while standalone

[Andrew Lunn]

> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
> Link: https://lore.kernel.org/netdev/CAFZh4h-JVWt80CrQWkFji7tZJahMfOToUJQgKS5s0_=9zzpvYQ@mail.gmail.com/
> Reported-by: Brian Hutchinson <b.hutchman@gmail.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v1->v2: targeting the 6.0 release candidates as opposed to the 5.19 rc's
>         from v1.
> 
> Again, this is compile-tested only, but the equivalent change was
> confirmed by Brian as working on a 5.10 kernel.
> 
> @maintainers: when should I submit the backports to "stable", for older
> trees?

Can Brian test it? Or at least cherry-pick it back to 5.10?

    Andrew

Re: [PATCH v2 net] net: dsa: microchip: make learning configurable and keep it off while standalone

[Jakub Kicinski]

On Thu, 18 Aug 2022 19:48:09 +0300 Vladimir Oltean wrote:
> Address learning should initially be turned off by the driver for port
> operation in standalone mode, then the DSA core handles changes to it
> via ds->ops->port_bridge_flags().
> 
> Leaving address learning enabled while ports are standalone breaks any
> kind of communication which involves port B receiving what port A has
> sent. Notably it breaks the ksz9477 driver used with a (non offloaded,
> ports act as if standalone) bonding interface in active-backup mode,
> when the ports are connected together through external switches, for
> redundancy purposes.
> 
> This fixes a major design flaw in the ksz9477 and ksz8795 drivers, which
> unconditionally leave address learning enabled even while ports operate
> as standalone.
> 
> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
> Link: https://lore.kernel.org/netdev/CAFZh4h-JVWt80CrQWkFji7tZJahMfOToUJQgKS5s0_=9zzpvYQ@mail.gmail.com/
> Reported-by: Brian Hutchinson <b.hutchman@gmail.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v1->v2: targeting the 6.0 release candidates as opposed to the 5.19 rc's
>         from v1.
> 
> Again, this is compile-tested only, but the equivalent change was
> confirmed by Brian as working on a 5.10 kernel.
> 
> @maintainers: when should I submit the backports to "stable", for older
> trees?

"when" as is how long after Thu PR or "when" as in under what
conditions?

Re: [PATCH v2 net] net: dsa: microchip: make learning configurable and keep it off while standalone

[Vladimir Oltean]

On Tue, Aug 23, 2022 at 02:38:31PM -0700, Jakub Kicinski wrote:
> > @maintainers: when should I submit the backports to "stable", for older
> > trees?
> 
> "when" as is how long after Thu PR or "when" as in under what
> conditions?

how long after the "net" pull request, yes.
I'm a bit confused as to how patches from "net" reach the stable queue.
If they do get there, I'm pretty confident that Greg or Sasha will send
out an email about patches failing to apply to this and that stable
branch, and I can reply to those with backports.

Re: [PATCH v2 net] net: dsa: microchip: make learning configurable and keep it off while standalone

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 18 Aug 2022 19:48:09 +0300 you wrote:
> Address learning should initially be turned off by the driver for port
> operation in standalone mode, then the DSA core handles changes to it
> via ds->ops->port_bridge_flags().
> 
> Leaving address learning enabled while ports are standalone breaks any
> kind of communication which involves port B receiving what port A has
> sent. Notably it breaks the ksz9477 driver used with a (non offloaded,
> ports act as if standalone) bonding interface in active-backup mode,
> when the ports are connected together through external switches, for
> redundancy purposes.
> 
> [...]

Here is the summary with links:
  - [v2,net] net: dsa: microchip: make learning configurable and keep it off while standalone
    https://git.kernel.org/netdev/net/c/15f7cfae912e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2 net] net: dsa: microchip: make learning configurable and keep it off while standalone

[Jakub Kicinski]

On Wed, 24 Aug 2022 00:42:53 +0300 Vladimir Oltean wrote:
> On Tue, Aug 23, 2022 at 02:38:31PM -0700, Jakub Kicinski wrote:
> > > @maintainers: when should I submit the backports to "stable", for older
> > > trees?  
> > 
> > "when" as is how long after Thu PR or "when" as in under what
> > conditions?  
> 
> how long after the "net" pull request, yes.
> I'm a bit confused as to how patches from "net" reach the stable queue.
> If they do get there, I'm pretty confident that Greg or Sasha will send
> out an email about patches failing to apply to this and that stable
> branch, and I can reply to those with backports.

Adding Greg, cause I should probably know but I don't. 

My understanding is that Greg and Sasha scan Linus's tree periodically
and everything with a Fixes tag is pretty much guaranteed to be
selected. Whether that's a hard guarantee IDK. Would be neat if it was
so we don't have to add the CC: stable lines.

Also not sure if it's preferred to wait for the failure notification 
or you should pre-queue the backport as soon as it reaches Linus.
I vague recall someone saying to wait for the notification...

Re: [PATCH v2 net] net: dsa: microchip: make learning configurable and keep it off while standalone

[Greg Kroah-Hartman]

On Tue, Aug 23, 2022 at 03:01:13PM -0700, Jakub Kicinski wrote:
> On Wed, 24 Aug 2022 00:42:53 +0300 Vladimir Oltean wrote:
> > On Tue, Aug 23, 2022 at 02:38:31PM -0700, Jakub Kicinski wrote:
> > > > @maintainers: when should I submit the backports to "stable", for older
> > > > trees?  
> > > 
> > > "when" as is how long after Thu PR or "when" as in under what
> > > conditions?  
> > 
> > how long after the "net" pull request, yes.
> > I'm a bit confused as to how patches from "net" reach the stable queue.
> > If they do get there, I'm pretty confident that Greg or Sasha will send
> > out an email about patches failing to apply to this and that stable
> > branch, and I can reply to those with backports.
> 
> Adding Greg, cause I should probably know but I don't. 
> 
> My understanding is that Greg and Sasha scan Linus's tree periodically
> and everything with a Fixes tag is pretty much guaranteed to be
> selected. Whether that's a hard guarantee IDK. Would be neat if it was
> so we don't have to add the CC: stable lines.

Please add cc: stable lines, that's the documented way to get a patch
into the stable kernel tree for the past 17+ years.

Only recently (5+ years?) have we also been triggering off of the
"Fixes:" tags, and we only started doing that for subsystems that never
was adding cc: stable tags :(

And also, the "Fixes:" tags are on the "slow path" to get into the
stable tree, sometimes taking a very long time, and sometimes just being
ignored entirely if we are busy with other work.  The only guaranteed
way is to tag it with cc: stable.

> Also not sure if it's preferred to wait for the failure notification 
> or you should pre-queue the backport as soon as it reaches Linus.
> I vague recall someone saying to wait for the notification...

It's easier for us if you wait for the rejection email, otherwise I have
to know to store it and save it for when I reject it in a few weeks in
the future, which is a pain on my end given the amount of email we have
to handle.

thanks,

greg k-h