[PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[Vivien Didelot]

In the new DTS bindings for DSA (dsa2), the "ethernet" and "link"
phandles are respectively mandatory and exclusive to CPU port and DSA
link device tree nodes.

Simplify dsa2.c a bit by checking the presence of such phandle instead
of checking the redundant "label" property.

Then the Linux philosophy for Ethernet switch ports is to expose them to
userspace as standard NICs by default. Thus use the standard enumerated
"eth%d" device name if no "label" property is provided for a user port.
This allows to save DTS files from subjective net device names.

Here's an example on a ZII Dev Rev B board without "label" properties:

    # ip link | grep ': ' | cut -d: -f2
     lo
     eth0
     eth1
     eth2@eth1
     eth3@eth1
     eth4@eth1
     eth5@eth1
     eth6@eth1
     eth7@eth1
     eth8@eth1
     eth9@eth1
     eth10@eth1
     eth11@eth1
     eth12@eth1

If one wants to rename an interface, udev rules can be used as usual, as
suggested in the switchdev documentation:

    # cat /etc/udev/rules.d/90-net-dsa.rules
    SUBSYSTEM=="net", ACTION=="add", ENV{DEVTYPE}=="dsa", NAME="sw$attr{phys_switch_id}p$attr{phys_port_id}"

    # ip link | awk '/@eth/ { split($2,a,"@"); print a[1]; }'
    sw00000000p00
    sw00000000p01
    sw00000000p02
    sw01000000p00
    sw01000000p01
    sw01000000p02
    sw02000000p00
    sw02000000p01
    sw02000000p02
    sw02000000p03
    sw02000000p04

Until the printing of netdev_phys_item_id structures is fixed in
net/core/net-sysfs.c, an external helper can be used like this:

    # cat /etc/udev/rules.d/90-net-dsa.rules
    SUBSYSTEM=="net", ACTION=="add", ENV{DEVTYPE}=="dsa", PROGRAM="/lib/udev/dsanitizer $attr{phys_switch_id} $attr{phys_port_id}", NAME="$result"

    # cat /lib/udev/dsanitizer
    #!/bin/sh
    echo $1 | sed -e 's,^0*,,' -e 's,0*$,,' | xargs printf sw%d
    echo $2 | sed -e 's,^0*,,' | xargs printf p%d

    # ip link | awk '/@eth/ { split($2,a,"@"); print a[1]; }'
    sw0p0
    sw0p1
    sw0p2
    sw1p0
    sw1p1
    sw1p2
    sw2p0
    sw2p1
    sw2p2
    sw2p3
    sw2p4

Of course the current behavior is unchanged, and the optional "label"
property for user ports has precedence over the enumerated name.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 Documentation/devicetree/bindings/net/dsa/dsa.txt | 20 ++++++++-----------
 net/dsa/dsa2.c                                    | 24 ++++-------------------
 2 files changed, 12 insertions(+), 32 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.txt b/Documentation/devicetree/bindings/net/dsa/dsa.txt
index a4a570fb2494..cfe8f64eca4f 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.txt
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.txt
@@ -34,13 +34,9 @@ Required properties:
 
 Each port children node must have the following mandatory properties:
 - reg			: Describes the port address in the switch
-- label			: Describes the label associated with this port, which
-                          will become the netdev name. Special labels are
-			  "cpu" to indicate a CPU port and "dsa" to
-			  indicate an uplink/downlink port between switches in
-			  the cluster.
 
-A port labelled "dsa" has the following mandatory property:
+An uplink/downlink port between switches in the cluster has the following
+mandatory property:
 
 - link			: Should be a list of phandles to other switch's DSA
 			  port. This port is used as the outgoing port
@@ -48,12 +44,17 @@ A port labelled "dsa" has the following mandatory property:
 			  information must be given, not just the one hop
 			  routes to neighbouring switches.
 
-A port labelled "cpu" has the following mandatory property:
+A CPU port has the following mandatory property:
 
 - ethernet		: Should be a phandle to a valid Ethernet device node.
                           This host device is what the switch port is
 			  connected to.
 
+A user port has the following optional property:
+
+- label			: Describes the label associated with this port, which
+                          will become the netdev name.
+
 Port child nodes may also contain the following optional standardised
 properties, described in binding documents:
 
@@ -107,7 +108,6 @@ linked into one DSA cluster.
 
 			switch0port5: port@5 {
 				reg = <5>;
-				label = "dsa";
 				phy-mode = "rgmii-txid";
 				link = <&switch1port6
 					&switch2port9>;
@@ -119,7 +119,6 @@ linked into one DSA cluster.
 
 			port@6 {
 				reg = <6>;
-				label = "cpu";
 				ethernet = <&fec1>;
 				fixed-link {
 					speed = <100>;
@@ -165,7 +164,6 @@ linked into one DSA cluster.
 
 			switch1port5: port@5 {
 				reg = <5>;
-				label = "dsa";
 				link = <&switch2port9>;
 				phy-mode = "rgmii-txid";
 				fixed-link {
@@ -176,7 +174,6 @@ linked into one DSA cluster.
 
 			switch1port6: port@6 {
 				reg = <6>;
-				label = "dsa";
 				phy-mode = "rgmii-txid";
 				link = <&switch0port5>;
 				fixed-link {
@@ -255,7 +252,6 @@ linked into one DSA cluster.
 
 			switch2port9: port@9 {
 				reg = <9>;
-				label = "dsa";
 				phy-mode = "rgmii-txid";
 				link = <&switch1port5
 					&switch0port5>;
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index bad119cee2a3..9526bdf2a34a 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -81,30 +81,12 @@ static void dsa_dst_del_ds(struct dsa_switch_tree *dst,
 
 static bool dsa_port_is_dsa(struct device_node *port)
 {
-	const char *name;
-
-	name = of_get_property(port, "label", NULL);
-	if (!name)
-		return false;
-
-	if (!strcmp(name, "dsa"))
-		return true;
-
-	return false;
+	return !!of_parse_phandle(port, "link", 0);
 }
 
 static bool dsa_port_is_cpu(struct device_node *port)
 {
-	const char *name;
-
-	name = of_get_property(port, "label", NULL);
-	if (!name)
-		return false;
-
-	if (!strcmp(name, "cpu"))
-		return true;
-
-	return false;
+	return !!of_parse_phandle(port, "ethernet", 0);
 }
 
 static bool dsa_ds_find_port(struct dsa_switch *ds,
@@ -268,6 +250,8 @@ static int dsa_user_port_apply(struct device_node *port, u32 index,
 	int err;
 
 	name = of_get_property(port, "label", NULL);
+	if (!name)
+		name = "eth%d";
 
 	err = dsa_slave_create(ds, ds->dev, index, name);
 	if (err) {
-- 
2.11.0

Re: [PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[Andrew Lunn]

> Until the printing of netdev_phys_item_id structures is fixed in
> net/core/net-sysfs.c, an external helper can be used like this:

Hi Vivien

As Florian pointed out, this cannot be changed. It is now part of the
ABI. We have to live with it printing little endian numbers as big
endian.

>     # cat /etc/udev/rules.d/90-net-dsa.rules
>     SUBSYSTEM=="net", ACTION=="add", ENV{DEVTYPE}=="dsa", PROGRAM="/lib/udev/dsanitizer $attr{phys_switch_id} $attr{phys_port_id}", NAME="$result"
> 
>     # cat /lib/udev/dsanitizer
>     #!/bin/sh
>     echo $1 | sed -e 's,^0*,,' -e 's,0*$,,' | xargs printf sw%d
>     echo $2 | sed -e 's,^0*,,' | xargs printf p%d
> 
>     # ip link | awk '/@eth/ { split($2,a,"@"); print a[1]; }'
>     sw0p0
>     sw0p1
>     sw0p2
>     sw1p0
>     sw1p1
>     sw1p2
>     sw2p0
>     sw2p1
>     sw2p2
>     sw2p3
>     sw2p4

Rather than recommending something, it might be better to point to the
Free Desktop "Predictable Network Interface Names" which is what most
people will end up with, if they rename:

https://www.freedesktop.org/wiki/Software/systemd/PredictableNetworkInterfaceNames/

It would also be good to test on a recent systemd system and see what
happens. What names does it pick?

	Andrew

Re: [PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> Until the printing of netdev_phys_item_id structures is fixed in
>> net/core/net-sysfs.c, an external helper can be used like this:
>
> As Florian pointed out, this cannot be changed. It is now part of the
> ABI. We have to live with it printing little endian numbers as big
> endian.

I totally understand the fact that ABI must not be changed. However we
should be aware that the current phys_switch_id of DSA is broken.

In addition to the minor issue of being hardly useable, it does not meet
the requirement described in the switchdev documentation of being unique
on a system. A switch ID in DSA is currently unique only to a DSA tree.
A system with two disjoint switch trees will have two switches with a
phys_switch_id of "00000000".

> Rather than recommending something, it might be better to point to the
> Free Desktop "Predictable Network Interface Names" which is what most
> people will end up with, if they rename:
>
> https://www.freedesktop.org/wiki/Software/systemd/PredictableNetworkInterfaceNames/
>
> It would also be good to test on a recent systemd system and see what
> happens. What names does it pick?

Note that the udev rules I gave in this commit message were only there
as examples of renaming DSA slave interfaces from userspace. This is
orthogonal with the purpose of this patch.

Thanks,

        Vivien

Re: [PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[Jiri Pirko]

Mon, Jan 09, 2017 at 12:15:52AM CET, vivien.didelot@savoirfairelinux.com wrote:
>In the new DTS bindings for DSA (dsa2), the "ethernet" and "link"
>phandles are respectively mandatory and exclusive to CPU port and DSA
>link device tree nodes.
>
>Simplify dsa2.c a bit by checking the presence of such phandle instead
>of checking the redundant "label" property.
>
>Then the Linux philosophy for Ethernet switch ports is to expose them to
>userspace as standard NICs by default. Thus use the standard enumerated
>"eth%d" device name if no "label" property is provided for a user port.
>This allows to save DTS files from subjective net device names.
>
>Here's an example on a ZII Dev Rev B board without "label" properties:
>
>    # ip link | grep ': ' | cut -d: -f2
>     lo
>     eth0
>     eth1
>     eth2@eth1
>     eth3@eth1
>     eth4@eth1
>     eth5@eth1
>     eth6@eth1
>     eth7@eth1
>     eth8@eth1
>     eth9@eth1
>     eth10@eth1
>     eth11@eth1
>     eth12@eth1
>
>If one wants to rename an interface, udev rules can be used as usual, as
>suggested in the switchdev documentation:
>
>    # cat /etc/udev/rules.d/90-net-dsa.rules
>    SUBSYSTEM=="net", ACTION=="add", ENV{DEVTYPE}=="dsa", NAME="sw$attr{phys_switch_id}p$attr{phys_port_id}"
>
>    # ip link | awk '/@eth/ { split($2,a,"@"); print a[1]; }'
>    sw00000000p00
>    sw00000000p01
>    sw00000000p02
>    sw01000000p00
>    sw01000000p01
>    sw01000000p02
>    sw02000000p00
>    sw02000000p01
>    sw02000000p02
>    sw02000000p03
>    sw02000000p04
>
>Until the printing of netdev_phys_item_id structures is fixed in
>net/core/net-sysfs.c, an external helper can be used like this:
>
>    # cat /etc/udev/rules.d/90-net-dsa.rules
>    SUBSYSTEM=="net", ACTION=="add", ENV{DEVTYPE}=="dsa", PROGRAM="/lib/udev/dsanitizer $attr{phys_switch_id} $attr{phys_port_id}", NAME="$result"

I know this is kind of confusing, but phys_port_id is to be used to
indicate same physical port that is shared by multiple netdevices- for
example sr-iov usecase. For switchdev usecase, you should use
phys_port_name.

I will add some documentation to kernel regarding this. But I see that
net/dsa/slave.c already implements .ndo_get_phys_port_id :(

I recently made changes in udev so it names the switch ports according
to phys_port_name, out of the box, without need for any rules:
https://github.com/systemd/systemd/pull/4506/commits/c960caa0c2a620fc506c6f0f7b6c40eeace48e4d

I guess that it should be enough for you to implement
ndo_get_phys_port_name.





>
>    # cat /lib/udev/dsanitizer
>    #!/bin/sh
>    echo $1 | sed -e 's,^0*,,' -e 's,0*$,,' | xargs printf sw%d
>    echo $2 | sed -e 's,^0*,,' | xargs printf p%d
>
>    # ip link | awk '/@eth/ { split($2,a,"@"); print a[1]; }'
>    sw0p0
>    sw0p1
>    sw0p2
>    sw1p0
>    sw1p1
>    sw1p2
>    sw2p0
>    sw2p1
>    sw2p2
>    sw2p3
>    sw2p4
>
>Of course the current behavior is unchanged, and the optional "label"
>property for user ports has precedence over the enumerated name.
>
>Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
>Acked-by: Uwe Kleine-König <uwe@kleine-koenig.org>
>---
> Documentation/devicetree/bindings/net/dsa/dsa.txt | 20 ++++++++-----------
> net/dsa/dsa2.c                                    | 24 ++++-------------------
> 2 files changed, 12 insertions(+), 32 deletions(-)
>
>diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.txt b/Documentation/devicetree/bindings/net/dsa/dsa.txt
>index a4a570fb2494..cfe8f64eca4f 100644
>--- a/Documentation/devicetree/bindings/net/dsa/dsa.txt
>+++ b/Documentation/devicetree/bindings/net/dsa/dsa.txt
>@@ -34,13 +34,9 @@ Required properties:
> 
> Each port children node must have the following mandatory properties:
> - reg			: Describes the port address in the switch
>-- label			: Describes the label associated with this port, which
>-                          will become the netdev name. Special labels are
>-			  "cpu" to indicate a CPU port and "dsa" to
>-			  indicate an uplink/downlink port between switches in
>-			  the cluster.
> 
>-A port labelled "dsa" has the following mandatory property:
>+An uplink/downlink port between switches in the cluster has the following
>+mandatory property:
> 
> - link			: Should be a list of phandles to other switch's DSA
> 			  port. This port is used as the outgoing port
>@@ -48,12 +44,17 @@ A port labelled "dsa" has the following mandatory property:
> 			  information must be given, not just the one hop
> 			  routes to neighbouring switches.
> 
>-A port labelled "cpu" has the following mandatory property:
>+A CPU port has the following mandatory property:
> 
> - ethernet		: Should be a phandle to a valid Ethernet device node.
>                           This host device is what the switch port is
> 			  connected to.
> 
>+A user port has the following optional property:
>+
>+- label			: Describes the label associated with this port, which
>+                          will become the netdev name.
>+
> Port child nodes may also contain the following optional standardised
> properties, described in binding documents:
> 
>@@ -107,7 +108,6 @@ linked into one DSA cluster.
> 
> 			switch0port5: port@5 {
> 				reg = <5>;
>-				label = "dsa";
> 				phy-mode = "rgmii-txid";
> 				link = <&switch1port6
> 					&switch2port9>;
>@@ -119,7 +119,6 @@ linked into one DSA cluster.
> 
> 			port@6 {
> 				reg = <6>;
>-				label = "cpu";
> 				ethernet = <&fec1>;
> 				fixed-link {
> 					speed = <100>;
>@@ -165,7 +164,6 @@ linked into one DSA cluster.
> 
> 			switch1port5: port@5 {
> 				reg = <5>;
>-				label = "dsa";
> 				link = <&switch2port9>;
> 				phy-mode = "rgmii-txid";
> 				fixed-link {
>@@ -176,7 +174,6 @@ linked into one DSA cluster.
> 
> 			switch1port6: port@6 {
> 				reg = <6>;
>-				label = "dsa";
> 				phy-mode = "rgmii-txid";
> 				link = <&switch0port5>;
> 				fixed-link {
>@@ -255,7 +252,6 @@ linked into one DSA cluster.
> 
> 			switch2port9: port@9 {
> 				reg = <9>;
>-				label = "dsa";
> 				phy-mode = "rgmii-txid";
> 				link = <&switch1port5
> 					&switch0port5>;
>diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
>index bad119cee2a3..9526bdf2a34a 100644
>--- a/net/dsa/dsa2.c
>+++ b/net/dsa/dsa2.c
>@@ -81,30 +81,12 @@ static void dsa_dst_del_ds(struct dsa_switch_tree *dst,
> 
> static bool dsa_port_is_dsa(struct device_node *port)
> {
>-	const char *name;
>-
>-	name = of_get_property(port, "label", NULL);
>-	if (!name)
>-		return false;
>-
>-	if (!strcmp(name, "dsa"))
>-		return true;
>-
>-	return false;
>+	return !!of_parse_phandle(port, "link", 0);
> }
> 
> static bool dsa_port_is_cpu(struct device_node *port)
> {
>-	const char *name;
>-
>-	name = of_get_property(port, "label", NULL);
>-	if (!name)
>-		return false;
>-
>-	if (!strcmp(name, "cpu"))
>-		return true;
>-
>-	return false;
>+	return !!of_parse_phandle(port, "ethernet", 0);
> }
> 
> static bool dsa_ds_find_port(struct dsa_switch *ds,
>@@ -268,6 +250,8 @@ static int dsa_user_port_apply(struct device_node *port, u32 index,
> 	int err;
> 
> 	name = of_get_property(port, "label", NULL);
>+	if (!name)
>+		name = "eth%d";
> 
> 	err = dsa_slave_create(ds, ds->dev, index, name);
> 	if (err) {
>-- 
>2.11.0
>

Re: [PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[Vivien Didelot]

Hi Jiri,

Jiri Pirko <jiri@resnulli.us> writes:

>>    # cat /etc/udev/rules.d/90-net-dsa.rules
>>    SUBSYSTEM=="net", ACTION=="add", ENV{DEVTYPE}=="dsa", PROGRAM="/lib/udev/dsanitizer $attr{phys_switch_id} $attr{phys_port_id}", NAME="$result"
>
> I know this is kind of confusing, but phys_port_id is to be used to
> indicate same physical port that is shared by multiple netdevices- for
> example sr-iov usecase. For switchdev usecase, you should use
> phys_port_name.
>
> I will add some documentation to kernel regarding this. But I see that
> net/dsa/slave.c already implements .ndo_get_phys_port_id :(
>
> I recently made changes in udev so it names the switch ports according
> to phys_port_name, out of the box, without need for any rules:
> https://github.com/systemd/systemd/pull/4506/commits/c960caa0c2a620fc506c6f0f7b6c40eeace48e4d

Thanks for the details. So if I understand correctly, what will be found
in phys_port_name will be used as is by udev to name the interface?

Extra question: shouldn't phys_port_{id,name} be switchdev attributes in
addition to SWITCHDEV_ATTR_ID_PORT_PARENT_ID?

> I guess that it should be enough for you to implement
> ndo_get_phys_port_name.

Well, if this name must be unique on a system, it's not likely to happen
until we agree that we use an ugly tXsYpZ template where X is a tree ID,
or we assign system-wide unique IDs to switches, which requires a bit of
changes.

I'm thinking, since DSA slaves are switchdev users, can't we all use a
switchdev helper to optionally get a system-wide unique name for a given
switch port? e.g. a template such as "swp%d"? I'd prefer that switchdev
and DSA do not diverge much when it comes to implement such attributes.

But again, this is not related to this patch ;-)

Thanks,

        Vivien

Re: [PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[Jiri Pirko]

Mon, Jan 09, 2017 at 04:04:50PM CET, vivien.didelot@savoirfairelinux.com wrote:
>Hi Jiri,
>
>Jiri Pirko <jiri@resnulli.us> writes:
>
>>>    # cat /etc/udev/rules.d/90-net-dsa.rules
>>>    SUBSYSTEM=="net", ACTION=="add", ENV{DEVTYPE}=="dsa", PROGRAM="/lib/udev/dsanitizer $attr{phys_switch_id} $attr{phys_port_id}", NAME="$result"
>>
>> I know this is kind of confusing, but phys_port_id is to be used to
>> indicate same physical port that is shared by multiple netdevices- for
>> example sr-iov usecase. For switchdev usecase, you should use
>> phys_port_name.
>>
>> I will add some documentation to kernel regarding this. But I see that
>> net/dsa/slave.c already implements .ndo_get_phys_port_id :(
>>
>> I recently made changes in udev so it names the switch ports according
>> to phys_port_name, out of the box, without need for any rules:
>> https://github.com/systemd/systemd/pull/4506/commits/c960caa0c2a620fc506c6f0f7b6c40eeace48e4d
>
>Thanks for the details. So if I understand correctly, what will be found
>in phys_port_name will be used as is by udev to name the interface?

Yes.


>
>Extra question: shouldn't phys_port_{id,name} be switchdev attributes in

Again, phys_port_id has nothing to do with switches. Should be removed
from dsa because its use there is incorrect.


>addition to SWITCHDEV_ATTR_ID_PORT_PARENT_ID?

Perhaps it could. Now it is a netdev op. I thinks it works nicely.


>
>> I guess that it should be enough for you to implement
>> ndo_get_phys_port_name.
>
>Well, if this name must be unique on a system, it's not likely to happen
>until we agree that we use an ugly tXsYpZ template where X is a tree ID,
>or we assign system-wide unique IDs to switches, which requires a bit of
>changes.

No. That should be unique within one switch. In mlxsw we name it "p1",
"p2", ...

The final netdev names are:
enp3s0np1, enp3s0np2, ...


>
>I'm thinking, since DSA slaves are switchdev users, can't we all use a
>switchdev helper to optionally get a system-wide unique name for a given
>switch port? e.g. a template such as "swp%d"? I'd prefer that switchdev
>and DSA do not diverge much when it comes to implement such attributes.

Again, not system-wide, just switch-wide.


>
>But again, this is not related to this patch ;-)

It is! You are using phys_port_id, which is completely wrong. You should
not use it.

Re: [PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[Vivien Didelot]

Hi Jiri,

Jiri Pirko <jiri@resnulli.us> writes:

>>Extra question: shouldn't phys_port_{id,name} be switchdev attributes in
>
> Again, phys_port_id has nothing to do with switches. Should be removed
> from dsa because its use there is incorrect.

Florian, since 3a543ef just got in, can it be reverted?

>>> I guess that it should be enough for you to implement
>>> ndo_get_phys_port_name.
>>
>>Well, if this name must be unique on a system, it's not likely to happen
>>until we agree that we use an ugly tXsYpZ template where X is a tree ID,
>>or we assign system-wide unique IDs to switches, which requires a bit of
>>changes.
>
> No. That should be unique within one switch. In mlxsw we name it "p1",
> "p2", ...
>
> The final netdev names are:
> enp3s0np1, enp3s0np2, ...

OK perfect then, "p%d" sounds good. You seems to avoid "p0" in mlxsw, is
there a reason for that?

>>But again, this is not related to this patch ;-)
>
> It is! You are using phys_port_id, which is completely wrong. You should
> not use it.

I can resend this patch without the udev examples in the commit message
if that can be less confusing.

Thanks,

        Vivien

Re: [PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[Andrew Lunn]

> > No. That should be unique within one switch. In mlxsw we name it "p1",
> > "p2", ...
> >
> > The final netdev names are:
> > enp3s0np1, enp3s0np2, ...
> 

mlxsw are pci devices, so it follows this convention, i think:

 *   [P<domain>]p<bus>s<slot>[f<function>][n<phys_port_name>|d<dev_port>]
 *                                          PCI geographical location

Our devices are not on PCI. So they won't follow this. I've no idea
what they actually follow, since some are MDIO devices, some are SPI
devices, some are memory mapped.

I'm not against making the label option, but i do want to better
understand what we get as a result, just to make sure it is sensible.

Vivien, could you try a recent udev and see what happens?

	Thanks
		Andrew

Re: [PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[Jiri Pirko]

Mon, Jan 09, 2017 at 04:45:33PM CET, vivien.didelot@savoirfairelinux.com wrote:
>Hi Jiri,
>
>Jiri Pirko <jiri@resnulli.us> writes:
>
>>>Extra question: shouldn't phys_port_{id,name} be switchdev attributes in
>>
>> Again, phys_port_id has nothing to do with switches. Should be removed
>> from dsa because its use there is incorrect.
>
>Florian, since 3a543ef just got in, can it be reverted?

Yes, please revert it. It is only in net-next.


>
>>>> I guess that it should be enough for you to implement
>>>> ndo_get_phys_port_name.
>>>
>>>Well, if this name must be unique on a system, it's not likely to happen
>>>until we agree that we use an ugly tXsYpZ template where X is a tree ID,
>>>or we assign system-wide unique IDs to switches, which requires a bit of
>>>changes.
>>
>> No. That should be unique within one switch. In mlxsw we name it "p1",
>> "p2", ...
>>
>> The final netdev names are:
>> enp3s0np1, enp3s0np2, ...
>
>OK perfect then, "p%d" sounds good. You seems to avoid "p0" in mlxsw, is
>there a reason for that?

We name these according to the front panel name. There's no "port 0"
on the front panel :)


>
>>>But again, this is not related to this patch ;-)
>>
>> It is! You are using phys_port_id, which is completely wrong. You should
>> not use it.
>
>I can resend this patch without the udev examples in the commit message
>if that can be less confusing.

Yes please.


>
>Thanks,
>
>        Vivien

Re: [PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[Jiri Pirko]

Mon, Jan 09, 2017 at 05:00:19PM CET, andrew@lunn.ch wrote:
>> > No. That should be unique within one switch. In mlxsw we name it "p1",
>> > "p2", ...
>> >
>> > The final netdev names are:
>> > enp3s0np1, enp3s0np2, ...
>> 
>
>mlxsw are pci devices, so it follows this convention, i think:
>
> *   [P<domain>]p<bus>s<slot>[f<function>][n<phys_port_name>|d<dev_port>]
> *                                          PCI geographical location
>
>Our devices are not on PCI. So they won't follow this. I've no idea
>what they actually follow, since some are MDIO devices, some are SPI
>devices, some are memory mapped.

Got it. We just have to make sure udev names them appropriately. 


>
>I'm not against making the label option, but i do want to better
>understand what we get as a result, just to make sure it is sensible.
>
>Vivien, could you try a recent udev and see what happens?
>
>	Thanks
>		Andrew

Re: [PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[Florian Fainelli]

On 01/09/2017 08:06 AM, Jiri Pirko wrote:
> Mon, Jan 09, 2017 at 04:45:33PM CET, vivien.didelot@savoirfairelinux.com wrote:
>> Hi Jiri,
>>
>> Jiri Pirko <jiri@resnulli.us> writes:
>>
>>>> Extra question: shouldn't phys_port_{id,name} be switchdev attributes in
>>>
>>> Again, phys_port_id has nothing to do with switches. Should be removed
>>> from dsa because its use there is incorrect.
>>
>> Florian, since 3a543ef just got in, can it be reverted?
> 
> Yes, please revert it. It is only in net-next.

Maybe the use case can be understood before reverting the change. How do
we actually the physical port number of an Ethernet switch per-port
network device? The name is not enough, because there are plenty of
cases where we need to manipulate a physical port number (be it just for
informational purposes).

Should we just amend the existing description of ndo_get_phys_port_id()?
Should we introduce another ndo for that?
-- 
Florian

Re: [PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[Jiri Pirko]

Mon, Jan 09, 2017 at 06:42:07PM CET, f.fainelli@gmail.com wrote:
>On 01/09/2017 08:06 AM, Jiri Pirko wrote:
>> Mon, Jan 09, 2017 at 04:45:33PM CET, vivien.didelot@savoirfairelinux.com wrote:
>>> Hi Jiri,
>>>
>>> Jiri Pirko <jiri@resnulli.us> writes:
>>>
>>>>> Extra question: shouldn't phys_port_{id,name} be switchdev attributes in
>>>>
>>>> Again, phys_port_id has nothing to do with switches. Should be removed
>>>> from dsa because its use there is incorrect.
>>>
>>> Florian, since 3a543ef just got in, can it be reverted?
>> 
>> Yes, please revert it. It is only in net-next.
>
>Maybe the use case can be understood before reverting the change. How do
>we actually the physical port number of an Ethernet switch per-port
>network device? The name is not enough, because there are plenty of
>cases where we need to manipulate a physical port number (be it just for
>informational purposes).

Like what?

Why the name is not enough? This is something propagated to userspace
and never used internally in kernel.

Btw, ndo_get_phys_port_id does not give you number, but arbitrary binary.


>
>Should we just amend the existing description of ndo_get_phys_port_id()?
>Should we introduce another ndo for that?
>-- 
>Florian

Re: [PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[Florian Fainelli]

On 01/09/2017 09:58 AM, Jiri Pirko wrote:
> Mon, Jan 09, 2017 at 06:42:07PM CET, f.fainelli@gmail.com wrote:
>> On 01/09/2017 08:06 AM, Jiri Pirko wrote:
>>> Mon, Jan 09, 2017 at 04:45:33PM CET, vivien.didelot@savoirfairelinux.com wrote:
>>>> Hi Jiri,
>>>>
>>>> Jiri Pirko <jiri@resnulli.us> writes:
>>>>
>>>>>> Extra question: shouldn't phys_port_{id,name} be switchdev attributes in
>>>>>
>>>>> Again, phys_port_id has nothing to do with switches. Should be removed
>>>>> from dsa because its use there is incorrect.
>>>>
>>>> Florian, since 3a543ef just got in, can it be reverted?
>>>
>>> Yes, please revert it. It is only in net-next.
>>
>> Maybe the use case can be understood before reverting the change. How do
>> we actually the physical port number of an Ethernet switch per-port
>> network device? The name is not enough, because there are plenty of
>> cases where we need to manipulate a physical port number (be it just for
>> informational purposes).
> 
> Like what?

Specifying the physical port number (and derive a queue number
eventually) for some ethtool (e.g: rxnfc)/tc (queue mapping) operations
where there is an action/queue/port destination argument that gets
programmed into the hardware.

You already have the originating port number from the interface you call
the method against, but you also need the destination port number since
that is what the HW understands.

Aside from that, it is useful for allowing interface naming in user
space if you don't want to use labels.

> 
> Why the name is not enough? This is something propagated to userspace
> and never used internally in kernel.

Because the name is not reflective of the port number in some switches.
In my case for instance, we have 5 ports that are named after the
entities they connect to (an integrated Gigabit PHY, two RGMII pads, one
MoCA interface, and the CPU)

0 -> gphy
1 -> rgmii_1
2 -> rgmii_2
7 -> moca
8 -> cpu

> 
> Btw, ndo_get_phys_port_id does not give you number, but arbitrary binary.

It's not entirely arbitrary for DSA switches since the port number is
stored in an u8 whose value is the port number in hexadecimal (as shown
by sysfs at least).
-- 
Florian

Re: [PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[Jiri Pirko]

Mon, Jan 09, 2017 at 07:06:39PM CET, f.fainelli@gmail.com wrote:
>On 01/09/2017 09:58 AM, Jiri Pirko wrote:
>> Mon, Jan 09, 2017 at 06:42:07PM CET, f.fainelli@gmail.com wrote:
>>> On 01/09/2017 08:06 AM, Jiri Pirko wrote:
>>>> Mon, Jan 09, 2017 at 04:45:33PM CET, vivien.didelot@savoirfairelinux.com wrote:
>>>>> Hi Jiri,
>>>>>
>>>>> Jiri Pirko <jiri@resnulli.us> writes:
>>>>>
>>>>>>> Extra question: shouldn't phys_port_{id,name} be switchdev attributes in
>>>>>>
>>>>>> Again, phys_port_id has nothing to do with switches. Should be removed
>>>>>> from dsa because its use there is incorrect.
>>>>>
>>>>> Florian, since 3a543ef just got in, can it be reverted?
>>>>
>>>> Yes, please revert it. It is only in net-next.
>>>
>>> Maybe the use case can be understood before reverting the change. How do
>>> we actually the physical port number of an Ethernet switch per-port
>>> network device? The name is not enough, because there are plenty of
>>> cases where we need to manipulate a physical port number (be it just for
>>> informational purposes).
>> 
>> Like what?
>
>Specifying the physical port number (and derive a queue number
>eventually) for some ethtool (e.g: rxnfc)/tc (queue mapping) operations
>where there is an action/queue/port destination argument that gets
>programmed into the hardware.

Could you point me to a real example? User command?


>
>You already have the originating port number from the interface you call
>the method against, but you also need the destination port number since
>that is what the HW understands.

This is internal to kernel? I fail to understand what you mean exactly.


>
>Aside from that, it is useful for allowing interface naming in user
>space if you don't want to use labels.
>
>> 
>> Why the name is not enough? This is something propagated to userspace
>> and never used internally in kernel.
>
>Because the name is not reflective of the port number in some switches.
>In my case for instance, we have 5 ports that are named after the
>entities they connect to (an integrated Gigabit PHY, two RGMII pads, one
>MoCA interface, and the CPU)
>

Again, I'm missing why you need a portnumber as a Integer to userspace.
>From driver, you can expose phys_port_name:

>0 -> gphy

"p0" or "gphy"

>1 -> rgmii_1

"p1" or "rgmii_1"

>2 -> rgmii_2

...

>7 -> moca
>8 -> cpu
>
>> 
>> Btw, ndo_get_phys_port_id does not give you number, but arbitrary binary.
>
>It's not entirely arbitrary for DSA switches since the port number is
>stored in an u8 whose value is the port number in hexadecimal (as shown
>by sysfs at least).

In mlxsw, we also have port number stored in u8. It is used to
communicate with hw of course. But outside the driver, this is exposed
as phys_port_name "pX".

Re: [PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[Florian Fainelli]

On 01/10/2017 01:55 AM, Jiri Pirko wrote:
> Mon, Jan 09, 2017 at 07:06:39PM CET, f.fainelli@gmail.com wrote:
>> On 01/09/2017 09:58 AM, Jiri Pirko wrote:
>>> Mon, Jan 09, 2017 at 06:42:07PM CET, f.fainelli@gmail.com wrote:
>>>> On 01/09/2017 08:06 AM, Jiri Pirko wrote:
>>>>> Mon, Jan 09, 2017 at 04:45:33PM CET, vivien.didelot@savoirfairelinux.com wrote:
>>>>>> Hi Jiri,
>>>>>>
>>>>>> Jiri Pirko <jiri@resnulli.us> writes:
>>>>>>
>>>>>>>> Extra question: shouldn't phys_port_{id,name} be switchdev attributes in
>>>>>>>
>>>>>>> Again, phys_port_id has nothing to do with switches. Should be removed
>>>>>>> from dsa because its use there is incorrect.
>>>>>>
>>>>>> Florian, since 3a543ef just got in, can it be reverted?
>>>>>
>>>>> Yes, please revert it. It is only in net-next.
>>>>
>>>> Maybe the use case can be understood before reverting the change. How do
>>>> we actually the physical port number of an Ethernet switch per-port
>>>> network device? The name is not enough, because there are plenty of
>>>> cases where we need to manipulate a physical port number (be it just for
>>>> informational purposes).
>>>
>>> Like what?
>>
>> Specifying the physical port number (and derive a queue number
>> eventually) for some ethtool (e.g: rxnfc)/tc (queue mapping) operations
>> where there is an action/queue/port destination argument that gets
>> programmed into the hardware.
> 
> Could you point me to a real example? User command?

ethtool --config-nfc moca flow-type udp4 src-ip 192.168.1.20 dst-ip \
        192.168.1.10 src-port 49884 dst-port 5001 action 2

Where 2 here designates a port number, users need to be able to look up
the physical port number corresponding to an interface to know which
value to put in this command.

Yes I know we can do the same thing with cls_flower, possibly by
referencing network devices directly.

> 
> 
>>
>> You already have the originating port number from the interface you call
>> the method against, but you also need the destination port number since
>> that is what the HW understands.
> 
> This is internal to kernel? I fail to understand what you mean exactly.

See the command above, from using the "moca" netdev here, we can access
the DSA private network device (dsa_slave_priv) structure and get the
port number from there, and pass this down to the switch driver. The
switch driver also takes another port number (and eventually a queue
number) to program classification filters.

> 
> 
>>
>> Aside from that, it is useful for allowing interface naming in user
>> space if you don't want to use labels.
>>
>>>
>>> Why the name is not enough? This is something propagated to userspace
>>> and never used internally in kernel.
>>
>> Because the name is not reflective of the port number in some switches.
>> In my case for instance, we have 5 ports that are named after the
>> entities they connect to (an integrated Gigabit PHY, two RGMII pads, one
>> MoCA interface, and the CPU)
>>
> 
> Again, I'm missing why you need a portnumber as a Integer to userspace.
> From driver, you can expose phys_port_name:

If we are exposing the port name here, we may as well expose the DSA
"label" instead of the physical port number number?

I don't deny my change may be misusing what phys_port_id was originally
designed for, but providing "p0" instead of "0" to user-space, what
value is there in adding the "p" in front really?
-- 
Florian

Re: [PATCH net-next v2] net: dsa: make "label" property optional for dsa2

[Jiri Pirko]

Tue, Jan 10, 2017 at 06:58:18PM CET, f.fainelli@gmail.com wrote:
>On 01/10/2017 01:55 AM, Jiri Pirko wrote:
>> Mon, Jan 09, 2017 at 07:06:39PM CET, f.fainelli@gmail.com wrote:
>>> On 01/09/2017 09:58 AM, Jiri Pirko wrote:
>>>> Mon, Jan 09, 2017 at 06:42:07PM CET, f.fainelli@gmail.com wrote:
>>>>> On 01/09/2017 08:06 AM, Jiri Pirko wrote:
>>>>>> Mon, Jan 09, 2017 at 04:45:33PM CET, vivien.didelot@savoirfairelinux.com wrote:
>>>>>>> Hi Jiri,
>>>>>>>
>>>>>>> Jiri Pirko <jiri@resnulli.us> writes:
>>>>>>>
>>>>>>>>> Extra question: shouldn't phys_port_{id,name} be switchdev attributes in
>>>>>>>>
>>>>>>>> Again, phys_port_id has nothing to do with switches. Should be removed
>>>>>>>> from dsa because its use there is incorrect.
>>>>>>>
>>>>>>> Florian, since 3a543ef just got in, can it be reverted?
>>>>>>
>>>>>> Yes, please revert it. It is only in net-next.
>>>>>
>>>>> Maybe the use case can be understood before reverting the change. How do
>>>>> we actually the physical port number of an Ethernet switch per-port
>>>>> network device? The name is not enough, because there are plenty of
>>>>> cases where we need to manipulate a physical port number (be it just for
>>>>> informational purposes).
>>>>
>>>> Like what?
>>>
>>> Specifying the physical port number (and derive a queue number
>>> eventually) for some ethtool (e.g: rxnfc)/tc (queue mapping) operations
>>> where there is an action/queue/port destination argument that gets
>>> programmed into the hardware.
>> 
>> Could you point me to a real example? User command?
>
>ethtool --config-nfc moca flow-type udp4 src-ip 192.168.1.20 dst-ip \
>        192.168.1.10 src-port 49884 dst-port 5001 action 2
>
>Where 2 here designates a port number, users need to be able to look up
>the physical port number corresponding to an interface to know which
>value to put in this command.

2 is not a port number but RX queue number. I believe you need to
ditinguish that and port_name. Not sure how are they related.



>
>Yes I know we can do the same thing with cls_flower, possibly by
>referencing network devices directly.

Yes, that is what you should do. I believe that using config-nfc is not
correct for this use-case.


>
>> 
>> 
>>>
>>> You already have the originating port number from the interface you call
>>> the method against, but you also need the destination port number since
>>> that is what the HW understands.
>> 
>> This is internal to kernel? I fail to understand what you mean exactly.
>
>See the command above, from using the "moca" netdev here, we can access
>the DSA private network device (dsa_slave_priv) structure and get the
>port number from there, and pass this down to the switch driver. The
>switch driver also takes another port number (and eventually a queue
>number) to program classification filters.
>
>> 
>> 
>>>
>>> Aside from that, it is useful for allowing interface naming in user
>>> space if you don't want to use labels.
>>>
>>>>
>>>> Why the name is not enough? This is something propagated to userspace
>>>> and never used internally in kernel.
>>>
>>> Because the name is not reflective of the port number in some switches.
>>> In my case for instance, we have 5 ports that are named after the
>>> entities they connect to (an integrated Gigabit PHY, two RGMII pads, one
>>> MoCA interface, and the CPU)
>>>
>> 
>> Again, I'm missing why you need a portnumber as a Integer to userspace.
>> From driver, you can expose phys_port_name:
>
>If we are exposing the port name here, we may as well expose the DSA
>"label" instead of the physical port number number?

Yeah, that makes sense.


>
>I don't deny my change may be misusing what phys_port_id was originally
>designed for, but providing "p0" instead of "0" to user-space, what
>value is there in adding the "p" in front really?

It's up to a driver. He knows how the front panel names look like.


>-- 
>Florian