Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Heiner Kallweit <hkallweit1@gmail.com>
To: Andrew Lunn <andrew@lunn.ch>, Pavel Machek <pavel@ucw.cz>
Cc: 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: Tue, 20 Jul 2021 22:29:21 +0200	[thread overview]
Message-ID: <3b7ad100-643e-c173-0d43-52e65d41c8c3@gmail.com> (raw)
In-Reply-To: <YPbu8xOFDRZWMTBe@lunn.ch>

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

  reply	other threads:[~2021-07-20 20:49 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-16 21:24 [PATCH net-next 0/5][pull request] 1GbE Intel Wired LAN Driver Updates 2021-07-16 Tony Nguyen
2021-07-16 21:24 ` [PATCH net-next 1/5] igc: Add possibility to add flex filter Tony Nguyen
2021-07-16 21:24 ` [PATCH net-next 2/5] igc: Integrate flex filter into ethtool ops Tony Nguyen
2021-07-16 21:24 ` [PATCH net-next 3/5] igc: Allow for Flex Filters to be installed Tony Nguyen
2021-07-16 21:24 ` [PATCH net-next 4/5] igc: Make flex filter more flexible Tony Nguyen
2021-07-16 21:24 ` [PATCH net-next 5/5] igc: Export LEDs Tony Nguyen
2021-07-16 21:56   ` Andrew Lunn
2021-07-18 22:10     ` Heiner Kallweit
2021-07-18 22:33       ` Andrew Lunn
2021-07-19  6:17         ` Kurt Kanzenbach
2021-07-19  9:46           ` Jakub Kicinski
2021-07-19  6:06     ` Kurt Kanzenbach
2021-07-19 13:15       ` Andrew Lunn
2021-07-20  8:54         ` Kurt Kanzenbach
2021-07-21 19:18         ` Marek Behún
2021-07-18 22:19   ` Heiner Kallweit
2021-07-19  0:40     ` Andrew Lunn
2021-07-20 15:00       ` Heiner Kallweit
2021-07-20 15:42         ` Andrew Lunn
2021-07-20 20:29           ` Heiner Kallweit [this message]
2021-07-21 14:35             ` Andrew Lunn
2021-07-21 16:02               ` Heiner Kallweit
2021-07-21 18:23               ` Pavel Machek
2021-07-21 18:25                 ` Pavel Machek
2021-07-21 18:45             ` Marek Behún
2021-07-21 19:50               ` Andrew Lunn
2021-07-21 20:07                 ` Marek Behún
2021-07-21 20:54                   ` Andrew Lunn
2021-07-21 21:31                     ` Marek Behún
2021-07-21 22:45                     ` Pavel Machek
2021-07-22  1:45                       ` Andrew Lunn
2021-07-22  2:19                         ` Marek Behún
2021-07-21 22:34                   ` Pavel Machek
2021-07-22  3:52                   ` Florian Fainelli
2021-07-26 17:42                   ` Jacek Anaszewski
2021-07-26 18:44                     ` Marek Behún
2021-07-26 20:59                     ` Heiner Kallweit
2021-07-27  0:06                       ` Marek Behún
2021-07-27  1:57                         ` Andrew Lunn
2021-07-27  8:15                           ` Michael Walle
2021-07-27 14:56                             ` Marek Behún
2021-07-27 15:03                               ` Michael Walle
2021-07-27 15:24                                 ` Andrew Lunn
2021-07-27 15:28                                 ` Marek Behún
2021-07-27 15:53                                   ` Michael Walle
2021-07-27 16:23                                     ` Andrew Lunn
2021-07-27 16:32                                     ` Marek Behún
2021-07-27 16:42                                       ` Andrew Lunn
2021-07-27 19:42                                       ` Michael Walle
2021-07-28 20:43                                       ` Heiner Kallweit
2021-07-29  6:39                                         ` Kurt Kanzenbach
2021-07-29  8:59                                         ` Marek Behún
2021-07-29 21:54                                           ` Heiner Kallweit
2021-08-10 17:29                                         ` Pavel Machek
2021-08-10 17:55                                           ` Marek Behún
2021-08-10 19:53                                             ` Pavel Machek
2021-08-10 20:53                                               ` Marek Behún
2021-08-17 19:02                                                 ` Pavel Machek
2021-08-25 15:26                                                   ` Marek Behún
2021-08-26 12:45                                                     ` Pavel Machek
2021-08-10 20:46                                           ` Heiner Kallweit
2021-08-10 21:21                                             ` Andrew Lunn
2021-07-27 13:55                           ` Marek Behún
2021-08-10 17:22                             ` Documentation for naming LEDs was " Pavel Machek
2021-07-19 21:48   ` Jesse Brandeburg
2021-07-20 13:31     ` Kurt Kanzenbach
2021-07-17  0:30 ` [PATCH net-next 0/5][pull request] 1GbE Intel Wired LAN Driver Updates 2021-07-16 patchwork-bot+netdevbpf
2021-07-17 17:36   ` Andrew Lunn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3b7ad100-643e-c173-0d43-52e65d41c8c3@gmail.com \
    --to=hkallweit1@gmail.com \
    --cc=andrew@lunn.ch \
    --cc=anthony.l.nguyen@intel.com \
    --cc=bigeasy@linutronix.de \
    --cc=davem@davemloft.net \
    --cc=dvorax.fuxbrumer@linux.intel.com \
    --cc=kuba@kernel.org \
    --cc=kurt@linutronix.de \
    --cc=linux-leds@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pavel@ucw.cz \
    --cc=sasha.neftin@intel.com \
    --cc=vinicius.gomes@intel.com \
    --cc=vitaly.lifshits@intel.com \
    --subject='Re: [PATCH net-next 5/5] igc: Export LEDs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).