Message-ID: <YpdE2sflpqRkWKns@shell.armlinux.org.uk>
Date:   Wed, 1 Jun 2022 11:52:10 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Josua Mayer <josua@...id-run.com>
Cc:     Ioana Ciornei <ioana.ciornei@....com>,
        "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

On Wed, Jun 01, 2022 at 01:18:12PM +0300, Josua Mayer wrote:
> 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.

Someone may also take the view that this is exactly what should happen.
The requested configuration was to bind the trigger to a network
interface called "eth8" and if that interface goes away (through
whatever means) and then something else comes back as "eth8" then
"eth8" should be used.

The same is true of netfilter rules. If you specify a rule that
matches an interface called "eth8" then you get that match on that
name irrespective of network interface renames. The rule doesn't get
updated because you've changed the interface name. So in that regard,
the netdev LED trigger is operating just the same way as other parts
of the network subsystem.

> 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.

That sounds like a bug - the netdev trigger will select the last
matching interface that it hears about when a netdev is registered or
its name is changed to something that matches what it's looking for.
So if you have multiple namespaces with identical names, the last
namespace with a matching interface name gets to control the LED.

However, I suspect the LED trigger folk may say "don't do that".

> 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.

The trigger specific properties will not exist until such time that
the trigger has been assigned - because each trigger is an entirely
separate chunk of code which might even be in a loadable module, and
each trigger driver is responsible for creating the properties that
it needs. I'm guessing this was part of the design of the LED
subsystem from the start, and it does make total sense.

> 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"

I'm guessing that's because udev effectively sorts or randomises
the attributes, and gives no guarantees what order the attributes
are written. Sounds like we need udev people to comment on that
point.

> 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.

I think this is all something to bring up with those who look after
the netdev trigger.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists