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

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