When a user port has a label in device tree, the corresponding
netdevice is "predictably named by the kernel".
Expose that information properly for the benefit of userspace tools
that make decisions based on the name_assign_type attribute,
e.g. a systemd-udev rule with "kernel" in NamePolicy.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
net/dsa/dsa2.c | 3 ---
net/dsa/slave.c | 13 +++++++++++--
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index e504a18fc125..522fc1b6e8c6 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1364,9 +1364,6 @@ static struct dsa_port *dsa_port_touch(struct dsa_switch *ds, int index)
static int dsa_port_parse_user(struct dsa_port *dp, const char *name)
{
- if (!name)
- name = "eth%d";
-
dp->type = DSA_PORT_TYPE_USER;
dp->name = name;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index a9fde48cffd4..dfefcc4a9ccf 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2374,16 +2374,25 @@ int dsa_slave_create(struct dsa_port *port)
{
struct net_device *master = dsa_port_to_master(port);
struct dsa_switch *ds = port->ds;
- const char *name = port->name;
struct net_device *slave_dev;
struct dsa_slave_priv *p;
+ const char *name;
+ int assign_type;
int ret;
if (!ds->num_tx_queues)
ds->num_tx_queues = 1;
+ if (port->name) {
+ name = port->name;
+ assign_type = NET_NAME_PREDICTABLE;
+ } else {
+ name = "eth%d";
+ assign_type = NET_NAME_UNKNOWN;
+ }
+
slave_dev = alloc_netdev_mqs(sizeof(struct dsa_slave_priv), name,
- NET_NAME_UNKNOWN, ether_setup,
+ assign_type, ether_setup,
ds->num_tx_queues, 1);
if (slave_dev == NULL)
return -ENOMEM;
--
2.37.2
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index a9fde48cffd4..dfefcc4a9ccf 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -2374,16 +2374,25 @@ int dsa_slave_create(struct dsa_port *port)
> {
> struct net_device *master = dsa_port_to_master(port);
> struct dsa_switch *ds = port->ds;
> - const char *name = port->name;
> struct net_device *slave_dev;
> struct dsa_slave_priv *p;
> + const char *name;
> + int assign_type;
> int ret;
>
> if (!ds->num_tx_queues)
> ds->num_tx_queues = 1;
>
> + if (port->name) {
> + name = port->name;
> + assign_type = NET_NAME_PREDICTABLE;
> + } else {
> + name = "eth%d";
> + assign_type = NET_NAME_UNKNOWN;
> + }
I know it is a change in behaviour, but it seems like NET_NAME_ENUM
should be used, not NET_NAME_UNKNOWN. alloc_etherdev_mqs() uses
NET_NAME_ENUM.
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/netdevice.h#L42
says that NET_NAME_UNKNOWN does not get passed to user space, but i
assume NET_NAME_ENUM does. So maybe changing it would be an ABI
change?
Humm, i don't know what the right thing is...
Andrew
On 11/11/2022 18.10, Andrew Lunn wrote:
>> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
>> index a9fde48cffd4..dfefcc4a9ccf 100644
>> --- a/net/dsa/slave.c
>> +++ b/net/dsa/slave.c
>> @@ -2374,16 +2374,25 @@ int dsa_slave_create(struct dsa_port *port)
>> {
>> struct net_device *master = dsa_port_to_master(port);
>> struct dsa_switch *ds = port->ds;
>> - const char *name = port->name;
>> struct net_device *slave_dev;
>> struct dsa_slave_priv *p;
>> + const char *name;
>> + int assign_type;
>> int ret;
>>
>> if (!ds->num_tx_queues)
>> ds->num_tx_queues = 1;
>>
>> + if (port->name) {
>> + name = port->name;
>> + assign_type = NET_NAME_PREDICTABLE;
>> + } else {
>> + name = "eth%d";
>> + assign_type = NET_NAME_UNKNOWN;
>> + }
>
> I know it is a change in behaviour, but it seems like NET_NAME_ENUM
> should be used, not NET_NAME_UNKNOWN. alloc_etherdev_mqs() uses
> NET_NAME_ENUM.
I don't really have any strong opinion on the case where we fall back to
eth%d, as its not relevant to any board I've worked on.
> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/netdevice.h#L42
> says that NET_NAME_UNKNOWN does not get passed to user space, but i
> assume NET_NAME_ENUM does. So maybe changing it would be an ABI
> change?
Well, the name_assign_type ABI is kind of silly. I mean, userspace knows
that when one gets EINVAL trying to read the value, that really means
that the value is NET_NAME_UNKNOWN. But I won't propose changing that.
However, what I do propose here is obviously already an ABI change; I
_want_ to expose more proper information in the case where the port has
a label, and just kept the NET_NAME_UNKNOWN for the eth%d case to make
the minimal change. But if people want to change that to NET_NAME_ENUM
while we're here, I can certainly do that. I can't think of any real
scenario where NET_NAME_ENUM would be treated differently than
NET_NAME_UNKNOWN - in both cases, userspace don't know that the name can
be trusted to be predictable.
Rasmus
On Sun, 13 Nov 2022 21:03:52 +0100 Rasmus Villemoes wrote:
> > I know it is a change in behaviour, but it seems like NET_NAME_ENUM
> > should be used, not NET_NAME_UNKNOWN. alloc_etherdev_mqs() uses
> > NET_NAME_ENUM.
>
> I don't really have any strong opinion on the case where we fall back to
> eth%d, as its not relevant to any board I've worked on.
>
> > https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/netdevice.h#L42
> > says that NET_NAME_UNKNOWN does not get passed to user space, but i
> > assume NET_NAME_ENUM does. So maybe changing it would be an ABI
> > change?
>
> Well, the name_assign_type ABI is kind of silly. I mean, userspace knows
> that when one gets EINVAL trying to read the value, that really means
> that the value is NET_NAME_UNKNOWN. But I won't propose changing that.
>
> However, what I do propose here is obviously already an ABI change; I
> _want_ to expose more proper information in the case where the port has
> a label, and just kept the NET_NAME_UNKNOWN for the eth%d case to make
> the minimal change. But if people want to change that to NET_NAME_ENUM
> while we're here, I can certainly do that. I can't think of any real
> scenario where NET_NAME_ENUM would be treated differently than
> NET_NAME_UNKNOWN - in both cases, userspace don't know that the name can
> be trusted to be predictable.
Apparently there may be a reason, see commit e9f656b7a214 ("net:
ethernet: set default assignment identifier to NET_NAME_ENUM")
so let's switch to ENUM while at it.
When a user port has a label in device tree, the corresponding
netdevice is "predictably named by the kernel".
Expose that information properly for the benefit of userspace tools
that make decisions based on the name_assign_type attribute,
e.g. a systemd-udev rule with "kernel" in NamePolicy.
Similarly, when we fall back to the eth%d scheme, the proper constant
to use is NET_NAME_ENUM. See also commit e9f656b7a214 ("net: ethernet:
set default assignment identifier to NET_NAME_ENUM"), which in turn
quoted commit 685343fc3ba6 ("net: add name_assign_type netdev
attribute"):
... when the kernel has given the interface a name using global
device enumeration based on order of discovery (ethX, wlanY, etc)
... are labelled NET_NAME_ENUM.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
v2: switch to NET_NAME_ENUM in the eth%d case (Andrew, Jakub). Update
commit message accordingly.
net/dsa/dsa2.c | 3 ---
net/dsa/slave.c | 13 +++++++++++--
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index e504a18fc125..522fc1b6e8c6 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1364,9 +1364,6 @@ static struct dsa_port *dsa_port_touch(struct dsa_switch *ds, int index)
static int dsa_port_parse_user(struct dsa_port *dp, const char *name)
{
- if (!name)
- name = "eth%d";
-
dp->type = DSA_PORT_TYPE_USER;
dp->name = name;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index a9fde48cffd4..821ab79bb60a 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2374,16 +2374,25 @@ int dsa_slave_create(struct dsa_port *port)
{
struct net_device *master = dsa_port_to_master(port);
struct dsa_switch *ds = port->ds;
- const char *name = port->name;
struct net_device *slave_dev;
struct dsa_slave_priv *p;
+ const char *name;
+ int assign_type;
int ret;
if (!ds->num_tx_queues)
ds->num_tx_queues = 1;
+ if (port->name) {
+ name = port->name;
+ assign_type = NET_NAME_PREDICTABLE;
+ } else {
+ name = "eth%d";
+ assign_type = NET_NAME_ENUM;
+ }
+
slave_dev = alloc_netdev_mqs(sizeof(struct dsa_slave_priv), name,
- NET_NAME_UNKNOWN, ether_setup,
+ assign_type, ether_setup,
ds->num_tx_queues, 1);
if (slave_dev == NULL)
return -ENOMEM;
--
2.37.2
> Apparently there may be a reason, see commit e9f656b7a214 ("net:
> ethernet: set default assignment identifier to NET_NAME_ENUM")
> so let's switch to ENUM while at it.
I would recommend two patches, making it easier to revert if we find
something in userspace breaks.
Andrew
On Tue, 15 Nov 2022 08:43:55 +0100 Rasmus Villemoes wrote:
> + if (port->name) {
> + name = port->name;
> + assign_type = NET_NAME_PREDICTABLE;
> + } else {
> + name = "eth%d";
> + assign_type = NET_NAME_ENUM;
Per Andrew's comment lets make the change in two steps.
Which one should come first is a judgment call :)
On 15/11/2022 17.38, Jakub Kicinski wrote:
> On Tue, 15 Nov 2022 08:43:55 +0100 Rasmus Villemoes wrote:
>> + if (port->name) {
>> + name = port->name;
>> + assign_type = NET_NAME_PREDICTABLE;
>> + } else {
>> + name = "eth%d";
>> + assign_type = NET_NAME_ENUM;
>
> Per Andrew's comment lets make the change in two steps.
> Which one should come first is a judgment call :)
OK. I think I'll actually do it in three steps, with the first being
this patch but with NET_NAME_UNKNOWN kept in both places (i.e. pure
refactoring), and the latter two just changing one assign_type at a
time, so they can be reverted independently.
Rasmus
On Tue, 15 Nov 2022 19:55:24 +0100 Rasmus Villemoes wrote:
> OK. I think I'll actually do it in three steps, with the first being
> this patch but with NET_NAME_UNKNOWN kept in both places (i.e. pure
> refactoring), and the latter two just changing one assign_type at a
> time, so they can be reverted independently.
Even better :)
The intention of commit 685343fc3ba6 ("net: add name_assign_type
netdev attribute") was clearly that drivers be switched over one by
one to select appropriate NET_NAME_* constants instead of
NET_NAME_UNKNOWN. This small series attempts to do that for DSA user
ports.
This is obviously and intentionally user-visible changes, so there's a
small chance that it could lead to a regression. To make it easy to
revert either of the "label in DT" and "fallback to eth%d" changes,
this is done as a refactoring which shouldn't introduce any functional
change (but by itself adds code which looks a little odd, with the two
identical assignments in the two branches), followed by changing the
constant used in each case in two different patches.
Rasmus Villemoes (3):
net: dsa: refactor name assignment for user ports
net: dsa: use NET_NAME_PREDICTABLE for user ports with name given in
DT
net: dsa: set name_assign_type to NET_NAME_ENUM for enumerated user
ports
net/dsa/dsa2.c | 3 ---
net/dsa/slave.c | 13 +++++++++++--
2 files changed, 11 insertions(+), 5 deletions(-)
--
2.37.2
The following two patches each have a (small) chance of causing
regressions for userspace and will in that case of course need to be
reverted.
In order to prepare for that and make those two patches independent
and individually revertable, refactor the code which sets the names
for user ports by moving the "fall back to eth%d if no label is given
in device tree" to dsa_slave_create().
No functional change (at least none intended).
Signed-off-by: Rasmus Villemoes <[email protected]>
---
net/dsa/dsa2.c | 3 ---
net/dsa/slave.c | 13 +++++++++++--
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index e504a18fc125..522fc1b6e8c6 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1364,9 +1364,6 @@ static struct dsa_port *dsa_port_touch(struct dsa_switch *ds, int index)
static int dsa_port_parse_user(struct dsa_port *dp, const char *name)
{
- if (!name)
- name = "eth%d";
-
dp->type = DSA_PORT_TYPE_USER;
dp->name = name;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index a9fde48cffd4..d19e9a536b8f 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2374,16 +2374,25 @@ int dsa_slave_create(struct dsa_port *port)
{
struct net_device *master = dsa_port_to_master(port);
struct dsa_switch *ds = port->ds;
- const char *name = port->name;
struct net_device *slave_dev;
struct dsa_slave_priv *p;
+ const char *name;
+ int assign_type;
int ret;
if (!ds->num_tx_queues)
ds->num_tx_queues = 1;
+ if (port->name) {
+ name = port->name;
+ assign_type = NET_NAME_UNKNOWN;
+ } else {
+ name = "eth%d";
+ assign_type = NET_NAME_UNKNOWN;
+ }
+
slave_dev = alloc_netdev_mqs(sizeof(struct dsa_slave_priv), name,
- NET_NAME_UNKNOWN, ether_setup,
+ assign_type, ether_setup,
ds->num_tx_queues, 1);
if (slave_dev == NULL)
return -ENOMEM;
--
2.37.2
When a user port has a label in device tree, the corresponding
netdevice is, to quote include/uapi/linux/netdevice.h, "predictably
named by the kernel". This is also explicitly one of the intended use
cases for NET_NAME_PREDICTABLE, quoting 685343fc3ba6 ("net: add
name_assign_type netdev attribute"):
NET_NAME_PREDICTABLE:
The ifname has been assigned by the kernel in a predictable way
[...] Examples include [...] and names deduced from hardware
properties (including being given explicitly by the firmware).
Expose that information properly for the benefit of userspace tools
that make decisions based on the name_assign_type attribute,
e.g. a systemd-udev rule with "kernel" in NamePolicy.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
net/dsa/slave.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index d19e9a536b8f..dfefcc4a9ccf 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2385,7 +2385,7 @@ int dsa_slave_create(struct dsa_port *port)
if (port->name) {
name = port->name;
- assign_type = NET_NAME_UNKNOWN;
+ assign_type = NET_NAME_PREDICTABLE;
} else {
name = "eth%d";
assign_type = NET_NAME_UNKNOWN;
--
2.37.2
When a user port does not have a label in device tree, and we thus
fall back to the eth%d scheme, the proper constant to use is
NET_NAME_ENUM. See also commit e9f656b7a214 ("net: ethernet: set
default assignment identifier to NET_NAME_ENUM"), which in turn quoted
commit 685343fc3ba6 ("net: add name_assign_type netdev attribute"):
... when the kernel has given the interface a name using global
device enumeration based on order of discovery (ethX, wlanY, etc)
... are labelled NET_NAME_ENUM.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
net/dsa/slave.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index dfefcc4a9ccf..821ab79bb60a 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2388,7 +2388,7 @@ int dsa_slave_create(struct dsa_port *port)
assign_type = NET_NAME_PREDICTABLE;
} else {
name = "eth%d";
- assign_type = NET_NAME_UNKNOWN;
+ assign_type = NET_NAME_ENUM;
}
slave_dev = alloc_netdev_mqs(sizeof(struct dsa_slave_priv), name,
--
2.37.2
On Wed, Nov 16, 2022 at 11:52:02AM +0100, Rasmus Villemoes wrote:
> The following two patches each have a (small) chance of causing
> regressions for userspace and will in that case of course need to be
> reverted.
>
> In order to prepare for that and make those two patches independent
> and individually revertable, refactor the code which sets the names
> for user ports by moving the "fall back to eth%d if no label is given
> in device tree" to dsa_slave_create().
>
> No functional change (at least none intended).
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On Wed, Nov 16, 2022 at 11:52:03AM +0100, Rasmus Villemoes wrote:
> When a user port has a label in device tree, the corresponding
> netdevice is, to quote include/uapi/linux/netdevice.h, "predictably
> named by the kernel". This is also explicitly one of the intended use
> cases for NET_NAME_PREDICTABLE, quoting 685343fc3ba6 ("net: add
> name_assign_type netdev attribute"):
>
> NET_NAME_PREDICTABLE:
> The ifname has been assigned by the kernel in a predictable way
> [...] Examples include [...] and names deduced from hardware
> properties (including being given explicitly by the firmware).
>
> Expose that information properly for the benefit of userspace tools
> that make decisions based on the name_assign_type attribute,
> e.g. a systemd-udev rule with "kernel" in NamePolicy.
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On Wed, Nov 16, 2022 at 11:52:04AM +0100, Rasmus Villemoes wrote:
> When a user port does not have a label in device tree, and we thus
> fall back to the eth%d scheme, the proper constant to use is
> NET_NAME_ENUM. See also commit e9f656b7a214 ("net: ethernet: set
> default assignment identifier to NET_NAME_ENUM"), which in turn quoted
> commit 685343fc3ba6 ("net: add name_assign_type netdev attribute"):
>
> ... when the kernel has given the interface a name using global
> device enumeration based on order of discovery (ethX, wlanY, etc)
> ... are labelled NET_NAME_ENUM.
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On 11/16/2022 2:52 AM, Rasmus Villemoes wrote:
> When a user port does not have a label in device tree, and we thus
> fall back to the eth%d scheme, the proper constant to use is
> NET_NAME_ENUM. See also commit e9f656b7a214 ("net: ethernet: set
> default assignment identifier to NET_NAME_ENUM"), which in turn quoted
> commit 685343fc3ba6 ("net: add name_assign_type netdev attribute"):
>
> ... when the kernel has given the interface a name using global
> device enumeration based on order of discovery (ethX, wlanY, etc)
> ... are labelled NET_NAME_ENUM.
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 11/16/2022 2:52 AM, Rasmus Villemoes wrote:
> The following two patches each have a (small) chance of causing
> regressions for userspace and will in that case of course need to be
> reverted.
>
> In order to prepare for that and make those two patches independent
> and individually revertable, refactor the code which sets the names
> for user ports by moving the "fall back to eth%d if no label is given
> in device tree" to dsa_slave_create().
>
> No functional change (at least none intended).
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 11/16/2022 2:52 AM, Rasmus Villemoes wrote:
> When a user port has a label in device tree, the corresponding
> netdevice is, to quote include/uapi/linux/netdevice.h, "predictably
> named by the kernel". This is also explicitly one of the intended use
> cases for NET_NAME_PREDICTABLE, quoting 685343fc3ba6 ("net: add
> name_assign_type netdev attribute"):
>
> NET_NAME_PREDICTABLE:
> The ifname has been assigned by the kernel in a predictable way
> [...] Examples include [...] and names deduced from hardware
> properties (including being given explicitly by the firmware).
>
> Expose that information properly for the benefit of userspace tools
> that make decisions based on the name_assign_type attribute,
> e.g. a systemd-udev rule with "kernel" in NamePolicy.
>
> Signed-off-by: Rasmus Villemoes <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
Hello:
This series was applied to netdev/net-next.git (master)
by Jakub Kicinski <[email protected]>:
On Wed, 16 Nov 2022 11:52:01 +0100 you wrote:
> The intention of commit 685343fc3ba6 ("net: add name_assign_type
> netdev attribute") was clearly that drivers be switched over one by
> one to select appropriate NET_NAME_* constants instead of
> NET_NAME_UNKNOWN. This small series attempts to do that for DSA user
> ports.
>
> This is obviously and intentionally user-visible changes, so there's a
> small chance that it could lead to a regression. To make it easy to
> revert either of the "label in DT" and "fallback to eth%d" changes,
> this is done as a refactoring which shouldn't introduce any functional
> change (but by itself adds code which looks a little odd, with the two
> identical assignments in the two branches), followed by changing the
> constant used in each case in two different patches.
>
> [...]
Here is the summary with links:
- [v3,1/3] net: dsa: refactor name assignment for user ports
https://git.kernel.org/netdev/net-next/c/0171a1d22bb9
- [v3,2/3] net: dsa: use NET_NAME_PREDICTABLE for user ports with name given in DT
https://git.kernel.org/netdev/net-next/c/6fdb03842040
- [v3,3/3] net: dsa: set name_assign_type to NET_NAME_ENUM for enumerated user ports
https://git.kernel.org/netdev/net-next/c/b8790661d90d
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html