| Message ID | 792598f4a1a3219b6517057c92559b0f0a95b419.1621349814.git.mchehab+huawei@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Adding support for controlling the leds found on Intel NUC | expand |
What possible configurations does this support? Does this blink on rx/tx activity for a specific ethernet port? There is a work in progress to add support for transparent offloading of LED triggers, with the netdev trigger being the first target. This means that after that is done, you could implement this driver so that when netdev trigger is enabled on a supported interface, your driver will offload the blinking to the HW. This should probably also work for HDD activity, but this would need a blockdev trigger first... Marek
Em Wed, 19 May 2021 10:02:53 +0200 Marek Behún <kabel@kernel.org> escreveu: > What possible configurations does this support? > > Does this blink on rx/tx activity for a specific ethernet port? > When the indicator is set to monitor Ethernet, it can work on either LAN1, LAN2 or both LAN interfaces. > There is a work in progress to add support for transparent offloading of > LED triggers, with the netdev trigger being the first target. > > This means that after that is done, you could implement this driver so > that when netdev trigger is enabled on a supported interface, your > driver will offload the blinking to the HW. On NUC leds, this is already offloaded to HW/firmware. All it takes is to tell the BIOS via ACPI/WMI what LED will be used for monitoring the Ethernet activity, and on what port(s). > This should probably also work for HDD activity, but this would need a > blockdev trigger first... HDD activity is also HW/firmware monitored. The only thing the Kernel needs to to is to select what LED will be set as the HDD activity indicator. Thanks, Mauro
On Wed, 19 May 2021 12:18:12 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > Em Wed, 19 May 2021 10:02:53 +0200 > Marek Behún <kabel@kernel.org> escreveu: > > > What possible configurations does this support? > > > > Does this blink on rx/tx activity for a specific ethernet port? > > > > When the indicator is set to monitor Ethernet, it can work on either > LAN1, LAN2 or both LAN interfaces. > > > There is a work in progress to add support for transparent offloading of > > LED triggers, with the netdev trigger being the first target. > > > > This means that after that is done, you could implement this driver so > > that when netdev trigger is enabled on a supported interface, your > > driver will offload the blinking to the HW. > > On NUC leds, this is already offloaded to HW/firmware. > > All it takes is to tell the BIOS via ACPI/WMI what LED will be used > for monitoring the Ethernet activity, and on what port(s). Can the LED be put into software controlled mode and also into HW controlled mode so that HW blinks the LED on ethernet activity? If so, then this is what I am talking about: transparent HW offloading of LED triggers: - I have a LED in /sys/class/leds - I set "netdev" trigger on this LED - I set ethernet interface for which the LED should blink - if the HW can blink the LED for that particular ethernet interface, the driver should use HW blinking... > > This should probably also work for HDD activity, but this would need a > > blockdev trigger first... > > HDD activity is also HW/firmware monitored. The only thing the Kernel > needs to to is to select what LED will be set as the HDD activity > indicator. Ditto as above, if we had a "blockdev" LED trigger. Marek
Em Wed, 19 May 2021 14:11:02 +0200 Marek Behún <kabel@kernel.org> escreveu: > On Wed, 19 May 2021 12:18:12 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > Em Wed, 19 May 2021 10:02:53 +0200 > > Marek Behún <kabel@kernel.org> escreveu: > > > > > What possible configurations does this support? > > > > > > Does this blink on rx/tx activity for a specific ethernet port? > > > > > > > When the indicator is set to monitor Ethernet, it can work on either > > LAN1, LAN2 or both LAN interfaces. > > > > > There is a work in progress to add support for transparent offloading of > > > LED triggers, with the netdev trigger being the first target. > > > > > > This means that after that is done, you could implement this driver so > > > that when netdev trigger is enabled on a supported interface, your > > > driver will offload the blinking to the HW. > > > > On NUC leds, this is already offloaded to HW/firmware. > > > > All it takes is to tell the BIOS via ACPI/WMI what LED will be used > > for monitoring the Ethernet activity, and on what port(s). > > Can the LED be put into software controlled mode and also into HW > controlled mode so that HW blinks the LED on ethernet activity? On a given time, a LED will either be in hardware-controlled mode or in Software-controlled one. It should be noticed that software-controlled mode should first be enabled by a BIOS option. The default is to be hardware-controlled. I don't intend to implement myself support for software-controlled mode in Kernel, as this will probably be a lot worse than letting the hardware control the led directly. See, changing the LED in software on NUC happens via ACPI calls, meaning that the Kernel should be interrupted in order to run some code inside the BIOS that will actually program the hardware. Switching to BIOS produces context switching and may also interrupt the other CPUs, as the BIOS may need to do CPU locking, in order to prevent L1/L2/L3 cache issues. On other words, if no extra care is taken, it could have bad side effects at the machine's performance and affect system's latency, eventually resulting on things like audio clicks and pops, if some audio is playing while such calls keep happening. So, IMO, there's very little sense on trying to re-implement the already existing hardware-controlled events via software emulation. > If so, then this is what I am talking about: transparent HW offloading > of LED triggers: > - I have a LED in /sys/class/leds > - I set "netdev" trigger on this LED > - I set ethernet interface for which the LED should blink > - if the HW can blink the LED for that particular ethernet interface, > the driver should use HW blinking... Sorry, but I guess I missed something here. Are you meaning to use the code under "ledtrig-netdev.c" or something else? The code at ledtrig-netdev.c allocates a trigger data, initializes a spin lock, initializes a delayed work, registers a notifier, sets a trigger interval, etc. It is perfectly fine for software-controlled LEDs, but none of those will ever be used by the NUC driver, if it only implements HW blinking for the Ethernet interfaces (and, as said before, there's little sense emulating it via software on such devices). Thanks, Mauro
On Wed, 19 May 2021 16:24:13 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > On other words, if no extra care is taken, it could have bad side > effects at the machine's performance and affect system's latency, > eventually resulting on things like audio clicks and pops, if some > audio is playing while such calls keep happening. In general we want for every LED that is registered into kernel as a LED classdev to be possible to control the brightness by software. If the hardware supports it, it should be available. There is a _blocking .brightness_set_blocking callback for LEDs which may block when setting brightness. But even if we did not want to support software control, the transparent trigger offloading is still relevant. See below. > So, IMO, there's very little sense on trying to re-implement the > already existing hardware-controlled events via software emulation. We have a misunderstanding here, probably because of my bad explanation, I will try to clarify. > Sorry, but I guess I missed something here. Are you meaning to use > the code under "ledtrig-netdev.c" or something else? > > The code at ledtrig-netdev.c allocates a trigger data, initializes a > spin lock, initializes a delayed work, registers a notifier, sets a > trigger interval, etc. It is perfectly fine for software-controlled > LEDs, but none of those will ever be used by the NUC driver, > if it only implements HW blinking for the Ethernet interfaces > (and, as said before, there's little sense emulating it via software > on such devices). The idea of transparent offloading of LED triggers to HW (if HW supports it) is to have a consistent and unified interface. Currently we have a driver (leds-ns2 I think) which allows putting the LED into HW controlled mode (to blink on SATA disk activity). This is done by writing 1 into /sys/class/leds/<LED>/sata. In your proposal you are creating several sysfs files: indicator hdd_default (notice difference from "sata" sysfs file in leds-ns2 driver) ethernet_type So the problem here is that this API is not unified. This is different from how leds-ns2 driver does this, and both of these solutions are wrong, because they are not extendable. The correct way to do this is via LED triggers, i.e. if I want a LED to blink on network activity, then I should use netdev trigger and nothing else. The netdev trigger should determine whether the underlying LED driver can set the LED to blink on network activity in HW. If HW supports it, netdev trigger should use this, otherwise netdev trigger should blink the LED in software. Currently the netdev trigger does the blinking in software only (code in "ledtrig-netdev.c" file). There is a WIP to add the necessary support for the netdev trigger to have the ability to offload blinking to HW. I will try to respin this WIP and send patches for review. Once netdev trigger supports this feature, you can implement your driver in this way. You can even make your driver depend on netdev trigger and set the specific LED into netdev triggering by default, and even forbidding anything else. But this is the corrent way to do this, instead of creating new sysfs API that is non-extendable. I am sorry that I did not explain this thoroughly in previous mails. Hopefully this explanation is better. Marek PS: This is relevant for disk activity as well.
Em Wed, 19 May 2021 17:55:03 +0200 Marek Behún <kabel@kernel.org> escreveu: > On Wed, 19 May 2021 16:24:13 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > On other words, if no extra care is taken, it could have bad side > > effects at the machine's performance and affect system's latency, > > eventually resulting on things like audio clicks and pops, if some > > audio is playing while such calls keep happening. > > In general we want for every LED that is registered into kernel as a LED > classdev to be possible to control the brightness by software. If the > hardware supports it, it should be available. This is supported, but maybe not the same way as on other drivers. There are two separate things: ON/OFF and LED brightness, when turned ON. On other words, NUC leds allow to set the brightness ranging from 0 to 100, but if the brightness is, let's say 50%, it means that, when the LED is triggered by the hardware: - ON would mean 50%; and - OFF would mean 0%. On other words, it actually adjusts the maximum brightness level. Btw, this also applies to software control, as the hardware can still blink the LED, the available properties for software control indicator are: - brightness. - blink behavior and frequency; - led color (available only if BIOS says that it is a multi-colored led); > There is a _blocking > .brightness_set_blocking callback for LEDs which may block when setting > brightness. > But even if we did not want to support software control, the transparent > trigger offloading is still relevant. See below. > > > So, IMO, there's very little sense on trying to re-implement the > > already existing hardware-controlled events via software emulation. > > We have a misunderstanding here, probably because of my bad > explanation, I will try to clarify. > > > Sorry, but I guess I missed something here. Are you meaning to use > > the code under "ledtrig-netdev.c" or something else? > > > > The code at ledtrig-netdev.c allocates a trigger data, initializes a > > spin lock, initializes a delayed work, registers a notifier, sets a > > trigger interval, etc. It is perfectly fine for software-controlled > > LEDs, but none of those will ever be used by the NUC driver, > > if it only implements HW blinking for the Ethernet interfaces > > (and, as said before, there's little sense emulating it via software > > on such devices). > > The idea of transparent offloading of LED triggers to HW (if HW > supports it) is to have a consistent and unified interface. Makes sense, but not sure if the current API will work. > Currently we have a driver (leds-ns2 I think) which allows putting the > LED into HW controlled mode (to blink on SATA disk activity). This is > done by writing 1 into /sys/class/leds/<LED>/sata. > > In your proposal you are creating several sysfs files: > indicator > hdd_default (notice difference from "sata" sysfs file in leds-ns2 > driver) > ethernet_type > > So the problem here is that this API is not unified. This is different > from how leds-ns2 driver does this, and both of these solutions are > wrong, because they are not extendable. Partially agreed, but I'm not so sure if the reverse is not true ;-) I mean, the current LED API was designed and tested on drivers that allow direct control of the LED (and then extended to some cases where the hardware allows offloading). The NUC API is just the opposite: there, the BIOS has full control of the hardware, but it provides an interface that allows changing the LED behavior, up to some extend. It also allows controlling the LED hardware and make it blink while it is suspended/hibernating, which is something that a direct LED control wouldn't allow. So, for instance, if we stick with the current LED API, there's no way to tell that the power LED should: - blink on every 5 seconds, using up to 20% of brightness when the system is suspended; - strobe on every 10 seconds using up to 50% of brightness when the system is hibernated; - use 100% of brigntness and don't blink when powered up. > The correct way to do this is via LED triggers, i.e. if I want a LED to > blink on network activity, then I should use netdev trigger and nothing > else. The netdev trigger should determine whether the underlying LED > driver can set the LED to blink on network activity in HW. If HW > supports it, netdev trigger should use this, otherwise netdev trigger > should blink the LED in software. I understand the desire of exposing the same API, but the current trigger code doesn't seem to be fit. I mean, the init sequence done at netdev_trig_activate(): trigger_data = kzalloc(sizeof(struct led_netdev_data), GFP_KERNEL); if (!trigger_data) return -ENOMEM; spin_lock_init(&trigger_data->lock); trigger_data->notifier.notifier_call = netdev_trig_notify; trigger_data->notifier.priority = 10; INIT_DELAYED_WORK(&trigger_data->work, netdev_trig_work); trigger_data->led_cdev = led_cdev; trigger_data->net_dev = NULL; trigger_data->device_name[0] = 0; trigger_data->mode = 0; atomic_set(&trigger_data->interval, msecs_to_jiffies(50)); trigger_data->last_activity = 0; led_set_trigger_data(led_cdev, trigger_data); rc = register_netdevice_notifier(&trigger_data->notifier); if (rc) kfree(trigger_data); doesn't make sense when the LED will be trigged by the hardware, and registering a notifier for netdevice is overkill. The exported attributes: static struct attribute *netdev_trig_attrs[] = { &dev_attr_device_name.attr, &dev_attr_link.attr, &dev_attr_rx.attr, &dev_attr_tx.attr, &dev_attr_interval.attr, NULL }; ATTRIBUTE_GROUPS(netdev_trig); also won't apply, as the NUC API doesn't support setting device_name, RX, TX, link or interval. Instead, it allows to set: - the maximum brightness; - the color (if the LED is multi-colored); - the physical port(s) that will be monitored: - LAN1 - LAN2 - LAN1+LAN2 where LAN1 and LAN2 are two physical ports behind the NUC device. The netdev layer knows those as "eno1" and "enp5s0" (not necessarily at the same order). Also, while netdev trigger seems to use just one device name, the NUC allows to monitor both interfaces at the same time. See, unfortunately I can't see a common API that would fit nicely on both cases. > Currently the netdev trigger does the blinking in software only > (code in "ledtrig-netdev.c" file). There is a WIP to add the necessary > support for the netdev trigger to have the ability to offload blinking > to HW. I will try to respin this WIP and send patches for review. > > Once netdev trigger supports this feature, you can implement your > driver in this way. You can even make your driver depend on netdev > trigger > and set the specific LED into netdev triggering by default, and > even forbidding anything else. This is also probably one of the differences from other hardware: In principle, *any* led can monitor *any* hardware event[1]. [1] There are some bitmaps at the interface that would allow the BIOS to restrict it, but, at least on the device I have (Hades Canyon), there's no such restriction: the same bitmap masks are returned for all LEDs. > But this is the corrent way to do this, > instead of creating new sysfs API that is non-extendable. > > I am sorry that I did not explain this thoroughly in previous mails. > Hopefully this explanation is better. Yes, it is a lot better. Thanks for the explanation! Still, as I pointed above, I'm so far unable to see much in common with the way the existing LED drivers work and the way NUC LEDS are controlled. So, as much I would love to just reuse something that already exists, perhaps it would make more sense to create a separate class for such kind of usage. > > Marek > > PS: This is relevant for disk activity as well. Thanks, Mauro
On Wed, 19 May 2021 20:30:14 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > Em Wed, 19 May 2021 17:55:03 +0200 > Marek Behún <kabel@kernel.org> escreveu: > > > On Wed, 19 May 2021 16:24:13 +0200 > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > > > On other words, if no extra care is taken, it could have bad side > > > effects at the machine's performance and affect system's latency, > > > eventually resulting on things like audio clicks and pops, if some > > > audio is playing while such calls keep happening. > > > > In general we want for every LED that is registered into kernel as > > a LED classdev to be possible to control the brightness by > > software. If the hardware supports it, it should be available. > > This is supported, but maybe not the same way as on other drivers. > > There are two separate things: ON/OFF and LED brightness, when turned > ON. > > On other words, NUC leds allow to set the brightness ranging from 0 > to 100, but if the brightness is, let's say 50%, it means that, when > the LED is triggered by the hardware: > > - ON would mean 50%; and > - OFF would mean 0%. Not a problem, there are other controller which also work this way, leds-turris-omnia for example. Also LED triggers are supposed to work this way: if a LED supports non-binary brightness (for exmaple 0-100), and the user sets brightness 50, and then a trigger, then the trigger should blink the LED with brightness 50. > On other words, it actually adjusts the maximum brightness level. > > Btw, this also applies to software control, as the hardware can still > blink the LED, the available properties for software control indicator > are: > - brightness. > - blink behavior and frequency; > - led color (available only if BIOS says that it is a > multi-colored led); - if the hw supports setting the LED to blink with a specific frequency (not depending on any other HW like disk or ethernet, just blinking), you should also implement the .blink_set method (look at Documentation/leds/leds-class.rst section Hardware accelerated blink of LEDs) - if BIOS says the LED is multi-colored, you should register it as multi-colored LED via multicolor framework > > There is a _blocking > > .brightness_set_blocking callback for LEDs which may block when > > setting brightness. > > But even if we did not want to support software control, the > > transparent trigger offloading is still relevant. See below. > > > > > So, IMO, there's very little sense on trying to re-implement the > > > already existing hardware-controlled events via software > > > emulation. > > > > We have a misunderstanding here, probably because of my bad > > explanation, I will try to clarify. > > > > > Sorry, but I guess I missed something here. Are you meaning to use > > > the code under "ledtrig-netdev.c" or something else? > > > > > > The code at ledtrig-netdev.c allocates a trigger data, > > > initializes a spin lock, initializes a delayed work, registers a > > > notifier, sets a trigger interval, etc. It is perfectly fine for > > > software-controlled LEDs, but none of those will ever be used by > > > the NUC driver, if it only implements HW blinking for the > > > Ethernet interfaces (and, as said before, there's little sense > > > emulating it via software on such devices). > > > > The idea of transparent offloading of LED triggers to HW (if HW > > supports it) is to have a consistent and unified interface. > > Makes sense, but not sure if the current API will work. > > > Currently we have a driver (leds-ns2 I think) which allows putting > > the LED into HW controlled mode (to blink on SATA disk activity). > > This is done by writing 1 into /sys/class/leds/<LED>/sata. > > > > In your proposal you are creating several sysfs files: > > indicator > > hdd_default (notice difference from "sata" sysfs file in leds-ns2 > > driver) > > ethernet_type > > > > So the problem here is that this API is not unified. This is > > different from how leds-ns2 driver does this, and both of these > > solutions are wrong, because they are not extendable. > > Partially agreed, but I'm not so sure if the reverse is not true ;-) > > I mean, the current LED API was designed and tested on drivers that > allow direct control of the LED (and then extended to some cases > where the hardware allows offloading). > > The NUC API is just the opposite: there, the BIOS has full control of > the hardware, but it provides an interface that allows changing > the LED behavior, up to some extend. It also allows controlling the > LED hardware and make it blink while it is suspended/hibernating, > which is something that a direct LED control wouldn't allow. If the controller can make the LED to do a specific behaviour when the system is suspended, please use the private LED trigger API and register a LED private trigger (a trigger that is only available for that specific LED) with a name like "suspend-indicator" or something. > So, for instance, if we stick with the current LED API, there's no > way to tell that the power LED should: > > - blink on every 5 seconds, using up to 20% of brightness > when the system is suspended; There is. Implement the .blink_set method. > - strobe on every 10 seconds using up to 50% of brightness > when the system is hibernated; Implement a LED private trigger. > - use 100% of brigntness and don't blink when powered up. Implement a LED private trigger. > > The correct way to do this is via LED triggers, i.e. if I want a > > LED to blink on network activity, then I should use netdev trigger > > and nothing else. The netdev trigger should determine whether the > > underlying LED driver can set the LED to blink on network activity > > in HW. If HW supports it, netdev trigger should use this, otherwise > > netdev trigger should blink the LED in software. > > I understand the desire of exposing the same API, but the current > trigger code doesn't seem to be fit. I mean, the init sequence > done at netdev_trig_activate(): > > trigger_data = kzalloc(sizeof(struct led_netdev_data), > GFP_KERNEL); if (!trigger_data) > return -ENOMEM; > > spin_lock_init(&trigger_data->lock); > > trigger_data->notifier.notifier_call = netdev_trig_notify; > trigger_data->notifier.priority = 10; > > INIT_DELAYED_WORK(&trigger_data->work, netdev_trig_work); > > trigger_data->led_cdev = led_cdev; > trigger_data->net_dev = NULL; > trigger_data->device_name[0] = 0; > > trigger_data->mode = 0; > atomic_set(&trigger_data->interval, msecs_to_jiffies(50)); > trigger_data->last_activity = 0; > > led_set_trigger_data(led_cdev, trigger_data); > > rc = register_netdevice_notifier(&trigger_data->notifier); > if (rc) > kfree(trigger_data); > > doesn't make sense when the LED will be trigged by the hardware, > and registering a notifier for netdevice is overkill. > > The exported attributes: > > static struct attribute *netdev_trig_attrs[] = { > &dev_attr_device_name.attr, > &dev_attr_link.attr, > &dev_attr_rx.attr, > &dev_attr_tx.attr, > &dev_attr_interval.attr, > NULL > }; > ATTRIBUTE_GROUPS(netdev_trig); > > also won't apply, as the NUC API doesn't support setting device_name, > RX, TX, link or interval. > > Instead, it allows to set: > - the maximum brightness; > - the color (if the LED is multi-colored); > - the physical port(s) that will be monitored: > - LAN1 > - LAN2 > - LAN1+LAN2 > > where LAN1 and LAN2 are two physical ports behind the NUC device. > The netdev layer knows those as "eno1" and "enp5s0" (not > necessarily at the same order). The only problem I see with trigger_data in this case is that only one netdevice can be set, while your LED can be configured to blink on activity on two netdevices. Otherwise all these settings are relevant. What your driver offload callback should do (once HW offloading of LED triggers is merged) is this: - the offload method is called by the netdev trigger - the offload method looks at the trigger_data structure. This contains settings rx, tx, link, interval, device_name/device. If the device_name is "eno1" or "end5s0" (or better, if the device points to one of the 2 netdevices that are supported by the HW), and if the rx, tx, link and interval parameters are configured to values that can be done by the LED controller, than put the LED into HW controlled state (to blink with these parameters on network activity on that netdevice) and return 0 - otherwise the offload method should return -EOPNOTSUPP, and the netdev trigger will know that the requested settings are not supported by the HW, and the netdev trigger will blink the LED in SW > Also, while netdev trigger seems to use just one device name, > the NUC allows to monitor both interfaces at the same time. Yes. This can be solved in the future by extending netdev trigger to support blinking on activity on multiple netdevices. I also thought about this for use with another HW (mv88e6xxx switch). > See, unfortunately I can't see a common API that would fit > nicely on both cases. Well I can. The only problem here is that it is not supported yet. I will try to send patches ASAP and poke Pavel to review them. > > Currently the netdev trigger does the blinking in software only > > (code in "ledtrig-netdev.c" file). There is a WIP to add the > > necessary support for the netdev trigger to have the ability to > > offload blinking to HW. I will try to respin this WIP and send > > patches for review. > > > > Once netdev trigger supports this feature, you can implement your > > driver in this way. You can even make your driver depend on netdev > > trigger > > > and set the specific LED into netdev triggering by default, and > > even forbidding anything else. > > This is also probably one of the differences from other hardware: > In principle, *any* led can monitor *any* hardware event[1]. > > [1] There are some bitmaps at the interface that would allow the > BIOS to restrict it, but, at least on the device I have > (Hades Canyon), there's no such restriction: the same bitmap > masks are returned for all LEDs. > > > But this is the corrent way to do this, > > instead of creating new sysfs API that is non-extendable. > > > > I am sorry that I did not explain this thoroughly in previous mails. > > Hopefully this explanation is better. > > Yes, it is a lot better. Thanks for the explanation! > > Still, as I pointed above, I'm so far unable to see much in common > with the way the existing LED drivers work and the way NUC LEDS are > controlled. > > So, as much I would love to just reuse something that already exists, > perhaps it would make more sense to create a separate class for such > kind of usage. The problem is that this API is not yet available in upstream kernel. I will try to push it and return to you. In the meantime let's see what others think about your code. Marek
Em Thu, 20 May 2021 13:00:14 +0200 Marek Behún <marek.behun@nic.cz> escreveu: > On Wed, 19 May 2021 20:30:14 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > Em Wed, 19 May 2021 17:55:03 +0200 > > Marek Behún <kabel@kernel.org> escreveu: > > > > > On Wed, 19 May 2021 16:24:13 +0200 > > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > > > > > On other words, if no extra care is taken, it could have bad side > > > > effects at the machine's performance and affect system's latency, > > > > eventually resulting on things like audio clicks and pops, if some > > > > audio is playing while such calls keep happening. > > > > > > In general we want for every LED that is registered into kernel as > > > a LED classdev to be possible to control the brightness by > > > software. If the hardware supports it, it should be available. > > > > This is supported, but maybe not the same way as on other drivers. > > > > There are two separate things: ON/OFF and LED brightness, when turned > > ON. > > > > On other words, NUC leds allow to set the brightness ranging from 0 > > to 100, but if the brightness is, let's say 50%, it means that, when > > the LED is triggered by the hardware: > > > > - ON would mean 50%; and > > - OFF would mean 0%. > > Not a problem, there are other controller which also work this way, > leds-turris-omnia for example. OK. > Also LED triggers are supposed to work > this way: if a LED supports non-binary brightness (for exmaple 0-100), > and the user sets brightness 50, and then a trigger, then the trigger > should blink the LED with brightness 50. > > > On other words, it actually adjusts the maximum brightness level. > > > > Btw, this also applies to software control, as the hardware can still > > blink the LED, the available properties for software control indicator > > are: > > - brightness. > > - blink behavior and frequency; > > - led color (available only if BIOS says that it is a > > multi-colored led); > > - if the hw supports setting the LED to blink with a specific frequency > (not depending on any other HW like disk or ethernet, just blinking), > you should also implement the .blink_set method (look at > Documentation/leds/leds-class.rst section Hardware accelerated blink > of LEDs) Ok. How the blink pattern is specified? NUC supports 3 different patterns (breathing, pulsing, strobing). > - if BIOS says the LED is multi-colored, you should register it as > multi-colored LED via multicolor framework Ok. I'll check how much work this will lead after we finish the API discussions, as all sysfs attributes that won't fit at the triggers will need to be implemented twice - one for mono-colored and another one for multicolored, as the priv pointer will use different structures. > > The exported attributes: > > > > static struct attribute *netdev_trig_attrs[] = { > > &dev_attr_device_name.attr, > > &dev_attr_link.attr, > > &dev_attr_rx.attr, > > &dev_attr_tx.attr, > > &dev_attr_interval.attr, > > NULL > > }; > > ATTRIBUTE_GROUPS(netdev_trig); > > > > also won't apply, as the NUC API doesn't support setting device_name, > > RX, TX, link or interval. > > > > Instead, it allows to set: > > - the maximum brightness; > > - the color (if the LED is multi-colored); > > - the physical port(s) that will be monitored: > > - LAN1 > > - LAN2 > > - LAN1+LAN2 > > > > where LAN1 and LAN2 are two physical ports behind the NUC device. > > The netdev layer knows those as "eno1" and "enp5s0" (not > > necessarily at the same order). > > The only problem I see with trigger_data in this case is that only one > netdevice can be set, while your LED can be configured to blink on > activity on two netdevices. > > Otherwise all these settings are relevant. > What your driver offload callback should do (once HW offloading of LED > triggers is merged) is this: > - the offload method is called by the netdev trigger > - the offload method looks at the trigger_data structure. This > contains settings rx, tx, link, interval, device_name/device. If the > device_name is "eno1" or "end5s0" (or better, if the device points > to one of the 2 netdevices that are supported by the HW), and if > the rx, tx, link and interval parameters are configured to values > that can be done by the LED controller, than put the LED into HW > controlled state (to blink with these parameters on network > activity on that netdevice) and return 0 > - otherwise the offload method should return -EOPNOTSUPP, and the > netdev trigger will know that the requested settings are not > supported by the HW, and the netdev trigger will blink the LED in > SW See, there's nothing that the driver can possible do with rx, tx, link, interval, device_name/device, as the the BIOS allows to set to "LAN1", "LAN2" or "LAN1+LAN2". the WMI interface doesn't provide *any* information about what LAN1 means. In the case of my NUC, there are even two different drivers: 00:1f.6 Ethernet controller: Intel Corporation Ethernet Connection (2) I219-LM (rev 31) Kernel modules: e1000e 05:00.0 Ethernet controller: Intel Corporation I210 Gigabit Network Connection (rev 03) Kernel modules: igb So, even the probing order can be different after a reboot. So, there isn't a portable way to tell if LAN1 means to either "eno1" or "end5s0"(*), as netdev and the associated net drivers talk with the hardware directly, and not via BIOS. So, the BIOS internal name is not known. Even the DMI tables don't tell it: Handle 0x000F, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: CON4501 Internal Connector Type: None External Reference Designator: LAN External Connector Type: RJ-45 Port Type: Network Port Handle 0x0010, DMI type 8, 9 bytes Port Connector Information Internal Reference Designator: CON4401 Internal Connector Type: None External Reference Designator: LAN External Connector Type: RJ-45 Port Type: Network Port (*) I'm using the interface names on this specific model, but I'm pretty sure other models will have different names and will even use different network drivers. The point is, while the current netdev trigger API can be generic enough when the LED is controlled by netdev, it is *not* generic enough to cover the case where it is trigged by the firmware, as the information exposed by the firmware can be (and it is on this case) completely different than what netdev exposes. > > Also, while netdev trigger seems to use just one device name, > > the NUC allows to monitor both interfaces at the same time. > > Yes. This can be solved in the future by extending netdev trigger to > support blinking on activity on multiple netdevices. I also thought > about this for use with another HW (mv88e6xxx switch). > > > See, unfortunately I can't see a common API that would fit > > nicely on both cases. > > Well I can. Then the API needs to change, in order to allow to abstract from netdev-centric view of Ethernet interfaces. Or, instead, some other trigger is needed for firmware-controlled events. - One thing that it is not clear to me: let's say that the LED called "front1" is currently handling Ethernet events, but the user wants to use, instead, the "front2" LED, disabling the "front1" one (or using for another event, like wifi, which is not monitored on BIOS default). How this can be done using the trigger's API? > The only problem here is that it is not supported yet. I will try to > send patches ASAP and poke Pavel to review them. Ok. Please c/c on such patches. > > > Currently the netdev trigger does the blinking in software only > > > (code in "ledtrig-netdev.c" file). There is a WIP to add the > > > necessary support for the netdev trigger to have the ability to > > > offload blinking to HW. I will try to respin this WIP and send > > > patches for review. > > > > > > Once netdev trigger supports this feature, you can implement your > > > driver in this way. You can even make your driver depend on netdev > > > trigger > > > > > and set the specific LED into netdev triggering by default, and > > > even forbidding anything else. > > > > This is also probably one of the differences from other hardware: > > In principle, *any* led can monitor *any* hardware event[1]. > > > > [1] There are some bitmaps at the interface that would allow the > > BIOS to restrict it, but, at least on the device I have > > (Hades Canyon), there's no such restriction: the same bitmap > > masks are returned for all LEDs. > > > > > But this is the corrent way to do this, > > > instead of creating new sysfs API that is non-extendable. > > > > > > I am sorry that I did not explain this thoroughly in previous mails. > > > Hopefully this explanation is better. > > > > Yes, it is a lot better. Thanks for the explanation! > > > > Still, as I pointed above, I'm so far unable to see much in common > > with the way the existing LED drivers work and the way NUC LEDS are > > controlled. > > > > So, as much I would love to just reuse something that already exists, > > perhaps it would make more sense to create a separate class for such > > kind of usage. > > The problem is that this API is not yet available in upstream kernel. I > will try to push it and return to you. Ok. > > In the meantime let's see what others think about your code. Ok, thank you! Thanks, Mauro
On Thu, 20 May 2021 18:00:28 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > Em Thu, 20 May 2021 13:00:14 +0200 > Marek Behún <marek.behun@nic.cz> escreveu: > > > On Wed, 19 May 2021 20:30:14 +0200 > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > > > Em Wed, 19 May 2021 17:55:03 +0200 > > > Marek Behún <kabel@kernel.org> escreveu: > > > > > > > On Wed, 19 May 2021 16:24:13 +0200 > > > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > > > > > > > On other words, if no extra care is taken, it could have bad side > > > > > effects at the machine's performance and affect system's latency, > > > > > eventually resulting on things like audio clicks and pops, if some > > > > > audio is playing while such calls keep happening. > > > > > > > > In general we want for every LED that is registered into kernel as > > > > a LED classdev to be possible to control the brightness by > > > > software. If the hardware supports it, it should be available. > > > > > > This is supported, but maybe not the same way as on other drivers. > > > > > > There are two separate things: ON/OFF and LED brightness, when turned > > > ON. > > > > > > On other words, NUC leds allow to set the brightness ranging from 0 > > > to 100, but if the brightness is, let's say 50%, it means that, when > > > the LED is triggered by the hardware: > > > > > > - ON would mean 50%; and > > > - OFF would mean 0%. > > > > Not a problem, there are other controller which also work this way, > > leds-turris-omnia for example. > > OK. > > > Also LED triggers are supposed to work > > this way: if a LED supports non-binary brightness (for exmaple 0-100), > > and the user sets brightness 50, and then a trigger, then the trigger > > should blink the LED with brightness 50. > > > > > On other words, it actually adjusts the maximum brightness level. > > > > > > Btw, this also applies to software control, as the hardware can still > > > blink the LED, the available properties for software control indicator > > > are: > > > - brightness. > > > - blink behavior and frequency; > > > - led color (available only if BIOS says that it is a > > > multi-colored led); > > > > - if the hw supports setting the LED to blink with a specific frequency > > (not depending on any other HW like disk or ethernet, just blinking), > > you should also implement the .blink_set method (look at > > Documentation/leds/leds-class.rst section Hardware accelerated blink > > of LEDs) > > Ok. > > How the blink pattern is specified? NUC supports 3 different > patterns (breathing, pulsing, strobing). > > > - if BIOS says the LED is multi-colored, you should register it as > > multi-colored LED via multicolor framework > > Ok. I'll check how much work this will lead after we finish the API > discussions, as all sysfs attributes that won't fit at the triggers > will need to be implemented twice - one for mono-colored and another one > for multicolored, as the priv pointer will use different structures. > > > > The exported attributes: > > > > > > static struct attribute *netdev_trig_attrs[] = { > > > &dev_attr_device_name.attr, > > > &dev_attr_link.attr, > > > &dev_attr_rx.attr, > > > &dev_attr_tx.attr, > > > &dev_attr_interval.attr, > > > NULL > > > }; > > > ATTRIBUTE_GROUPS(netdev_trig); > > > > > > also won't apply, as the NUC API doesn't support setting device_name, > > > RX, TX, link or interval. > > > > > > Instead, it allows to set: > > > - the maximum brightness; > > > - the color (if the LED is multi-colored); > > > - the physical port(s) that will be monitored: > > > - LAN1 > > > - LAN2 > > > - LAN1+LAN2 > > > > > > where LAN1 and LAN2 are two physical ports behind the NUC device. > > > The netdev layer knows those as "eno1" and "enp5s0" (not > > > necessarily at the same order). > > > > The only problem I see with trigger_data in this case is that only one > > netdevice can be set, while your LED can be configured to blink on > > activity on two netdevices. > > > > Otherwise all these settings are relevant. > > What your driver offload callback should do (once HW offloading of LED > > triggers is merged) is this: > > - the offload method is called by the netdev trigger > > - the offload method looks at the trigger_data structure. This > > contains settings rx, tx, link, interval, device_name/device. If the > > device_name is "eno1" or "end5s0" (or better, if the device points > > to one of the 2 netdevices that are supported by the HW), and if > > the rx, tx, link and interval parameters are configured to values > > that can be done by the LED controller, than put the LED into HW > > controlled state (to blink with these parameters on network > > activity on that netdevice) and return 0 > > - otherwise the offload method should return -EOPNOTSUPP, and the > > netdev trigger will know that the requested settings are not > > supported by the HW, and the netdev trigger will blink the LED in > > SW > > See, there's nothing that the driver can possible do with > rx, tx, link, interval, device_name/device, as the the BIOS allows > to set to "LAN1", "LAN2" or "LAN1+LAN2". the WMI interface doesn't > provide *any* information about what LAN1 means. On the contrary, there is something the driver can do with these attributes. If the specific combination is not supported, the driver should return -EOPNOTSUPP in the trigger_offload method and let the netdev trigger do the work in software. What exactly do the LEDs do when configured to blink on activity on a network device? Do they just blink on RX/TX, and otherwise are off? Or are they on when a cable is plugged, blink on rx/tx and otherwise off? > > In the case of my NUC, there are even two different drivers: > > 00:1f.6 Ethernet controller: Intel Corporation Ethernet Connection (2) I219-LM (rev 31) > Kernel modules: e1000e > > 05:00.0 Ethernet controller: Intel Corporation I210 Gigabit Network Connection (rev 03) > Kernel modules: igb > > So, even the probing order can be different after a reboot. > > So, there isn't a portable way to tell if LAN1 means to either > "eno1" or "end5s0"(*), as netdev and the associated net drivers > talk with the hardware directly, and not via BIOS. So, the BIOS > internal name is not known. Even the DMI tables don't tell it: > > Handle 0x000F, DMI type 8, 9 bytes > Port Connector Information > Internal Reference Designator: CON4501 > Internal Connector Type: None > External Reference Designator: LAN > External Connector Type: RJ-45 > Port Type: Network Port > > Handle 0x0010, DMI type 8, 9 bytes > Port Connector Information > Internal Reference Designator: CON4401 > Internal Connector Type: None > External Reference Designator: LAN > External Connector Type: RJ-45 > Port Type: Network Port > > (*) I'm using the interface names on this specific model, but > I'm pretty sure other models will have different names > and will even use different network drivers. Have you looked into DSDT and SSDT tables? > The point is, while the current netdev trigger API can be generic > enough when the LED is controlled by netdev, it is *not* > generic enough to cover the case where it is trigged by the > firmware, as the information exposed by the firmware can be > (and it is on this case) completely different than what netdev > exposes. If even DSDT data is not enough to reliably find out which of the 2 network interfaces belongs to which LED setting, the worst case scenario here is for your driver to need to have a list containing this information for specific motherboards, and other people can then extend the driver to support their motherboards as well. > > > > Also, while netdev trigger seems to use just one device name, > > > the NUC allows to monitor both interfaces at the same time. > > > > Yes. This can be solved in the future by extending netdev trigger to > > support blinking on activity on multiple netdevices. I also thought > > about this for use with another HW (mv88e6xxx switch). > > > > > See, unfortunately I can't see a common API that would fit > > > nicely on both cases. > > > > Well I can. > > Then the API needs to change, in order to allow to abstract from > netdev-centric view of Ethernet interfaces. Or, instead, some > other trigger is needed for firmware-controlled events. No. If the necessary information for determining which network interface pairs to LED1 and which to LED2 cannot be reliably determined from ACPI tables, then IMO the driver should specify this information for each motherboard that wants to use this feature. > - > > One thing that it is not clear to me: let's say that the LED > called "front1" is currently handling Ethernet events, but > the user wants to use, instead, the "front2" LED, disabling > the "front1" one (or using for another event, like wifi, which > is not monitored on BIOS default). > > How this can be done using the trigger's API? cd /sys/class/leds/front1 echo none >trigger cd /sys/class/leds/front2 echo netdev >trigger echo ifname >device_name echo 1 >rx echo 1 >tx
Em Thu, 20 May 2021 18:36:33 +0200 Marek Behún <kabel@kernel.org> escreveu: > On Thu, 20 May 2021 18:00:28 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > Em Thu, 20 May 2021 13:00:14 +0200 > > Marek Behún <marek.behun@nic.cz> escreveu: > > > > > On Wed, 19 May 2021 20:30:14 +0200 > > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > > > > > Em Wed, 19 May 2021 17:55:03 +0200 > > > > Marek Behún <kabel@kernel.org> escreveu: > > > > > > > > > On Wed, 19 May 2021 16:24:13 +0200 > > > > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > > > > > > > > > On other words, if no extra care is taken, it could have bad side > > > > > > effects at the machine's performance and affect system's latency, > > > > > > eventually resulting on things like audio clicks and pops, if some > > > > > > audio is playing while such calls keep happening. > > > > > > > > > > In general we want for every LED that is registered into kernel as > > > > > a LED classdev to be possible to control the brightness by > > > > > software. If the hardware supports it, it should be available. > > > > > > > > This is supported, but maybe not the same way as on other drivers. > > > > > > > > There are two separate things: ON/OFF and LED brightness, when turned > > > > ON. > > > > > > > > On other words, NUC leds allow to set the brightness ranging from 0 > > > > to 100, but if the brightness is, let's say 50%, it means that, when > > > > the LED is triggered by the hardware: > > > > > > > > - ON would mean 50%; and > > > > - OFF would mean 0%. > > > > > > Not a problem, there are other controller which also work this way, > > > leds-turris-omnia for example. > > > > OK. > > > > > Also LED triggers are supposed to work > > > this way: if a LED supports non-binary brightness (for exmaple 0-100), > > > and the user sets brightness 50, and then a trigger, then the trigger > > > should blink the LED with brightness 50. > > > > > > > On other words, it actually adjusts the maximum brightness level. > > > > > > > > Btw, this also applies to software control, as the hardware can still > > > > blink the LED, the available properties for software control indicator > > > > are: > > > > - brightness. > > > > - blink behavior and frequency; > > > > - led color (available only if BIOS says that it is a > > > > multi-colored led); > > > > > > - if the hw supports setting the LED to blink with a specific frequency > > > (not depending on any other HW like disk or ethernet, just blinking), > > > you should also implement the .blink_set method (look at > > > Documentation/leds/leds-class.rst section Hardware accelerated blink > > > of LEDs) > > > > Ok. > > > > How the blink pattern is specified? NUC supports 3 different > > patterns (breathing, pulsing, strobing). > > > > > - if BIOS says the LED is multi-colored, you should register it as > > > multi-colored LED via multicolor framework > > > > Ok. I'll check how much work this will lead after we finish the API > > discussions, as all sysfs attributes that won't fit at the triggers > > will need to be implemented twice - one for mono-colored and another one > > for multicolored, as the priv pointer will use different structures. > > > > > > The exported attributes: > > > > > > > > static struct attribute *netdev_trig_attrs[] = { > > > > &dev_attr_device_name.attr, > > > > &dev_attr_link.attr, > > > > &dev_attr_rx.attr, > > > > &dev_attr_tx.attr, > > > > &dev_attr_interval.attr, > > > > NULL > > > > }; > > > > ATTRIBUTE_GROUPS(netdev_trig); > > > > > > > > also won't apply, as the NUC API doesn't support setting device_name, > > > > RX, TX, link or interval. > > > > > > > > Instead, it allows to set: > > > > - the maximum brightness; > > > > - the color (if the LED is multi-colored); > > > > - the physical port(s) that will be monitored: > > > > - LAN1 > > > > - LAN2 > > > > - LAN1+LAN2 > > > > > > > > where LAN1 and LAN2 are two physical ports behind the NUC device. > > > > The netdev layer knows those as "eno1" and "enp5s0" (not > > > > necessarily at the same order). > > > > > > The only problem I see with trigger_data in this case is that only one > > > netdevice can be set, while your LED can be configured to blink on > > > activity on two netdevices. > > > > > > Otherwise all these settings are relevant. > > > What your driver offload callback should do (once HW offloading of LED > > > triggers is merged) is this: > > > - the offload method is called by the netdev trigger > > > - the offload method looks at the trigger_data structure. This > > > contains settings rx, tx, link, interval, device_name/device. If the > > > device_name is "eno1" or "end5s0" (or better, if the device points > > > to one of the 2 netdevices that are supported by the HW), and if > > > the rx, tx, link and interval parameters are configured to values > > > that can be done by the LED controller, than put the LED into HW > > > controlled state (to blink with these parameters on network > > > activity on that netdevice) and return 0 > > > - otherwise the offload method should return -EOPNOTSUPP, and the > > > netdev trigger will know that the requested settings are not > > > supported by the HW, and the netdev trigger will blink the LED in > > > SW > > > > See, there's nothing that the driver can possible do with > > rx, tx, link, interval, device_name/device, as the the BIOS allows > > to set to "LAN1", "LAN2" or "LAN1+LAN2". the WMI interface doesn't > > provide *any* information about what LAN1 means. > > On the contrary, there is something the driver can do with these > attributes. If the specific combination is not supported, the driver > should return -EOPNOTSUPP in the trigger_offload method and let the > netdev trigger do the work in software. Letting netdev to trigger is something we don't want to allow, as this can cause side effects, making it doing slow the system due to BIOS calls for no good reason. > What exactly do the LEDs do > when configured to blink on activity on a network device? Do they just > blink on RX/TX, and otherwise are off? Or are they on when a cable is > plugged, blink on rx/tx and otherwise off? They are on when a cable is plugged, blink on rx/tx and otherwise off. Worth mentioning that, besides the LEDs controlled by this driver, each RJ-45 port also a couple leds that behave just like normal RJ-45 ones: a yellow led for Ethernet PHY detection and a green one for traffic. > > > > > In the case of my NUC, there are even two different drivers: > > > > 00:1f.6 Ethernet controller: Intel Corporation Ethernet Connection (2) I219-LM (rev 31) > > Kernel modules: e1000e > > > > 05:00.0 Ethernet controller: Intel Corporation I210 Gigabit Network Connection (rev 03) > > Kernel modules: igb > > > > So, even the probing order can be different after a reboot. > > > > So, there isn't a portable way to tell if LAN1 means to either > > "eno1" or "end5s0"(*), as netdev and the associated net drivers > > talk with the hardware directly, and not via BIOS. So, the BIOS > > internal name is not known. Even the DMI tables don't tell it: > > > > Handle 0x000F, DMI type 8, 9 bytes > > Port Connector Information > > Internal Reference Designator: CON4501 > > Internal Connector Type: None > > External Reference Designator: LAN > > External Connector Type: RJ-45 > > Port Type: Network Port > > > > Handle 0x0010, DMI type 8, 9 bytes > > Port Connector Information > > Internal Reference Designator: CON4401 > > Internal Connector Type: None > > External Reference Designator: LAN > > External Connector Type: RJ-45 > > Port Type: Network Port > > > > (*) I'm using the interface names on this specific model, but > > I'm pretty sure other models will have different names > > and will even use different network drivers. > > Have you looked into DSDT and SSDT tables? It doesn't help. # ./generate/unix/bin/acpidump -b # for i in *.dat; do # ./generate/unix/bin/iasl -d $i # done $ grep -i lan *dsl dsdt.dsl: \_SB.PCI0.GLAN.GPEH () dsdt.dsl: Device (GLAN) dsdt.dsl: Notify (GLAN, 0x02) // Device Wake dsdt.dsl: _HID = "ELAN2097" dsdt.dsl: Name (_MLS, Package (0x01) // _MLS: Multiple Language String > > The point is, while the current netdev trigger API can be generic > > enough when the LED is controlled by netdev, it is *not* > > generic enough to cover the case where it is trigged by the > > firmware, as the information exposed by the firmware can be > > (and it is on this case) completely different than what netdev > > exposes. > > If even DSDT data is not enough to reliably find out which of the 2 > network interfaces belongs to which LED setting, the worst case scenario > here is for your driver to need to have a list containing this > information for specific motherboards, and other people can then extend > the driver to support their motherboards as well. Needing something like that sucks and it is hard to maintain, and depends on people reporting issues. Ok, on some cases, there are no other options, but this is not the case here, as the user of such API that wants to monitor just a single interface (default is to monitor both) can easily ask the driver to monitor LAN1. If it doesn't work, switch to LAN2. That's a way more elegant than adding some guessing code that would be checking for the machine codes, eventually printing a warning and disabling support for monitoring LAN when the machine is not properly identified. Also, implementing such table can be painful. I can't see an easy way to implement it, specially without having any information about how all other models that support the WMI API are shipped, and how to map "LAN1", "LAN2" into something that matches netdev detection. OK, if each one have a different BUS ID, a mapping table could associate each one with a different BUS ID, and then some logic at netdev would convert BUS ID into the device name. > > > > Also, while netdev trigger seems to use just one device name, > > > > the NUC allows to monitor both interfaces at the same time. > > > > > > Yes. This can be solved in the future by extending netdev trigger to > > > support blinking on activity on multiple netdevices. I also thought > > > about this for use with another HW (mv88e6xxx switch). > > > > > > > See, unfortunately I can't see a common API that would fit > > > > nicely on both cases. > > > > > > Well I can. > > > > Then the API needs to change, in order to allow to abstract from > > netdev-centric view of Ethernet interfaces. Or, instead, some > > other trigger is needed for firmware-controlled events. > > No. If the necessary information for determining which network > interface pairs to LED1 and which to LED2 cannot be reliably determined > from ACPI tables, then IMO the driver should specify this information > for each motherboard that wants to use this feature. What's the gain of adding such extra complexity to the driver? All the user wants is to blink a led only for one of the LAN ports. Denying it and using a more complex API doesn't make much sense, IMO. > > - > > > > One thing that it is not clear to me: let's say that the LED > > called "front1" is currently handling Ethernet events, but > > the user wants to use, instead, the "front2" LED, disabling > > the "front1" one (or using for another event, like wifi, which > > is not monitored on BIOS default). > > > > How this can be done using the trigger's API? > > cd /sys/class/leds/front1 > echo none >trigger > cd /sys/class/leds/front2 > echo netdev >trigger Clear enough to me. > echo ifname >device_name > echo 1 >rx > echo 1 >tx And that's the part that it makes no sense for this hardware ;-) It can't identify RX/TX in separate. It can only monitor both RX and TX at the same time. So, for this specific device, neither "rx", "tx" or "interval" attributes should be shown. Provided that we could use the Ethernet label as "device_name", this would work: echo "LAN1+LAN2" > device_name echo "LAN1" > device_name echo "LAN2" > device_name or, in order to avoid confusion, we could use a different name on this specific driver, like: echo "LAN1+LAN2" > port_name echo "LAN1" > port_name echo "LAN2" > port_name FYI, the default (LAN1+LAN2) is usually good enough for most use cases, for a couple of reasons: - I guess only high end models have more than one Eth port; - Most people use just one LAN cable; - the back panel leds at the RJ45 are enough to check if ethernet signal was detected. Yet, at least for me, having the NUC led monitoring eth is interesting, as here I use the LAN ports to connect with some testing hardware. So, when the LED is on, it means that my hardware is turned on and the Ethernet connection should be working. Thanks, Mauro
On Thu, 20 May 2021 20:59:33 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > On the contrary, there is something the driver can do with these > > attributes. If the specific combination is not supported, the driver > > should return -EOPNOTSUPP in the trigger_offload method and let the > > netdev trigger do the work in software. > > Letting netdev to trigger is something we don't want to allow, as this > can cause side effects, making it doing slow the system due to BIOS calls > for no good reason. > > > What exactly do the LEDs do > > when configured to blink on activity on a network device? Do they just > > blink on RX/TX, and otherwise are off? Or are they on when a cable is > > plugged, blink on rx/tx and otherwise off? > > They are on when a cable is plugged, blink on rx/tx and otherwise off. > > Worth mentioning that, besides the LEDs controlled by this driver, each > RJ-45 port also a couple leds that behave just like normal RJ-45 ones: > a yellow led for Ethernet PHY detection and a green one for traffic. So what the LED does when configured for ethernet is almost equivalent to netdev setting [link=1, rx=1, activity=1]. Almost because we still have the correct device setting and interval setting. Theoretically what you can do is deny the netdev trigger for every other netdev setting (since, according to you, it would use too much CPU time in BIOS via software control). This could be done by the offload method returning another error value, or maybe just returning 0 and printing info about this into kernel log. I wonder what others think about this possible resolution. > > Have you looked into DSDT and SSDT tables? > > It doesn't help. Pity :( > > If even DSDT data is not enough to reliably find out which of the 2 > > network interfaces belongs to which LED setting, the worst case scenario > > here is for your driver to need to have a list containing this > > information for specific motherboards, and other people can then extend > > the driver to support their motherboards as well. > > Needing something like that sucks and it is hard to maintain, > and depends on people reporting issues. I don't see much difference between this and various drivers having different OF compatible strings for different chips all supported by one driver. > Ok, on some cases, there are no other options, but this is not > the case here, as the user of such API that wants to monitor > just a single interface (default is to monitor both) can easily > ask the driver to monitor LAN1. If it doesn't work, switch to LAN2. > > That's a way more elegant than adding some guessing code that > would be checking for the machine codes, eventually printing > a warning and disabling support for monitoring LAN when the > machine is not properly identified. > > Also, implementing such table can be painful. I can't see an > easy way to implement it, specially without having any information > about how all other models that support the WMI API are shipped, > and how to map "LAN1", "LAN2" into something that matches netdev > detection. OK, if each one have a different BUS ID, > a mapping table could associate each one with a different BUS > ID, and then some logic at netdev would convert BUS ID into > the device name. > > > > > > Also, while netdev trigger seems to use just one device name, > > > > > the NUC allows to monitor both interfaces at the same time. > > > > > > > > Yes. This can be solved in the future by extending netdev trigger to > > > > support blinking on activity on multiple netdevices. I also thought > > > > about this for use with another HW (mv88e6xxx switch). > > > > > > > > > See, unfortunately I can't see a common API that would fit > > > > > nicely on both cases. > > > > > > > > Well I can. > > > > > > Then the API needs to change, in order to allow to abstract from > > > netdev-centric view of Ethernet interfaces. Or, instead, some > > > other trigger is needed for firmware-controlled events. > > > > No. If the necessary information for determining which network > > interface pairs to LED1 and which to LED2 cannot be reliably determined > > from ACPI tables, then IMO the driver should specify this information > > for each motherboard that wants to use this feature. > > What's the gain of adding such extra complexity to the driver? Having a consistent API on different devices is a benefit in itself, I would think. > All the user wants is to blink a led only for one of the LAN ports. > > Denying it and using a more complex API doesn't make much sense, IMO. As I see it you are the one wanting to introduce more complexity into the sysfs ABI. There is already a solution together with documentation and everything for when the user wants to "blink a led only for one of the LAN ports". It is the netdev trigger. And you want to complicate that ABI. > > > - > > > > > > One thing that it is not clear to me: let's say that the LED > > > called "front1" is currently handling Ethernet events, but > > > the user wants to use, instead, the "front2" LED, disabling > > > the "front1" one (or using for another event, like wifi, which > > > is not monitored on BIOS default). > > > > > > How this can be done using the trigger's API? > > > > cd /sys/class/leds/front1 > > echo none >trigger > > cd /sys/class/leds/front2 > > echo netdev >trigger > > Clear enough to me. > > > echo ifname >device_name > > echo 1 >rx > > echo 1 >tx > > And that's the part that it makes no sense for this hardware ;-) > > It can't identify RX/TX in separate. It can only monitor both RX and TX at > the same time. > > So, for this specific device, neither "rx", "tx" or "interval" > attributes should be shown. If a netdev setting is not supported by the HW, it should be done in SW. You say that for this controller it would be bad to do in SW, because it would take too much time in BIOS calls. (I wonder how much...) If this is really the case then I still think it is more preferable to do this via netdev trigger, and forbid settings not supported by HW. The result could be: # I want the LED to blink on ethernet activity $ echo netdev >trigger # ok, but I only wan't it to blink on rx activity, not tx $ echo 0 >tx Operation not supported. Pavel, what is your opinion here? Marek
Em Thu, 20 May 2021 22:07:03 +0200 Marek Behún <kabel@kernel.org> escreveu: > On Thu, 20 May 2021 20:59:33 +0200 > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote: > > > > On the contrary, there is something the driver can do with these > > > attributes. If the specific combination is not supported, the driver > > > should return -EOPNOTSUPP in the trigger_offload method and let the > > > netdev trigger do the work in software. > > > > Letting netdev to trigger is something we don't want to allow, as this > > can cause side effects, making it doing slow the system due to BIOS calls > > for no good reason. > > > > > What exactly do the LEDs do > > > when configured to blink on activity on a network device? Do they just > > > blink on RX/TX, and otherwise are off? Or are they on when a cable is > > > plugged, blink on rx/tx and otherwise off? > > > > They are on when a cable is plugged, blink on rx/tx and otherwise off. > > > > Worth mentioning that, besides the LEDs controlled by this driver, each > > RJ-45 port also a couple leds that behave just like normal RJ-45 ones: > > a yellow led for Ethernet PHY detection and a green one for traffic. > > So what the LED does when configured for ethernet is almost equivalent > to netdev setting [link=1, rx=1, activity=1]. Almost because we still have > the correct device setting and interval setting. > > Theoretically what you can do is deny the netdev trigger for every > other netdev setting (since, according to you, it would use too much > CPU time in BIOS via software control). This could be done by the > offload method returning another error value, or maybe just returning 0 > and printing info about this into kernel log. I wonder what others > think about this possible resolution. IMO, it would be preferred to have a different trigger here, as this is not a netdev-based trigger. So, its implementation should not call: register_netdevice_notifier() nor set timers, etc. See, the hardware won't use any information provided by the netdev, trigger and the API is not the same, as the hardware trigger only wants to know if just one interface will trigger the led or both. > > > If even DSDT data is not enough to reliably find out which of the 2 > > > network interfaces belongs to which LED setting, the worst case scenario > > > here is for your driver to need to have a list containing this > > > information for specific motherboards, and other people can then extend > > > the driver to support their motherboards as well. > > > > Needing something like that sucks and it is hard to maintain, > > and depends on people reporting issues. > > I don't see much difference between this and various drivers having > different OF compatible strings for different chips all supported by > one driver. It is somewhat similar: on my experiences, the upstream OF compatibles are almost always outdated uptream: only OOT Kernels have the full OF stuff :-p The major difference is that hardware vendors usually develop and provide OF. In this case, you want users to fill bug reports that will ask a Kernel developer to add new entries for their machines to work properly. While we do this on other drivers, doing that is time consuming and may lead into errors. Believe me: this is needed on media drivers, as there are things like GPIOs that are device-specific. It is a pain to maintain those things. > > > > Then the API needs to change, in order to allow to abstract from > > > > netdev-centric view of Ethernet interfaces. Or, instead, some > > > > other trigger is needed for firmware-controlled events. > > > > > > No. If the necessary information for determining which network > > > interface pairs to LED1 and which to LED2 cannot be reliably determined > > > from ACPI tables, then IMO the driver should specify this information > > > for each motherboard that wants to use this feature. > > > > What's the gain of adding such extra complexity to the driver? > > Having a consistent API on different devices is a benefit in itself, I > would think. It wouldn't be consistent. Hardware sees the events on different ways than netdev, and associating netdev's view of the interface with the hardware's view will always be an issue, on any driver that would trigger such kind of events. See, let's assume someone would implement a hardware trigger for the LEDs on an 48-ports Ethernet switch, and different versions of such hub would use different Ethernet drivers. No matter how netdev sees the hardware, or if some of the ports can be replaced (some devices allow port hot-plugging), from userspace perspective, what it really matter is the port number as seen at the switch panel, no matter how netdev sees it. So, for hardware-triggered events, the hardware "label" is a lot more relevant than any linux-internal representation. > > All the user wants is to blink a led only for one of the LAN ports. > > > > Denying it and using a more complex API doesn't make much sense, IMO. > > As I see it you are the one wanting to introduce more complexity into > the sysfs ABI. There is already a solution together with documentation > and everything for when the user wants to "blink a led only for one of > the LAN ports". It is the netdev trigger. And you want to complicate > that ABI. No. The existing in-kernel API is to blink a led based on software events originated from netdev from a single network port. It could monitor an interface that doesn't physically exist (a virtual network interface, like tun0). It could even monitor traffic on a single VLAN, if the interface is specified like eth0.100[1]. [1] As we're discussing API here, I didn't test/check if the current implementation allows monitoring virtual and VLAN interfaces. From API's perspective, it makes perfect sense to be able to monitor any physical or logical interface supported by netdev. The HW trigger is different: led blinks if the hardware detects Ethernet activity on one or more physical interfaces. See, the netdev trigger monitors a netdev event with may or may not be due to an Ethernet port. An Ethernet HW trigger monitors activity on a set of physical Ethernet ports. In essence, the only thing in common is that both triggers are related to network, but they're actually monitoring two different types of events. Merging them into a single one sounds a conceptual mistake on my eyes. > You say that for this controller it would be bad to do in SW, because it > would take too much time in BIOS calls. (I wonder how much...) Yeah, it would be interesting to know how much. However, measuring BIOS latency and time spent on such calls can be tricky: one needs to use a high-res clock that it is not used anywhere else, in order to measure it. Thanks, Mauro
Hi! > > > See, there's nothing that the driver can possible do with > > > rx, tx, link, interval, device_name/device, as the the BIOS allows > > > to set to "LAN1", "LAN2" or "LAN1+LAN2". the WMI interface doesn't > > > provide *any* information about what LAN1 means. > > > > On the contrary, there is something the driver can do with these > > attributes. If the specific combination is not supported, the driver > > should return -EOPNOTSUPP in the trigger_offload method and let the > > netdev trigger do the work in software. > > Letting netdev to trigger is something we don't want to allow, as this > can cause side effects, making it doing slow the system due to BIOS calls > for no good reason. I'm with Marek here. Please listen to him. Yes, operating LEDs can cost some CPU cycles. That's the case on most hardware. Yet we want to support most triggers on most hardware. Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
Hi! > > You say that for this controller it would be bad to do in SW, because it > > would take too much time in BIOS calls. (I wonder how much...) > > Yeah, it would be interesting to know how much. However, measuring BIOS > latency and time spent on such calls can be tricky: one needs to use a > high-res clock that it is not used anywhere else, in order to measure > it. I'm sure you can time kernel compilation while LEDs are using software netdev trigger, for example. Pavel -- http://www.livejournal.com/~pavelmachek
Em Wed, 26 May 2021 16:47:51 +0200 Pavel Machek <pavel@ucw.cz> escreveu: > Hi! > > > > > See, there's nothing that the driver can possible do with > > > > rx, tx, link, interval, device_name/device, as the the BIOS allows > > > > to set to "LAN1", "LAN2" or "LAN1+LAN2". the WMI interface doesn't > > > > provide *any* information about what LAN1 means. > > > > > > On the contrary, there is something the driver can do with these > > > attributes. If the specific combination is not supported, the driver > > > should return -EOPNOTSUPP in the trigger_offload method and let the > > > netdev trigger do the work in software. > > > > Letting netdev to trigger is something we don't want to allow, as this > > can cause side effects, making it doing slow the system due to BIOS calls > > for no good reason. > > I'm with Marek here. Please listen to him. > > Yes, operating LEDs can cost some CPU cycles. That's the case on most > hardware. Yet we want to support most triggers on most hardware. There are two separate things here: 1. a "normal" LED operation can indeed take some CPU cycles, specially if done via an I2C bus. Yet, the Kernel will still be kept running. A BIOS call means that the Kernel will remain interrupted until when the BIOS decide to return control back to it. This affects all CPUs, and not just the one that would be busy setting the LED. So, it is not a matter of just wasting some CPU cycles, but, instead, on potentially preventing all CPUs to run Kernel code, if the BIOS decides to lock until it finishes the LED setting and decides to return the control back to Linux. In practice, depending on what workload is used, their real time requirements, and what the BIOS does (which may vary from device to device and on different BIOS versions) this could cause loses. This will mainly affect Real Time (e. g. audio and video apps). A realistic test would be to make the LED blink as fast as possible, while a pro-Audio device using JACK outputs something using the smallest possible delay and see if blinking the leds would cause any audio issues. While I'm not against allowing using a software-triggered event, as this *can* cause userspace problems, IMO the user needs to explicitly allow the usage of it and be aware when a software trigger is used. Letting the LEDs core or driver to fallback to software and cause disturbance at userspace doesn't sound right. 2. a netdev trigger monitors a different event than the hardware event. See, if we have something like: LAN1 ----> eno1 ---> eno1.100 # VLAN 100 traffic at eno1 port | +-> eno1.200 # VLAN 200 traffic at eno1 LAN2 ---> enp5s0 ---> enp5s0.100 # VLAN 100 traffic at enp5s0 port | +-> enp5s0.200 # VLAN 200 traffic at enp5s0 The hardware triggered event monitors a group of physical interfaces, e. g.: - none - LAN1 - LAN2 - both LAN1 and LAN2 (default) The netdev trigger monitors software events at the network stack. So, it can be set to monitor a single software representation of an interface, e. g. it can monitor either: - eno1 - enp5so - eno1.100 - eno1.200 - enp5s0.100 - enp5so.200 It doesn't allow monitoring multiple interfaces at the same time. So, it is not possible to monitor both eno1 and enp5so. Also, even if it would be possible, what hardware detects could be different than what the network stack detects. On other words, even if we keep the BIOS default on front2 led and set front3 led to netdev trigger for eno1 they will blink differently, as one would be monitoring a single interface and another one will monitor both. It will even more different if netdev is set to monitor, let's say, VLAN 100 traffic at eno1.100, while hw trigger is set to LAN1+LAN2. Thanks, Mauro
Em Wed, 26 May 2021 16:51:12 +0200 Pavel Machek <pavel@ucw.cz> escreveu: > Hi! > > > > You say that for this controller it would be bad to do in SW, because it > > > would take too much time in BIOS calls. (I wonder how much...) > > > > Yeah, it would be interesting to know how much. However, measuring BIOS > > latency and time spent on such calls can be tricky: one needs to use a > > high-res clock that it is not used anywhere else, in order to measure > > it. > > I'm sure you can time kernel compilation while LEDs are using software > netdev trigger, for example. I can't see how. I mean, the problem is to monitor the impact of the BIOS call with may affect not only the application using the LED, but all other applications running at the same time on different CPUs, as the BIOS may want/need to lock other CPUs while its code is running, as it can't see the Linux CPU locks. Thanks, Mauro
diff --git a/drivers/leds/leds-nuc.c b/drivers/leds/leds-nuc.c index 4d4ea6fbeff4..f84ec5662f5c 100644 --- a/drivers/leds/leds-nuc.c +++ b/drivers/leds/leds-nuc.c @@ -1698,12 +1698,100 @@ static ssize_t store_hdd_default(struct device *dev, return len; } +/* Ethernet type */ +static const char * const ethernet_type[] = { + "LAN1", + "LAN2", + "LAN1+LAN2" +}; + +static ssize_t show_ethernet_type(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct led_classdev *cdev = dev_get_drvdata(dev); + struct nuc_nmi_led *led = container_of(cdev, struct nuc_nmi_led, cdev); + u8 input[NUM_INPUT_ARGS] = { 0 }; + u8 output[NUM_OUTPUT_ARGS]; + int ctrl, ret, val, i, n; + int size = PAGE_SIZE; + char *p = buf; + + if (led->indicator != LED_IND_ETHERNET) + return -EINVAL; + + ctrl = led->reg_table[led->indicator][LED_FUNC_ETH_TYPE]; + + if (!nuc_wmi_test_control(dev, led, ctrl)) + return -ENODEV; + + input[0] = LED_NEW_GET_CONTROL_ITEM; + input[1] = led->id; + input[2] = led->indicator; + input[3] = ctrl; + + ret = nuc_nmi_cmd(dev, LED_NEW_GET_STATUS, input, output); + if (ret) + return ret; + + val = output[0]; + + for (i = 0; i < ARRAY_SIZE(ethernet_type); i++) { + if (i == val) + n = scnprintf(p, size, "[%s] ", ethernet_type[i]); + else + n = scnprintf(p, size, "%s ", ethernet_type[i]); + p += n; + size -= n; + } + size -= scnprintf(p, size, "\n"); + + return PAGE_SIZE - size; +} + +static ssize_t store_ethernet_type(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t len) +{ + struct led_classdev *cdev = dev_get_drvdata(dev); + struct nuc_nmi_led *led = container_of(cdev, struct nuc_nmi_led, cdev); + u8 input[NUM_INPUT_ARGS] = { 0 }; + int ctrl, val, ret; + const char *tmp; + + if (led->indicator != LED_IND_ETHERNET) + return -EINVAL; + + ctrl = led->reg_table[led->indicator][LED_FUNC_ETH_TYPE]; + + if (!nuc_wmi_test_control(dev, led, ctrl)) + return -ENODEV; + + for (val = 0; val < ARRAY_SIZE(ethernet_type); val++) + if (!strcasecmp(tmp, ethernet_type[val])) + break; + + if (val >= ARRAY_SIZE(ethernet_type)) + return -EINVAL; + + input[0] = led->id; + input[1] = led->indicator; + input[2] = ctrl; + input[3] = val; + + ret = nuc_nmi_cmd(dev, LED_SET_VALUE, input, NULL); + if (ret) + return ret; + + return len; +} static LED_ATTR_RW(indicator); static LED_ATTR_RW(color); static LED_ATTR_RW(blink_behavior); static LED_ATTR_RW(blink_frequency); static LED_ATTR_RW(hdd_default); +static LED_ATTR_RW(ethernet_type); LED_ATTR_POWER_STATE_RW(s0_brightness, brightness, 0); LED_ATTR_POWER_STATE_RW(s0_blink_behavior, blink_behavior, 0); @@ -1732,6 +1820,7 @@ LED_ATTR_POWER_STATE_RW(standby_blink_frequency, blink_frequency, 2); static struct attribute *nuc_wmi_led_attr[] = { &dev_attr_indicator.attr, &dev_attr_hdd_default.attr, + &dev_attr_ethernet_type.attr, NULL, };
The Ethernet type indicator can be configured to show the status of LAN1, LAN1 or both. Add support for it. Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> --- drivers/leds/leds-nuc.c | 89 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+)