[PATCH net-next 0/2] net: dsa: make "label" property optional

[PATCH net-next 0/2] net: dsa: make "label" property optional

[Vivien Didelot]

Patch 1/2 makes the "label" property in new DSA bindings optional. This
doesn't change the current behavior with existing DTS files.

As Linux considers the Ethernet switch ports as normal NICs by default,
not providing a "label" property for user ports results in using the
standard "ethX" network device name. Giving a "label" overwrites this.

Patch 2/2 removes the labels for the ZII Rev B board as an example.

Vivien Didelot (2):
  net: dsa: make "label" property optional for dsa2
  arm: dts: vf610-zii-dev-rev-b: remove ports label

 Documentation/devicetree/bindings/net/dsa/dsa.txt | 20 ++++++++-----------
 arch/arm/boot/dts/vf610-zii-dev-rev-b.dts         | 16 ---------------
 net/dsa/dsa2.c                                    | 24 ++++-------------------
 3 files changed, 12 insertions(+), 48 deletions(-)

-- 
2.11.0

[PATCH net-next 1/2] 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.

If one wants to rename an interface, udev rules can be used as usual.
The sysfs phys_port_id and phys_switch_id also provide physical data.

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>
---
 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 1/2] net: dsa: make "label" property optional for dsa2

[Andrew Lunn]

> If one wants to rename an interface, udev rules can be used as usual.

Hi Vivien

Do you have some examples?

A quick look at udevadm info suggests we can use

ATTR{phys_port_id} and ATTR{phys_switch_id}

Humm, it would be nice to know why the second switch has a
phys_switch_id of 01000000.

How is systemd naming them without udev rules?

    Andrew

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

[Florian Fainelli]

On 01/06/2017 02:20 PM, Andrew Lunn wrote:
>> If one wants to rename an interface, udev rules can be used as usual.
> 
> Hi Vivien
> 
> Do you have some examples?
> 
> A quick look at udevadm info suggests we can use
> 
> ATTR{phys_port_id} and ATTR{phys_switch_id}
> 
> Humm, it would be nice to know why the second switch has a
> phys_switch_id of 01000000.

Well, that's a combination of being on a little endian system and using
%*phN as a printk formatter, and the type being 32-bit long (int). I
agree this does not feel natural, but since this is sysfs (so ABI in a
way), it's going to be though to change now.
-- 
Florian

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

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1049 bytes --]

Hello,

On 01/06/2017 11:47 PM, Florian Fainelli wrote:
> On 01/06/2017 02:20 PM, Andrew Lunn wrote:
>>> If one wants to rename an interface, udev rules can be used as usual.
>>
>> Hi Vivien
>>
>> Do you have some examples?
>>
>> A quick look at udevadm info suggests we can use
>>
>> ATTR{phys_port_id} and ATTR{phys_switch_id}
>>
>> Humm, it would be nice to know why the second switch has a
>> phys_switch_id of 01000000.
> 
> Well, that's a combination of being on a little endian system and using
> %*phN as a printk formatter, and the type being 32-bit long (int). I
> agree this does not feel natural, but since this is sysfs (so ABI in a
> way), it's going to be though to change now.

for most subsystems I know the internally used (and more or less
automatically determined) numbers are not to be considered stable.
For example the GPIO made it possible to let userspace work with GPIOs
without knowing (or having to determine) the gpio numbers.

I don't know if/how this applies here.

Best regards
Uwe



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1087 bytes --]

Hello,

On 01/06/2017 11:00 PM, Vivien Didelot 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.
> 
> If one wants to rename an interface, udev rules can be used as usual.
> The sysfs phys_port_id and phys_switch_id also provide physical data.
> 
> 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>

Thanks
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH net-next 2/2] arm: dts: vf610-zii-dev-rev-b: remove ports label

[Vivien Didelot]

Now that the "label" property is optional for Ethernet switch ports,
remove them in the ZII Dev Rev B board DTS.

On a Rev B board, once eth1 is up, this DTS now exposes to userspace:

    # 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

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 arch/arm/boot/dts/vf610-zii-dev-rev-b.dts | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
index 7ea617e47fe4..f9c8810aed7c 100644
--- a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
+++ b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
@@ -104,25 +104,21 @@
 					#size-cells = <0>;
 					port@0 {
 						reg = <0>;
-						label = "lan0";
 						phy-handle = <&switch0phy0>;
 					};
 
 					port@1 {
 						reg = <1>;
-						label = "lan1";
 						phy-handle = <&switch0phy1>;
 					};
 
 					port@2 {
 						reg = <2>;
-						label = "lan2";
 						phy-handle = <&switch0phy2>;
 					};
 
 					switch0port5: port@5 {
 						reg = <5>;
-						label = "dsa";
 						phy-mode = "rgmii-txid";
 						link = <&switch1port6
 							&switch2port9>;
@@ -134,7 +130,6 @@
 
 					port@6 {
 						reg = <6>;
-						label = "cpu";
 						ethernet = <&fec1>;
 						fixed-link {
 							speed = <100>;
@@ -186,25 +181,21 @@
 					#size-cells = <0>;
 					port@0 {
 						reg = <0>;
-						label = "lan3";
 						phy-handle = <&switch1phy0>;
 					};
 
 					port@1 {
 						reg = <1>;
-						label = "lan4";
 						phy-handle = <&switch1phy1>;
 					};
 
 					port@2 {
 						reg = <2>;
-						label = "lan5";
 						phy-handle = <&switch1phy2>;
 					};
 
 					switch1port5: port@5 {
 						reg = <5>;
-						label = "dsa";
 						link = <&switch2port9>;
 						phy-mode = "rgmii-txid";
 						fixed-link {
@@ -215,7 +206,6 @@
 
 					switch1port6: port@6 {
 						reg = <6>;
-						label = "dsa";
 						phy-mode = "rgmii-txid";
 						link = <&switch0port5>;
 						fixed-link {
@@ -263,22 +253,18 @@
 					#size-cells = <0>;
 					port@0 {
 						reg = <0>;
-						label = "lan6";
 					};
 
 					port@1 {
 						reg = <1>;
-						label = "lan7";
 					};
 
 					port@2 {
 						reg = <2>;
-						label = "lan8";
 					};
 
 					port@3 {
 						reg = <3>;
-						label = "optical3";
 						fixed-link {
 							speed = <1000>;
 							full-duplex;
@@ -289,7 +275,6 @@
 
 					port@4 {
 						reg = <4>;
-						label = "optical4";
 						fixed-link {
 							speed = <1000>;
 							full-duplex;
@@ -300,7 +285,6 @@
 
 					switch2port9: port@9 {
 						reg = <9>;
-						label = "dsa";
 						phy-mode = "rgmii-txid";
 						link = <&switch1port5
 							&switch0port5>;
-- 
2.11.0

Re: [PATCH net-next 2/2] arm: dts: vf610-zii-dev-rev-b: remove ports label

[Andrew Lunn]

On Fri, Jan 06, 2017 at 05:00:43PM -0500, Vivien Didelot wrote:
> Now that the "label" property is optional for Ethernet switch ports,
> remove them in the ZII Dev Rev B board DTS.
>
> On a Rev B board, once eth1 is up, this DTS now exposes to userspace:
> 
>     # 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

It exposes this, this time. Next time, it could be:

      eth0
      eth1@eth0
      eth2@eth0
      eth3@eth0
      eth4@eth0
      eth5@eth0
      eth6@eth0
      eth7@eth0
      eth8@eth0
      eth9@eth0
      eth10@eth0
      eth11@eth0
      eth12

depending on how the base interfaces enumerate.

We have gone from deterministic names to non-deterministic names for
the switch ports. We now must have udev rules, if we want
deterministic names.

If the names where not deterministic before, i would of agreed to
this. But they are deterministic, set by device tree, and set to match
some physical property of the hardware, generally the label on the
case/PCB.

If somebody were to produce a switch on a PCIe card, or a USB bus,
things then are non-deterministic, and leaving the kernel to assign a
name is O.K. So i think the first patch is O.K, but i don't like this
patch.

	Andrew