netdev - Re: [PATCH net-next 5/5] igc: Export LEDs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YPbu8xOFDRZWMTBe@lunn.ch>
Date:   Tue, 20 Jul 2021 17:42:43 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Pavel Machek <pavel@....cz>,
        Tony Nguyen <anthony.l.nguyen@...el.com>, davem@...emloft.net,
        kuba@...nel.org, Kurt Kanzenbach <kurt@...utronix.de>,
        netdev@...r.kernel.org, sasha.neftin@...el.com,
        vitaly.lifshits@...el.com, vinicius.gomes@...el.com,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Dvora Fuxbrumer <dvorax.fuxbrumer@...ux.intel.com>,
        "linux-leds@...r.kernel.org" <linux-leds@...r.kernel.org>
Subject: Re: [PATCH net-next 5/5] igc: Export LEDs

> I checked the LED subsystem and didn't find a way to place the LED
> sysfs files in a place other than /sys/class/leds. Maybe Pavel can
> comment on whether I just missed something.

https://lwn.net/ml/linux-kernel/20200908000300.6982-1-marek.behun@nic.cz/

It comments the sys files appear under
/sys/class/net/<ifname>/phydev/leds/. phydev is a symbolic link to the
phy devices, provided by the phydev subsystem. So they are actually
attached to the PHY device. And this appears to be due to:

	ret = devm_led_classdev_register_ext(&phydev->mdio.dev, &led->cdev, &init_data);

The LEDs are parented to the phy device. This has the nice side affect
that PHYs are not part of the network name space. You can rename the
interface, /sys/class/net/<ifname> changes, but the symbolic link
still points to the phy device.

When not using phydev, it probably gets trickier. You probably want
the LEDs parented to the PCI device, and you need to follow a
different symbolic link out of /sys/class/net/<ifname>/ to find the
LED.

There was talk of adding an ledtool, which knows about these
links. But i pushed for it to be added to ethtool. Until we get an
implementation actually merged, that is academic.

> For r8169 I'm facing a similar challenge like Kurt. Most family
> members support three LED's:
> - Per LED a mode 0 .. 15 can be set that defines which link speed(s)
>   and/or activity is indicated.
> - Period and duty cycle for blinking can be controlled, but this
>   setting applies to all three LED's.

Cross LED settings is a problem. The Marvell PHYs have a number of
independent modes. Plus they have some shared modes which cross LEDs.
At the moment, there is no good model for these shared modes.

We have to look at the trade offs here. At the moment we have at least
3 different ways of setting PHY LEDs via DT. Because each driver does
it its own way, it probably allows full access to all features. But it
is very unfriendly. Adopting Linux LEDs allows us to have a single
uniform API for all these PHY LEDs, and probably all MAC drivers which
don't use PHY drivers. But at the expense of probably not supporting
all features of the hardware. My opinion is, we should ignore some of
the hardware features in order to get a simple to use uniform
interface for all LEDs, which probably covers the features most people
are interested in anyway.

	Andrew