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: <YPV6+PQq1fvH8aSy@lunn.ch>
Date:   Mon, 19 Jul 2021 15:15:36 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Kurt Kanzenbach <kurt@...utronix.de>
Cc:     Tony Nguyen <anthony.l.nguyen@...el.com>, davem@...emloft.net,
        kuba@...nel.org, 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>
Subject: Re: [PATCH net-next 5/5] igc: Export LEDs

On Mon, Jul 19, 2021 at 08:06:41AM +0200, Kurt Kanzenbach wrote:
> Hi Andrew,
> 
> On Fri Jul 16 2021, Andrew Lunn wrote:
> > On Fri, Jul 16, 2021 at 02:24:27PM -0700, Tony Nguyen wrote:
> >> From: Kurt Kanzenbach <kurt@...utronix.de>
> >> 
> >> Each i225 has three LEDs. Export them via the LED class framework.
> >> 
> >> Each LED is controllable via sysfs. Example:
> >> 
> >> $ cd /sys/class/leds/igc_led0
> >> $ cat brightness      # Current Mode
> >> $ cat max_brightness  # 15
> >> $ echo 0 > brightness # Mode 0
> >> $ echo 1 > brightness # Mode 1
> >> 
> >> The brightness field here reflects the different LED modes ranging
> >> from 0 to 15.
> >
> > What do you mean by mode? Do you mean blink mode? Like On means 1G
> > link, and it blinks for packet TX?
> 
> There are different modes such as ON, OFF, LINK established, LINK
> activity, PAUSED ... Blinking is controlled by a different register.
> 
> Are there better ways to export this?

As i said in another email, using LED triggers. For simple link speed
indication, take a look at CONFIG_LED_TRIGGER_PHY. This is purely
software, and probably not what you want. The more complex offload of
to LED control to hardware in the PHY subsystem it still going around
and around. The basic idea is agreed, just not the implementation.
However, most of the implementation is of no help to you, since Intel
drivers ignore the kernel PHY drivers and do their own. But the basic
idea and style of user space API should be kept the same. So take a
look at the work Marek Behún has been doing, e.g.

https://lwn.net/Articles/830947/

and a more recent version

https://lore.kernel.org/netdev/20210602144439.4d20b295@dellmb/T/#m4e97c9016597fb40849c104c7c0e3bc10d5c1ff5

Looking at the basic idea of using LED triggers and offloading
them. Please also try to make us of generic names for these triggers,
so the PHY subsystem might also use the same or similar names when it
eventually gets something merged.

Please also Cc: The LED maintainers and LED list. Doing that from the
start would of avoided this revert, since you would of get earlier
feedback about this.

	 Andrew