[RFC PATCH net-next 0/7] DSA master state tracking

[RFC PATCH net-next 0/7] DSA master state tracking

[Vladimir Oltean]

This patch set is provided solely for review purposes (therefore not to
be applied anywhere) and for Ansuel to test whether they resolve the
slowdown reported here:
https://patchwork.kernel.org/project/netdevbpf/cover/20211207145942.7444-1-ansuelsmth@gmail.com/

It does conflict with net-next due to other patches that are in my tree,
and which were also posted here and would need to be picked ("Rework DSA
bridge TX forwarding offload API"):
https://patchwork.kernel.org/project/netdevbpf/cover/20211206165758.1553882-1-vladimir.oltean@nxp.com/

Additionally, for Ansuel's work there is also a logical dependency with
this series ("Replace DSA dp->priv with tagger-owned storage"):
https://patchwork.kernel.org/project/netdevbpf/cover/20211208200504.3136642-1-vladimir.oltean@nxp.com/

To get both dependency series, the following commands should be sufficient:
git b4 20211206165758.1553882-1-vladimir.oltean@nxp.com
git b4 20211208200504.3136642-1-vladimir.oltean@nxp.com

where "git b4" is an alias in ~/.gitconfig:
[b4]
	midmask = https://lore.kernel.org/r/%s
[alias]
	b4 = "!f() { b4 am -t -o - $@ | git am -3; }; f"

The patches posted here are mainly to offer a consistent
"master_up"/"master_going_down" chain of events to switches, without
duplicates, and always starting with "master_up" and ending with
"master_going_down". This way, drivers should know when they can perform
Ethernet-based register access.

Vladimir Oltean (7):
  net: dsa: only bring down user ports assigned to a given DSA master
  net: dsa: refactor the NETDEV_GOING_DOWN master tracking into separate
    function
  net: dsa: use dsa_tree_for_each_user_port in
    dsa_tree_master_going_down()
  net: dsa: provide switch operations for tracking the master state
  net: dsa: stop updating master MTU from master.c
  net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
  net: dsa: replay master state events in
    dsa_tree_{setup,teardown}_master

 include/net/dsa.h  |  8 +++++++
 net/dsa/dsa2.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++--
 net/dsa/dsa_priv.h | 11 ++++++++++
 net/dsa/master.c   | 29 +++-----------------------
 net/dsa/slave.c    | 32 +++++++++++++++-------------
 net/dsa/switch.c   | 29 ++++++++++++++++++++++++++
 6 files changed, 118 insertions(+), 43 deletions(-)

-- 
2.25.1

[RFC PATCH net-next 1/7] net: dsa: only bring down user ports assigned to a given DSA master

[Vladimir Oltean]

This is an adaptation of commit c0a8a9c27493 ("net: dsa: automatically
bring user ports down when master goes down") for multiple DSA masters.
When a DSA master goes down, only the user ports under its control
should go down too, the others can still send/receive traffic.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/slave.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 2b153b366118..f76c96e27868 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2364,6 +2364,9 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
 			if (!dsa_port_is_user(dp))
 				continue;
 
+			if (dp->cpu_dp != cpu_dp)
+				continue;
+
 			list_add(&dp->slave->close_list, &close_list);
 		}
 
-- 
2.25.1

[RFC PATCH net-next 2/7] net: dsa: refactor the NETDEV_GOING_DOWN master tracking into separate function

[Vladimir Oltean]

For symmetry with a yet-to-be-added function named dsa_tree_master_up,
move the logic that handles a NETDEV_GOING_DOWN netdev notifier on a DSA
master into a dedicated function.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa2.c     | 19 +++++++++++++++++++
 net/dsa/dsa_priv.h |  2 ++
 net/dsa/slave.c    | 25 ++++++-------------------
 3 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 8814fa0e44c8..438304a22e0f 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1187,6 +1187,25 @@ int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
 	return err;
 }
 
+void dsa_tree_master_going_down(struct dsa_switch_tree *dst,
+				struct net_device *master)
+{
+	struct dsa_port *dp, *cpu_dp = master->dsa_ptr;
+	LIST_HEAD(close_list);
+
+	list_for_each_entry(dp, &dst->ports, list) {
+		if (!dsa_port_is_user(dp))
+			continue;
+
+		if (dp->cpu_dp != cpu_dp)
+			continue;
+
+		list_add(&dp->slave->close_list, &close_list);
+	}
+
+	dev_close_many(&close_list, true);
+}
+
 static struct dsa_port *dsa_port_touch(struct dsa_switch *ds, int index)
 {
 	struct dsa_switch_tree *dst = ds->dst;
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 38ce5129a33d..21bd11b9d706 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -506,6 +506,8 @@ int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
 			      struct net_device *master,
 			      const struct dsa_device_ops *tag_ops,
 			      const struct dsa_device_ops *old_tag_ops);
+void dsa_tree_master_going_down(struct dsa_switch_tree *dst,
+				struct net_device *master);
 unsigned int dsa_bridge_num_get(const struct net_device *bridge_dev, int max);
 void dsa_bridge_num_put(const struct net_device *bridge_dev,
 			unsigned int bridge_num);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index f76c96e27868..4b91157790bb 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2350,29 +2350,16 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
 		return notifier_from_errno(err);
 	}
 	case NETDEV_GOING_DOWN: {
-		struct dsa_port *dp, *cpu_dp;
-		struct dsa_switch_tree *dst;
-		LIST_HEAD(close_list);
+		if (netdev_uses_dsa(dev)) {
+			struct dsa_port *cpu_dp = dev->dsa_ptr;
+			struct dsa_switch_tree *dst = cpu_dp->ds->dst;
 
-		if (!netdev_uses_dsa(dev))
-			return NOTIFY_DONE;
+			dsa_tree_master_going_down(dst, dev);
 
-		cpu_dp = dev->dsa_ptr;
-		dst = cpu_dp->ds->dst;
-
-		list_for_each_entry(dp, &dst->ports, list) {
-			if (!dsa_port_is_user(dp))
-				continue;
-
-			if (dp->cpu_dp != cpu_dp)
-				continue;
-
-			list_add(&dp->slave->close_list, &close_list);
+			return NOTIFY_OK;
 		}
 
-		dev_close_many(&close_list, true);
-
-		return NOTIFY_OK;
+		return NOTIFY_DONE;
 	}
 	default:
 		break;
-- 
2.25.1

[RFC PATCH net-next 3/7] net: dsa: use dsa_tree_for_each_user_port in dsa_tree_master_going_down()

[Vladimir Oltean]

Consume 3 lines of code by using the dedicated iterator over the user
ports of a DSA switch tree.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa2.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 438304a22e0f..9c490a326e6f 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1193,10 +1193,7 @@ void dsa_tree_master_going_down(struct dsa_switch_tree *dst,
 	struct dsa_port *dp, *cpu_dp = master->dsa_ptr;
 	LIST_HEAD(close_list);
 
-	list_for_each_entry(dp, &dst->ports, list) {
-		if (!dsa_port_is_user(dp))
-			continue;
-
+	dsa_tree_for_each_user_port(dp, dst) {
 		if (dp->cpu_dp != cpu_dp)
 			continue;
 
-- 
2.25.1

[RFC PATCH net-next 4/7] net: dsa: provide switch operations for tracking the master state

[Vladimir Oltean]

Certain drivers may need to send management traffic to the switch for
things like register access, FDB dump, etc, to accelerate what their
slow bus (SPI, I2C, MDIO) can already do.

Ethernet is faster (especially in bulk transactions) but is also more
unreliable, since the user may decide to bring the DSA master down (or
not bring it up), therefore severing the link between the host and the
attached switch.

Drivers needing Ethernet-based register access already should have
fallback logic to the slow bus if the Ethernet method fails, but that
fallback may be based on a timeout, and the I/O to the switch may slow
down to a halt if the master is down, because every Ethernet packet will
have to time out. The driver also doesn't have the option to turn off
Ethernet-based I/O momentarily, because it wouldn't know when to turn it
back on.

Which is where this change comes in. By tracking NETDEV_UP and
NETDEV_GOING_DOWN events on the DSA master, we should know when this
interface becomes available for traffic. Provide this information to
switches so they can use it as they wish.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/net/dsa.h  |  8 ++++++++
 net/dsa/dsa2.c     | 14 ++++++++++++++
 net/dsa/dsa_priv.h |  9 +++++++++
 net/dsa/slave.c    | 12 ++++++++++++
 net/dsa/switch.c   | 29 +++++++++++++++++++++++++++++
 5 files changed, 72 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index bdf308a5c55e..65aef079b156 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1011,6 +1011,14 @@ struct dsa_switch_ops {
 	int	(*tag_8021q_vlan_add)(struct dsa_switch *ds, int port, u16 vid,
 				      u16 flags);
 	int	(*tag_8021q_vlan_del)(struct dsa_switch *ds, int port, u16 vid);
+
+	/*
+	 * DSA master tracking operations
+	 */
+	void	(*master_up)(struct dsa_switch *ds,
+			     const struct net_device *master);
+	void	(*master_going_down)(struct dsa_switch *ds,
+				     const struct net_device *master);
 };
 
 #define DSA_DEVLINK_PARAM_DRIVER(_id, _name, _type, _cmodes)		\
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 9c490a326e6f..fe3a3d05ee24 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1187,12 +1187,26 @@ int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
 	return err;
 }
 
+void dsa_tree_master_up(struct dsa_switch_tree *dst, struct net_device *master)
+{
+	struct dsa_notifier_master_state_info info = {
+		.master = master,
+	};
+
+	dsa_tree_notify(dst, DSA_NOTIFIER_MASTER_UP, &info);
+}
+
 void dsa_tree_master_going_down(struct dsa_switch_tree *dst,
 				struct net_device *master)
 {
 	struct dsa_port *dp, *cpu_dp = master->dsa_ptr;
+	struct dsa_notifier_master_state_info info = {
+		.master = master,
+	};
 	LIST_HEAD(close_list);
 
+	dsa_tree_notify(dst, DSA_NOTIFIER_MASTER_GOING_DOWN, &info);
+
 	dsa_tree_for_each_user_port(dp, dst) {
 		if (dp->cpu_dp != cpu_dp)
 			continue;
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 21bd11b9d706..107f934ca592 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -43,6 +43,8 @@ enum {
 	DSA_NOTIFIER_MRP_DEL_RING_ROLE,
 	DSA_NOTIFIER_TAG_8021Q_VLAN_ADD,
 	DSA_NOTIFIER_TAG_8021Q_VLAN_DEL,
+	DSA_NOTIFIER_MASTER_UP,
+	DSA_NOTIFIER_MASTER_GOING_DOWN,
 };
 
 /* DSA_NOTIFIER_AGEING_TIME */
@@ -126,6 +128,12 @@ struct dsa_notifier_tag_8021q_vlan_info {
 	u16 vid;
 };
 
+/* DSA_NOTIFIER_MASTER_* */
+struct dsa_notifier_master_state_info {
+	const struct net_device *master;
+	struct netlink_ext_ack *extack;
+};
+
 struct dsa_switchdev_event_work {
 	struct dsa_switch *ds;
 	int port;
@@ -506,6 +514,7 @@ int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
 			      struct net_device *master,
 			      const struct dsa_device_ops *tag_ops,
 			      const struct dsa_device_ops *old_tag_ops);
+void dsa_tree_master_up(struct dsa_switch_tree *dst, struct net_device *master);
 void dsa_tree_master_going_down(struct dsa_switch_tree *dst,
 				struct net_device *master);
 unsigned int dsa_bridge_num_get(const struct net_device *bridge_dev, int max);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 4b91157790bb..ff0e8a173996 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2349,6 +2349,18 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
 		err = dsa_port_lag_change(dp, info->lower_state_info);
 		return notifier_from_errno(err);
 	}
+	case NETDEV_UP: {
+		if (netdev_uses_dsa(dev)) {
+			struct dsa_port *cpu_dp = dev->dsa_ptr;
+			struct dsa_switch_tree *dst = cpu_dp->ds->dst;
+
+			dsa_tree_master_up(dst, dev);
+
+			return NOTIFY_OK;
+		}
+
+		return NOTIFY_OK;
+	}
 	case NETDEV_GOING_DOWN: {
 		if (netdev_uses_dsa(dev)) {
 			struct dsa_port *cpu_dp = dev->dsa_ptr;
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 9c92edd96961..553b67478428 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -699,6 +699,29 @@ dsa_switch_mrp_del_ring_role(struct dsa_switch *ds,
 	return 0;
 }
 
+static int dsa_switch_master_up(struct dsa_switch *ds,
+				struct dsa_notifier_master_state_info *info)
+{
+	if (!ds->ops->master_up)
+		return 0;
+
+	ds->ops->master_up(ds, info->master);
+
+	return 0;
+}
+
+static int
+dsa_switch_master_going_down(struct dsa_switch *ds,
+			     struct dsa_notifier_master_state_info *info)
+{
+	if (!ds->ops->master_going_down)
+		return 0;
+
+	ds->ops->master_going_down(ds, info->master);
+
+	return 0;
+}
+
 static int dsa_switch_event(struct notifier_block *nb,
 			    unsigned long event, void *info)
 {
@@ -784,6 +807,12 @@ static int dsa_switch_event(struct notifier_block *nb,
 	case DSA_NOTIFIER_TAG_8021Q_VLAN_DEL:
 		err = dsa_switch_tag_8021q_vlan_del(ds, info);
 		break;
+	case DSA_NOTIFIER_MASTER_UP:
+		err = dsa_switch_master_up(ds, info);
+		break;
+	case DSA_NOTIFIER_MASTER_GOING_DOWN:
+		err = dsa_switch_master_going_down(ds, info);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
-- 
2.25.1

[RFC PATCH net-next 5/7] net: dsa: stop updating master MTU from master.c

[Vladimir Oltean]

The dev_set_mtu() call from dsa_master_setup() has been effectively
superseded by the dsa_slave_change_mtu(slave_dev, ETH_DATA_LEN) that is
done from dsa_slave_create() for each user port. This function also
updates the master MTU according to the largest user port MTU from the
tree. Therefore, updating the master MTU through a separate code path
isn't needed.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/master.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/net/dsa/master.c b/net/dsa/master.c
index e8e19857621b..f4efb244f91d 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -330,28 +330,13 @@ static const struct attribute_group dsa_group = {
 	.attrs	= dsa_slave_attrs,
 };
 
-static void dsa_master_reset_mtu(struct net_device *dev)
-{
-	int err;
-
-	rtnl_lock();
-	err = dev_set_mtu(dev, ETH_DATA_LEN);
-	if (err)
-		netdev_dbg(dev,
-			   "Unable to reset MTU to exclude DSA overheads\n");
-	rtnl_unlock();
-}
-
 static struct lock_class_key dsa_master_addr_list_lock_key;
 
 int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
 {
-	const struct dsa_device_ops *tag_ops = cpu_dp->tag_ops;
 	struct dsa_switch *ds = cpu_dp->ds;
 	struct device_link *consumer_link;
-	int mtu, ret;
-
-	mtu = ETH_DATA_LEN + dsa_tag_protocol_overhead(tag_ops);
+	int ret;
 
 	/* The DSA master must use SET_NETDEV_DEV for this to work. */
 	consumer_link = device_link_add(ds->dev, dev->dev.parent,
@@ -361,13 +346,6 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
 			   "Failed to create a device link to DSA switch %s\n",
 			   dev_name(ds->dev));
 
-	rtnl_lock();
-	ret = dev_set_mtu(dev, mtu);
-	rtnl_unlock();
-	if (ret)
-		netdev_warn(dev, "error %d setting MTU to %d to include DSA overhead\n",
-			    ret, mtu);
-
 	/* If we use a tagging format that doesn't have an ethertype
 	 * field, make sure that all packets from this point on get
 	 * sent to the tag format's receive function.
@@ -405,7 +383,6 @@ void dsa_master_teardown(struct net_device *dev)
 	sysfs_remove_group(&dev->dev.kobj, &dsa_group);
 	dsa_netdev_ops_set(dev, NULL);
 	dsa_master_ethtool_teardown(dev);
-	dsa_master_reset_mtu(dev);
 	dsa_master_set_promiscuity(dev, -1);
 
 	dev->dsa_ptr = NULL;
-- 
2.25.1

[RFC PATCH net-next 6/7] net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}

[Vladimir Oltean]

DSA needs to simulate master tracking events when a binding is first
with a DSA master established and torn down, in order to give drivers
the simplifying guarantee that ->master_up and ->master_going_down calls
are made in exactly this order. To avoid races, we need to block the
reception of NETDEV_UP/NETDEV_GOING_DOWN events in the netdev notifier
chain while we are changing the master's dev->dsa_ptr (this changes what
netdev_uses_dsa(dev) reports).

The dsa_master_setup() and dsa_master_teardown() functions optionally
require the rtnl_mutex to be held, if the tagger needs the master to be
promiscuous, these functions call dev_set_promiscuity(). Move the
rtnl_lock() from that function and make it top-level.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa2.c   | 8 ++++++++
 net/dsa/master.c | 4 ++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index fe3a3d05ee24..dc104023d351 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1015,6 +1015,8 @@ static int dsa_tree_setup_master(struct dsa_switch_tree *dst)
 	struct dsa_port *dp;
 	int err;
 
+	rtnl_lock();
+
 	list_for_each_entry(dp, &dst->ports, list) {
 		if (dsa_port_is_cpu(dp)) {
 			err = dsa_master_setup(dp->master, dp);
@@ -1023,6 +1025,8 @@ static int dsa_tree_setup_master(struct dsa_switch_tree *dst)
 		}
 	}
 
+	rtnl_unlock();
+
 	return 0;
 }
 
@@ -1030,9 +1034,13 @@ static void dsa_tree_teardown_master(struct dsa_switch_tree *dst)
 {
 	struct dsa_port *dp;
 
+	rtnl_lock();
+
 	list_for_each_entry(dp, &dst->ports, list)
 		if (dsa_port_is_cpu(dp))
 			dsa_master_teardown(dp->master);
+
+	rtnl_unlock();
 }
 
 static int dsa_tree_setup_lags(struct dsa_switch_tree *dst)
diff --git a/net/dsa/master.c b/net/dsa/master.c
index f4efb244f91d..2199104ca7df 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -267,9 +267,9 @@ static void dsa_master_set_promiscuity(struct net_device *dev, int inc)
 	if (!ops->promisc_on_master)
 		return;
 
-	rtnl_lock();
+	ASSERT_RTNL();
+
 	dev_set_promiscuity(dev, inc);
-	rtnl_unlock();
 }
 
 static ssize_t tagging_show(struct device *d, struct device_attribute *attr,
-- 
2.25.1

[RFC PATCH net-next 7/7] net: dsa: replay master state events in dsa_tree_{setup,teardown}_master

[Vladimir Oltean]

In order for switch driver to be able to make simple and reliable use of
the master tracking operations, they must also be notified of the
initial state of the DSA master, not just of the changes. This is
because they might enable certain features only during the time when
they know that the DSA master is up and running.

Therefore, this change explicitly checks the state of the DSA master
under the same rtnl_mutex as we were holding during the
dsa_master_setup() and dsa_master_teardown() call. The idea being that
if the DSA master became operational in between the moment in which it
became a DSA master (dsa_master_setup set dev->dsa_ptr) and the moment
when we checked for master->flags & IFF_UP, there is a chance that we
would emit a ->master_up() event twice. We need to avoid that by
serializing the concurrent netdevice event with us. If the netdevice
event started before, we force it to finish before we begin, because we
take rtnl_lock before making netdev_uses_dsa() return true. So we also
handle that early event and do nothing on it. Similarly, if the
dev_open() attempt is concurrent with us, it will attempt to take the
rtnl_mutex, but we're holding it. We'll see that the master flag IFF_UP
isn't set, then when we release the rtnl_mutex we'll process the
NETDEV_UP notifier.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa2.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index dc104023d351..abf385852bb6 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1022,6 +1022,10 @@ static int dsa_tree_setup_master(struct dsa_switch_tree *dst)
 			err = dsa_master_setup(dp->master, dp);
 			if (err)
 				return err;
+
+			/* Replay master state event */
+			if (dp->master->flags & IFF_UP)
+				dsa_tree_master_up(dst, dp->master);
 		}
 	}
 
@@ -1036,9 +1040,15 @@ static void dsa_tree_teardown_master(struct dsa_switch_tree *dst)
 
 	rtnl_lock();
 
-	list_for_each_entry(dp, &dst->ports, list)
-		if (dsa_port_is_cpu(dp))
+	list_for_each_entry(dp, &dst->ports, list) {
+		if (dsa_port_is_cpu(dp)) {
+			/* Replay master state event */
+			if (dp->master->flags & IFF_UP)
+				dsa_tree_master_going_down(dst, dp->master);
+
 			dsa_master_teardown(dp->master);
+		}
+	}
 
 	rtnl_unlock();
 }
-- 
2.25.1

Re: [RFC PATCH net-next 0/7] DSA master state tracking

[Ansuel Smith]

On Thu, Dec 09, 2021 at 12:32:23AM +0200, Vladimir Oltean wrote:
> This patch set is provided solely for review purposes (therefore not to
> be applied anywhere) and for Ansuel to test whether they resolve the
> slowdown reported here:
> https://patchwork.kernel.org/project/netdevbpf/cover/20211207145942.7444-1-ansuelsmth@gmail.com/
> 
> It does conflict with net-next due to other patches that are in my tree,
> and which were also posted here and would need to be picked ("Rework DSA
> bridge TX forwarding offload API"):
> https://patchwork.kernel.org/project/netdevbpf/cover/20211206165758.1553882-1-vladimir.oltean@nxp.com/
> 
> Additionally, for Ansuel's work there is also a logical dependency with
> this series ("Replace DSA dp->priv with tagger-owned storage"):
> https://patchwork.kernel.org/project/netdevbpf/cover/20211208200504.3136642-1-vladimir.oltean@nxp.com/
> 
> To get both dependency series, the following commands should be sufficient:
> git b4 20211206165758.1553882-1-vladimir.oltean@nxp.com
> git b4 20211208200504.3136642-1-vladimir.oltean@nxp.com
> 
> where "git b4" is an alias in ~/.gitconfig:
> [b4]
> 	midmask = https://lore.kernel.org/r/%s
> [alias]
> 	b4 = "!f() { b4 am -t -o - $@ | git am -3; }; f"
> 
> The patches posted here are mainly to offer a consistent
> "master_up"/"master_going_down" chain of events to switches, without
> duplicates, and always starting with "master_up" and ending with
> "master_going_down". This way, drivers should know when they can perform
> Ethernet-based register access.
> 
> Vladimir Oltean (7):
>   net: dsa: only bring down user ports assigned to a given DSA master
>   net: dsa: refactor the NETDEV_GOING_DOWN master tracking into separate
>     function
>   net: dsa: use dsa_tree_for_each_user_port in
>     dsa_tree_master_going_down()
>   net: dsa: provide switch operations for tracking the master state
>   net: dsa: stop updating master MTU from master.c
>   net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
>   net: dsa: replay master state events in
>     dsa_tree_{setup,teardown}_master
> 
>  include/net/dsa.h  |  8 +++++++
>  net/dsa/dsa2.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++--
>  net/dsa/dsa_priv.h | 11 ++++++++++
>  net/dsa/master.c   | 29 +++-----------------------
>  net/dsa/slave.c    | 32 +++++++++++++++-------------
>  net/dsa/switch.c   | 29 ++++++++++++++++++++++++++
>  6 files changed, 118 insertions(+), 43 deletions(-)
> 
> -- 
> 2.25.1
> 

I applied this patch and it does work correctly. Sadly the problem is
not solved and still the packet are not tracked correctly. What I notice
is that everything starts to work as soon as the master is set to
promiiscuous mode. Wonder if we should track that event instead of
simple up?

Here is a bootlog [0]. I added some log when the function timeouts and when
master up is actually called.
Current implementation for this is just a bool that is set to true on
master up and false on master going down. (final version should use
locking to check if an Ethernet transation is in progress)

[0] https://pastebin.com/7w2kgG7a

-- 
	Ansuel

Re: [RFC PATCH net-next 0/7] DSA master state tracking

[Vladimir Oltean]

On Thu, Dec 09, 2021 at 04:05:59AM +0100, Ansuel Smith wrote:
> On Thu, Dec 09, 2021 at 12:32:23AM +0200, Vladimir Oltean wrote:
> > This patch set is provided solely for review purposes (therefore not to
> > be applied anywhere) and for Ansuel to test whether they resolve the
> > slowdown reported here:
> > https://patchwork.kernel.org/project/netdevbpf/cover/20211207145942.7444-1-ansuelsmth@gmail.com/
> > 
> > It does conflict with net-next due to other patches that are in my tree,
> > and which were also posted here and would need to be picked ("Rework DSA
> > bridge TX forwarding offload API"):
> > https://patchwork.kernel.org/project/netdevbpf/cover/20211206165758.1553882-1-vladimir.oltean@nxp.com/
> > 
> > Additionally, for Ansuel's work there is also a logical dependency with
> > this series ("Replace DSA dp->priv with tagger-owned storage"):
> > https://patchwork.kernel.org/project/netdevbpf/cover/20211208200504.3136642-1-vladimir.oltean@nxp.com/
> > 
> > To get both dependency series, the following commands should be sufficient:
> > git b4 20211206165758.1553882-1-vladimir.oltean@nxp.com
> > git b4 20211208200504.3136642-1-vladimir.oltean@nxp.com
> > 
> > where "git b4" is an alias in ~/.gitconfig:
> > [b4]
> > 	midmask = https://lore.kernel.org/r/%25s
> > [alias]
> > 	b4 = "!f() { b4 am -t -o - $@ | git am -3; }; f"
> > 
> > The patches posted here are mainly to offer a consistent
> > "master_up"/"master_going_down" chain of events to switches, without
> > duplicates, and always starting with "master_up" and ending with
> > "master_going_down". This way, drivers should know when they can perform
> > Ethernet-based register access.
> > 
> > Vladimir Oltean (7):
> >   net: dsa: only bring down user ports assigned to a given DSA master
> >   net: dsa: refactor the NETDEV_GOING_DOWN master tracking into separate
> >     function
> >   net: dsa: use dsa_tree_for_each_user_port in
> >     dsa_tree_master_going_down()
> >   net: dsa: provide switch operations for tracking the master state
> >   net: dsa: stop updating master MTU from master.c
> >   net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
> >   net: dsa: replay master state events in
> >     dsa_tree_{setup,teardown}_master
> > 
> >  include/net/dsa.h  |  8 +++++++
> >  net/dsa/dsa2.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++--
> >  net/dsa/dsa_priv.h | 11 ++++++++++
> >  net/dsa/master.c   | 29 +++-----------------------
> >  net/dsa/slave.c    | 32 +++++++++++++++-------------
> >  net/dsa/switch.c   | 29 ++++++++++++++++++++++++++
> >  6 files changed, 118 insertions(+), 43 deletions(-)
> > 
> > -- 
> > 2.25.1
> > 
> 
> I applied this patch and it does work correctly. Sadly the problem is
> not solved and still the packet are not tracked correctly. What I notice
> is that everything starts to work as soon as the master is set to
> promiiscuous mode. Wonder if we should track that event instead of
> simple up?
> 
> Here is a bootlog [0]. I added some log when the function timeouts and when
> master up is actually called.
> Current implementation for this is just a bool that is set to true on
> master up and false on master going down. (final version should use
> locking to check if an Ethernet transation is in progress)
> 
> [0] https://pastebin.com/7w2kgG7a

This is strange. What MAC DA do the ack packets have? Could you give us
a pcap with the request and reply packets (not necessarily now)?
Can you try to set ".promisc_on_master = true" in qca_netdev_ops?

Re: [RFC PATCH net-next 0/7] DSA master state tracking

[Ansuel Smith]

On Thu, Dec 09, 2021 at 02:28:30PM +0000, Vladimir Oltean wrote:
> On Thu, Dec 09, 2021 at 04:05:59AM +0100, Ansuel Smith wrote:
> > On Thu, Dec 09, 2021 at 12:32:23AM +0200, Vladimir Oltean wrote:
> > > This patch set is provided solely for review purposes (therefore not to
> > > be applied anywhere) and for Ansuel to test whether they resolve the
> > > slowdown reported here:
> > > https://patchwork.kernel.org/project/netdevbpf/cover/20211207145942.7444-1-ansuelsmth@gmail.com/
> > > 
> > > It does conflict with net-next due to other patches that are in my tree,
> > > and which were also posted here and would need to be picked ("Rework DSA
> > > bridge TX forwarding offload API"):
> > > https://patchwork.kernel.org/project/netdevbpf/cover/20211206165758.1553882-1-vladimir.oltean@nxp.com/
> > > 
> > > Additionally, for Ansuel's work there is also a logical dependency with
> > > this series ("Replace DSA dp->priv with tagger-owned storage"):
> > > https://patchwork.kernel.org/project/netdevbpf/cover/20211208200504.3136642-1-vladimir.oltean@nxp.com/
> > > 
> > > To get both dependency series, the following commands should be sufficient:
> > > git b4 20211206165758.1553882-1-vladimir.oltean@nxp.com
> > > git b4 20211208200504.3136642-1-vladimir.oltean@nxp.com
> > > 
> > > where "git b4" is an alias in ~/.gitconfig:
> > > [b4]
> > > 	midmask = https://lore.kernel.org/r/%25s
> > > [alias]
> > > 	b4 = "!f() { b4 am -t -o - $@ | git am -3; }; f"
> > > 
> > > The patches posted here are mainly to offer a consistent
> > > "master_up"/"master_going_down" chain of events to switches, without
> > > duplicates, and always starting with "master_up" and ending with
> > > "master_going_down". This way, drivers should know when they can perform
> > > Ethernet-based register access.
> > > 
> > > Vladimir Oltean (7):
> > >   net: dsa: only bring down user ports assigned to a given DSA master
> > >   net: dsa: refactor the NETDEV_GOING_DOWN master tracking into separate
> > >     function
> > >   net: dsa: use dsa_tree_for_each_user_port in
> > >     dsa_tree_master_going_down()
> > >   net: dsa: provide switch operations for tracking the master state
> > >   net: dsa: stop updating master MTU from master.c
> > >   net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
> > >   net: dsa: replay master state events in
> > >     dsa_tree_{setup,teardown}_master
> > > 
> > >  include/net/dsa.h  |  8 +++++++
> > >  net/dsa/dsa2.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++--
> > >  net/dsa/dsa_priv.h | 11 ++++++++++
> > >  net/dsa/master.c   | 29 +++-----------------------
> > >  net/dsa/slave.c    | 32 +++++++++++++++-------------
> > >  net/dsa/switch.c   | 29 ++++++++++++++++++++++++++
> > >  6 files changed, 118 insertions(+), 43 deletions(-)
> > > 
> > > -- 
> > > 2.25.1
> > > 
> > 
> > I applied this patch and it does work correctly. Sadly the problem is
> > not solved and still the packet are not tracked correctly. What I notice
> > is that everything starts to work as soon as the master is set to
> > promiiscuous mode. Wonder if we should track that event instead of
> > simple up?
> > 
> > Here is a bootlog [0]. I added some log when the function timeouts and when
> > master up is actually called.
> > Current implementation for this is just a bool that is set to true on
> > master up and false on master going down. (final version should use
> > locking to check if an Ethernet transation is in progress)
> > 
> > [0] https://pastebin.com/7w2kgG7a
> 
> This is strange. What MAC DA do the ack packets have? Could you give us
> a pcap with the request and reply packets (not necessarily now)?

If you want I can give you a pcap from a router bootup to the setup with
no ethernet cable attached. I notice the switch sends some packet at the
bootup for some reason but they are not Ethernet mdio packet or other
type. It seems they are not even tagged (doesn't have qca tag) as the
header mode is disabled by default)
Let me know if you need just a pcap for the Ethernet mdio transaction or
from a bootup. I assume it would be better from a bootup? (they are not
tons of packet and the mdio Ethernet ones are easy to notice.)

> Can you try to set ".promisc_on_master = true" in qca_netdev_ops?

I already tried and here [0] is a log. I notice with promisc_on_master
the "eth0 entered promiscuous mode" is missing. Is that correct?
Unless I was tired and misread the code, the info should be printed
anyway. Also looking at the comments for promisc_on_master I don't think
that should be applied to this tagger.

[0] https://pastebin.com/MN2ttVpr

-- 
	Ansuel

Re: [RFC PATCH net-next 0/7] DSA master state tracking

[Vladimir Oltean]

On Thu, Dec 09, 2021 at 03:44:14PM +0100, Ansuel Smith wrote:
> On Thu, Dec 09, 2021 at 02:28:30PM +0000, Vladimir Oltean wrote:
> > On Thu, Dec 09, 2021 at 04:05:59AM +0100, Ansuel Smith wrote:
> > > On Thu, Dec 09, 2021 at 12:32:23AM +0200, Vladimir Oltean wrote:
> > > > This patch set is provided solely for review purposes (therefore not to
> > > > be applied anywhere) and for Ansuel to test whether they resolve the
> > > > slowdown reported here:
> > > > https://patchwork.kernel.org/project/netdevbpf/cover/20211207145942.7444-1-ansuelsmth@gmail.com/
> > > > 
> > > > It does conflict with net-next due to other patches that are in my tree,
> > > > and which were also posted here and would need to be picked ("Rework DSA
> > > > bridge TX forwarding offload API"):
> > > > https://patchwork.kernel.org/project/netdevbpf/cover/20211206165758.1553882-1-vladimir.oltean@nxp.com/
> > > > 
> > > > Additionally, for Ansuel's work there is also a logical dependency with
> > > > this series ("Replace DSA dp->priv with tagger-owned storage"):
> > > > https://patchwork.kernel.org/project/netdevbpf/cover/20211208200504.3136642-1-vladimir.oltean@nxp.com/
> > > > 
> > > > To get both dependency series, the following commands should be sufficient:
> > > > git b4 20211206165758.1553882-1-vladimir.oltean@nxp.com
> > > > git b4 20211208200504.3136642-1-vladimir.oltean@nxp.com
> > > > 
> > > > where "git b4" is an alias in ~/.gitconfig:
> > > > [b4]
> > > > 	midmask = https://lore.kernel.org/r/%25s
> > > > [alias]
> > > > 	b4 = "!f() { b4 am -t -o - $@ | git am -3; }; f"
> > > > 
> > > > The patches posted here are mainly to offer a consistent
> > > > "master_up"/"master_going_down" chain of events to switches, without
> > > > duplicates, and always starting with "master_up" and ending with
> > > > "master_going_down". This way, drivers should know when they can perform
> > > > Ethernet-based register access.
> > > > 
> > > > Vladimir Oltean (7):
> > > >   net: dsa: only bring down user ports assigned to a given DSA master
> > > >   net: dsa: refactor the NETDEV_GOING_DOWN master tracking into separate
> > > >     function
> > > >   net: dsa: use dsa_tree_for_each_user_port in
> > > >     dsa_tree_master_going_down()
> > > >   net: dsa: provide switch operations for tracking the master state
> > > >   net: dsa: stop updating master MTU from master.c
> > > >   net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
> > > >   net: dsa: replay master state events in
> > > >     dsa_tree_{setup,teardown}_master
> > > > 
> > > >  include/net/dsa.h  |  8 +++++++
> > > >  net/dsa/dsa2.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++--
> > > >  net/dsa/dsa_priv.h | 11 ++++++++++
> > > >  net/dsa/master.c   | 29 +++-----------------------
> > > >  net/dsa/slave.c    | 32 +++++++++++++++-------------
> > > >  net/dsa/switch.c   | 29 ++++++++++++++++++++++++++
> > > >  6 files changed, 118 insertions(+), 43 deletions(-)
> > > > 
> > > > -- 
> > > > 2.25.1
> > > > 
> > > 
> > > I applied this patch and it does work correctly. Sadly the problem is
> > > not solved and still the packet are not tracked correctly. What I notice
> > > is that everything starts to work as soon as the master is set to
> > > promiiscuous mode. Wonder if we should track that event instead of
> > > simple up?
> > > 
> > > Here is a bootlog [0]. I added some log when the function timeouts and when
> > > master up is actually called.
> > > Current implementation for this is just a bool that is set to true on
> > > master up and false on master going down. (final version should use
> > > locking to check if an Ethernet transation is in progress)
> > > 
> > > [0] https://pastebin.com/7w2kgG7a
> > 
> > This is strange. What MAC DA do the ack packets have? Could you give us
> > a pcap with the request and reply packets (not necessarily now)?
> 
> If you want I can give you a pcap from a router bootup to the setup with
> no ethernet cable attached. I notice the switch sends some packet at the
> bootup for some reason but they are not Ethernet mdio packet or other
> type. It seems they are not even tagged (doesn't have qca tag) as the
> header mode is disabled by default)
> Let me know if you need just a pcap for the Ethernet mdio transaction or
> from a bootup. I assume it would be better from a bootup? (they are not
> tons of packet and the mdio Ethernet ones are easy to notice.)

Anything that contains some request and response packets should do, as
long as they're relatively easy to spot. But as stated, this can wait
for a while, I don't think that promiscuity is the issue, after your
second reply.

> > Can you try to set ".promisc_on_master = true" in qca_netdev_ops?
> 
> I already tried and here [0] is a log. I notice with promisc_on_master
> the "eth0 entered promiscuous mode" is missing. Is that correct?
> Unless I was tired and misread the code, the info should be printed
> anyway. Also looking at the comments for promisc_on_master I don't think
> that should be applied to this tagger.
> 
> [0] https://pastebin.com/MN2ttVpr

It isn't missing, it's right there on line 11.
I think the problem is that we also need to track the operstate of the
master (netif_oper_up via NETDEV_CHANGE) before declaring it as good to go.
You can see that this is exactly the line after which the timeouts disappear:

[    7.146901] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready

I didn't really want to go there, because now I'm not sure how to
synthesize the information for the switch drivers to consume it.
Anyway I've prepared a v2 patchset and I'll send it out very soon.

Re: [RFC PATCH net-next 0/7] DSA master state tracking

[Ansuel Smith]

On Thu, Dec 09, 2021 at 05:33:17PM +0000, Vladimir Oltean wrote:
> On Thu, Dec 09, 2021 at 03:44:14PM +0100, Ansuel Smith wrote:
> > On Thu, Dec 09, 2021 at 02:28:30PM +0000, Vladimir Oltean wrote:
> > > On Thu, Dec 09, 2021 at 04:05:59AM +0100, Ansuel Smith wrote:
> > > > On Thu, Dec 09, 2021 at 12:32:23AM +0200, Vladimir Oltean wrote:
> > > > > This patch set is provided solely for review purposes (therefore not to
> > > > > be applied anywhere) and for Ansuel to test whether they resolve the
> > > > > slowdown reported here:
> > > > > https://patchwork.kernel.org/project/netdevbpf/cover/20211207145942.7444-1-ansuelsmth@gmail.com/
> > > > > 
> > > > > It does conflict with net-next due to other patches that are in my tree,
> > > > > and which were also posted here and would need to be picked ("Rework DSA
> > > > > bridge TX forwarding offload API"):
> > > > > https://patchwork.kernel.org/project/netdevbpf/cover/20211206165758.1553882-1-vladimir.oltean@nxp.com/
> > > > > 
> > > > > Additionally, for Ansuel's work there is also a logical dependency with
> > > > > this series ("Replace DSA dp->priv with tagger-owned storage"):
> > > > > https://patchwork.kernel.org/project/netdevbpf/cover/20211208200504.3136642-1-vladimir.oltean@nxp.com/
> > > > > 
> > > > > To get both dependency series, the following commands should be sufficient:
> > > > > git b4 20211206165758.1553882-1-vladimir.oltean@nxp.com
> > > > > git b4 20211208200504.3136642-1-vladimir.oltean@nxp.com
> > > > > 
> > > > > where "git b4" is an alias in ~/.gitconfig:
> > > > > [b4]
> > > > > 	midmask = https://lore.kernel.org/r/%25s
> > > > > [alias]
> > > > > 	b4 = "!f() { b4 am -t -o - $@ | git am -3; }; f"
> > > > > 
> > > > > The patches posted here are mainly to offer a consistent
> > > > > "master_up"/"master_going_down" chain of events to switches, without
> > > > > duplicates, and always starting with "master_up" and ending with
> > > > > "master_going_down". This way, drivers should know when they can perform
> > > > > Ethernet-based register access.
> > > > > 
> > > > > Vladimir Oltean (7):
> > > > >   net: dsa: only bring down user ports assigned to a given DSA master
> > > > >   net: dsa: refactor the NETDEV_GOING_DOWN master tracking into separate
> > > > >     function
> > > > >   net: dsa: use dsa_tree_for_each_user_port in
> > > > >     dsa_tree_master_going_down()
> > > > >   net: dsa: provide switch operations for tracking the master state
> > > > >   net: dsa: stop updating master MTU from master.c
> > > > >   net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
> > > > >   net: dsa: replay master state events in
> > > > >     dsa_tree_{setup,teardown}_master
> > > > > 
> > > > >  include/net/dsa.h  |  8 +++++++
> > > > >  net/dsa/dsa2.c     | 52 ++++++++++++++++++++++++++++++++++++++++++++--
> > > > >  net/dsa/dsa_priv.h | 11 ++++++++++
> > > > >  net/dsa/master.c   | 29 +++-----------------------
> > > > >  net/dsa/slave.c    | 32 +++++++++++++++-------------
> > > > >  net/dsa/switch.c   | 29 ++++++++++++++++++++++++++
> > > > >  6 files changed, 118 insertions(+), 43 deletions(-)
> > > > > 
> > > > > -- 
> > > > > 2.25.1
> > > > > 
> > > > 
> > > > I applied this patch and it does work correctly. Sadly the problem is
> > > > not solved and still the packet are not tracked correctly. What I notice
> > > > is that everything starts to work as soon as the master is set to
> > > > promiiscuous mode. Wonder if we should track that event instead of
> > > > simple up?
> > > > 
> > > > Here is a bootlog [0]. I added some log when the function timeouts and when
> > > > master up is actually called.
> > > > Current implementation for this is just a bool that is set to true on
> > > > master up and false on master going down. (final version should use
> > > > locking to check if an Ethernet transation is in progress)
> > > > 
> > > > [0] https://pastebin.com/7w2kgG7a
> > > 
> > > This is strange. What MAC DA do the ack packets have? Could you give us
> > > a pcap with the request and reply packets (not necessarily now)?
> > 
> > If you want I can give you a pcap from a router bootup to the setup with
> > no ethernet cable attached. I notice the switch sends some packet at the
> > bootup for some reason but they are not Ethernet mdio packet or other
> > type. It seems they are not even tagged (doesn't have qca tag) as the
> > header mode is disabled by default)
> > Let me know if you need just a pcap for the Ethernet mdio transaction or
> > from a bootup. I assume it would be better from a bootup? (they are not
> > tons of packet and the mdio Ethernet ones are easy to notice.)
> 
> Anything that contains some request and response packets should do, as
> long as they're relatively easy to spot. But as stated, this can wait
> for a while, I don't think that promiscuity is the issue, after your
> second reply.
>

Ok will send a pcap. Any preferred way to send it?

> > > Can you try to set ".promisc_on_master = true" in qca_netdev_ops?
> > 
> > I already tried and here [0] is a log. I notice with promisc_on_master
> > the "eth0 entered promiscuous mode" is missing. Is that correct?
> > Unless I was tired and misread the code, the info should be printed
> > anyway. Also looking at the comments for promisc_on_master I don't think
> > that should be applied to this tagger.
> > 
> > [0] https://pastebin.com/MN2ttVpr
> 
> It isn't missing, it's right there on line 11.

Oww didn't notice that!

> I think the problem is that we also need to track the operstate of the
> master (netif_oper_up via NETDEV_CHANGE) before declaring it as good to go.
> You can see that this is exactly the line after which the timeouts disappear:
> 
> [    7.146901] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> 
> I didn't really want to go there, because now I'm not sure how to
> synthesize the information for the switch drivers to consume it.
> Anyway I've prepared a v2 patchset and I'll send it out very soon.

Wonder if we should leave the driver decide when it's ready by parsing
the different state? (And change
the up ops to something like a generic change?)

-- 
	Ansuel

Re: [RFC PATCH net-next 0/7] DSA master state tracking

[Vladimir Oltean]

On Thu, Dec 09, 2021 at 06:44:38PM +0100, Ansuel Smith wrote:
> > I think the problem is that we also need to track the operstate of the
> > master (netif_oper_up via NETDEV_CHANGE) before declaring it as good to go.
> > You can see that this is exactly the line after which the timeouts disappear:
> > 
> > [    7.146901] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> > 
> > I didn't really want to go there, because now I'm not sure how to
> > synthesize the information for the switch drivers to consume it.
> > Anyway I've prepared a v2 patchset and I'll send it out very soon.
> 
> Wonder if we should leave the driver decide when it's ready by parsing
> the different state? (And change
> the up ops to something like a generic change?)

There isn't just one state to track, which is precisely the problem that
I had to deal with for v2. The master is operational during the time
frame between NETDEV_UP and NETDEV_GOING_DOWN, intersected with the
interval during which netif_oper_up(master) is true. So in the simple
state propagation approach, DSA would need to provide at least two ops
to switches, one for admin state and the other for oper state. And the
switch driver would need to AND the two and keep state by itself.
Letting the driver make the decision would have been acceptable to me if
we could have 3 ops and a common implementation, something like this:

static void qca8k_master_state_change(struct dsa_switch *ds,
				      const struct dsa_master *master)
{
	bool operational = (master->flags & IFF_UP) && netif_oper_up(master);
}

	.master_admin_state_change	= qca8k_master_state_change,
	.master_oper_state_change	= qca8k_master_state_change,

but the problem is that during NETDEV_GOING_DOWN, master->flags & IFF_UP
is still true, so this wouldn't work. And replacing the NETDEV_GOING_DOWN
notifier with the NETDEV_DOWN one would solve that problem, but it would
no longer guarantee that the switch can disable this feature without
timeouts before the master is down - because now it _is_ down.

Re: [RFC PATCH net-next 0/7] DSA master state tracking

[Vladimir Oltean]

On Thu, Dec 09, 2021 at 06:44:38PM +0100, Ansuel Smith wrote:
> Ok will send a pcap. Any preferred way to send it?

Email attachment should be fine.

Re: [RFC PATCH net-next 0/7] DSA master state tracking

[Ansuel Smith]

On Thu, Dec 09, 2021 at 05:56:17PM +0000, Vladimir Oltean wrote:
> On Thu, Dec 09, 2021 at 06:44:38PM +0100, Ansuel Smith wrote:
> > > I think the problem is that we also need to track the operstate of the
> > > master (netif_oper_up via NETDEV_CHANGE) before declaring it as good to go.
> > > You can see that this is exactly the line after which the timeouts disappear:
> > > 
> > > [    7.146901] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> > > 
> > > I didn't really want to go there, because now I'm not sure how to
> > > synthesize the information for the switch drivers to consume it.
> > > Anyway I've prepared a v2 patchset and I'll send it out very soon.
> > 
> > Wonder if we should leave the driver decide when it's ready by parsing
> > the different state? (And change
> > the up ops to something like a generic change?)
> 
> There isn't just one state to track, which is precisely the problem that
> I had to deal with for v2. The master is operational during the time
> frame between NETDEV_UP and NETDEV_GOING_DOWN, intersected with the
> interval during which netif_oper_up(master) is true. So in the simple
> state propagation approach, DSA would need to provide at least two ops
> to switches, one for admin state and the other for oper state. And the
> switch driver would need to AND the two and keep state by itself.
> Letting the driver make the decision would have been acceptable to me if
> we could have 3 ops and a common implementation, something like this:
> 
> static void qca8k_master_state_change(struct dsa_switch *ds,
> 				      const struct dsa_master *master)
> {
> 	bool operational = (master->flags & IFF_UP) && netif_oper_up(master);
> }
> 
> 	.master_admin_state_change	= qca8k_master_state_change,
> 	.master_oper_state_change	= qca8k_master_state_change,
> 
> but the problem is that during NETDEV_GOING_DOWN, master->flags & IFF_UP
> is still true, so this wouldn't work. And replacing the NETDEV_GOING_DOWN
> notifier with the NETDEV_DOWN one would solve that problem, but it would
> no longer guarantee that the switch can disable this feature without
> timeouts before the master is down - because now it _is_ down.

Ok will have to test v2 and check if this is also fixed.

-- 
	Ansuel

Re: [RFC PATCH net-next 0/7] DSA master state tracking

[Ansuel Smith]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]

On Thu, Dec 09, 2021 at 05:57:20PM +0000, Vladimir Oltean wrote:
> On Thu, Dec 09, 2021 at 06:44:38PM +0100, Ansuel Smith wrote:
> > Ok will send a pcap. Any preferred way to send it?
> 
> Email attachment should be fine.

This is a pcap done on the cpu port eth0.
All the packet with lenght 60 are the Ethernet mdio packet.
The capture is done with no cable connected so don't know why there are
some broadcast for ipv6.
Special care about the fact that the qca tag is always present in the
EtherType.
Mdio request qca tag 8181
mdio ack qca tag b887

(I should really investigate the extra packet... In theory they are not
sent (and i remember checking this) by the tagger as they have bit 5:4
set to 0x2... From Documentation these bit are reserved and in our code
we always set them to zero. The switch by itself sends broadcast tagged
packet and respond itself?)

I attached the pcap.
-- 
	Ansuel

[-- Attachment #2: pkts.pcap --]
[-- Type: application/vnd.tcpdump.pcap, Size: 29944 bytes --]