Date:   Sat, 24 Mar 2018 10:07:27 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Jiri Pirko <jiri@...nulli.us>
CC:     Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
        davem@...emloft.net, idosch@...lanox.com,
        jakub.kicinski@...ronome.com, mlxsw@...lanox.com,
        vivien.didelot@...oirfairelinux.com, michael.chan@...adcom.com,
        ganeshgr@...lsio.com, saeedm@...lanox.com,
        simon.horman@...ronome.com, pieter.jansenvanvuuren@...ronome.com,
        john.hurley@...ronome.com, dirk.vandermerwe@...ronome.com,
        alexander.h.duyck@...el.com, ogerlitz@...lanox.com,
        dsahern@...il.com, vijaya.guvva@...ium.com,
        satananda.burla@...ium.com, raghu.vatsavayi@...ium.com,
        felix.manlunas@...ium.com, gospo@...adcom.com,
        sathya.perla@...adcom.com, vasundhara-v.volam@...adcom.com,
        tariqt@...lanox.com, eranbe@...lanox.com,
        jeffrey.t.kirsher@...el.com
Subject: 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 <jiri@...nulli.us> wrote:
>Sat, Mar 24, 2018 at 03:35:09PM CET, f.fainelli@...il.com wrote:
>>On March 24, 2018 12:45:51 AM PDT, Jiri Pirko <jiri@...nulli.us>
>wrote:
>>>Fri, Mar 23, 2018 at 04:24:12PM CET, andrew@...n.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@...n.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@...-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?

-- 
Florian

Powered by blists - more mailing lists