Re: [PATCH net-next v2] net: dsa: make "label" property optional for dsa2
From: Jiri Pirko
Date: Mon Jan 09 2017 - 02:32:47 EST
Mon, Jan 09, 2017 at 12:15:52AM CET, vivien.didelot@xxxxxxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxxxxxxx>
>Acked-by: Uwe Kleine-König <uwe@xxxxxxxxxxxxxxxxx>
>---
> 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
>