| Message ID | 20201019171746.991720-1-f.fainelli@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net] net: Have netpoll bring-up DSA management interface | expand |
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@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 >
On 10/19/20 1:02 PM, Vladimir Oltean wrote: > 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@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? I really do not have a problem with that approach however other stacked devices like 802.1Q do not do that. It certainly scales a lot better to do this within DSA rather than sprinkling DSA specific knowledge throughout the network stack. Maybe for "configuration less" stacked devices such as DSA, 802.1Q (bridge ports?), bond etc. it would be acceptable to ensure that the lower device is always brought up? PS: if you quote below the git version, it would appear that Thunderbird just eats that part of the mail... another bug to submit there. -- Florian
On Mon, Oct 19, 2020 at 02:03:40PM -0700, Florian Fainelli wrote: > > Completely crazy and outlandish idea, I know, but what's wrong with > > doing this in DSA? > > I really do not have a problem with that approach however other stacked > devices like 802.1Q do not do that. It certainly scales a lot better to > do this within DSA rather than sprinkling DSA specific knowledge > throughout the network stack. Maybe for "configuration less" stacked > devices such as DSA, 802.1Q (bridge ports?), bond etc. it would be > acceptable to ensure that the lower device is always brought up? For upper interfaces with more than one lower (bridge, bond) I'm not so sure. For uppers with a single lower (DSA, 8021q), it's pretty much a no-brainer to me. Question is, where to code this? I think it's ok to leave it in DSA, then 8021q could copy it as well if there was a need.
On Tue, 20 Oct 2020 00:19:16 +0300 Vladimir Oltean wrote: > On Mon, Oct 19, 2020 at 02:03:40PM -0700, Florian Fainelli wrote: > > > Completely crazy and outlandish idea, I know, but what's wrong with > > > doing this in DSA? > > > > I really do not have a problem with that approach however other stacked > > devices like 802.1Q do not do that. It certainly scales a lot better to > > do this within DSA rather than sprinkling DSA specific knowledge > > throughout the network stack. Maybe for "configuration less" stacked > > devices such as DSA, 802.1Q (bridge ports?), bond etc. it would be > > acceptable to ensure that the lower device is always brought up? > > For upper interfaces with more than one lower (bridge, bond) I'm not so > sure. For uppers with a single lower (DSA, 8021q), it's pretty much a > no-brainer to me. Question is, where to code this? I think it's ok to > leave it in DSA, then 8021q could copy it as well if there was a need. FWIW no strong preference here. Maybe I'd lean slightly towards Florian's approach since we can go to the always upping the CPU netdev from that, if we start with auto-upping CPU netdev - user space may depend on that in general so we can't go back. But up to you folks, this seems like a DSA-specific problem, vlans don't get created before user space is up (AFAIK), so there is no compelling reason to change them in my mind. Florian for you patch specifially - can't we use netdev_for_each_lower_dev()?
On 10/20/20 6:12 PM, Jakub Kicinski wrote: > On Tue, 20 Oct 2020 00:19:16 +0300 Vladimir Oltean wrote: >> On Mon, Oct 19, 2020 at 02:03:40PM -0700, Florian Fainelli wrote: >>>> Completely crazy and outlandish idea, I know, but what's wrong with >>>> doing this in DSA? >>> >>> I really do not have a problem with that approach however other stacked >>> devices like 802.1Q do not do that. It certainly scales a lot better to >>> do this within DSA rather than sprinkling DSA specific knowledge >>> throughout the network stack. Maybe for "configuration less" stacked >>> devices such as DSA, 802.1Q (bridge ports?), bond etc. it would be >>> acceptable to ensure that the lower device is always brought up? >> >> For upper interfaces with more than one lower (bridge, bond) I'm not so >> sure. For uppers with a single lower (DSA, 8021q), it's pretty much a >> no-brainer to me. Question is, where to code this? I think it's ok to >> leave it in DSA, then 8021q could copy it as well if there was a need. > > FWIW no strong preference here. Maybe I'd lean slightly towards > Florian's approach since we can go to the always upping the CPU netdev > from that, if we start with auto-upping CPU netdev - user space may > depend on that in general so we can't go back. > > But up to you folks, this seems like a DSA-specific problem, vlans don't > get created before user space is up (AFAIK), so there is no compelling > reason to change them in my mind. Right I remembered in my previous job we had a patch that would support creating VLAN devices when specified over ipconfig on the kernel command line, but that as never upstream AFAICT. > > Florian for you patch specifially - can't we use > netdev_for_each_lower_dev()? Looks like I forgot to respond here, yes we could do that because we do call netdev_upper_dev_link() in net/dsa/slave.c. Let me re-post with that done. -- Florian
On 11/16/20 3:06 PM, Florian Fainelli wrote: > On 10/20/20 6:12 PM, Jakub Kicinski wrote: >> On Tue, 20 Oct 2020 00:19:16 +0300 Vladimir Oltean wrote: >>> On Mon, Oct 19, 2020 at 02:03:40PM -0700, Florian Fainelli wrote: >>>>> Completely crazy and outlandish idea, I know, but what's wrong with >>>>> doing this in DSA? >>>> >>>> I really do not have a problem with that approach however other stacked >>>> devices like 802.1Q do not do that. It certainly scales a lot better to >>>> do this within DSA rather than sprinkling DSA specific knowledge >>>> throughout the network stack. Maybe for "configuration less" stacked >>>> devices such as DSA, 802.1Q (bridge ports?), bond etc. it would be >>>> acceptable to ensure that the lower device is always brought up? >>> >>> For upper interfaces with more than one lower (bridge, bond) I'm not so >>> sure. For uppers with a single lower (DSA, 8021q), it's pretty much a >>> no-brainer to me. Question is, where to code this? I think it's ok to >>> leave it in DSA, then 8021q could copy it as well if there was a need. >> >> FWIW no strong preference here. Maybe I'd lean slightly towards >> Florian's approach since we can go to the always upping the CPU netdev >> from that, if we start with auto-upping CPU netdev - user space may >> depend on that in general so we can't go back. >> >> But up to you folks, this seems like a DSA-specific problem, vlans don't >> get created before user space is up (AFAIK), so there is no compelling >> reason to change them in my mind. > > Right I remembered in my previous job we had a patch that would support > creating VLAN devices when specified over ipconfig on the kernel command > line, but that as never upstream AFAICT. > >> >> Florian for you patch specifially - can't we use >> netdev_for_each_lower_dev()? > > Looks like I forgot to respond here, yes we could do that because we do > call netdev_upper_dev_link() in net/dsa/slave.c. Let me re-post with > that done. I remember now there was a reason for me to "open code" this, and this is because since the patch is intended to be a bug fix, I wanted it to be independent from: 2f1e8ea726e9 ("net: dsa: link interfaces with the DSA master to get rid of lockdep warnings") which we would be depending on and is only two-ish releases away. Let me know if you prefer different fixes for different branches. -- Florian
On Mon, Nov 16, 2020 at 03:20:37PM -0800, Florian Fainelli wrote: > I remember now there was a reason for me to "open code" this, and this > is because since the patch is intended to be a bug fix, I wanted it to > be independent from: 2f1e8ea726e9 ("net: dsa: link interfaces with the > DSA master to get rid of lockdep warnings") > > which we would be depending on and is only two-ish releases away. Let me > know if you prefer different fixes for different branches. Florian, I studied a little bit the reason why DSA wants the master interface to be open before the slave is (not enough to actually submit a fully tested patch, though). It's because Lennert Buytenhek wanted to ensure that DSA only manages the promiscuity of interfaces that are up, because of a bugfix from 2008. See commit: commit df02c6ff2e3937379b31ea161b53229134fe92f7 Author: Lennert Buytenhek <buytenh@marvell.com> Date: Mon Nov 10 21:53:12 2008 -0800 dsa: fix master interface allmulti/promisc handling Before commit b6c40d68ff6498b7f63ddf97cf0aa818d748dee7 ("net: only invoke dev->change_rx_flags when device is UP"), the dsa driver could sort-of get away with only fiddling with the master interface's allmulti/promisc counts in ->change_rx_flags() and not touching them in ->open() or ->stop(). After this commit (note that it was merged almost simultaneously with the dsa patches, which is why this wasn't caught initially), the breakage that was already there became more apparent. Since it makes no sense to keep the master interface's allmulti or promisc count pinned for a slave interface that is down, copy the vlan driver's sync logic (which does exactly what we want) over to dsa to fix this. Bug report from Dirk Teurlings <dirk@upexia.nl> and Peter van Valderen <linux@ddcrew.com>. Signed-off-by: Lennert Buytenhek <buytenh@marvell.com> Tested-by: Dirk Teurlings <dirk@upexia.nl> Tested-by: Peter van Valderen <linux@ddcrew.com> Signed-off-by: David S. Miller <davem@davemloft.net> followed by commit commit d2615bf450694c1302d86b9cc8a8958edfe4c3a4 Author: Vlad Yasevich <vyasevic@redhat.com> Date: Tue Nov 19 20:47:15 2013 -0500 net: core: Always propagate flag changes to interfaces The following commit: b6c40d68ff6498b7f63ddf97cf0aa818d748dee7 net: only invoke dev->change_rx_flags when device is UP tried to fix a problem with VLAN devices and promiscuouse flag setting. The issue was that VLAN device was setting a flag on an interface that was down, thus resulting in bad promiscuity count. This commit blocked flag propagation to any device that is currently down. A later commit: deede2fabe24e00bd7e246eb81cd5767dc6fcfc7 vlan: Don't propagate flag changes on down interfaces fixed VLAN code to only propagate flags when the VLAN interface is up, thus fixing the same issue as above, only localized to VLAN. The problem we have now is that if we have create a complex stack involving multiple software devices like bridges, bonds, and vlans, then it is possible that the flags would not propagate properly to the physical devices. A simple examle of the scenario is the following: eth0----> bond0 ----> bridge0 ---> vlan50 If bond0 or eth0 happen to be down at the time bond0 is added to the bridge, then eth0 will never have promisc mode set which is currently required for operation as part of the bridge. As a result, packets with vlan50 will be dropped by the interface. The only 2 devices that implement the special flag handling are VLAN and DSA and they both have required code to prevent incorrect flag propagation. As a result we can remove the generic solution introduced in b6c40d68ff6498b7f63ddf97cf0aa818d748dee7 and leave it to the individual devices to decide whether they will block flag propagation or not. Reported-by: Stefan Priebe <s.priebe@profihost.ag> Suggested-by: Veaceslav Falico <vfalico@redhat.com> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com> Signed-off-by: David S. Miller <davem@davemloft.net> So Vlad Yasevich, through his patch, leaves this decision at the sole will of DSA now, it is no longer mandated by the networking core that the master interface must be up when changing its promiscuity. As from my point of view? I think we're missing the forest for the trees by requiring the user to bring up the master interface. Even _if_ there still was a problem with promiscuity when the master is down (which btw I couldn't reproduce), then who's stopping DSA from simply bringing the master interface up in the first place... I think from a user's point of view this would be a beneficial change.
On Mon, 16 Nov 2020 15:20:37 -0800 Florian Fainelli wrote: > >> Florian for you patch specifially - can't we use > >> netdev_for_each_lower_dev()? > > > > Looks like I forgot to respond here, yes we could do that because we do > > call netdev_upper_dev_link() in net/dsa/slave.c. Let me re-post with > > that done. > > I remember now there was a reason for me to "open code" this, and this > is because since the patch is intended to be a bug fix, I wanted it to > be independent from: 2f1e8ea726e9 ("net: dsa: link interfaces with the > DSA master to get rid of lockdep warnings") > > which we would be depending on and is only two-ish releases away. Let me > know if you prefer different fixes for different branches. Ah, makes sense, we can apply this and then clean up in net-next. Just mention that in the commit message. FWIW you'll need to repost anyway once the discussion with Vladimir is resolved, because this is in the old patchwork instance :)
On Mon, Nov 16, 2020 at 03:47:10PM -0800, Jakub Kicinski wrote: > On Mon, 16 Nov 2020 15:20:37 -0800 Florian Fainelli wrote: > > >> Florian for you patch specifially - can't we use > > >> netdev_for_each_lower_dev()? > > > > > > Looks like I forgot to respond here, yes we could do that because we do > > > call netdev_upper_dev_link() in net/dsa/slave.c. Let me re-post with > > > that done. > > > > I remember now there was a reason for me to "open code" this, and this > > is because since the patch is intended to be a bug fix, I wanted it to > > be independent from: 2f1e8ea726e9 ("net: dsa: link interfaces with the > > DSA master to get rid of lockdep warnings") > > > > which we would be depending on and is only two-ish releases away. Let me > > know if you prefer different fixes for different branches. > > Ah, makes sense, we can apply this and then clean up in net-next. Just > mention that in the commit message. FWIW you'll need to repost anyway > once the discussion with Vladimir is resolved, because this is in the > old patchwork instance :) Yeah, I think Florian just wants netconsole to work in stable kernels, which is a fair point. As for my 16-line patch that I suggested to him in the initial reply, what do you think, would that be a "stable" candidate? We would be introducing a fairly user-visible change (removing one step that is mentioned as necessary in the documentation), do you think it would benefit the users more to also have that behavior change backported to all LTS kernels, or just keep it as something new for v5.11? Either way, if it doesn't qualify as a patch for "stable", then I guess Florian should just resubmit his patch on net/core/netpoll.c.
On Tue, 17 Nov 2020 01:54:05 +0200 Vladimir Oltean wrote: > Yeah, I think Florian just wants netconsole to work in stable kernels, > which is a fair point. As for my 16-line patch that I suggested to him > in the initial reply, what do you think, would that be a "stable" > candidate? We would be introducing a fairly user-visible change > (removing one step that is mentioned as necessary in the documentation), > do you think it would benefit the users more to also have that behavior > change backported to all LTS kernels, or just keep it as something new > for v5.11? Yeah, I'd think that's too risky for a backport.
On Mon, Nov 16, 2020 at 04:04:49PM -0800, Jakub Kicinski wrote: > On Tue, 17 Nov 2020 01:54:05 +0200 Vladimir Oltean wrote: > > Yeah, I think Florian just wants netconsole to work in stable kernels, > > which is a fair point. As for my 16-line patch that I suggested to him > > in the initial reply, what do you think, would that be a "stable" > > candidate? We would be introducing a fairly user-visible change > > (removing one step that is mentioned as necessary in the documentation), > > do you think it would benefit the users more to also have that behavior > > change backported to all LTS kernels, or just keep it as something new > > for v5.11? > > Yeah, I'd think that's too risky for a backport. Ok, I retract my charges then. Florian, depending on how quickly you have time to resend, I might be able to double-test your patch tonight before I go to sleep.
On 11/16/20 4:12 PM, Vladimir Oltean wrote: > On Mon, Nov 16, 2020 at 04:04:49PM -0800, Jakub Kicinski wrote: >> On Tue, 17 Nov 2020 01:54:05 +0200 Vladimir Oltean wrote: >>> Yeah, I think Florian just wants netconsole to work in stable kernels, >>> which is a fair point. As for my 16-line patch that I suggested to him >>> in the initial reply, what do you think, would that be a "stable" >>> candidate? We would be introducing a fairly user-visible change >>> (removing one step that is mentioned as necessary in the documentation), >>> do you think it would benefit the users more to also have that behavior >>> change backported to all LTS kernels, or just keep it as something new >>> for v5.11? >> >> Yeah, I'd think that's too risky for a backport. > > Ok, I retract my charges then. Florian, depending on how quickly you > have time to resend, I might be able to double-test your patch tonight > before I go to sleep. Please give it a spin, I can resend tomorrow first thing in the morning. Thanks! -- Florian
On Mon, Oct 19, 2020 at 10:17:44AM -0700, Florian Fainelli wrote: > DSA network devices rely on having their DSA management interface up and > running otherwise their ndo_open() will return -ENETDOWN. Without doing > this it would not be possible to use DSA devices as netconsole when > configured on the command line. 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. > > 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. Or to that of the swapper process, when netpoll_setup is called from initcall context. > > Fixes: 04ff53f96a93 ("net: dsa: Add netconsole support") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- Tested-by: Vladimir Oltean <olteanv@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; Not amazing, but does the job. > + > + 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; > + } > + } > + > 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 >
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; + } + } + if (netdev_master_upper_dev_get(ndev)) { np_err(np, "%s is a slave device, aborting\n", np->dev_name); err = -EBUSY;
DSA network devices rely on having their DSA management interface up and running otherwise their ndo_open() will return -ENETDOWN. Without doing this it would not be possible to use DSA devices as netconsole when configured on the command line. 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. 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. Fixes: 04ff53f96a93 ("net: dsa: Add netconsole support") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- net/core/netpoll.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)