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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 20.07.2021 17:42, Andrew Lunn wrote:
>> 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.
> 

Thanks for the hint, Andrew. If I make &netdev->dev the parent,
then I get:

ll /sys/class/leds/
total 0
lrwxrwxrwx 1 root root 0 Jul 20 21:37 led0 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led0
lrwxrwxrwx 1 root root 0 Jul 20 21:37 led1 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led1
lrwxrwxrwx 1 root root 0 Jul 20 21:37 led2 -> ../../devices/pci0000:00/0000:00:1d.0/0000:03:00.0/net/enp3s0/led2

Now the (linked) LED devices are under /sys/class/net/<ifname>, but still
the primary LED devices are under /sys/class/leds and their names have
to be unique therefore. The LED subsystem takes care of unique names,
but in case of a second network interface the LED device name suddenly
would be led0_1 (IIRC). So the names wouldn't be predictable, and I think
that's not what we want.
We could use something like led0_<pci_id>, but then userspace would have
to do echo foo > /sys/class/net/<ifname>/led0*/bar, and that's also not
nice.

> 	Andrew
> 
Heiner