Date:   Wed, 1 Jun 2022 13:18:12 +0300
From:   Josua Mayer <josua@...id-run.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>,
        Ioana Ciornei <ioana.ciornei@....com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Rob Herring <robh+dt@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>, rafal@...ecki.pl
Subject: Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP
 connectors

Hi Russell,

Thank you for the examples below!
Stable names do help in some cases, but not in all.

I did some testing the other day with renaming network interfaces 
through the ip command:
ip link set dev eth8 down
ip link set dev eth12 down
ip link set eth12 name eth20
ip link set eth8 name eth12
ip link set eth20 name eth8
ip link set eth8 up
ip link set dev eth12 up

Swapping interface names like this seems a perfectly legal thing for 
userspace to do. I personally would in such case expect the previous LED 
assignment to move along with the name change, however this does not happen.
Instead after the interface rename, the LEDs are effectively swapped.

Further the netdev trigger implementation seems incorrect when you add 
network namespaces.
Two namespaces can each contain a device named eth0, but the netdev 
trigger does not look at the namespace id when matching events to 
triggers, just the name.

Is it intended for userspace to track interface renames and reassign LEDs?
Or should the trigger driver watch for name changes and adapt accordingly?

Finally I also noticed that the netdev trigger by default does not 
propagate any information to the LED.
All properties - link, rx, tx are 0.
Attempts at setting this by default through udev were widely unsuccessful:
E.g. before setting the trigger property to netdev, the device_name or 
link properties do not exist.
Therefore a rule that sets trigger and link and device at the same time 
does not function:
SUBSYSTEM=="leds", ACTION=="add|change", 
ENV{OF_FULLNAME}=="/leds/led_c1_at", ATTR{trigger}="netdev", 
ATTR{link}="1", ATTR{device_name}="eth0"

It appears necessary to use 2 rules, one that selects netdev, another 
one that chooses what property to show, e.g. link;
and finally some rule that tracks the netdev name and updates 
device_name property accordingly.
All while watching out for infinite loops because the property changes 
appear to trigger more change events, and e.g. setting trigger to netdev 
again causes another change event and resets the properties ...

I get the impression that this is very complex and might be described 
much better in device-tree, at least when a vendor makes explicit 
decisions to the purpose of each led.

Thee has been a recent patchset floating this list by Rafał Miłecki,
which I very much liked:
[PATCH RESEND PoC] leds: trigger: netdev: support DT "trigger-sources" 
property

It does allow declaring the relation from dpmac to led in dts as I would 
have expected.

In addition I believe there should be a way in dts to also set a default 
for what information to show, e.g.
default-function = "link";

And finally dynamic tracking of the interface name.

I would be willing to work on the last two ideas, if this is an 
acceptable approach.

Am 11.05.22 um 16:39 schrieb Russell King (Oracle):
> On Wed, May 11, 2022 at 01:22:22PM +0000, Ioana Ciornei wrote:
>> On Tue, May 10, 2022 at 12:44:41PM +0300, Josua Mayer wrote:
>>
>>> One issue is that the interfaces don't have stable names. It purely depends
>>> on probe order,
>>> which is controlled by sending commands to the networking coprocessor.
>>>
>>> We actually get asked this question sometimes how to have stable device
>>> names, and so far the answer has been systemd services with explicit sleep
>>> to force the order.
>>> But this is a different topic.
>>>
>>
>> Stable names can be achieved using some udev rules based on the OF node.
>> For example, I am using the following rules on a Clearfog CX LX2:
>>
>> [root@...arfog-cx-lx2 ~] # cat /etc/udev/rules.d/70-persistent-net.rules
>> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@...000000/dpmacs/ethernet@7", NAME="eth7"
>> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@...000000/dpmacs/ethernet@8", NAME="eth8"
>> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@...000000/dpmacs/ethernet@9", NAME="eth9"
>> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@...000000/dpmacs/ethernet@a", NAME="eth10"
>> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@...000000/dpmacs/ethernet@11", NAME="eth17"
Yes this seems to work okay.
I feel that it is wrong for userspace to recognize particular dts nodes, 
but it *does* work.

> 
> Or by using systemd - for example, on the Armada 38x Clearfog platform,
> I use:
> 
> /etc/systemd/network/01-ded.link:
> [Match]
> Path=platform-f1070000.ethernet
> [Link]
> MACAddressPolicy=none
> Name=eno0
> 
> /etc/systemd/network/02-sw.link:
> [Match]
> Path=platform-f1030000.ethernet
> [Link]
> MACAddressPolicy=none
> Name=eno1
> 
> /etc/systemd/network/03-sfp.link:
> [Match]
> Path=platform-f1034000.ethernet
> [Link]
> MACAddressPolicy=none
> Name=eno2
> 

sincerely
Josua Mayer

Powered by blists - more mailing lists