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

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

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
>