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 <[email protected]>
---
drivers/leds/leds-nuc.c | 89 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 89 insertions(+)
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,
};
--
2.31.1
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 <[email protected]> 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 <[email protected]> wrote:
> Em Wed, 19 May 2021 10:02:53 +0200
> Marek Behún <[email protected]> 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 <[email protected]> escreveu:
> On Wed, 19 May 2021 12:18:12 +0200
> Mauro Carvalho Chehab <[email protected]> wrote:
>
> > Em Wed, 19 May 2021 10:02:53 +0200
> > Marek Behún <[email protected]> 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 <[email protected]> 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 <[email protected]> escreveu:
> On Wed, 19 May 2021 16:24:13 +0200
> Mauro Carvalho Chehab <[email protected]> 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 <[email protected]> wrote:
> Em Wed, 19 May 2021 17:55:03 +0200
> Marek Behún <[email protected]> escreveu:
>
> > On Wed, 19 May 2021 16:24:13 +0200
> > Mauro Carvalho Chehab <[email protected]> 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 <[email protected]> escreveu:
> On Wed, 19 May 2021 20:30:14 +0200
> Mauro Carvalho Chehab <[email protected]> wrote:
>
> > Em Wed, 19 May 2021 17:55:03 +0200
> > Marek Behún <[email protected]> escreveu:
> >
> > > On Wed, 19 May 2021 16:24:13 +0200
> > > Mauro Carvalho Chehab <[email protected]> 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 20:59:33 +0200
Mauro Carvalho Chehab <[email protected]> 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
On Thu, 20 May 2021 18:00:28 +0200
Mauro Carvalho Chehab <[email protected]> wrote:
> Em Thu, 20 May 2021 13:00:14 +0200
> Marek Behún <[email protected]> escreveu:
>
> > On Wed, 19 May 2021 20:30:14 +0200
> > Mauro Carvalho Chehab <[email protected]> wrote:
> >
> > > Em Wed, 19 May 2021 17:55:03 +0200
> > > Marek Behún <[email protected]> escreveu:
> > >
> > > > On Wed, 19 May 2021 16:24:13 +0200
> > > > Mauro Carvalho Chehab <[email protected]> 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 <[email protected]> escreveu:
> On Thu, 20 May 2021 18:00:28 +0200
> Mauro Carvalho Chehab <[email protected]> wrote:
>
> > Em Thu, 20 May 2021 13:00:14 +0200
> > Marek Behún <[email protected]> escreveu:
> >
> > > On Wed, 19 May 2021 20:30:14 +0200
> > > Mauro Carvalho Chehab <[email protected]> wrote:
> > >
> > > > Em Wed, 19 May 2021 17:55:03 +0200
> > > > Marek Behún <[email protected]> escreveu:
> > > >
> > > > > On Wed, 19 May 2021 16:24:13 +0200
> > > > > Mauro Carvalho Chehab <[email protected]> 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
Em Thu, 20 May 2021 22:07:03 +0200
Marek Behún <[email protected]> escreveu:
> On Thu, 20 May 2021 20:59:33 +0200
> Mauro Carvalho Chehab <[email protected]> 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!
> > 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
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
Em Wed, 26 May 2021 16:47:51 +0200
Pavel Machek <[email protected]> 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 <[email protected]> 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