Allow setting netdev LED trigger as default when given LED DT node has
the `trigger-sources` property pointing to a node corresponding to a
network device.
The specific netdev trigger mode is determined from the `function` LED
property.
Example:
eth0: ethernet@30000 {
compatible = "xyz";
#trigger-source-cells = <0>;
};
led {
color = <LED_COLOR_ID_GREEN>;
function = LED_FUNCTION_LINK;
trigger-sources = <ð0>;
};
Signed-off-by: Marek Behún <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: [email protected]
---
drivers/leds/trigger/ledtrig-netdev.c | 80 ++++++++++++++++++++++++++-
include/dt-bindings/leds/common.h | 1 +
2 files changed, 80 insertions(+), 1 deletion(-)
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index d5e774d830215..99fc2f0c68e12 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -20,6 +20,7 @@
#include <linux/list.h>
#include <linux/module.h>
#include <linux/netdevice.h>
+#include <linux/of_net.h>
#include <linux/spinlock.h>
#include <linux/timer.h>
#include "../leds.h"
@@ -389,6 +390,70 @@ static void netdev_trig_work(struct work_struct *work)
(atomic_read(&trigger_data->interval)*2));
}
+static bool netdev_trig_of_parse(struct led_classdev *led_cdev,
+ struct led_netdev_data *trigger_data)
+{
+ struct of_phandle_args args;
+ struct net_device *netdev;
+ struct device_node *np;
+ const char *function;
+ unsigned long mode;
+ int err;
+
+ /* netdev trigger can have only one source */
+ if (of_led_count_trigger_sources(led_cdev) != 1)
+ return false;
+
+ err = of_led_get_trigger_source(led_cdev, 0, &args);
+ if (err)
+ return false;
+
+ netdev = of_find_net_device_by_node(args.np);
+ if (!netdev)
+ return false;
+
+ np = dev_of_node(led_cdev->dev);
+ if (!np)
+ return false;
+
+ err = of_property_read_string(np, "function", &function);
+ if (err && err != -ENOENT) {
+ dev_warn(led_cdev->dev, "Failed parsing function for %pOF!\n",
+ np);
+ return false;
+ } else if (err == -ENOENT) {
+ /* default function is link */
+ function = LED_FUNCTION_LINK;
+ }
+
+ mode = 0;
+ if (!strcmp(function, LED_FUNCTION_LINK)) {
+ set_bit(NETDEV_LED_LINK, &mode);
+ } else if (!strcmp(function, LED_FUNCTION_ACTIVITY)) {
+ set_bit(NETDEV_LED_TX, &mode);
+ set_bit(NETDEV_LED_RX, &mode);
+ } else if (!strcmp(function, LED_FUNCTION_RX)) {
+ set_bit(NETDEV_LED_RX, &mode);
+ } else if (!strcmp(function, LED_FUNCTION_TX)) {
+ set_bit(NETDEV_LED_TX, &mode);
+ } else {
+ dev_dbg(led_cdev->dev,
+ "Unsupported netdev trigger function for %pOF!\n", np);
+ return false;
+ }
+
+ if (trigger_data) {
+ dev_hold(netdev);
+ trigger_data->net_dev = netdev;
+ memcpy(trigger_data->device_name, netdev->name, IFNAMSIZ);
+ trigger_data->mode = mode;
+ if (netif_carrier_ok(netdev))
+ set_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
+ }
+
+ return true;
+}
+
static int netdev_trig_activate(struct led_classdev *led_cdev)
{
struct led_netdev_data *trigger_data;
@@ -414,10 +479,17 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
trigger_data->last_activity = 0;
led_set_trigger_data(led_cdev, trigger_data);
+ netdev_trig_of_parse(led_cdev, trigger_data);
rc = register_netdevice_notifier(&trigger_data->notifier);
- if (rc)
+ if (rc) {
+ if (trigger_data->net_dev)
+ dev_put(trigger_data->net_dev);
kfree(trigger_data);
+ } else {
+ if (trigger_data->net_dev)
+ set_baseline_state(trigger_data);
+ }
return rc;
}
@@ -436,10 +508,16 @@ static void netdev_trig_deactivate(struct led_classdev *led_cdev)
kfree(trigger_data);
}
+static bool netdev_trig_has_valid_source(struct led_classdev *led_cdev)
+{
+ return netdev_trig_of_parse(led_cdev, NULL);
+}
+
static struct led_trigger netdev_led_trigger = {
.name = "netdev",
.activate = netdev_trig_activate,
.deactivate = netdev_trig_deactivate,
+ .has_valid_source = netdev_trig_has_valid_source,
.groups = netdev_trig_groups,
};
diff --git a/include/dt-bindings/leds/common.h b/include/dt-bindings/leds/common.h
index 52b619d44ba25..c7f9d34d60206 100644
--- a/include/dt-bindings/leds/common.h
+++ b/include/dt-bindings/leds/common.h
@@ -77,6 +77,7 @@
#define LED_FUNCTION_HEARTBEAT "heartbeat"
#define LED_FUNCTION_INDICATOR "indicator"
#define LED_FUNCTION_LAN "lan"
+#define LED_FUNCTION_LINK "link"
#define LED_FUNCTION_MAIL "mail"
#define LED_FUNCTION_MTD "mtd"
#define LED_FUNCTION_PANIC "panic"
--
2.26.2
Hi Marek,
On 9/15/20 5:26 PM, Marek Behún wrote:
> Allow setting netdev LED trigger as default when given LED DT node has
> the `trigger-sources` property pointing to a node corresponding to a
> network device.
>
> The specific netdev trigger mode is determined from the `function` LED
> property.
>
> Example:
> eth0: ethernet@30000 {
> compatible = "xyz";
> #trigger-source-cells = <0>;
> };
>
> led {
> color = <LED_COLOR_ID_GREEN>;
> function = LED_FUNCTION_LINK;
> trigger-sources = <ð0>;
> };
>
> Signed-off-by: Marek Behún <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: [email protected]
> ---
> drivers/leds/trigger/ledtrig-netdev.c | 80 ++++++++++++++++++++++++++-
> include/dt-bindings/leds/common.h | 1 +
> 2 files changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index d5e774d830215..99fc2f0c68e12 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -20,6 +20,7 @@
[...]
> static int netdev_trig_activate(struct led_classdev *led_cdev)
> {
> struct led_netdev_data *trigger_data;
> @@ -414,10 +479,17 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
> trigger_data->last_activity = 0;
>
> led_set_trigger_data(led_cdev, trigger_data);
> + netdev_trig_of_parse(led_cdev, trigger_data);
Please be aware of LED_INIT_DEFAULT_TRIGGER flag - it would make
sense to use it here so as not to unnecessarily call
netdev_trig_of_parse(), which makes sense only if trigger will be
default, I presume.
See timer_trig_activate() in drivers/leds/trigger/ledtrig-timer.c
for reference.
--
Best regards,
Jacek Anaszewski
On Tue, 15 Sep 2020 23:35:25 +0200
Jacek Anaszewski <[email protected]> wrote:
> Hi Marek,
>
> On 9/15/20 5:26 PM, Marek Behún wrote:
> > Allow setting netdev LED trigger as default when given LED DT node has
> > the `trigger-sources` property pointing to a node corresponding to a
> > network device.
> >
> > The specific netdev trigger mode is determined from the `function` LED
> > property.
> >
> > Example:
> > eth0: ethernet@30000 {
> > compatible = "xyz";
> > #trigger-source-cells = <0>;
> > };
> >
> > led {
> > color = <LED_COLOR_ID_GREEN>;
> > function = LED_FUNCTION_LINK;
> > trigger-sources = <ð0>;
> > };
> >
> > Signed-off-by: Marek Behún <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: [email protected]
> > ---
> > drivers/leds/trigger/ledtrig-netdev.c | 80 ++++++++++++++++++++++++++-
> > include/dt-bindings/leds/common.h | 1 +
> > 2 files changed, 80 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> > index d5e774d830215..99fc2f0c68e12 100644
> > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > @@ -20,6 +20,7 @@
> [...]
>
> > static int netdev_trig_activate(struct led_classdev *led_cdev)
> > {
> > struct led_netdev_data *trigger_data;
> > @@ -414,10 +479,17 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
> > trigger_data->last_activity = 0;
> >
> > led_set_trigger_data(led_cdev, trigger_data);
> > + netdev_trig_of_parse(led_cdev, trigger_data);
>
> Please be aware of LED_INIT_DEFAULT_TRIGGER flag - it would make
> sense to use it here so as not to unnecessarily call
> netdev_trig_of_parse(), which makes sense only if trigger will be
> default, I presume.
>
> See timer_trig_activate() in drivers/leds/trigger/ledtrig-timer.c
> for reference.
>
Hmmm. Jacek, all the triggers that work with the macro
LED_INIT_DEFAULT_TRIGGER are oneshot, timer and pattern.
If this macro is set, they all call pattern_init function where they
read led-pattern from fwnode.
But there is no device tree in Linux sources using this property.
In fact the command
git grep led-pattern
yields only 2 files:
Documentation/devicetree/bindings/leds/common.yaml
drivers/leds/led-core.c
What is the purpose if no device tree uses this property? Is this used
from other fwnode sources, like acpi or efi?
The reason why I am asking this is that the `led-pattern` property in
device tree goes against the principle of device tree, that it
shouldn't set settings settable from userspace, only describe the
devices on the system and how they are connected to each other.
This is the same reason why multi-CPU DSA proposals which proposed to
set mappings between CPU ports and user ports were rejected...
Marek
On 9/16/20 2:15 AM, Marek Behun wrote:
> On Tue, 15 Sep 2020 23:35:25 +0200
> Jacek Anaszewski <[email protected]> wrote:
>
>> Hi Marek,
>>
>> On 9/15/20 5:26 PM, Marek Behún wrote:
>>> Allow setting netdev LED trigger as default when given LED DT node has
>>> the `trigger-sources` property pointing to a node corresponding to a
>>> network device.
>>>
>>> The specific netdev trigger mode is determined from the `function` LED
>>> property.
>>>
>>> Example:
>>> eth0: ethernet@30000 {
>>> compatible = "xyz";
>>> #trigger-source-cells = <0>;
>>> };
>>>
>>> led {
>>> color = <LED_COLOR_ID_GREEN>;
>>> function = LED_FUNCTION_LINK;
>>> trigger-sources = <ð0>;
>>> };
>>>
>>> Signed-off-by: Marek Behún <[email protected]>
>>> Cc: Rob Herring <[email protected]>
>>> Cc: [email protected]
>>> ---
>>> drivers/leds/trigger/ledtrig-netdev.c | 80 ++++++++++++++++++++++++++-
>>> include/dt-bindings/leds/common.h | 1 +
>>> 2 files changed, 80 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
>>> index d5e774d830215..99fc2f0c68e12 100644
>>> --- a/drivers/leds/trigger/ledtrig-netdev.c
>>> +++ b/drivers/leds/trigger/ledtrig-netdev.c
>>> @@ -20,6 +20,7 @@
>> [...]
>>
>>> static int netdev_trig_activate(struct led_classdev *led_cdev)
>>> {
>>> struct led_netdev_data *trigger_data;
>>> @@ -414,10 +479,17 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
>>> trigger_data->last_activity = 0;
>>>
>>> led_set_trigger_data(led_cdev, trigger_data);
>>> + netdev_trig_of_parse(led_cdev, trigger_data);
>>
>> Please be aware of LED_INIT_DEFAULT_TRIGGER flag - it would make
>> sense to use it here so as not to unnecessarily call
>> netdev_trig_of_parse(), which makes sense only if trigger will be
>> default, I presume.
>>
>> See timer_trig_activate() in drivers/leds/trigger/ledtrig-timer.c
>> for reference.
>>
>
> Hmmm. Jacek, all the triggers that work with the macro
> LED_INIT_DEFAULT_TRIGGER are oneshot, timer and pattern.
> If this macro is set, they all call pattern_init function where they
> read led-pattern from fwnode.
The fact that they all call pattern_init() does not mean that
the use of flag is limited only to this type of triggers.
It has been introduced to allow initialization of default trigger
with required parameters, but in the same time, not to enforce
the same initial parameters each next time the trigger is set
for the LED.
>
> But there is no device tree in Linux sources using this property.
> In fact the command
> git grep led-pattern
> yields only 2 files:
> Documentation/devicetree/bindings/leds/common.yaml
> drivers/leds/led-core.c
>
> What is the purpose if no device tree uses this property? Is this used
> from other fwnode sources, like acpi or efi?
This is mainly useful for debugging purposes, probably that's why
it is not present in official dts files yet.
> The reason why I am asking this is that the `led-pattern` property in
> device tree goes against the principle of device tree, that it
> shouldn't set settings settable from userspace, only describe the
> devices on the system and how they are connected to each other.
If that was a hard principle then we wouldn't have properties like
linux,default-trigger. I have once asked Rob about that - see the reply
at the and of message [0].
[0] https://lore.kernel.org/linux-leds/20181025195444.GA12737@bogus/
--
Best regards,
Jacek Anaszewski
Hi!
> Allow setting netdev LED trigger as default when given LED DT node has
> the `trigger-sources` property pointing to a node corresponding to a
> network device.
>
> The specific netdev trigger mode is determined from the `function` LED
> property.
Sounds reasonable.
> + netdev = of_find_net_device_by_node(args.np);
> + if (!netdev)
> + return false;
> +
> + np = dev_of_node(led_cdev->dev);
> + if (!np)
> + return false;
Missing of_node_put?
> +++ b/include/dt-bindings/leds/common.h
> @@ -77,6 +77,7 @@
> #define LED_FUNCTION_HEARTBEAT "heartbeat"
> #define LED_FUNCTION_INDICATOR "indicator"
> #define LED_FUNCTION_LAN "lan"
> +#define LED_FUNCTION_LINK "link"
> #define LED_FUNCTION_MAIL "mail"
> #define LED_FUNCTION_MTD "mtd"
> #define LED_FUNCTION_PANIC "panic"
We have function "lan" already defined; "link" would do mostly same
thing. Should we use "lan"? Or should we delete "lan" and replace it
with "link"?
Best regards,
Pavel
--
http://www.livejournal.com/~pavelmachek
On Wed, 25 Nov 2020 11:42:42 +0100
Pavel Machek <[email protected]> wrote:
> Hi!
>
> > Allow setting netdev LED trigger as default when given LED DT node has
> > the `trigger-sources` property pointing to a node corresponding to a
> > network device.
> >
> > The specific netdev trigger mode is determined from the `function` LED
> > property.
>
> Sounds reasonable.
>
> > + netdev = of_find_net_device_by_node(args.np);
> > + if (!netdev)
> > + return false;
> > +
> > + np = dev_of_node(led_cdev->dev);
> > + if (!np)
> > + return false;
>
> Missing of_node_put?
I will look into this.
>
> > +++ b/include/dt-bindings/leds/common.h
> > @@ -77,6 +77,7 @@
> > #define LED_FUNCTION_HEARTBEAT "heartbeat"
> > #define LED_FUNCTION_INDICATOR "indicator"
> > #define LED_FUNCTION_LAN "lan"
> > +#define LED_FUNCTION_LINK "link"
> > #define LED_FUNCTION_MAIL "mail"
> > #define LED_FUNCTION_MTD "mtd"
> > #define LED_FUNCTION_PANIC "panic"
>
> We have function "lan" already defined; "link" would do mostly same
> thing. Should we use "lan"? Or should we delete "lan" and replace it
> with "link"?
Removal of "lan" should depend on whether it is used somewhere...
But I think "link" is more sensible since:
- it can distinguish better from "activity" if "activity" should mean
blinking on RX/TX
- the interface must not necessarily be a LAN (in the sense that on
routers we have WAN and LAN and possibly other, user defined
networks).
We could use "lan" though to mean "link" and "activity" together (ie.
LED on when interface linked, LED blinking on RX/TX).
We have to decide whether specifying more LED functions to one LED in
DT should look like:
function = LED_FUNCTION_LINK, LED_FUNCTION_ACTIVITY;
or rather like
function = LED_FUNCTION_LAN;
The problem with the second one is that we would need specific
functions for different compound modes (if we wanted to do that), eg:
function = LED_FUNCTION_LAN_RX;
Marek