Re: [PATCH net] net: Have netpoll bring-up DSA management interface

[Vladimir Oltean]

On Mon, Oct 19, 2020 at 10:17:44AM -0700, Florian Fainelli wrote:
> These devices also do not utilize the upper/lower linking so the
> check about the netpoll device having upper is not going to be a
> problem.

They do as of 2f1e8ea726e9 ("net: dsa: link interfaces with the DSA
master to get rid of lockdep warnings"), don't they? The question is why
that doesn't work, and the answer is, I believe, that the linkage needs
to be the other way around than DSA has it.

> 
> The solution adopted here is identical to the one done for
> net/ipv4/ipconfig.c with 728c02089a0e ("net: ipv4: handle DSA enabled
> master network devices"), with the network namespace scope being
> restricted to that of the process configuring netpoll.

... and further restricted to the only network namespace that DSA
supports. As a side note, we should declare NETIF_F_NETNS_LOCAL_BIT for
DSA interfaces.

> 
> Fixes: 04ff53f96a93 ("net: dsa: Add netconsole support")
> Signed-off-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
> ---
>  net/core/netpoll.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index c310c7c1cef7..960948290001 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -29,6 +29,7 @@
>  #include <linux/slab.h>
>  #include <linux/export.h>
>  #include <linux/if_vlan.h>
> +#include <net/dsa.h>
>  #include <net/tcp.h>
>  #include <net/udp.h>
>  #include <net/addrconf.h>
> @@ -657,15 +658,15 @@ EXPORT_SYMBOL_GPL(__netpoll_setup);
>  
>  int netpoll_setup(struct netpoll *np)
>  {
> -	struct net_device *ndev = NULL;
> +	struct net_device *ndev = NULL, *dev = NULL;
> +	struct net *net = current->nsproxy->net_ns;
>  	struct in_device *in_dev;
>  	int err;
>  
>  	rtnl_lock();
> -	if (np->dev_name[0]) {
> -		struct net *net = current->nsproxy->net_ns;
> +	if (np->dev_name[0])
>  		ndev = __dev_get_by_name(net, np->dev_name);
> -	}
> +
>  	if (!ndev) {
>  		np_err(np, "%s doesn't exist, aborting\n", np->dev_name);
>  		err = -ENODEV;
> @@ -673,6 +674,19 @@ int netpoll_setup(struct netpoll *np)
>  	}
>  	dev_hold(ndev);
>  
> +	/* bring up DSA management network devices up first */
> +	for_each_netdev(net, dev) {
> +		if (!netdev_uses_dsa(dev))
> +			continue;
> +
> +		err = dev_change_flags(dev, dev->flags | IFF_UP, NULL);
> +		if (err < 0) {
> +			np_err(np, "%s failed to open %s\n",
> +			       np->dev_name, dev->name);
> +			goto put;
> +		}
> +	}
> +

Completely crazy and outlandish idea, I know, but what's wrong with
doing this in DSA?

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 33788b5c1742..e5927c4498a2 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -68,8 +68,14 @@ static int dsa_slave_open(struct net_device *dev)
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	int err;
 
-	if (!(master->flags & IFF_UP))
-		return -ENETDOWN;
+	if (!(master->flags & IFF_UP)) {
+		err = dev_change_flags(master, master->flags | IFF_UP, NULL);
+		if (err < 0) {
+			netdev_err(dev, "failed to open master %s\n",
+				   master->name);
+			goto out;
+		}
+	}
 
 	if (!ether_addr_equal(dev->dev_addr, master->dev_addr)) {
 		err = dev_uc_add(master, dev->dev_addr);
-- 
2.25.1

It has the benefit that user space can now remove DSA-specific
workarounds, like systemd-networkd with BindCarrier:
https://github.com/systemd/systemd/issues/7478
And we could remove one of the 2 bullets in the "Common pitfalls using
DSA setups" chapter:
https://www.kernel.org/doc/Documentation/networking/dsa/dsa.txt
And....
We could remove the DSA workaround from net/ipv4/ipconfig.c as well.
Just saying.

>  	if (netdev_master_upper_dev_get(ndev)) {
>  		np_err(np, "%s is a slave device, aborting\n", np->dev_name);
>  		err = -EBUSY;
> -- 
> 2.25.1
>