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

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

[Florian Fainelli]

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(-)

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;
-- 
2.25.1

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@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
> 

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

[Florian Fainelli]

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

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

[Vladimir Oltean]

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.

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

[Jakub Kicinski]

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()?

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

[Florian Fainelli]

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

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

[Florian Fainelli]

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

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

[Vladimir Oltean]

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.

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

[Jakub Kicinski]

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 :)

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

[Vladimir Oltean]

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.

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

[Jakub Kicinski]

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.

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

[Vladimir Oltean]

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.

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

[Florian Fainelli]

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

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:
> 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
>