From: Josua Mayer <josua@solid-run•com>
To: "Russell King (Oracle)" <linux@armlinux•org.uk>,
Ioana Ciornei <ioana.ciornei@nxp•com>
Cc: "netdev@vger•kernel.org" <netdev@vger•kernel.org>,
"David S. Miller" <davem@davemloft•net>,
Jakub Kicinski <kuba@kernel•org>,
Rob Herring <robh+dt@kernel•org>, Andrew Lunn <andrew@lunn•ch>,
Heiner Kallweit <hkallweit1@gmail•com>,
rafal@milecki•pl
Subject: Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors
Date: Wed, 1 Jun 2022 13:18:12 +0300 [thread overview]
Message-ID: <7c70ab93-6d35-52f5-ab11-e3b4ecd622f2@solid-run.com> (raw)
In-Reply-To: <Ynu8ixB5cm3zy6Yx@shell.armlinux.org.uk>
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@clearfog-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@80c000000/dpmacs/ethernet@7", NAME="eth7"
>> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@8", NAME="eth8"
>> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@9", NAME="eth9"
>> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@a", NAME="eth10"
>> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/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
next prev parent reply other threads:[~2022-06-01 10:18 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-09 12:29 [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors Josua Mayer
2022-05-09 12:49 ` Andrew Lunn
2022-05-10 8:56 ` Josua Mayer
2022-05-10 12:13 ` Andrew Lunn
2022-05-11 10:26 ` Russell King (Oracle)
2022-05-11 14:48 ` Ioana Ciornei
2022-05-11 10:12 ` Russell King (Oracle)
2022-05-11 15:48 ` Ioana Ciornei
2022-05-18 7:42 ` Josua Mayer
2022-05-09 15:54 ` Russell King (Oracle)
2022-05-10 9:44 ` Josua Mayer
2022-05-11 10:21 ` Russell King (Oracle)
2022-05-11 13:22 ` Ioana Ciornei
2022-05-11 13:39 ` Russell King (Oracle)
2022-06-01 10:18 ` Josua Mayer [this message]
2022-06-01 10:52 ` Russell King (Oracle)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7c70ab93-6d35-52f5-ab11-e3b4ecd622f2@solid-run.com \
--to=josua@solid-run$(echo .)com \
--cc=andrew@lunn$(echo .)ch \
--cc=davem@davemloft$(echo .)net \
--cc=hkallweit1@gmail$(echo .)com \
--cc=ioana.ciornei@nxp$(echo .)com \
--cc=kuba@kernel$(echo .)org \
--cc=linux@armlinux$(echo .)org.uk \
--cc=netdev@vger$(echo .)kernel.org \
--cc=rafal@milecki$(echo .)pl \
--cc=robh+dt@kernel$(echo .)org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox