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

[RFC PATCH v2 net-next 0/4] 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/

The patches posted here are mainly to offer a consistent
"master_state_change" chain of events to switches, without duplicates,
and always starting with operational=true and ending with
operational=false. This way, drivers should know when they can perform
Ethernet-based register access, and need not care about more than that.

Changes in v2:
- dropped some useless patches
- also check master operstate.

Vladimir Oltean (4):
  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  | 11 +++++++
 net/dsa/dsa2.c     | 80 +++++++++++++++++++++++++++++++++++++++++++---
 net/dsa/dsa_priv.h | 13 ++++++++
 net/dsa/master.c   | 29 ++---------------
 net/dsa/slave.c    | 27 ++++++++++++++++
 net/dsa/switch.c   | 15 +++++++++
 6 files changed, 145 insertions(+), 30 deletions(-)

-- 
2.25.1

[RFC PATCH v2 net-next 1/4] 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_CHANGE,
NETDEV_UP and NETDEV_GOING_DOWN events on the DSA master, we should know
the exact interval of time during which this interface is reliably
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  | 11 +++++++++++
 net/dsa/dsa2.c     | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 net/dsa/dsa_priv.h | 13 +++++++++++++
 net/dsa/slave.c    | 27 +++++++++++++++++++++++++++
 net/dsa/switch.c   | 15 +++++++++++++++
 5 files changed, 112 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index bdf308a5c55e..8690b9c6d674 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -296,6 +296,10 @@ struct dsa_port {
 	struct list_head	fdbs;
 	struct list_head	mdbs;
 
+	/* Master state bits, valid only on CPU ports */
+	u8 master_admin_up:1,
+	   master_oper_up:1;
+
 	bool setup;
 };
 
@@ -1011,6 +1015,13 @@ 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_state_change)(struct dsa_switch *ds,
+				       const struct net_device *master,
+				       bool operational);
 };
 
 #define DSA_DEVLINK_PARAM_DRIVER(_id, _name, _type, _cmodes)		\
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 8814fa0e44c8..a6cb3470face 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1187,6 +1187,52 @@ int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
 	return err;
 }
 
+static void dsa_tree_master_state_change(struct dsa_switch_tree *dst,
+					 struct net_device *master)
+{
+	struct dsa_notifier_master_state_info info;
+	struct dsa_port *cpu_dp = master->dsa_ptr;
+
+	info.master = master;
+	info.operational = cpu_dp->master_admin_up && cpu_dp->master_oper_up;
+
+	dsa_tree_notify(dst, DSA_NOTIFIER_MASTER_STATE_CHANGE, &info);
+}
+
+void dsa_tree_master_admin_state_change(struct dsa_switch_tree *dst,
+					struct net_device *master,
+					bool up)
+{
+	struct dsa_port *cpu_dp = master->dsa_ptr;
+	bool notify = false;
+
+	if ((cpu_dp->master_admin_up && cpu_dp->master_oper_up) !=
+	    (up && cpu_dp->master_oper_up))
+		notify = true;
+
+	cpu_dp->master_admin_up = up;
+
+	if (notify)
+		dsa_tree_master_state_change(dst, master);
+}
+
+void dsa_tree_master_oper_state_change(struct dsa_switch_tree *dst,
+				       struct net_device *master,
+				       bool up)
+{
+	struct dsa_port *cpu_dp = master->dsa_ptr;
+	bool notify = false;
+
+	if ((cpu_dp->master_admin_up && cpu_dp->master_oper_up) !=
+	    (cpu_dp->master_admin_up && up))
+		notify = true;
+
+	cpu_dp->master_oper_up = up;
+
+	if (notify)
+		dsa_tree_master_state_change(dst, master);
+}
+
 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..c47864446adc 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -43,6 +43,7 @@ enum {
 	DSA_NOTIFIER_MRP_DEL_RING_ROLE,
 	DSA_NOTIFIER_TAG_8021Q_VLAN_ADD,
 	DSA_NOTIFIER_TAG_8021Q_VLAN_DEL,
+	DSA_NOTIFIER_MASTER_STATE_CHANGE,
 };
 
 /* DSA_NOTIFIER_AGEING_TIME */
@@ -126,6 +127,12 @@ struct dsa_notifier_tag_8021q_vlan_info {
 	u16 vid;
 };
 
+/* DSA_NOTIFIER_MASTER_STATE_CHANGE */
+struct dsa_notifier_master_state_info {
+	const struct net_device *master;
+	bool operational;
+};
+
 struct dsa_switchdev_event_work {
 	struct dsa_switch *ds;
 	int port;
@@ -506,6 +513,12 @@ 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_admin_state_change(struct dsa_switch_tree *dst,
+					struct net_device *master,
+					bool up);
+void dsa_tree_master_oper_state_change(struct dsa_switch_tree *dst,
+				       struct net_device *master,
+				       bool up);
 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 2b153b366118..9f3b25c08c13 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2349,6 +2349,31 @@ 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_CHANGE: {
+		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_oper_state_change(dst, dev,
+							  netif_oper_up(dev));
+
+			return NOTIFY_OK;
+		}
+
+		return NOTIFY_DONE;
+	}
+	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_admin_state_change(dst, dev, true);
+
+			return NOTIFY_OK;
+		}
+
+		return NOTIFY_DONE;
+	}
 	case NETDEV_GOING_DOWN: {
 		struct dsa_port *dp, *cpu_dp;
 		struct dsa_switch_tree *dst;
@@ -2360,6 +2385,8 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
 		cpu_dp = dev->dsa_ptr;
 		dst = cpu_dp->ds->dst;
 
+		dsa_tree_master_admin_state_change(dst, dev, false);
+
 		list_for_each_entry(dp, &dst->ports, list) {
 			if (!dsa_port_is_user(dp))
 				continue;
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 9c92edd96961..78816a6805c8 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -699,6 +699,18 @@ dsa_switch_mrp_del_ring_role(struct dsa_switch *ds,
 	return 0;
 }
 
+static int
+dsa_switch_master_state_change(struct dsa_switch *ds,
+			       struct dsa_notifier_master_state_info *info)
+{
+	if (!ds->ops->master_state_change)
+		return 0;
+
+	ds->ops->master_state_change(ds, info->master, info->operational);
+
+	return 0;
+}
+
 static int dsa_switch_event(struct notifier_block *nb,
 			    unsigned long event, void *info)
 {
@@ -784,6 +796,9 @@ 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_STATE_CHANGE:
+		err = dsa_switch_master_state_change(ds, info);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
-- 
2.25.1

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

[Vladimir Oltean]

On Thu, Dec 09, 2021 at 07:39:24PM +0200, Vladimir Oltean wrote:
> 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_CHANGE,
> NETDEV_UP and NETDEV_GOING_DOWN events on the DSA master, we should know
> the exact interval of time during which this interface is reliably
> 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  | 11 +++++++++++
>  net/dsa/dsa2.c     | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  net/dsa/dsa_priv.h | 13 +++++++++++++
>  net/dsa/slave.c    | 27 +++++++++++++++++++++++++++
>  net/dsa/switch.c   | 15 +++++++++++++++
>  5 files changed, 112 insertions(+)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index bdf308a5c55e..8690b9c6d674 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -296,6 +296,10 @@ struct dsa_port {
>  	struct list_head	fdbs;
>  	struct list_head	mdbs;
>  
> +	/* Master state bits, valid only on CPU ports */
> +	u8 master_admin_up:1,
> +	   master_oper_up:1;
> +
>  	bool setup;
>  };
>  
> @@ -1011,6 +1015,13 @@ 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_state_change)(struct dsa_switch *ds,
> +				       const struct net_device *master,
> +				       bool operational);
>  };
>  
>  #define DSA_DEVLINK_PARAM_DRIVER(_id, _name, _type, _cmodes)		\
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index 8814fa0e44c8..a6cb3470face 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -1187,6 +1187,52 @@ int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
>  	return err;
>  }
>  
> +static void dsa_tree_master_state_change(struct dsa_switch_tree *dst,
> +					 struct net_device *master)
> +{
> +	struct dsa_notifier_master_state_info info;
> +	struct dsa_port *cpu_dp = master->dsa_ptr;
> +
> +	info.master = master;
> +	info.operational = cpu_dp->master_admin_up && cpu_dp->master_oper_up;
> +
> +	dsa_tree_notify(dst, DSA_NOTIFIER_MASTER_STATE_CHANGE, &info);
> +}
> +
> +void dsa_tree_master_admin_state_change(struct dsa_switch_tree *dst,
> +					struct net_device *master,
> +					bool up)
> +{
> +	struct dsa_port *cpu_dp = master->dsa_ptr;
> +	bool notify = false;
> +
> +	if ((cpu_dp->master_admin_up && cpu_dp->master_oper_up) !=
> +	    (up && cpu_dp->master_oper_up))
> +		notify = true;
> +
> +	cpu_dp->master_admin_up = up;
> +
> +	if (notify)
> +		dsa_tree_master_state_change(dst, master);
> +}
> +
> +void dsa_tree_master_oper_state_change(struct dsa_switch_tree *dst,
> +				       struct net_device *master,
> +				       bool up)
> +{
> +	struct dsa_port *cpu_dp = master->dsa_ptr;
> +	bool notify = false;
> +
> +	if ((cpu_dp->master_admin_up && cpu_dp->master_oper_up) !=
> +	    (cpu_dp->master_admin_up && up))
> +		notify = true;
> +
> +	cpu_dp->master_oper_up = up;
> +
> +	if (notify)
> +		dsa_tree_master_state_change(dst, master);
> +}
> +
>  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..c47864446adc 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -43,6 +43,7 @@ enum {
>  	DSA_NOTIFIER_MRP_DEL_RING_ROLE,
>  	DSA_NOTIFIER_TAG_8021Q_VLAN_ADD,
>  	DSA_NOTIFIER_TAG_8021Q_VLAN_DEL,
> +	DSA_NOTIFIER_MASTER_STATE_CHANGE,
>  };
>  
>  /* DSA_NOTIFIER_AGEING_TIME */
> @@ -126,6 +127,12 @@ struct dsa_notifier_tag_8021q_vlan_info {
>  	u16 vid;
>  };
>  
> +/* DSA_NOTIFIER_MASTER_STATE_CHANGE */
> +struct dsa_notifier_master_state_info {
> +	const struct net_device *master;
> +	bool operational;
> +};
> +
>  struct dsa_switchdev_event_work {
>  	struct dsa_switch *ds;
>  	int port;
> @@ -506,6 +513,12 @@ 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_admin_state_change(struct dsa_switch_tree *dst,
> +					struct net_device *master,
> +					bool up);
> +void dsa_tree_master_oper_state_change(struct dsa_switch_tree *dst,
> +				       struct net_device *master,
> +				       bool up);
>  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 2b153b366118..9f3b25c08c13 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -2349,6 +2349,31 @@ 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_CHANGE: {
> +		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_oper_state_change(dst, dev,
> +							  netif_oper_up(dev));

must also add a call here to change the admin state, due to the fact
that linkwatch_do_dev may call netdev_state_change() after dev_activate().
So it seems that "case NETDEV_CHANGE" and "case NETDEV_UP" may share the
same implementation, like this:

	case NETDEV_CHANGE:
	case 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_admin_state_change(dst, dev,
							   qdisc_tx_is_noop(dev));
			dsa_tree_master_oper_state_change(dst, dev,
							  netif_oper_up(dev));

			return NOTIFY_OK;
		}

		return NOTIFY_DONE;
	}

Would be good to also add some comments.

> +
> +			return NOTIFY_OK;
> +		}
> +
> +		return NOTIFY_DONE;
> +	}
> +	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_admin_state_change(dst, dev, true);

s/true/qdisc_tx_is_noop(dev)/

> +
> +			return NOTIFY_OK;
> +		}
> +
> +		return NOTIFY_DONE;
> +	}
>  	case NETDEV_GOING_DOWN: {
>  		struct dsa_port *dp, *cpu_dp;
>  		struct dsa_switch_tree *dst;
> @@ -2360,6 +2385,8 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
>  		cpu_dp = dev->dsa_ptr;
>  		dst = cpu_dp->ds->dst;
>  
> +		dsa_tree_master_admin_state_change(dst, dev, false);
> +
>  		list_for_each_entry(dp, &dst->ports, list) {
>  			if (!dsa_port_is_user(dp))
>  				continue;
> diff --git a/net/dsa/switch.c b/net/dsa/switch.c
> index 9c92edd96961..78816a6805c8 100644
> --- a/net/dsa/switch.c
> +++ b/net/dsa/switch.c
> @@ -699,6 +699,18 @@ dsa_switch_mrp_del_ring_role(struct dsa_switch *ds,
>  	return 0;
>  }
>  
> +static int
> +dsa_switch_master_state_change(struct dsa_switch *ds,
> +			       struct dsa_notifier_master_state_info *info)
> +{
> +	if (!ds->ops->master_state_change)
> +		return 0;
> +
> +	ds->ops->master_state_change(ds, info->master, info->operational);
> +
> +	return 0;
> +}
> +
>  static int dsa_switch_event(struct notifier_block *nb,
>  			    unsigned long event, void *info)
>  {
> @@ -784,6 +796,9 @@ 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_STATE_CHANGE:
> +		err = dsa_switch_master_state_change(ds, info);
> +		break;
>  	default:
>  		err = -EOPNOTSUPP;
>  		break;
> -- 
> 2.25.1
>

[RFC PATCH v2 net-next 2/4] 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 v2 net-next 3/4] 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 a6cb3470face..6d4422c9e334 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

Re: [RFC PATCH v2 net-next 3/4] net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}

[Vladimir Oltean]

On Thu, Dec 09, 2021 at 07:39:26PM +0200, Vladimir Oltean wrote:
> 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).

This paragraph needs to be updated. For one, "->master_up and
->master_going down calls are made in exactly this order" needs to
become something like "->master_change calls are made only when the
master's readiness state to pass traffic changes". Then, "block the
reception of NETDEV_UP/NETDEV_GOING_DOWN" must also mention
"NETDEV_CHANGE".

> 
> 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 a6cb3470face..6d4422c9e334 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 v2 net-next 4/4] 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 | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 6d4422c9e334..c86c9688e8cc 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1019,9 +1019,17 @@ static int dsa_tree_setup_master(struct dsa_switch_tree *dst)
 
 	list_for_each_entry(dp, &dst->ports, list) {
 		if (dsa_port_is_cpu(dp)) {
-			err = dsa_master_setup(dp->master, dp);
+			struct net_device *master = dp->master;
+
+			err = dsa_master_setup(master, dp);
 			if (err)
 				return err;
+
+			/* Replay master state event */
+			dsa_tree_master_admin_state_change(dst, master,
+							   master->flags & IFF_UP);
+			dsa_tree_master_oper_state_change(dst, master,
+							  netif_oper_up(master));
 		}
 	}
 
@@ -1036,9 +1044,19 @@ 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))
-			dsa_master_teardown(dp->master);
+	list_for_each_entry(dp, &dst->ports, list) {
+		if (dsa_port_is_cpu(dp)) {
+			struct net_device *master = dp->master;
+
+			/* Synthesizing an "admin down" state is sufficient for
+			 * the switches to get a notification if the master is
+			 * currently up and running.
+			 */
+			dsa_tree_master_admin_state_change(dst, master, false);
+
+			dsa_master_teardown(master);
+		}
+	}
 
 	rtnl_unlock();
 }
-- 
2.25.1

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

[Vladimir Oltean]

On Thu, Dec 09, 2021 at 07:39:27PM +0200, Vladimir Oltean wrote:
> 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

s/master->flags & IFF_UP/the master being up/ (the condition will be
more complex, no need to spell it out

> would emit a ->master_up() event twice. We need to avoid that by

s/master_up() event twice/master_state_change() call with no actual
state change.

> 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 | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index 6d4422c9e334..c86c9688e8cc 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -1019,9 +1019,17 @@ static int dsa_tree_setup_master(struct dsa_switch_tree *dst)
>  
>  	list_for_each_entry(dp, &dst->ports, list) {
>  		if (dsa_port_is_cpu(dp)) {
> -			err = dsa_master_setup(dp->master, dp);
> +			struct net_device *master = dp->master;
> +
> +			err = dsa_master_setup(master, dp);
>  			if (err)
>  				return err;
> +
> +			/* Replay master state event */
> +			dsa_tree_master_admin_state_change(dst, master,
> +							   master->flags & IFF_UP);

It would be good to add a "bool admin_up = (master->flags & IFF_UP) && !qdisc_tx_is_noop(master)",
to avoid the line getting too long.

> +			dsa_tree_master_oper_state_change(dst, master,
> +							  netif_oper_up(master));
>  		}
>  	}
>  
> @@ -1036,9 +1044,19 @@ 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))
> -			dsa_master_teardown(dp->master);
> +	list_for_each_entry(dp, &dst->ports, list) {
> +		if (dsa_port_is_cpu(dp)) {
> +			struct net_device *master = dp->master;
> +
> +			/* Synthesizing an "admin down" state is sufficient for
> +			 * the switches to get a notification if the master is
> +			 * currently up and running.
> +			 */
> +			dsa_tree_master_admin_state_change(dst, master, false);
> +
> +			dsa_master_teardown(master);
> +		}
> +	}
>  
>  	rtnl_unlock();
>  }
> -- 
> 2.25.1
>

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

[Ansuel Smith]

On Thu, Dec 09, 2021 at 07:39:23PM +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/
> 
> The patches posted here are mainly to offer a consistent
> "master_state_change" chain of events to switches, without duplicates,
> and always starting with operational=true and ending with
> operational=false. This way, drivers should know when they can perform
> Ethernet-based register access, and need not care about more than that.
> 
> Changes in v2:
> - dropped some useless patches
> - also check master operstate.
> 
> Vladimir Oltean (4):
>   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  | 11 +++++++
>  net/dsa/dsa2.c     | 80 +++++++++++++++++++++++++++++++++++++++++++---
>  net/dsa/dsa_priv.h | 13 ++++++++
>  net/dsa/master.c   | 29 ++---------------
>  net/dsa/slave.c    | 27 ++++++++++++++++
>  net/dsa/switch.c   | 15 +++++++++
>  6 files changed, 145 insertions(+), 30 deletions(-)
> 
> -- 
> 2.25.1
> 

Hi, I tested this v2 and I still have 2 ethernet mdio failing on init.
I don't think we have other way to track this. Am I wrong?

All works correctly with this and promisc_on_master.
If you have other test, feel free to send me other stuff to test.

(I'm starting to think the fail is caused by some delay that the switch
require to actually start accepting packet or from the reinit? But I'm
not sure... don't know if you notice something from the pcap)

-- 
	Ansuel

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

[Vladimir Oltean]

On Fri, Dec 10, 2021 at 04:37:52AM +0100, Ansuel Smith wrote:
> On Thu, Dec 09, 2021 at 07:39:23PM +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/
> > 
> > The patches posted here are mainly to offer a consistent
> > "master_state_change" chain of events to switches, without duplicates,
> > and always starting with operational=true and ending with
> > operational=false. This way, drivers should know when they can perform
> > Ethernet-based register access, and need not care about more than that.
> > 
> > Changes in v2:
> > - dropped some useless patches
> > - also check master operstate.
> > 
> > Vladimir Oltean (4):
> >   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  | 11 +++++++
> >  net/dsa/dsa2.c     | 80 +++++++++++++++++++++++++++++++++++++++++++---
> >  net/dsa/dsa_priv.h | 13 ++++++++
> >  net/dsa/master.c   | 29 ++---------------
> >  net/dsa/slave.c    | 27 ++++++++++++++++
> >  net/dsa/switch.c   | 15 +++++++++
> >  6 files changed, 145 insertions(+), 30 deletions(-)
> > 
> > -- 
> > 2.25.1
> > 
> 
> Hi, I tested this v2 and I still have 2 ethernet mdio failing on init.
> I don't think we have other way to track this. Am I wrong?
> 
> All works correctly with this and promisc_on_master.
> If you have other test, feel free to send me other stuff to test.
> 
> (I'm starting to think the fail is caused by some delay that the switch
> require to actually start accepting packet or from the reinit? But I'm
> not sure... don't know if you notice something from the pcap)

I've opened the pcap just now. The Ethernet MDIO packets are
non-standard. When the DSA master receives them, it expects the first 6
octets to be the MAC DA, because that's the format of an Ethernet frame.
But the packets have this other format, according to your own writing:

/* Specific define for in-band MDIO read/write with Ethernet packet */
#define QCA_HDR_MDIO_SEQ_LEN           4 /* 4 byte for the seq */
#define QCA_HDR_MDIO_COMMAND_LEN       4 /* 4 byte for the command */
#define QCA_HDR_MDIO_DATA1_LEN         4 /* First 4 byte for the mdio data */
#define QCA_HDR_MDIO_HEADER_LEN        (QCA_HDR_MDIO_SEQ_LEN + \
                                       QCA_HDR_MDIO_COMMAND_LEN + \
                                       QCA_HDR_MDIO_DATA1_LEN)

#define QCA_HDR_MDIO_DATA2_LEN         12 /* Other 12 byte for the mdio data */
#define QCA_HDR_MDIO_PADDING_LEN       34 /* Padding to reach the min Ethernet packet */

The first 6 octets change like crazy in your pcap. Definitely can't add
that to the RX filter of the DSA master.

So yes, promisc_on_master is precisely what you need, it exists for
situations like this.

Considering this, I guess it works?

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

[Ansuel Smith]

On Fri, Dec 10, 2021 at 05:02:42PM +0000, Vladimir Oltean wrote:
> On Fri, Dec 10, 2021 at 04:37:52AM +0100, Ansuel Smith wrote:
> > On Thu, Dec 09, 2021 at 07:39:23PM +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/
> > > 
> > > The patches posted here are mainly to offer a consistent
> > > "master_state_change" chain of events to switches, without duplicates,
> > > and always starting with operational=true and ending with
> > > operational=false. This way, drivers should know when they can perform
> > > Ethernet-based register access, and need not care about more than that.
> > > 
> > > Changes in v2:
> > > - dropped some useless patches
> > > - also check master operstate.
> > > 
> > > Vladimir Oltean (4):
> > >   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  | 11 +++++++
> > >  net/dsa/dsa2.c     | 80 +++++++++++++++++++++++++++++++++++++++++++---
> > >  net/dsa/dsa_priv.h | 13 ++++++++
> > >  net/dsa/master.c   | 29 ++---------------
> > >  net/dsa/slave.c    | 27 ++++++++++++++++
> > >  net/dsa/switch.c   | 15 +++++++++
> > >  6 files changed, 145 insertions(+), 30 deletions(-)
> > > 
> > > -- 
> > > 2.25.1
> > > 
> > 
> > Hi, I tested this v2 and I still have 2 ethernet mdio failing on init.
> > I don't think we have other way to track this. Am I wrong?
> > 
> > All works correctly with this and promisc_on_master.
> > If you have other test, feel free to send me other stuff to test.
> > 
> > (I'm starting to think the fail is caused by some delay that the switch
> > require to actually start accepting packet or from the reinit? But I'm
> > not sure... don't know if you notice something from the pcap)
> 
> I've opened the pcap just now. The Ethernet MDIO packets are
> non-standard. When the DSA master receives them, it expects the first 6
> octets to be the MAC DA, because that's the format of an Ethernet frame.
> But the packets have this other format, according to your own writing:
> 
> /* Specific define for in-band MDIO read/write with Ethernet packet */
> #define QCA_HDR_MDIO_SEQ_LEN           4 /* 4 byte for the seq */
> #define QCA_HDR_MDIO_COMMAND_LEN       4 /* 4 byte for the command */
> #define QCA_HDR_MDIO_DATA1_LEN         4 /* First 4 byte for the mdio data */
> #define QCA_HDR_MDIO_HEADER_LEN        (QCA_HDR_MDIO_SEQ_LEN + \
>                                        QCA_HDR_MDIO_COMMAND_LEN + \
>                                        QCA_HDR_MDIO_DATA1_LEN)
> 
> #define QCA_HDR_MDIO_DATA2_LEN         12 /* Other 12 byte for the mdio data */
> #define QCA_HDR_MDIO_PADDING_LEN       34 /* Padding to reach the min Ethernet packet */
> 
> The first 6 octets change like crazy in your pcap. Definitely can't add
> that to the RX filter of the DSA master.
> 
> So yes, promisc_on_master is precisely what you need, it exists for
> situations like this.
> 
> Considering this, I guess it works?

Yes it works! We can totally accept 2 mdio timeout out of a good way to
track the master port. It's probably related to other stuff like switch
delay or other.

Wonder the next step is wait for this to be accepted and then I can
propose a v3 of my patch? Or net-next is closed now and I should just
send v3 RFC saying it does depend on this?

-- 
	Ansuel

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

[Vladimir Oltean]

On Fri, Dec 10, 2021 at 06:10:45PM +0100, Ansuel Smith wrote:
> On Fri, Dec 10, 2021 at 05:02:42PM +0000, Vladimir Oltean wrote:
> > On Fri, Dec 10, 2021 at 04:37:52AM +0100, Ansuel Smith wrote:
> > > On Thu, Dec 09, 2021 at 07:39:23PM +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/
> > > > 
> > > > The patches posted here are mainly to offer a consistent
> > > > "master_state_change" chain of events to switches, without duplicates,
> > > > and always starting with operational=true and ending with
> > > > operational=false. This way, drivers should know when they can perform
> > > > Ethernet-based register access, and need not care about more than that.
> > > > 
> > > > Changes in v2:
> > > > - dropped some useless patches
> > > > - also check master operstate.
> > > > 
> > > > Vladimir Oltean (4):
> > > >   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  | 11 +++++++
> > > >  net/dsa/dsa2.c     | 80 +++++++++++++++++++++++++++++++++++++++++++---
> > > >  net/dsa/dsa_priv.h | 13 ++++++++
> > > >  net/dsa/master.c   | 29 ++---------------
> > > >  net/dsa/slave.c    | 27 ++++++++++++++++
> > > >  net/dsa/switch.c   | 15 +++++++++
> > > >  6 files changed, 145 insertions(+), 30 deletions(-)
> > > > 
> > > > -- 
> > > > 2.25.1
> > > > 
> > > 
> > > Hi, I tested this v2 and I still have 2 ethernet mdio failing on init.
> > > I don't think we have other way to track this. Am I wrong?
> > > 
> > > All works correctly with this and promisc_on_master.
> > > If you have other test, feel free to send me other stuff to test.
> > > 
> > > (I'm starting to think the fail is caused by some delay that the switch
> > > require to actually start accepting packet or from the reinit? But I'm
> > > not sure... don't know if you notice something from the pcap)
> > 
> > I've opened the pcap just now. The Ethernet MDIO packets are
> > non-standard. When the DSA master receives them, it expects the first 6
> > octets to be the MAC DA, because that's the format of an Ethernet frame.
> > But the packets have this other format, according to your own writing:
> > 
> > /* Specific define for in-band MDIO read/write with Ethernet packet */
> > #define QCA_HDR_MDIO_SEQ_LEN           4 /* 4 byte for the seq */
> > #define QCA_HDR_MDIO_COMMAND_LEN       4 /* 4 byte for the command */
> > #define QCA_HDR_MDIO_DATA1_LEN         4 /* First 4 byte for the mdio data */
> > #define QCA_HDR_MDIO_HEADER_LEN        (QCA_HDR_MDIO_SEQ_LEN + \
> >                                        QCA_HDR_MDIO_COMMAND_LEN + \
> >                                        QCA_HDR_MDIO_DATA1_LEN)
> > 
> > #define QCA_HDR_MDIO_DATA2_LEN         12 /* Other 12 byte for the mdio data */
> > #define QCA_HDR_MDIO_PADDING_LEN       34 /* Padding to reach the min Ethernet packet */
> > 
> > The first 6 octets change like crazy in your pcap. Definitely can't add
> > that to the RX filter of the DSA master.
> > 
> > So yes, promisc_on_master is precisely what you need, it exists for
> > situations like this.
> > 
> > Considering this, I guess it works?
> 
> Yes it works! We can totally accept 2 mdio timeout out of a good way to
> track the master port. It's probably related to other stuff like switch
> delay or other.
> 
> Wonder the next step is wait for this to be accepted and then I can
> propose a v3 of my patch? Or net-next is closed now and I should just
> send v3 RFC saying it does depend on this?

Wait a minute, I don't think I understood your previous reply.
With promisc_on_master, is there or is there not any timeout?
My understanding was this: DSA tells you when the master is up and
operational. That information is correct, except it isn't sufficient and
you don't see the replies back. Later during boot, you have some init
scripts triggered by user space that create a bridge interface and put
the switch ports under the bridge. The bridge puts the switch interfaces
in promiscuous mode, because that's what bridges do. Then DSA propagates
the promiscuous mode from the switch ports to the DSA master, and once
the master is promiscuous, the Ethernet MDIO starts working too.
Now, with promisc_on_master set, the DSA master is already promiscuous
by the time DSA tells you that it's up and running. Hence your message
that "All works correctly with this and promisc_on_master."
What did I misunderstand?

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

[Ansuel Smith]

On Fri, Dec 10, 2021 at 05:15:30PM +0000, Vladimir Oltean wrote:
> On Fri, Dec 10, 2021 at 06:10:45PM +0100, Ansuel Smith wrote:
> > On Fri, Dec 10, 2021 at 05:02:42PM +0000, Vladimir Oltean wrote:
> > > On Fri, Dec 10, 2021 at 04:37:52AM +0100, Ansuel Smith wrote:
> > > > On Thu, Dec 09, 2021 at 07:39:23PM +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/
> > > > > 
> > > > > The patches posted here are mainly to offer a consistent
> > > > > "master_state_change" chain of events to switches, without duplicates,
> > > > > and always starting with operational=true and ending with
> > > > > operational=false. This way, drivers should know when they can perform
> > > > > Ethernet-based register access, and need not care about more than that.
> > > > > 
> > > > > Changes in v2:
> > > > > - dropped some useless patches
> > > > > - also check master operstate.
> > > > > 
> > > > > Vladimir Oltean (4):
> > > > >   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  | 11 +++++++
> > > > >  net/dsa/dsa2.c     | 80 +++++++++++++++++++++++++++++++++++++++++++---
> > > > >  net/dsa/dsa_priv.h | 13 ++++++++
> > > > >  net/dsa/master.c   | 29 ++---------------
> > > > >  net/dsa/slave.c    | 27 ++++++++++++++++
> > > > >  net/dsa/switch.c   | 15 +++++++++
> > > > >  6 files changed, 145 insertions(+), 30 deletions(-)
> > > > > 
> > > > > -- 
> > > > > 2.25.1
> > > > > 
> > > > 
> > > > Hi, I tested this v2 and I still have 2 ethernet mdio failing on init.
> > > > I don't think we have other way to track this. Am I wrong?
> > > > 
> > > > All works correctly with this and promisc_on_master.
> > > > If you have other test, feel free to send me other stuff to test.
> > > > 
> > > > (I'm starting to think the fail is caused by some delay that the switch
> > > > require to actually start accepting packet or from the reinit? But I'm
> > > > not sure... don't know if you notice something from the pcap)
> > > 
> > > I've opened the pcap just now. The Ethernet MDIO packets are
> > > non-standard. When the DSA master receives them, it expects the first 6
> > > octets to be the MAC DA, because that's the format of an Ethernet frame.
> > > But the packets have this other format, according to your own writing:
> > > 
> > > /* Specific define for in-band MDIO read/write with Ethernet packet */
> > > #define QCA_HDR_MDIO_SEQ_LEN           4 /* 4 byte for the seq */
> > > #define QCA_HDR_MDIO_COMMAND_LEN       4 /* 4 byte for the command */
> > > #define QCA_HDR_MDIO_DATA1_LEN         4 /* First 4 byte for the mdio data */
> > > #define QCA_HDR_MDIO_HEADER_LEN        (QCA_HDR_MDIO_SEQ_LEN + \
> > >                                        QCA_HDR_MDIO_COMMAND_LEN + \
> > >                                        QCA_HDR_MDIO_DATA1_LEN)
> > > 
> > > #define QCA_HDR_MDIO_DATA2_LEN         12 /* Other 12 byte for the mdio data */
> > > #define QCA_HDR_MDIO_PADDING_LEN       34 /* Padding to reach the min Ethernet packet */
> > > 
> > > The first 6 octets change like crazy in your pcap. Definitely can't add
> > > that to the RX filter of the DSA master.
> > > 
> > > So yes, promisc_on_master is precisely what you need, it exists for
> > > situations like this.
> > > 
> > > Considering this, I guess it works?
> > 
> > Yes it works! We can totally accept 2 mdio timeout out of a good way to
> > track the master port. It's probably related to other stuff like switch
> > delay or other.
> > 
> > Wonder the next step is wait for this to be accepted and then I can
> > propose a v3 of my patch? Or net-next is closed now and I should just
> > send v3 RFC saying it does depend on this?
> 
> Wait a minute, I don't think I understood your previous reply.
> With promisc_on_master, is there or is there not any timeout?

With promisc_on_master I have only 2 timeout.

> My understanding was this: DSA tells you when the master is up and
> operational. That information is correct, except it isn't sufficient and
> you don't see the replies back. Later during boot, you have some init
> scripts triggered by user space that create a bridge interface and put
> the switch ports under the bridge. The bridge puts the switch interfaces
> in promiscuous mode, because that's what bridges do. Then DSA propagates
> the promiscuous mode from the switch ports to the DSA master, and once
> the master is promiscuous, the Ethernet MDIO starts working too.
> Now, with promisc_on_master set, the DSA master is already promiscuous
> by the time DSA tells you that it's up and running. Hence your message
> that "All works correctly with this and promisc_on_master."
> What did I misunderstand?

You got all correct. But still I have these 2 timeout at the very start.
Let me give you another pastebin to make this more clear. [0]
Transaction done is when the Ethernet packet is received and processed.
I added some pr with the events received by switch.c

I should check if the tagger receive some packet before the
"function timeout". 
What I mean with "acceptable state" is that aside from the 2
timeout everything else works correctly withtout any slowdown in the
init process.

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

-- 
	Ansuel

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

[Ansuel Smith]

On Fri, Dec 10, 2021 at 06:29:32PM +0100, Ansuel Smith wrote:
> On Fri, Dec 10, 2021 at 05:15:30PM +0000, Vladimir Oltean wrote:
> > On Fri, Dec 10, 2021 at 06:10:45PM +0100, Ansuel Smith wrote:
> > > On Fri, Dec 10, 2021 at 05:02:42PM +0000, Vladimir Oltean wrote:
> > > > On Fri, Dec 10, 2021 at 04:37:52AM +0100, Ansuel Smith wrote:
> > > > > On Thu, Dec 09, 2021 at 07:39:23PM +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/
> > > > > > 
> > > > > > The patches posted here are mainly to offer a consistent
> > > > > > "master_state_change" chain of events to switches, without duplicates,
> > > > > > and always starting with operational=true and ending with
> > > > > > operational=false. This way, drivers should know when they can perform
> > > > > > Ethernet-based register access, and need not care about more than that.
> > > > > > 
> > > > > > Changes in v2:
> > > > > > - dropped some useless patches
> > > > > > - also check master operstate.
> > > > > > 
> > > > > > Vladimir Oltean (4):
> > > > > >   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  | 11 +++++++
> > > > > >  net/dsa/dsa2.c     | 80 +++++++++++++++++++++++++++++++++++++++++++---
> > > > > >  net/dsa/dsa_priv.h | 13 ++++++++
> > > > > >  net/dsa/master.c   | 29 ++---------------
> > > > > >  net/dsa/slave.c    | 27 ++++++++++++++++
> > > > > >  net/dsa/switch.c   | 15 +++++++++
> > > > > >  6 files changed, 145 insertions(+), 30 deletions(-)
> > > > > > 
> > > > > > -- 
> > > > > > 2.25.1
> > > > > > 
> > > > > 
> > > > > Hi, I tested this v2 and I still have 2 ethernet mdio failing on init.
> > > > > I don't think we have other way to track this. Am I wrong?
> > > > > 
> > > > > All works correctly with this and promisc_on_master.
> > > > > If you have other test, feel free to send me other stuff to test.
> > > > > 
> > > > > (I'm starting to think the fail is caused by some delay that the switch
> > > > > require to actually start accepting packet or from the reinit? But I'm
> > > > > not sure... don't know if you notice something from the pcap)
> > > > 
> > > > I've opened the pcap just now. The Ethernet MDIO packets are
> > > > non-standard. When the DSA master receives them, it expects the first 6
> > > > octets to be the MAC DA, because that's the format of an Ethernet frame.
> > > > But the packets have this other format, according to your own writing:
> > > > 
> > > > /* Specific define for in-band MDIO read/write with Ethernet packet */
> > > > #define QCA_HDR_MDIO_SEQ_LEN           4 /* 4 byte for the seq */
> > > > #define QCA_HDR_MDIO_COMMAND_LEN       4 /* 4 byte for the command */
> > > > #define QCA_HDR_MDIO_DATA1_LEN         4 /* First 4 byte for the mdio data */
> > > > #define QCA_HDR_MDIO_HEADER_LEN        (QCA_HDR_MDIO_SEQ_LEN + \
> > > >                                        QCA_HDR_MDIO_COMMAND_LEN + \
> > > >                                        QCA_HDR_MDIO_DATA1_LEN)
> > > > 
> > > > #define QCA_HDR_MDIO_DATA2_LEN         12 /* Other 12 byte for the mdio data */
> > > > #define QCA_HDR_MDIO_PADDING_LEN       34 /* Padding to reach the min Ethernet packet */
> > > > 
> > > > The first 6 octets change like crazy in your pcap. Definitely can't add
> > > > that to the RX filter of the DSA master.
> > > > 
> > > > So yes, promisc_on_master is precisely what you need, it exists for
> > > > situations like this.
> > > > 
> > > > Considering this, I guess it works?
> > > 
> > > Yes it works! We can totally accept 2 mdio timeout out of a good way to
> > > track the master port. It's probably related to other stuff like switch
> > > delay or other.
> > > 
> > > Wonder the next step is wait for this to be accepted and then I can
> > > propose a v3 of my patch? Or net-next is closed now and I should just
> > > send v3 RFC saying it does depend on this?
> > 
> > Wait a minute, I don't think I understood your previous reply.
> > With promisc_on_master, is there or is there not any timeout?
> 
> With promisc_on_master I have only 2 timeout.
> 
> > My understanding was this: DSA tells you when the master is up and
> > operational. That information is correct, except it isn't sufficient and
> > you don't see the replies back. Later during boot, you have some init
> > scripts triggered by user space that create a bridge interface and put
> > the switch ports under the bridge. The bridge puts the switch interfaces
> > in promiscuous mode, because that's what bridges do. Then DSA propagates
> > the promiscuous mode from the switch ports to the DSA master, and once
> > the master is promiscuous, the Ethernet MDIO starts working too.
> > Now, with promisc_on_master set, the DSA master is already promiscuous
> > by the time DSA tells you that it's up and running. Hence your message
> > that "All works correctly with this and promisc_on_master."
> > What did I misunderstand?
> 
> You got all correct. But still I have these 2 timeout at the very start.
> Let me give you another pastebin to make this more clear. [0]
> Transaction done is when the Ethernet packet is received and processed.
> I added some pr with the events received by switch.c
> 
> I should check if the tagger receive some packet before the
> "function timeout". 
> What I mean with "acceptable state" is that aside from the 2
> timeout everything else works correctly withtout any slowdown in the
> init process.
> 
> [0] https://pastebin.com/VfGB5hAQ
> 
> -- 
> 	Ansuel

Ok I added more tracing and packet are received to the tagger right
after the log from ipv6 "link becomes ready". That log just check if the
interface is up and if it does have a valid sched.
I notice after link becomes ready we have a CHANGE event for eth0. That
should be the correct way to understand when the cpu port is actually
usable.
(just to make it clear before the link becomes ready no packet is
received to the tagger and the completion timeouts)

-- 
	Ansuel

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

[Ansuel Smith]

On Fri, Dec 10, 2021 at 07:04:48PM +0100, Ansuel Smith wrote:
> On Fri, Dec 10, 2021 at 06:29:32PM +0100, Ansuel Smith wrote:
> > On Fri, Dec 10, 2021 at 05:15:30PM +0000, Vladimir Oltean wrote:
> > > On Fri, Dec 10, 2021 at 06:10:45PM +0100, Ansuel Smith wrote:
> > > > On Fri, Dec 10, 2021 at 05:02:42PM +0000, Vladimir Oltean wrote:
> > > > > On Fri, Dec 10, 2021 at 04:37:52AM +0100, Ansuel Smith wrote:
> > > > > > On Thu, Dec 09, 2021 at 07:39:23PM +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/
> > > > > > > 
> > > > > > > The patches posted here are mainly to offer a consistent
> > > > > > > "master_state_change" chain of events to switches, without duplicates,
> > > > > > > and always starting with operational=true and ending with
> > > > > > > operational=false. This way, drivers should know when they can perform
> > > > > > > Ethernet-based register access, and need not care about more than that.
> > > > > > > 
> > > > > > > Changes in v2:
> > > > > > > - dropped some useless patches
> > > > > > > - also check master operstate.
> > > > > > > 
> > > > > > > Vladimir Oltean (4):
> > > > > > >   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  | 11 +++++++
> > > > > > >  net/dsa/dsa2.c     | 80 +++++++++++++++++++++++++++++++++++++++++++---
> > > > > > >  net/dsa/dsa_priv.h | 13 ++++++++
> > > > > > >  net/dsa/master.c   | 29 ++---------------
> > > > > > >  net/dsa/slave.c    | 27 ++++++++++++++++
> > > > > > >  net/dsa/switch.c   | 15 +++++++++
> > > > > > >  6 files changed, 145 insertions(+), 30 deletions(-)
> > > > > > > 
> > > > > > > -- 
> > > > > > > 2.25.1
> > > > > > > 
> > > > > > 
> > > > > > Hi, I tested this v2 and I still have 2 ethernet mdio failing on init.
> > > > > > I don't think we have other way to track this. Am I wrong?
> > > > > > 
> > > > > > All works correctly with this and promisc_on_master.
> > > > > > If you have other test, feel free to send me other stuff to test.
> > > > > > 
> > > > > > (I'm starting to think the fail is caused by some delay that the switch
> > > > > > require to actually start accepting packet or from the reinit? But I'm
> > > > > > not sure... don't know if you notice something from the pcap)
> > > > > 
> > > > > I've opened the pcap just now. The Ethernet MDIO packets are
> > > > > non-standard. When the DSA master receives them, it expects the first 6
> > > > > octets to be the MAC DA, because that's the format of an Ethernet frame.
> > > > > But the packets have this other format, according to your own writing:
> > > > > 
> > > > > /* Specific define for in-band MDIO read/write with Ethernet packet */
> > > > > #define QCA_HDR_MDIO_SEQ_LEN           4 /* 4 byte for the seq */
> > > > > #define QCA_HDR_MDIO_COMMAND_LEN       4 /* 4 byte for the command */
> > > > > #define QCA_HDR_MDIO_DATA1_LEN         4 /* First 4 byte for the mdio data */
> > > > > #define QCA_HDR_MDIO_HEADER_LEN        (QCA_HDR_MDIO_SEQ_LEN + \
> > > > >                                        QCA_HDR_MDIO_COMMAND_LEN + \
> > > > >                                        QCA_HDR_MDIO_DATA1_LEN)
> > > > > 
> > > > > #define QCA_HDR_MDIO_DATA2_LEN         12 /* Other 12 byte for the mdio data */
> > > > > #define QCA_HDR_MDIO_PADDING_LEN       34 /* Padding to reach the min Ethernet packet */
> > > > > 
> > > > > The first 6 octets change like crazy in your pcap. Definitely can't add
> > > > > that to the RX filter of the DSA master.
> > > > > 
> > > > > So yes, promisc_on_master is precisely what you need, it exists for
> > > > > situations like this.
> > > > > 
> > > > > Considering this, I guess it works?
> > > > 
> > > > Yes it works! We can totally accept 2 mdio timeout out of a good way to
> > > > track the master port. It's probably related to other stuff like switch
> > > > delay or other.
> > > > 
> > > > Wonder the next step is wait for this to be accepted and then I can
> > > > propose a v3 of my patch? Or net-next is closed now and I should just
> > > > send v3 RFC saying it does depend on this?
> > > 
> > > Wait a minute, I don't think I understood your previous reply.
> > > With promisc_on_master, is there or is there not any timeout?
> > 
> > With promisc_on_master I have only 2 timeout.
> > 
> > > My understanding was this: DSA tells you when the master is up and
> > > operational. That information is correct, except it isn't sufficient and
> > > you don't see the replies back. Later during boot, you have some init
> > > scripts triggered by user space that create a bridge interface and put
> > > the switch ports under the bridge. The bridge puts the switch interfaces
> > > in promiscuous mode, because that's what bridges do. Then DSA propagates
> > > the promiscuous mode from the switch ports to the DSA master, and once
> > > the master is promiscuous, the Ethernet MDIO starts working too.
> > > Now, with promisc_on_master set, the DSA master is already promiscuous
> > > by the time DSA tells you that it's up and running. Hence your message
> > > that "All works correctly with this and promisc_on_master."
> > > What did I misunderstand?
> > 
> > You got all correct. But still I have these 2 timeout at the very start.
> > Let me give you another pastebin to make this more clear. [0]
> > Transaction done is when the Ethernet packet is received and processed.
> > I added some pr with the events received by switch.c
> > 
> > I should check if the tagger receive some packet before the
> > "function timeout". 
> > What I mean with "acceptable state" is that aside from the 2
> > timeout everything else works correctly withtout any slowdown in the
> > init process.
> > 
> > [0] https://pastebin.com/VfGB5hAQ
> > 
> > -- 
> > 	Ansuel
> 
> Ok I added more tracing and packet are received to the tagger right
> after the log from ipv6 "link becomes ready". That log just check if the
> interface is up and if it does have a valid sched.
> I notice after link becomes ready we have a CHANGE event for eth0. That
> should be the correct way to understand when the cpu port is actually
> usable.
> (just to make it clear before the link becomes ready no packet is
> received to the tagger and the completion timeouts)
> 
> -- 
> 	Ansuel

Sorry for the triple message spam... I have a solution. It seems packet
are processed as soon as dev_activate is called (so a qdisk is assigned)
By adding another bool like master_oper_ready and

void dsa_tree_master_oper_state_ready(struct dsa_switch_tree *dst,
                                      struct net_device *master,
                                      bool up);

static void dsa_tree_master_state_change(struct dsa_switch_tree *dst,
                                        struct net_device *master)
{
       struct dsa_notifier_master_state_info info;
       struct dsa_port *cpu_dp = master->dsa_ptr;

       info.master = master;
       info.operational = cpu_dp->master_admin_up && cpu_dp->master_oper_up && cpu_dp->master_oper_ready;

       dsa_tree_notify(dst, DSA_NOTIFIER_MASTER_STATE_CHANGE, &info);
}

void dsa_tree_master_oper_state_ready(struct dsa_switch_tree *dst,
                                      struct net_device *master,
                                      bool up)
{
       struct dsa_port *cpu_dp = master->dsa_ptr;
       bool notify = false;

       if ((cpu_dp->master_oper_ready && cpu_dp->master_oper_ready) !=
           (cpu_dp->master_oper_ready && up))
               notify = true;

       cpu_dp->master_oper_ready = up;

       if (notify)
               dsa_tree_master_state_change(dst, master);
}

In slave.c at the NETDEV_CHANGE event the additional
dsa_tree_master_oper_state_ready(dst, dev, dev_ingress_queue(dev));
we have no timeout function. I just tested this and it works right away.

Think we need this additional check to make sure the tagger can finally
accept packet from the switch.

With this added I think this is ready.

-- 
	Ansuel

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

[Vladimir Oltean]

On Fri, Dec 10, 2021 at 08:10:21PM +0100, Ansuel Smith wrote:
> > Ok I added more tracing and packet are received to the tagger right
> > after the log from ipv6 "link becomes ready". That log just check if the
> > interface is up and if it does have a valid sched.
> > I notice after link becomes ready we have a CHANGE event for eth0. That
> > should be the correct way to understand when the cpu port is actually
> > usable.
> > (just to make it clear before the link becomes ready no packet is
> > received to the tagger and the completion timeouts)
> > 
> > -- 
> > 	Ansuel
> 
> Sorry for the triple message spam... I have a solution. It seems packet
> are processed as soon as dev_activate is called (so a qdisk is assigned)
> By adding another bool like master_oper_ready and
> 
> void dsa_tree_master_oper_state_ready(struct dsa_switch_tree *dst,
>                                       struct net_device *master,
>                                       bool up);
> 
> static void dsa_tree_master_state_change(struct dsa_switch_tree *dst,
>                                         struct net_device *master)
> {
>        struct dsa_notifier_master_state_info info;
>        struct dsa_port *cpu_dp = master->dsa_ptr;
> 
>        info.master = master;
>        info.operational = cpu_dp->master_admin_up && cpu_dp->master_oper_up && cpu_dp->master_oper_ready;
> 
>        dsa_tree_notify(dst, DSA_NOTIFIER_MASTER_STATE_CHANGE, &info);
> }
> 
> void dsa_tree_master_oper_state_ready(struct dsa_switch_tree *dst,
>                                       struct net_device *master,
>                                       bool up)
> {
>        struct dsa_port *cpu_dp = master->dsa_ptr;
>        bool notify = false;
> 
>        if ((cpu_dp->master_oper_ready && cpu_dp->master_oper_ready) !=
>            (cpu_dp->master_oper_ready && up))
>                notify = true;
> 
>        cpu_dp->master_oper_ready = up;
> 
>        if (notify)
>                dsa_tree_master_state_change(dst, master);
> }
> 
> In slave.c at the NETDEV_CHANGE event the additional
> dsa_tree_master_oper_state_ready(dst, dev, dev_ingress_queue(dev));
> we have no timeout function. I just tested this and it works right away.
> 
> Think we need this additional check to make sure the tagger can finally
> accept packet from the switch.
> 
> With this added I think this is ready.

Why ingress_queue?
I was looking at dev_activate() too, especially since net/ipv6/addrconf.c uses:

/* Check if link is ready: is it up and is a valid qdisc available */
static inline bool addrconf_link_ready(const struct net_device *dev)
{
	return netif_oper_up(dev) && !qdisc_tx_is_noop(dev);
}

and you can see that qdisc_tx_is_noop() checks for the qdisc on TX
queues, not ingress qdisc (which makes more sense anyway).

Anyway the reason why I didn't say anything about this is because I
don't yet understand how it is supposed to work. Specifically:

rtnl_lock

dev_open()
-> __dev_open()
   -> dev->flags |= IFF_UP;
   -> dev_activate()
      -> transition_one_qdisc()
-> call_netdevice_notifiers(NETDEV_UP, dev);

rtnl_unlock

so the qdisc should have already transitioned by the time NETDEV_UP is
emitted.

and since we already require a NETDEV_UP to have occurred, or dev->flags
to contain IFF_UP, I simply don't understand the following
(a) why would the qdisc be noop when we catch NETDEV_UP
(b) who calls netdev_state_change() (or __dev_notify_flags ?!) after the
    qdisc changes on a TX queue? If no one, then I'm not sure how we can
    reliably check for the state of the qdisc if we aren't notified
    about changes to it.

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

[Ansuel Smith]

On Fri, Dec 10, 2021 at 07:27:24PM +0000, Vladimir Oltean wrote:
> On Fri, Dec 10, 2021 at 08:10:21PM +0100, Ansuel Smith wrote:
> > > Ok I added more tracing and packet are received to the tagger right
> > > after the log from ipv6 "link becomes ready". That log just check if the
> > > interface is up and if it does have a valid sched.
> > > I notice after link becomes ready we have a CHANGE event for eth0. That
> > > should be the correct way to understand when the cpu port is actually
> > > usable.
> > > (just to make it clear before the link becomes ready no packet is
> > > received to the tagger and the completion timeouts)
> > > 
> > > -- 
> > > 	Ansuel
> > 
> > Sorry for the triple message spam... I have a solution. It seems packet
> > are processed as soon as dev_activate is called (so a qdisk is assigned)
> > By adding another bool like master_oper_ready and
> > 
> > void dsa_tree_master_oper_state_ready(struct dsa_switch_tree *dst,
> >                                       struct net_device *master,
> >                                       bool up);
> > 
> > static void dsa_tree_master_state_change(struct dsa_switch_tree *dst,
> >                                         struct net_device *master)
> > {
> >        struct dsa_notifier_master_state_info info;
> >        struct dsa_port *cpu_dp = master->dsa_ptr;
> > 
> >        info.master = master;
> >        info.operational = cpu_dp->master_admin_up && cpu_dp->master_oper_up && cpu_dp->master_oper_ready;
> > 
> >        dsa_tree_notify(dst, DSA_NOTIFIER_MASTER_STATE_CHANGE, &info);
> > }
> > 
> > void dsa_tree_master_oper_state_ready(struct dsa_switch_tree *dst,
> >                                       struct net_device *master,
> >                                       bool up)
> > {
> >        struct dsa_port *cpu_dp = master->dsa_ptr;
> >        bool notify = false;
> > 
> >        if ((cpu_dp->master_oper_ready && cpu_dp->master_oper_ready) !=
> >            (cpu_dp->master_oper_ready && up))
> >                notify = true;
> > 
> >        cpu_dp->master_oper_ready = up;
> > 
> >        if (notify)
> >                dsa_tree_master_state_change(dst, master);
> > }
> > 
> > In slave.c at the NETDEV_CHANGE event the additional
> > dsa_tree_master_oper_state_ready(dst, dev, dev_ingress_queue(dev));
> > we have no timeout function. I just tested this and it works right away.
> > 
> > Think we need this additional check to make sure the tagger can finally
> > accept packet from the switch.
> > 
> > With this added I think this is ready.
> 
> Why ingress_queue?
> I was looking at dev_activate() too, especially since net/ipv6/addrconf.c uses:
> 
> /* Check if link is ready: is it up and is a valid qdisc available */
> static inline bool addrconf_link_ready(const struct net_device *dev)
> {
> 	return netif_oper_up(dev) && !qdisc_tx_is_noop(dev);
> }
> 
> and you can see that qdisc_tx_is_noop() checks for the qdisc on TX
> queues, not ingress qdisc (which makes more sense anyway).
> 
> Anyway the reason why I didn't say anything about this is because I
> don't yet understand how it is supposed to work. Specifically:
> 
> rtnl_lock
> 
> dev_open()
> -> __dev_open()
>    -> dev->flags |= IFF_UP;
>    -> dev_activate()
>       -> transition_one_qdisc()
> -> call_netdevice_notifiers(NETDEV_UP, dev);
> 
> rtnl_unlock
> 
> so the qdisc should have already transitioned by the time NETDEV_UP is
> emitted.
> 
> and since we already require a NETDEV_UP to have occurred, or dev->flags
> to contain IFF_UP, I simply don't understand the following
> (a) why would the qdisc be noop when we catch NETDEV_UP
> (b) who calls netdev_state_change() (or __dev_notify_flags ?!) after the
>     qdisc changes on a TX queue? If no one, then I'm not sure how we can
>     reliably check for the state of the qdisc if we aren't notified
>     about changes to it.

The ipv6 check is just a hint. The real clue was the second
NETDEV_CHANGE called by linkwatch_do_dev in link_watch.c
That is the one that calls the CHANGE event before the ready stuff.

I had problem tracking this as the change logic is "emit CHANGE when flags
change" but netdev_state_change is also called for other reason and one
example is dev_activate/dev_deactivate from linkwatch_do_dev.
It seems a bit confusing that a generic state change is called even when
flags are not changed and because of this is a bit problematic track why
the CHANGE event was called.

Wonder if linkwatch_do_dev should be changed and introduce a flag? But
that seems problematic if for whatever reason a driver use the CHANGE
event to track exactly dev_activate/deactivate.

-- 
	Ansuel

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

[Vladimir Oltean]

On Fri, Dec 10, 2021 at 08:45:43PM +0100, Ansuel Smith wrote:
> > Anyway the reason why I didn't say anything about this is because I
> > don't yet understand how it is supposed to work. Specifically:
> > 
> > rtnl_lock
> > 
> > dev_open()
> > -> __dev_open()
> >    -> dev->flags |= IFF_UP;
> >    -> dev_activate()
> >       -> transition_one_qdisc()
> > -> call_netdevice_notifiers(NETDEV_UP, dev);
> > 
> > rtnl_unlock
> > 
> > so the qdisc should have already transitioned by the time NETDEV_UP is
> > emitted.
> > 
> > and since we already require a NETDEV_UP to have occurred, or dev->flags
> > to contain IFF_UP, I simply don't understand the following
> > (a) why would the qdisc be noop when we catch NETDEV_UP
> > (b) who calls netdev_state_change() (or __dev_notify_flags ?!) after the
> >     qdisc changes on a TX queue? If no one, then I'm not sure how we can
> >     reliably check for the state of the qdisc if we aren't notified
> >     about changes to it.
> 
> The ipv6 check is just a hint. The real clue was the second
> NETDEV_CHANGE called by linkwatch_do_dev in link_watch.c
> That is the one that calls the CHANGE event before the ready stuff.
> 
> I had problem tracking this as the change logic is "emit CHANGE when flags
> change" but netdev_state_change is also called for other reason and one
> example is dev_activate/dev_deactivate from linkwatch_do_dev.
> It seems a bit confusing that a generic state change is called even when
> flags are not changed and because of this is a bit problematic track why
> the CHANGE event was called.
> 
> Wonder if linkwatch_do_dev should be changed and introduce a flag? But
> that seems problematic if for whatever reason a driver use the CHANGE
> event to track exactly dev_activate/deactivate.

Yes, I had my own "aha" moment just minutes before you sent this email
about linkwatch_do_dev. So indeed that's the source of both the
dev_activate(), as well as the netdev_state_change() notifier.

As to my previous question (why would the qdisc be noop when we catch
NETDEV_UP): the answer is of course in the code as well:

dev_activate() has:
	if (!netif_carrier_ok(dev))
		/* Delay activation until next carrier-on event */
		return;

which is then actually picked up from linkwatch_do_dev().

Let's not change linkwatch_do_dev(), I just wanted to understand why it
works. Please confirm that it also works for you to make master_admin_up
depend on qdisc_tx_is_noop() instead of the current ingress_queue check,
then add a comment stating the mechanism through which we are tracking
the dev_activate() calls, and then this should be good to go.
I'd like you to pick up the patches and post them together with your
driver changes. I can't post the patches on my own since I don't have
any use for them. I'll leave a few more "review" comments on them in a
minute.

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

[Ansuel Smith]

On Fri, Dec 10, 2021 at 07:54:42PM +0000, Vladimir Oltean wrote:
> On Fri, Dec 10, 2021 at 08:45:43PM +0100, Ansuel Smith wrote:
> > > Anyway the reason why I didn't say anything about this is because I
> > > don't yet understand how it is supposed to work. Specifically:
> > > 
> > > rtnl_lock
> > > 
> > > dev_open()
> > > -> __dev_open()
> > >    -> dev->flags |= IFF_UP;
> > >    -> dev_activate()
> > >       -> transition_one_qdisc()
> > > -> call_netdevice_notifiers(NETDEV_UP, dev);
> > > 
> > > rtnl_unlock
> > > 
> > > so the qdisc should have already transitioned by the time NETDEV_UP is
> > > emitted.
> > > 
> > > and since we already require a NETDEV_UP to have occurred, or dev->flags
> > > to contain IFF_UP, I simply don't understand the following
> > > (a) why would the qdisc be noop when we catch NETDEV_UP
> > > (b) who calls netdev_state_change() (or __dev_notify_flags ?!) after the
> > >     qdisc changes on a TX queue? If no one, then I'm not sure how we can
> > >     reliably check for the state of the qdisc if we aren't notified
> > >     about changes to it.
> > 
> > The ipv6 check is just a hint. The real clue was the second
> > NETDEV_CHANGE called by linkwatch_do_dev in link_watch.c
> > That is the one that calls the CHANGE event before the ready stuff.
> > 
> > I had problem tracking this as the change logic is "emit CHANGE when flags
> > change" but netdev_state_change is also called for other reason and one
> > example is dev_activate/dev_deactivate from linkwatch_do_dev.
> > It seems a bit confusing that a generic state change is called even when
> > flags are not changed and because of this is a bit problematic track why
> > the CHANGE event was called.
> > 
> > Wonder if linkwatch_do_dev should be changed and introduce a flag? But
> > that seems problematic if for whatever reason a driver use the CHANGE
> > event to track exactly dev_activate/deactivate.
> 
> Yes, I had my own "aha" moment just minutes before you sent this email
> about linkwatch_do_dev. So indeed that's the source of both the
> dev_activate(), as well as the netdev_state_change() notifier.
> 
> As to my previous question (why would the qdisc be noop when we catch
> NETDEV_UP): the answer is of course in the code as well:
> 
> dev_activate() has:
> 	if (!netif_carrier_ok(dev))
> 		/* Delay activation until next carrier-on event */
> 		return;
> 
> which is then actually picked up from linkwatch_do_dev().
> 
> Let's not change linkwatch_do_dev(), I just wanted to understand why it
> works. Please confirm that it also works for you to make master_admin_up
> depend on qdisc_tx_is_noop() instead of the current ingress_queue check,
> then add a comment stating the mechanism through which we are tracking
> the dev_activate() calls, and then this should be good to go.
> I'd like you to pick up the patches and post them together with your
> driver changes. I can't post the patches on my own since I don't have
> any use for them. I'll leave a few more "review" comments on them in a
> minute.

Ok will do the test, but I'm positive about that.
So the idea is to send a v3 rfc with the depends of the tagger-owned
private data. Add to my series your series with this extra check.
(when I will post v3 feel free to tell me if I messed code credits)

Is the additional bool and function correct or should we merge them and
assume a link up only when we both have the flag and the qdisc?

-- 
	Ansuel