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: <20210721233105.65468fd2@thinkpad>
Date:   Wed, 21 Jul 2021 23:31:05 +0200
From:   Marek Behún <kabel@...nel.org>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        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

On Wed, 21 Jul 2021 22:54:36 +0200
Andrew Lunn <andrew@...n.ch> wrote:

> > > > Basically the LED name is of the format
> > > >   devicename:color:function    
> 
> > 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.  
> 
> I guess this is most likely for Ethernet LEDs, some sort of bus
> identifier. But Ethernet makes use of all sorts of busses, so you will
> also see USB, memory mapped for SOCs, MDIO, SPI, etc.

That's why I think we should group them all under a name like ethmac0,
ethmac1, ... We want to do this for PHY controlled LEDs (ethphy0,
ethphy1, ...)

> > 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"  
> 
> And what happens when the controller is just a tiny bit of silicon in
> the corner of something else, not a standalone device? Would this be
> 'igc', for LEDs controlled by the IGC Ethernet controller? 'mv88e6xxx'
> for Marvell Ethernet switches? 

This is one of the reasons why I prefer the first scheme.

> Also, function is totally unclear. The whole reason we want to use
> Linux LEDs is triggers, and it is the selected trigger which
> determines the function.

As I said there are two "schools of thought" for this as well.
Devicetree deprecated the `linux,default-trigger` DT property and
`function` property should be used instead. Jacek's then defined some
function definition constants in include/dt-bindings/leds/common.h and
sent a proposal for function to trigger mappings
  https://lore.kernel.org/linux-leds/20200920162625.14754-1-jacek.anaszewski@gmail.com/
But this was not implemented, and I together with Pavel do not agree
with this proposal, and I proposed something different:
  https://lore.kernel.org/linux-leds/20200920184422.60c04194@nic.cz/
Since function to trigger mappings is not yet implemented in the code,
we can still decide.

Do you think I should a poll more kernel developers about their
opinions?

> Colour is also an issue. The IGC Ethernet controller has no idea what
> colour the LEDs are in the RG-45 socket. And this is generic to
> Ethernet MAC and PHY chips. The data sheets never mention colour.  You
> might know the colour in DT (and maybe ACPI) systems where you have
> specific information about the board. But in for PCIe card, USB
> dongles, etc, colour is unknown.

The LED core (function led_compose_name in drivers/leds/led-core.c)
skips color and function if they are not present in fwnode, i.e.
  "mmc0::"

I guess in the case of igc, if the color is not known, and if we can
agree on the first scheme for choosing the devicename part, then the
LED names could be, depending on the scheme for function, either
  "ethmac0::lan-0"
  "ethmac0::lan-1"
  "ethmac0::lan-2"
or
  "ethmac0::link"
  "ethmac0::activity"
  "ethmac0::rx"

(If there is color defined in ACPI / DTS though, it should be also
 used.)

So basically we need to decide on these two things:
- scheme for device name
- scheme for function to default trigger mappings

Marek