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

From: Heiner Kallweit <hkallweit1@gmail.com>
To: "Jacek Anaszewski" <jacek.anaszewski@gmail.com>,
	"Marek Behún" <kabel@kernel.org>, "Andrew Lunn" <andrew@lunn.ch>
Cc: Pavel Machek <pavel@ucw.cz>,
	Tony Nguyen <anthony.l.nguyen@intel.com>,
	davem@davemloft.net, kuba@kernel.org,
	Kurt Kanzenbach <kurt@linutronix.de>,
	netdev@vger.kernel.org, sasha.neftin@intel.com,
	vitaly.lifshits@intel.com, vinicius.gomes@intel.com,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	Dvora Fuxbrumer <dvorax.fuxbrumer@linux.intel.com>,
	"linux-leds@vger.kernel.org" <linux-leds@vger.kernel.org>
Subject: Re: [PATCH net-next 5/5] igc: Export LEDs
Date: Mon, 26 Jul 2021 22:59:05 +0200	[thread overview]
Message-ID: <6d2697b1-f0f6-aa9f-579c-48a7abb8559d@gmail.com> (raw)
In-Reply-To: <4d8db4ce-0413-1f41-544d-fe665d3e104c@gmail.com>

On 26.07.2021 19:42, Jacek Anaszewski wrote:
> Hi Marek,
> 
> On 7/21/21 10:07 PM, Marek Behún wrote:
>> On Wed, 21 Jul 2021 21:50:07 +0200
>> Andrew Lunn <andrew@lunn.ch> wrote:
>>
>>>> Hi Heiner,
>>>>
>>>> in sysfs, all devices registered under LED class will have symlinks in
>>>> /sys/class/leds. This is how device classes work in Linux.
>>>>
>>>> There is a standardized format for LED device names, please look at
>>>> Documentation/leds/leds-class.rst.
>>>>
>>>> Basically the LED name is of the format
>>>>    devicename:color:function
>>>
>>> The interesting part here is, what does devicename mean, in this
>>> context?
>>>
>>> We cannot use the interface name, because it is not unique, and user
>>> space can change it whenever it wants. So we probably need to build
>>> something around the bus ID, e.g. pci_id. Which is not very friendly
>>> :-(
>>
>> Unfortunately there isn't consensus about what the devicename should
>> mean. There are two "schools of thought":
>>
>> 1. device name of the trigger source for the LED, i.e. if the LED
>>     blinks on activity on mmc0, the devicename should be mmc0. We have
>>     talked about this in the discussions about ethernet PHYs.
>>     In the case of the igc driver if the LEDs are controlled by the MAC,
>>     I guess some PCI identifier would be OK. Or maybe ethernet-mac
>>     identifier, if we have something like that? (Since we can't use
>>     interface names due to the possibility of renaming.)
>>
>>     Pavel and I are supporters of this scheme.
>>
>> 2. device name of the LED controller. For example LEDs controlled by
>>     the maxim,max77650-led controller (leds-max77650.c) define device
>>     name as "max77650"
>>
>>     Jacek supports this scheme.
> 
> I believe you must have misinterpreted some my of my statements.
> The whole effort behind LED naming unification was getting rid of
> hardware device names in favour of Linux provided device names.
> See e.g. the excerpt from Documentation/leds/leds-class.rst:
> 
> - devicename:
>         it should refer to a unique identifier created by the kernel,
>         like e.g. phyN for network devices or inputN for input devices, rather
>         than to the hardware; the information related to the product and the bus
>         to which given device is hooked is available in sysfs and can be
>         retrieved using get_led_device_info.sh script from tools/leds; generally
>         this section is expected mostly for LEDs that are somehow associated with
>         other devices.
> 
> 
The issue with this is mentioned by Andrew a few lines before. At least in
network subsystem the kernel identifiers can be changed from userspace.
Typical example is the interface renaming from eth0 to e.g. enp1s0.
Then a LED eth0-led1 would have to be automagically renamed to enp1s0-led1.
An option for this could be to make a LED renaming function subscribe to
interface name change notifications. But this looks to me to like using a
quite big hammer for a rather small nail.