[PATCH net-next] net: ipconfig: allow IP-Config over DSA devices

[PATCH net-next] net: ipconfig: allow IP-Config over DSA devices

[Florian Fainelli]

Hi David, Lennert, Jiri,

This small patch allows us to use the kernel IP auto-configuration on DSA
enabled devices.

I initially started implementing the netdev_upper_dev_link() calls for the
DSA slave devices, but ended up realizing that although this might be useful,
the other drivers or protocols implementing these master/slave relantionship
are the bonding driver and the VLAN code.

None of these interfaces (bonding or VLAN) can be created by the kernel
without modifications, which means that user-space is there, and so we could
pivot_root over a NFS mounted share for instance, hence making the master/slave
net_device relationship not so useful for IP-Config.

This is not the case with DSA devices which are solely created by the kernel
based on platform configuration.

Let me know your thoughts. If you feel like something like:
netdev_is_upper_dev() or something like that is better.

Thanks!

Florian Fainelli (1):
  net: ipconfig: handle DSA enabled network devices

 net/ipv4/ipconfig.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH net-next] net: ipconfig: handle DSA enabled network devices

[Florian Fainelli]

The logic to configure a network interface for kernel IP
auto-configuration is very simplistic, and does not handle the case
where a device is stacked onto another such as with DSA. This causes the
kernel not to open and configure e.g: eth0 which is the master network
device for its slave network devices.

DSA slave devices do check whether their master device is UP in their
ndo_open() callback, and will return an error if it is not the case,
thus preventing them from being usable.

On the reverse path, make sure we do not attempt to close a DSA-enabled
device as this would implicitely prevent the slave DSA network device
from operating.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/ipv4/ipconfig.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
index b3e86ea7b71b..c06430270482 100644
--- a/net/ipv4/ipconfig.c
+++ b/net/ipv4/ipconfig.c
@@ -211,9 +211,9 @@ static int __init ic_open_devs(void)
 	last = &ic_first_dev;
 	rtnl_lock();
 
-	/* bring loopback device up first */
+	/* bring loopback and DSA master devices up first */
 	for_each_netdev(&init_net, dev) {
-		if (!(dev->flags & IFF_LOOPBACK))
+		if (!(dev->flags & IFF_LOOPBACK) && !dev->dsa_ptr)
 			continue;
 		if (dev_change_flags(dev, dev->flags | IFF_UP) < 0)
 			pr_err("IP-Config: Failed to open %s\n", dev->name);
@@ -307,7 +307,8 @@ static void __init ic_close_devs(void)
 	while ((d = next)) {
 		next = d->next;
 		dev = d->dev;
-		if (dev != ic_dev) {
+		/* Bring down non-DSA master and unused devices */
+		if (dev != ic_dev && !dev->dsa_ptr) {
 			DBG(("IP-Config: Downing %s\n", dev->name));
 			dev_change_flags(dev, d->flags);
 		}
-- 
1.9.1

Re: [PATCH net-next] net: ipconfig: allow IP-Config over DSA devices

[Jiri Pirko]

Fri, May 30, 2014 at 08:08:45PM CEST, f.fainelli@gmail.com wrote:
>Hi David, Lennert, Jiri,
>
>This small patch allows us to use the kernel IP auto-configuration on DSA
>enabled devices.
>
>I initially started implementing the netdev_upper_dev_link() calls for the
>DSA slave devices, but ended up realizing that although this might be useful,
>the other drivers or protocols implementing these master/slave relantionship
>are the bonding driver and the VLAN code.
>
>None of these interfaces (bonding or VLAN) can be created by the kernel
>without modifications, which means that user-space is there, and so we could
>pivot_root over a NFS mounted share for instance, hence making the master/slave
>net_device relationship not so useful for IP-Config.
>
>This is not the case with DSA devices which are solely created by the kernel
>based on platform configuration.
>
>Let me know your thoughts. If you feel like something like:
>netdev_is_upper_dev() or something like that is better.

uppers and lowers should not be used by switches. If they were, it would
block the usage of ports in bond/bridge/ovs. I did that myself in my
first RFC patchset but realized that it make no sense.

What I have in mind and I believe that many people nodded to is an
exported (netlink, sysfs) value of switch id. That can be generated
randomly or from some hw id. Please see following git tree:

https://github.com/jpirko/net-next-rocker

On the tip, there are rocker patches combined with the switch
infrastructure patches. The switch id is there implemented for dsa and
rocker.

This is based on the RFC patchset I sent some while ago on netdev
mailing list.

Please tell me what do you think.

Thanks.

Jiri

>
>Thanks!
>
>Florian Fainelli (1):
>  net: ipconfig: handle DSA enabled network devices
>
> net/ipv4/ipconfig.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
>-- 
>1.9.1
>

Re: [PATCH net-next] net: ipconfig: allow IP-Config over DSA devices

[Florian Fainelli]

2014-05-31 0:04 GMT-07:00 Jiri Pirko <jiri@resnulli.us>:
> Fri, May 30, 2014 at 08:08:45PM CEST, f.fainelli@gmail.com wrote:
>>Hi David, Lennert, Jiri,
>>
>>This small patch allows us to use the kernel IP auto-configuration on DSA
>>enabled devices.
>>
>>I initially started implementing the netdev_upper_dev_link() calls for the
>>DSA slave devices, but ended up realizing that although this might be useful,
>>the other drivers or protocols implementing these master/slave relantionship
>>are the bonding driver and the VLAN code.
>>
>>None of these interfaces (bonding or VLAN) can be created by the kernel
>>without modifications, which means that user-space is there, and so we could
>>pivot_root over a NFS mounted share for instance, hence making the master/slave
>>net_device relationship not so useful for IP-Config.
>>
>>This is not the case with DSA devices which are solely created by the kernel
>>based on platform configuration.
>>
>>Let me know your thoughts. If you feel like something like:
>>netdev_is_upper_dev() or something like that is better.
>
> uppers and lowers should not be used by switches. If they were, it would
> block the usage of ports in bond/bridge/ovs. I did that myself in my
> first RFC patchset but realized that it make no sense.
>
> What I have in mind and I believe that many people nodded to is an
> exported (netlink, sysfs) value of switch id. That can be generated
> randomly or from some hw id. Please see following git tree:
>
> https://github.com/jpirko/net-next-rocker
>
> On the tip, there are rocker patches combined with the switch
> infrastructure patches. The switch id is there implemented for dsa and
> rocker.
>
> This is based on the RFC patchset I sent some while ago on netdev
> mailing list.
>
> Please tell me what do you think.

The way the switch_id attribute is implemented, is currently a
property of the slave aka per-port net_devices, which makes sense.
That does not entirely help me in my case, which is bringing up the
master/parent network device of these slave devices. We are still
missing a "generic" way to tell how a slave per-port network device
relates to its "conduit" network device.
-- 
Florian

Re: [PATCH net-next] net: ipconfig: allow IP-Config over DSA devices

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Fri, 30 May 2014 11:08:45 -0700

> Let me know your thoughts. If you feel like something like:
> netdev_is_upper_dev() or something like that is better.

I'm not happy with this change for several reasons.

First, I don't like the idea that ipconfig works for some stacked
devices and not for others.  I'd rather that we simply accept that
direct devices are the only thing supported.

Secondly, the logic in that ipconfig loop is so confusing.  I can't
even figure out what that dsa pointer test is trying to really
accomplish in the second hunk of your patch.

I'd like to defer this for now, sorry.

Re: [PATCH net-next] net: ipconfig: allow IP-Config over DSA devices

[Florian Fainelli]

2014-06-05 0:01 GMT-07:00 David Miller <davem@davemloft.net>:
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Fri, 30 May 2014 11:08:45 -0700
>
>> Let me know your thoughts. If you feel like something like:
>> netdev_is_upper_dev() or something like that is better.
>
> I'm not happy with this change for several reasons.
>
> First, I don't like the idea that ipconfig works for some stacked
> devices and not for others.

Right, although as I described DSA devices are likely to be the only
real kernel-only created devices here, other stacked devices such as
bonds, vlans and tunnels for instance do require either kernel
modifications to be created by the kernel, or user-space.

>  I'd rather that we simply accept that
> direct devices are the only thing supported.

Would you be willing to revise this position if we can come up with a
generic infrastructure for telling whether a network device depends on
another one and its master device needs to be opened first? Should we
instead force the slave devices in net/dsa/slave.c to open its master
device if it is not UP?

>
> Secondly, the logic in that ipconfig loop is so confusing.  I can't
> even figure out what that dsa pointer test is trying to really
> accomplish in the second hunk of your patch.
>
> I'd like to defer this for now, sorry.

Thanks for your comments.
-- 
Florian