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@xxxxxx/ > > 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
