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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

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
>