2014-01-30 13:19:52

by Tom Gundersen

[permalink] [raw]
Subject: [PATCH] net: set default DEVTYPE for all ethernet based devices

In systemd's networkd and udevd, we would like to give the administrator a
simple way to filter net devices by their DEVTYPE [0][1]. Other software
such as ConnMan and NetworkManager uses a similar filtering already.

Currently, plain ethernet devices have DEVTYPE=(null). This patch sets the
devtype to "ethernet" instead. This avoids the need for special-casing the
DEVTYPE=(null) case in userspace, and also avoids false positives, as there
are several other types of netdevs that also have DEVTYPE=(null).

Notice that this is done, as suggested by Marcel, in alloc_etherdev_mqs(),
and as best I can tell it will not give any false positives. I considered
doing it in ether_setup() instead as that seemed more intuitive, but that
would give a lot of false positives indeed.

[0]: <http://www.freedesktop.org/software/systemd/man/systemd-networkd.service.html#Type>
[1]: <http://www.freedesktop.org/software/systemd/man/udev.html#Type>

Signed-off-by: Tom Gundersen <[email protected]>
Cc: Marcel Holtmann <[email protected]>
Cc: Greg KH <[email protected]>
Cc: Kay Sievers <[email protected]>
---
net/ethernet/eth.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 8f032ba..b76dc17 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -369,6 +369,10 @@ void ether_setup(struct net_device *dev)
}
EXPORT_SYMBOL(ether_setup);

+static const struct device_type eth_type = {
+ .name = "ethernet",
+};
+
/**
* alloc_etherdev_mqs - Allocates and sets up an Ethernet device
* @sizeof_priv: Size of additional driver-private structure to be allocated
@@ -387,7 +391,13 @@ EXPORT_SYMBOL(ether_setup);
struct net_device *alloc_etherdev_mqs(int sizeof_priv, unsigned int txqs,
unsigned int rxqs)
{
- return alloc_netdev_mqs(sizeof_priv, "eth%d", ether_setup, txqs, rxqs);
+ struct net_device* dev;
+
+ dev = alloc_netdev_mqs(sizeof_priv, "eth%d", ether_setup, txqs, rxqs);
+ if (dev)
+ dev->dev.type = &eth_type;
+
+ return dev;
}
EXPORT_SYMBOL(alloc_etherdev_mqs);

--
1.8.5.3


2014-01-30 15:06:15

by Veaceslav Falico

[permalink] [raw]
Subject: Re: [PATCH] net: set default DEVTYPE for all ethernet based devices

On Thu, Jan 30, 2014 at 02:20:02PM +0100, Tom Gundersen wrote:
>In systemd's networkd and udevd, we would like to give the administrator a
>simple way to filter net devices by their DEVTYPE [0][1]. Other software
>such as ConnMan and NetworkManager uses a similar filtering already.
>
>Currently, plain ethernet devices have DEVTYPE=(null). This patch sets the
>devtype to "ethernet" instead. This avoids the need for special-casing the
>DEVTYPE=(null) case in userspace, and also avoids false positives, as there
>are several other types of netdevs that also have DEVTYPE=(null).

There are quite a few users at least in usb and wireless drivers:

net#git grep alloc_etherdev drivers/net/wireless/ drivers/net/usb | wc -l
18

In usb, though, there might be some false positives of this grep, as
there are a few devices which might be considered ethernet.

>
>Notice that this is done, as suggested by Marcel, in alloc_etherdev_mqs(),
>and as best I can tell it will not give any false positives. I considered
>doing it in ether_setup() instead as that seemed more intuitive, but that
>would give a lot of false positives indeed.
>
>[0]: <http://www.freedesktop.org/software/systemd/man/systemd-networkd.service.html#Type>
>[1]: <http://www.freedesktop.org/software/systemd/man/udev.html#Type>
>
>Signed-off-by: Tom Gundersen <[email protected]>
>Cc: Marcel Holtmann <[email protected]>
>Cc: Greg KH <[email protected]>
>Cc: Kay Sievers <[email protected]>
>---
> net/ethernet/eth.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
>diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
>index 8f032ba..b76dc17 100644
>--- a/net/ethernet/eth.c
>+++ b/net/ethernet/eth.c
>@@ -369,6 +369,10 @@ void ether_setup(struct net_device *dev)
> }
> EXPORT_SYMBOL(ether_setup);
>
>+static const struct device_type eth_type = {
>+ .name = "ethernet",
>+};
>+
> /**
> * alloc_etherdev_mqs - Allocates and sets up an Ethernet device
> * @sizeof_priv: Size of additional driver-private structure to be allocated
>@@ -387,7 +391,13 @@ EXPORT_SYMBOL(ether_setup);
> struct net_device *alloc_etherdev_mqs(int sizeof_priv, unsigned int txqs,
> unsigned int rxqs)
> {
>- return alloc_netdev_mqs(sizeof_priv, "eth%d", ether_setup, txqs, rxqs);
>+ struct net_device* dev;
>+
>+ dev = alloc_netdev_mqs(sizeof_priv, "eth%d", ether_setup, txqs, rxqs);
>+ if (dev)
>+ dev->dev.type = &eth_type;
>+
>+ return dev;
> }
> EXPORT_SYMBOL(alloc_etherdev_mqs);
>
>--
>1.8.5.3
>
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to [email protected]
>More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-01-31 00:28:45

by David Miller

[permalink] [raw]
Subject: Re: [PATCH] net: set default DEVTYPE for all ethernet based devices

From: Tom Gundersen <[email protected]>
Date: Thu, 30 Jan 2014 14:20:02 +0100

> In systemd's networkd and udevd, we would like to give the administrator a
> simple way to filter net devices by their DEVTYPE [0][1]. Other software
> such as ConnMan and NetworkManager uses a similar filtering already.
>
> Currently, plain ethernet devices have DEVTYPE=(null). This patch sets the
> devtype to "ethernet" instead. This avoids the need for special-casing the
> DEVTYPE=(null) case in userspace, and also avoids false positives, as there
> are several other types of netdevs that also have DEVTYPE=(null).
>
> Notice that this is done, as suggested by Marcel, in alloc_etherdev_mqs(),
> and as best I can tell it will not give any false positives. I considered
> doing it in ether_setup() instead as that seemed more intuitive, but that
> would give a lot of false positives indeed.
>
> [0]: <http://www.freedesktop.org/software/systemd/man/systemd-networkd.service.html#Type>
> [1]: <http://www.freedesktop.org/software/systemd/man/udev.html#Type>
>
> Signed-off-by: Tom Gundersen <[email protected]>

Assuming that all users of alloc_etherdev*() are ethernet devices is
really not going to work.

2014-01-31 00:54:55

by Tom Gundersen

[permalink] [raw]
Subject: Re: [PATCH] net: set default DEVTYPE for all ethernet based devices

Hi Veaceslav,

Thanks for your quick reply.

On Thu, Jan 30, 2014 at 4:05 PM, Veaceslav Falico <[email protected]> wrote:
> On Thu, Jan 30, 2014 at 02:20:02PM +0100, Tom Gundersen wrote:
>>
>> In systemd's networkd and udevd, we would like to give the administrator a
>> simple way to filter net devices by their DEVTYPE [0][1]. Other software
>> such as ConnMan and NetworkManager uses a similar filtering already.
>>
>> Currently, plain ethernet devices have DEVTYPE=(null). This patch sets the
>> devtype to "ethernet" instead. This avoids the need for special-casing the
>> DEVTYPE=(null) case in userspace, and also avoids false positives, as
>> there
>> are several other types of netdevs that also have DEVTYPE=(null).
>
>
> There are quite a few users at least in usb and wireless drivers:
>
> net#git grep alloc_etherdev drivers/net/wireless/ drivers/net/usb | wc -l
> 18
>
> In usb, though, there might be some false positives of this grep, as
> there are a few devices which might be considered ethernet.

Ah, yes I missed the #define of alloc_etherdev(). Looking through
these, it shouldn't be too hard to keep this patch and additionally
fix up the false positives to opt-out of setting the DEVTYPE. Does
that sound like something that would be acceptable?

Cheers,

Tom

2014-01-31 10:07:26

by Veaceslav Falico

[permalink] [raw]
Subject: Re: [PATCH] net: set default DEVTYPE for all ethernet based devices

On Fri, Jan 31, 2014 at 01:54:03AM +0100, Tom Gundersen wrote:
>Hi Veaceslav,
>
>Thanks for your quick reply.
>
>On Thu, Jan 30, 2014 at 4:05 PM, Veaceslav Falico <[email protected]> wrote:
>> On Thu, Jan 30, 2014 at 02:20:02PM +0100, Tom Gundersen wrote:
>>>
>>> In systemd's networkd and udevd, we would like to give the administrator a
>>> simple way to filter net devices by their DEVTYPE [0][1]. Other software
>>> such as ConnMan and NetworkManager uses a similar filtering already.
>>>
>>> Currently, plain ethernet devices have DEVTYPE=(null). This patch sets the
>>> devtype to "ethernet" instead. This avoids the need for special-casing the
>>> DEVTYPE=(null) case in userspace, and also avoids false positives, as
>>> there
>>> are several other types of netdevs that also have DEVTYPE=(null).
>>
>>
>> There are quite a few users at least in usb and wireless drivers:
>>
>> net#git grep alloc_etherdev drivers/net/wireless/ drivers/net/usb | wc -l
>> 18
>>
>> In usb, though, there might be some false positives of this grep, as
>> there are a few devices which might be considered ethernet.
>
>Ah, yes I missed the #define of alloc_etherdev(). Looking through
>these, it shouldn't be too hard to keep this patch and additionally
>fix up the false positives to opt-out of setting the DEVTYPE. Does
>that sound like something that would be acceptable?

Sure, I guess it would be nice to add something like alloc_netdev() (or any
other name for 'generic' network device) and alloc_wirelessdev() for
wireless - so that alloc_*dev() would be small inline wrappers for
alloc_netdev() and setting the type. I didn't check deep enough though, so
I might have overlooked something :).

I've taken a bit deeper look at USB network drivers and it seems that all
of them are ethernet, whilst usbnet (generic network framework for usb
networking devices, used for quite a few drivers) already tries to guess
the type - default is ethernet, and if there are W[WL]AN flags set - update
devtype accordingly. So you might want to take a look at usbnet_probe(), if
that'll suit your needs.

>
>Cheers,
>
>Tom