Re: [patch net-next RFC 00/12] devlink: introduce port flavours and common phys_port_name generation

On March 24, 2018 9:07:49 AM PDT, Jiri Pirko [off-list ref] wrote:

Sat, Mar 24, 2018 at 03:35:09PM CET, f.fainelli@gmail.com wrote:

quoted

On March 24, 2018 12:45:51 AM PDT, Jiri Pirko [off-list ref]

wrote:

quoted

quoted

Fri, Mar 23, 2018 at 04:24:12PM CET, andrew@lunn.ch wrote:

quoted

On Fri, Mar 23, 2018 at 03:59:35PM +0100, Jiri Pirko wrote:

quoted

Fri, Mar 23, 2018 at 02:43:57PM CET, andrew@lunn.ch wrote:

quoted

quoted

I tested this for mlxsw and nfp. I have no way to test this on

DSA hw,

quoted

quoted

quoted

quoted

I would really appretiate DSA guys to test this.

Hi Jiri

With the missing break added, i get:

root@zii-devel-b:~# ./iproute2/devlink/devlink port 
mdio_bus/0.1:00/0: type eth netdev lan0 flavour physical number 0
mdio_bus/0.1:00/1: type eth netdev lan1 flavour physical number 1
mdio_bus/0.1:00/2: type eth netdev lan2 flavour physical number 2
mdio_bus/0.1:00/3: type notset
mdio_bus/0.1:00/4: type notset
mdio_bus/0.1:00/5: type notset flavour dsa number 5
mdio_bus/0.1:00/6: type notset flavour cpu number 6
mdio_bus/0.2:00/0: type eth netdev lan3 flavour physical number 0
mdio_bus/0.2:00/1: type eth netdev lan4 flavour physical number 1
mdio_bus/0.2:00/2: type eth netdev lan5 flavour physical number 2
mdio_bus/0.2:00/3: type notset
mdio_bus/0.2:00/4: type notset
mdio_bus/0.2:00/5: type notset flavour dsa number 5
mdio_bus/0.2:00/6: type notset flavour dsa number 6
mdio_bus/0.4:00/0: type eth netdev lan6 flavour physical number 0
mdio_bus/0.4:00/1: type eth netdev lan7 flavour physical number 1
mdio_bus/0.4:00/2: type eth netdev lan8 flavour physical number 2
mdio_bus/0.4:00/3: type eth netdev optical3 flavour physical

number

quoted

quoted

3

quoted

quoted

quoted

mdio_bus/0.4:00/4: type eth netdev optical4 flavour physical

number

quoted

quoted

4

quoted

quoted

quoted

mdio_bus/0.4:00/5: type notset
mdio_bus/0.4:00/6: type notset
mdio_bus/0.4:00/7: type notset
mdio_bus/0.4:00/8: type notset
mdio_bus/0.4:00/9: type notset flavour dsa number 9

quoted

That is basically front panel number for physical ports.

You cannot make that assumption. As you can see here, we have 3

ports

quoted

quoted

quoted

with the number 0.

Look at clearfog, armada-388-clearfog.dts. port 0=lan5, port 1=lan4
port 2=lan3, port 3=lan2, port 4=lan1, port 5=cpu, port 6=lan6.

The hardware and mechanical engineer is free to wire switch ports to
the front panel however they want. That is why we put the netdev

name

quoted

quoted

quoted

in device tree.

Got it. Hmm, so I think that the port number can be made optional and
when it is present, it would be used to generate phys_port_name. If
not, perhaps devlink port index could be used instead.

So iiuc, you don't really need phys_port_name in dsa, as it provides
misleading names, right? Why is it implemented then?

We do need phys_port_name because there are switch configuration

operations, e.g: ethtool::rxnfc which take a port number and queue
number as part of the action to redirect a packet for instance. There
is no way to obtain this physical port number other than either knowing
it and hard coding it (not great) or scanning the device tree and look
for the "reg" property value. phys_port_name gets you that and it is
easy for an application to scan /sys/class/net/ on startup to get to
know that (and other stuff as well).

Hmm. That sounds like misuse of phys_port_name. The original purpose
was
to provide names as they are actually written on the front panel.

Ok, if we look back at the history of the changes I had implemented ndo_phys_port_id() to return the port number initially, but this was wrong and reverted based on your feedback, and then ndo_phys_port_name() was implemented with your reviewed-by tag:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/net/dsa/slave.c?id=3a543ef479868e36c95935de320608a7e41466ca
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/net/dsa/slave.c?id=592050b2541407d033da18226d3644644832d082
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/net/dsa/slave.c?id=592050b2541407d033da18226d3644644832d082

Now that this is reported through sysfs it unfortunately becomes ABI and we should not be breaking user applications relying on that and there is at least one I know of...

What is an appropriate attribute to use to return the physical port number within a given switch?

-- 
Florian