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.faine...@gmail.com>
> ---
> 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
>
