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

[Jiri Pirko <jiri@resnulli.us>]

Sat, Mar 24, 2018 at 06:07:27PM CET, f.fainelli@gmail.com wrote:
>On March 24, 2018 9:07:49 AM PDT, Jiri Pirko <jiri@resnulli.us> wrote:
>>Sat, Mar 24, 2018 at 03:35:09PM CET, f.fainelli@gmail.com wrote:
>>>On March 24, 2018 12:45:51 AM PDT, Jiri Pirko <jiri@resnulli.us>
>>wrote:
>>>>Fri, Mar 23, 2018 at 04:24:12PM CET, andrew@lunn.ch wrote:
>>>>>On Fri, Mar 23, 2018 at 03:59:35PM +0100, Jiri Pirko wrote:
>>>>>> Fri, Mar 23, 2018 at 02:43:57PM CET, andrew@lunn.ch wrote:
>>>>>> >> I tested this for mlxsw and nfp. I have no way to test this on
>>>>DSA hw,
>>>>>> >> 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
>>>>3
>>>>>> >mdio_bus/0.4:00/4: type eth netdev optical4 flavour physical
>>number
>>>>4
>>>>>> >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
>>>>>
>>>>>> That is basically front panel number for physical ports.
>>>>>
>>>>>You cannot make that assumption. As you can see here, we have 3
>>ports
>>>>>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
>>>>>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?

I don't think there's one out there. I tried to add it in this patchset.