Add hardware control support for the Netdev trigger.
The trigger on config change will check if the requested trigger can set
to blink mode using LED hardware mode and if every blink mode is supported,
the trigger will enable hardware mode with the requested configuration.
If there is at least one trigger that is not supported and can't run in
hardware mode, then software mode will be used instead.
A validation is done on every value change and on fail the old value is
restored and -EINVAL is returned.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/leds/trigger/ledtrig-netdev.c | 155 +++++++++++++++++++++++++-
1 file changed, 149 insertions(+), 6 deletions(-)
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index dd63cadb896e..ed019cb5867c 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -37,6 +37,7 @@
*/
struct led_netdev_data {
+ enum led_blink_modes blink_mode;
spinlock_t lock;
struct delayed_work work;
@@ -53,11 +54,105 @@ struct led_netdev_data {
bool carrier_link_up;
};
+struct netdev_led_attr_detail {
+ char *name;
+ bool hardware_only;
+ enum led_trigger_netdev_modes bit;
+};
+
+static struct netdev_led_attr_detail attr_details[] = {
+ { .name = "link", .bit = TRIGGER_NETDEV_LINK},
+ { .name = "tx", .bit = TRIGGER_NETDEV_TX},
+ { .name = "rx", .bit = TRIGGER_NETDEV_RX},
+};
+
+static bool validate_baseline_state(struct led_netdev_data *trigger_data)
+{
+ struct led_classdev *led_cdev = trigger_data->led_cdev;
+ struct netdev_led_attr_detail *detail;
+ u32 hw_blink_mode_supported = 0;
+ bool force_sw = false;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
+ detail = &attr_details[i];
+
+ /* Mode not active, skip */
+ if (!test_bit(detail->bit, &trigger_data->mode))
+ continue;
+
+ /* Hardware only mode enabled on software controlled led */
+ if (led_cdev->blink_mode == SOFTWARE_CONTROLLED &&
+ detail->hardware_only)
+ return false;
+
+ /* Check if the mode supports hardware mode */
+ if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
+ /* With a net dev set, force software mode.
+ * With modes are handled by hardware, led will blink
+ * based on his own events and will ignore any event
+ * from the provided dev.
+ */
+ if (trigger_data->net_dev) {
+ force_sw = true;
+ continue;
+ }
+
+ /* With empty dev, check if the mode is supported */
+ if (led_trigger_blink_mode_is_supported(led_cdev, detail->bit))
+ hw_blink_mode_supported |= BIT(detail->bit);
+ }
+ }
+
+ /* We can't run modes handled by both software and hardware.
+ * Check if we run hardware modes and check if all the modes
+ * can be handled by hardware.
+ */
+ if (hw_blink_mode_supported && hw_blink_mode_supported != trigger_data->mode)
+ return false;
+
+ /* Modes are valid. Decide now the running mode to later
+ * set the baseline.
+ * Software mode is enforced with net_dev set. With an empty
+ * one hardware mode is selected by default (if supported).
+ */
+ if (force_sw || led_cdev->blink_mode == SOFTWARE_CONTROLLED)
+ trigger_data->blink_mode = SOFTWARE_CONTROLLED;
+ else
+ trigger_data->blink_mode = HARDWARE_CONTROLLED;
+
+ return true;
+}
+
static void set_baseline_state(struct led_netdev_data *trigger_data)
{
+ int i;
int current_brightness;
+ struct netdev_led_attr_detail *detail;
struct led_classdev *led_cdev = trigger_data->led_cdev;
+ /* Modes already validated. Directly apply hw trigger modes */
+ if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
+ /* We are refreshing the blink modes. Reset them */
+ led_cdev->hw_control_configure(led_cdev, BIT(TRIGGER_NETDEV_LINK),
+ BLINK_MODE_ZERO);
+
+ for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
+ detail = &attr_details[i];
+
+ if (!test_bit(detail->bit, &trigger_data->mode))
+ continue;
+
+ led_cdev->hw_control_configure(led_cdev, BIT(detail->bit),
+ BLINK_MODE_ENABLE);
+ }
+
+ led_cdev->hw_control_start(led_cdev);
+
+ return;
+ }
+
+ /* Handle trigger modes by software */
current_brightness = led_cdev->brightness;
if (current_brightness)
led_cdev->blink_brightness = current_brightness;
@@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev,
size_t size)
{
struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
+ struct net_device *old_net = trigger_data->net_dev;
+ char old_device_name[IFNAMSIZ];
if (size >= IFNAMSIZ)
return -EINVAL;
+ /* Backup old device name */
+ memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
+
cancel_delayed_work_sync(&trigger_data->work);
spin_lock_bh(&trigger_data->lock);
@@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev,
trigger_data->net_dev =
dev_get_by_name(&init_net, trigger_data->device_name);
+ if (!validate_baseline_state(trigger_data)) {
+ /* Restore old net_dev and device_name */
+ if (trigger_data->net_dev)
+ dev_put(trigger_data->net_dev);
+
+ dev_hold(old_net);
+ trigger_data->net_dev = old_net;
+ memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ);
+
+ spin_unlock_bh(&trigger_data->lock);
+ return -EINVAL;
+ }
+
trigger_data->carrier_link_up = false;
if (trigger_data->net_dev != NULL)
trigger_data->carrier_link_up = netif_carrier_ok(trigger_data->net_dev);
@@ -159,7 +272,7 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
size_t size, enum led_trigger_netdev_modes attr)
{
struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
- unsigned long state;
+ unsigned long state, old_mode = trigger_data->mode;
int ret;
int bit;
@@ -184,6 +297,12 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
else
clear_bit(bit, &trigger_data->mode);
+ if (!validate_baseline_state(trigger_data)) {
+ /* Restore old mode on validation fail */
+ trigger_data->mode = old_mode;
+ return -EINVAL;
+ }
+
set_baseline_state(trigger_data);
return size;
@@ -220,6 +339,8 @@ static ssize_t interval_store(struct device *dev,
size_t size)
{
struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
+ int old_interval = atomic_read(&trigger_data->interval);
+ u32 old_mode = trigger_data->mode;
unsigned long value;
int ret;
@@ -228,13 +349,22 @@ static ssize_t interval_store(struct device *dev,
return ret;
/* impose some basic bounds on the timer interval */
- if (value >= 5 && value <= 10000) {
- cancel_delayed_work_sync(&trigger_data->work);
+ if (value < 5 || value > 10000)
+ return -EINVAL;
+
+ cancel_delayed_work_sync(&trigger_data->work);
+
+ atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
- atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
- set_baseline_state(trigger_data); /* resets timer */
+ if (!validate_baseline_state(trigger_data)) {
+ /* Restore old interval on validation error */
+ atomic_set(&trigger_data->interval, old_interval);
+ trigger_data->mode = old_mode;
+ return -EINVAL;
}
+ set_baseline_state(trigger_data); /* resets timer */
+
return size;
}
@@ -368,13 +498,25 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
trigger_data->mode = 0;
atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
trigger_data->last_activity = 0;
+ if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
+ /* With hw mode enabled reset any rule set by default */
+ if (led_cdev->hw_control_status(led_cdev)) {
+ rc = led_cdev->hw_control_configure(led_cdev, BIT(TRIGGER_NETDEV_LINK),
+ BLINK_MODE_ZERO);
+ if (rc)
+ goto err;
+ }
+ }
led_set_trigger_data(led_cdev, trigger_data);
rc = register_netdevice_notifier(&trigger_data->notifier);
if (rc)
- kfree(trigger_data);
+ goto err;
+ return 0;
+err:
+ kfree(trigger_data);
return rc;
}
@@ -394,6 +536,7 @@ static void netdev_trig_deactivate(struct led_classdev *led_cdev)
static struct led_trigger netdev_led_trigger = {
.name = "netdev",
+ .supported_blink_modes = SOFTWARE_HARDWARE,
.activate = netdev_trig_activate,
.deactivate = netdev_trig_deactivate,
.groups = netdev_trig_groups,
--
2.37.2
Hi Christian,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pavel-leds/for-next]
[also build test ERROR on robh/for-next net-next/master net/master linus/master v6.1 next-20221215]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
base: git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
patch link: https://lore.kernel.org/r/20221214235438.30271-7-ansuelsmth%40gmail.com
patch subject: [PATCH v7 06/11] leds: trigger: netdev: add hardware control support
config: arc-randconfig-r005-20221214
compiler: arceb-elf-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/eb6fea2dd465c5aa879ed5c47d7109bd441797c2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
git checkout eb6fea2dd465c5aa879ed5c47d7109bd441797c2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/leds/trigger/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/leds/trigger/ledtrig-netdev.c: In function 'validate_baseline_state':
>> drivers/leds/trigger/ledtrig-netdev.c:102:29: error: implicit declaration of function 'led_trigger_blink_mode_is_supported'; did you mean 'led_trigger_blink_oneshot'? [-Werror=implicit-function-declaration]
102 | if (led_trigger_blink_mode_is_supported(led_cdev, detail->bit))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| led_trigger_blink_oneshot
drivers/leds/trigger/ledtrig-netdev.c: In function 'set_baseline_state':
>> drivers/leds/trigger/ledtrig-netdev.c:137:25: error: 'struct led_classdev' has no member named 'hw_control_configure'
137 | led_cdev->hw_control_configure(led_cdev, BIT(TRIGGER_NETDEV_LINK),
| ^~
>> drivers/leds/trigger/ledtrig-netdev.c:138:48: error: 'BLINK_MODE_ZERO' undeclared (first use in this function)
138 | BLINK_MODE_ZERO);
| ^~~~~~~~~~~~~~~
drivers/leds/trigger/ledtrig-netdev.c:138:48: note: each undeclared identifier is reported only once for each function it appears in
drivers/leds/trigger/ledtrig-netdev.c:146:33: error: 'struct led_classdev' has no member named 'hw_control_configure'
146 | led_cdev->hw_control_configure(led_cdev, BIT(detail->bit),
| ^~
>> drivers/leds/trigger/ledtrig-netdev.c:147:56: error: 'BLINK_MODE_ENABLE' undeclared (first use in this function); did you mean 'IF_LINK_MODE_DEFAULT'?
147 | BLINK_MODE_ENABLE);
| ^~~~~~~~~~~~~~~~~
| IF_LINK_MODE_DEFAULT
>> drivers/leds/trigger/ledtrig-netdev.c:150:25: error: 'struct led_classdev' has no member named 'hw_control_start'
150 | led_cdev->hw_control_start(led_cdev);
| ^~
drivers/leds/trigger/ledtrig-netdev.c: In function 'netdev_trig_activate':
>> drivers/leds/trigger/ledtrig-netdev.c:503:29: error: 'struct led_classdev' has no member named 'hw_control_status'
503 | if (led_cdev->hw_control_status(led_cdev)) {
| ^~
drivers/leds/trigger/ledtrig-netdev.c:504:38: error: 'struct led_classdev' has no member named 'hw_control_configure'
504 | rc = led_cdev->hw_control_configure(led_cdev, BIT(TRIGGER_NETDEV_LINK),
| ^~
drivers/leds/trigger/ledtrig-netdev.c:505:61: error: 'BLINK_MODE_ZERO' undeclared (first use in this function)
505 | BLINK_MODE_ZERO);
| ^~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +102 drivers/leds/trigger/ledtrig-netdev.c
68
69 static bool validate_baseline_state(struct led_netdev_data *trigger_data)
70 {
71 struct led_classdev *led_cdev = trigger_data->led_cdev;
72 struct netdev_led_attr_detail *detail;
73 u32 hw_blink_mode_supported = 0;
74 bool force_sw = false;
75 int i;
76
77 for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
78 detail = &attr_details[i];
79
80 /* Mode not active, skip */
81 if (!test_bit(detail->bit, &trigger_data->mode))
82 continue;
83
84 /* Hardware only mode enabled on software controlled led */
85 if (led_cdev->blink_mode == SOFTWARE_CONTROLLED &&
86 detail->hardware_only)
87 return false;
88
89 /* Check if the mode supports hardware mode */
90 if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
91 /* With a net dev set, force software mode.
92 * With modes are handled by hardware, led will blink
93 * based on his own events and will ignore any event
94 * from the provided dev.
95 */
96 if (trigger_data->net_dev) {
97 force_sw = true;
98 continue;
99 }
100
101 /* With empty dev, check if the mode is supported */
> 102 if (led_trigger_blink_mode_is_supported(led_cdev, detail->bit))
103 hw_blink_mode_supported |= BIT(detail->bit);
104 }
105 }
106
107 /* We can't run modes handled by both software and hardware.
108 * Check if we run hardware modes and check if all the modes
109 * can be handled by hardware.
110 */
111 if (hw_blink_mode_supported && hw_blink_mode_supported != trigger_data->mode)
112 return false;
113
114 /* Modes are valid. Decide now the running mode to later
115 * set the baseline.
116 * Software mode is enforced with net_dev set. With an empty
117 * one hardware mode is selected by default (if supported).
118 */
119 if (force_sw || led_cdev->blink_mode == SOFTWARE_CONTROLLED)
120 trigger_data->blink_mode = SOFTWARE_CONTROLLED;
121 else
122 trigger_data->blink_mode = HARDWARE_CONTROLLED;
123
124 return true;
125 }
126
127 static void set_baseline_state(struct led_netdev_data *trigger_data)
128 {
129 int i;
130 int current_brightness;
131 struct netdev_led_attr_detail *detail;
132 struct led_classdev *led_cdev = trigger_data->led_cdev;
133
134 /* Modes already validated. Directly apply hw trigger modes */
135 if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
136 /* We are refreshing the blink modes. Reset them */
> 137 led_cdev->hw_control_configure(led_cdev, BIT(TRIGGER_NETDEV_LINK),
> 138 BLINK_MODE_ZERO);
139
140 for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
141 detail = &attr_details[i];
142
143 if (!test_bit(detail->bit, &trigger_data->mode))
144 continue;
145
146 led_cdev->hw_control_configure(led_cdev, BIT(detail->bit),
> 147 BLINK_MODE_ENABLE);
148 }
149
> 150 led_cdev->hw_control_start(led_cdev);
151
152 return;
153 }
154
155 /* Handle trigger modes by software */
156 current_brightness = led_cdev->brightness;
157 if (current_brightness)
158 led_cdev->blink_brightness = current_brightness;
159 if (!led_cdev->blink_brightness)
160 led_cdev->blink_brightness = led_cdev->max_brightness;
161
162 if (!trigger_data->carrier_link_up) {
163 led_set_brightness(led_cdev, LED_OFF);
164 } else {
165 if (test_bit(TRIGGER_NETDEV_LINK, &trigger_data->mode))
166 led_set_brightness(led_cdev,
167 led_cdev->blink_brightness);
168 else
169 led_set_brightness(led_cdev, LED_OFF);
170
171 /* If we are looking for RX/TX start periodically
172 * checking stats
173 */
174 if (test_bit(TRIGGER_NETDEV_TX, &trigger_data->mode) ||
175 test_bit(TRIGGER_NETDEV_RX, &trigger_data->mode))
176 schedule_delayed_work(&trigger_data->work, 0);
177 }
178 }
179
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi,
thanks for the v7 series.
Am Donnerstag, 15. Dezember 2022, 00:54:33 CET schrieb Christian Marangi:
> Add hardware control support for the Netdev trigger.
> The trigger on config change will check if the requested trigger can set
> to blink mode using LED hardware mode and if every blink mode is supported,
> the trigger will enable hardware mode with the requested configuration.
> If there is at least one trigger that is not supported and can't run in
> hardware mode, then software mode will be used instead.
> A validation is done on every value change and on fail the old value is
> restored and -EINVAL is returned.
>
> Signed-off-by: Christian Marangi <[email protected]>
> ---
> drivers/leds/trigger/ledtrig-netdev.c | 155 +++++++++++++++++++++++++-
> 1 file changed, 149 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c
> b/drivers/leds/trigger/ledtrig-netdev.c index dd63cadb896e..ed019cb5867c
> 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -37,6 +37,7 @@
> */
>
> struct led_netdev_data {
> + enum led_blink_modes blink_mode;
> spinlock_t lock;
>
> struct delayed_work work;
> @@ -53,11 +54,105 @@ struct led_netdev_data {
> bool carrier_link_up;
> };
>
> +struct netdev_led_attr_detail {
> + char *name;
> + bool hardware_only;
> + enum led_trigger_netdev_modes bit;
> +};
> +
> +static struct netdev_led_attr_detail attr_details[] = {
> + { .name = "link", .bit = TRIGGER_NETDEV_LINK},
> + { .name = "tx", .bit = TRIGGER_NETDEV_TX},
> + { .name = "rx", .bit = TRIGGER_NETDEV_RX},
> +};
> +
> +static bool validate_baseline_state(struct led_netdev_data *trigger_data)
> +{
> + struct led_classdev *led_cdev = trigger_data->led_cdev;
> + struct netdev_led_attr_detail *detail;
> + u32 hw_blink_mode_supported = 0;
> + bool force_sw = false;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> + detail = &attr_details[i];
> +
> + /* Mode not active, skip */
> + if (!test_bit(detail->bit, &trigger_data->mode))
> + continue;
> +
> + /* Hardware only mode enabled on software controlled led
*/
> + if (led_cdev->blink_mode == SOFTWARE_CONTROLLED &&
> + detail->hardware_only)
> + return false;
> +
> + /* Check if the mode supports hardware mode */
> + if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> + /* With a net dev set, force software mode.
> + * With modes are handled by hardware, led will
blink
> + * based on his own events and will ignore any
event
> + * from the provided dev.
> + */
> + if (trigger_data->net_dev) {
> + force_sw = true;
> + continue;
> + }
> +
> + /* With empty dev, check if the mode is
supported */
> + if
(led_trigger_blink_mode_is_supported(led_cdev, detail->bit))
> + hw_blink_mode_supported |= BIT(detail-
>bit);
Shouldn't this be BIT(detail->bit)?
> + }
> + }
> +
> + /* We can't run modes handled by both software and hardware.
> + * Check if we run hardware modes and check if all the modes
> + * can be handled by hardware.
> + */
> + if (hw_blink_mode_supported && hw_blink_mode_supported !=
> trigger_data->mode) + return false;
> +
> + /* Modes are valid. Decide now the running mode to later
> + * set the baseline.
> + * Software mode is enforced with net_dev set. With an empty
> + * one hardware mode is selected by default (if supported).
> + */
> + if (force_sw || led_cdev->blink_mode == SOFTWARE_CONTROLLED)
IMHO '|| !hw_blink_mode_supported' should be added here for blink_modes. This
might happen if a PHY LED is SOFTWARE_HARDWARE_CONTROLLED, but some blink mode
is not supported by hardware, thus hw_blink_mode_supported=0.
Best regards,
Alexander
> + trigger_data->blink_mode = SOFTWARE_CONTROLLED;
> + else
> + trigger_data->blink_mode = HARDWARE_CONTROLLED;
> +
> + return true;
> +}
> +
> static void set_baseline_state(struct led_netdev_data *trigger_data)
> {
> + int i;
> int current_brightness;
> + struct netdev_led_attr_detail *detail;
> struct led_classdev *led_cdev = trigger_data->led_cdev;
>
> + /* Modes already validated. Directly apply hw trigger modes */
> + if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
> + /* We are refreshing the blink modes. Reset them */
> + led_cdev->hw_control_configure(led_cdev,
BIT(TRIGGER_NETDEV_LINK),
> + BLINK_MODE_ZERO);
> +
> + for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> + detail = &attr_details[i];
> +
> + if (!test_bit(detail->bit, &trigger_data->mode))
> + continue;
> +
> + led_cdev->hw_control_configure(led_cdev,
BIT(detail->bit),
> +
BLINK_MODE_ENABLE);
Shouldn't this be BIT(detail->bit)?
> + }
> +
> + led_cdev->hw_control_start(led_cdev);
> +
> + return;
> + }
> +
> + /* Handle trigger modes by software */
> current_brightness = led_cdev->brightness;
> if (current_brightness)
> led_cdev->blink_brightness = current_brightness;
> @@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev,
> size_t size)
> {
> struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> + struct net_device *old_net = trigger_data->net_dev;
> + char old_device_name[IFNAMSIZ];
>
> if (size >= IFNAMSIZ)
> return -EINVAL;
>
> + /* Backup old device name */
> + memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> +
> cancel_delayed_work_sync(&trigger_data->work);
>
> spin_lock_bh(&trigger_data->lock);
> @@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev,
> trigger_data->net_dev =
> dev_get_by_name(&init_net, trigger_data->device_name);
>
> + if (!validate_baseline_state(trigger_data)) {
> + /* Restore old net_dev and device_name */
> + if (trigger_data->net_dev)
> + dev_put(trigger_data->net_dev);
> +
> + dev_hold(old_net);
> + trigger_data->net_dev = old_net;
> + memcpy(trigger_data->device_name, old_device_name,
IFNAMSIZ);
> +
> + spin_unlock_bh(&trigger_data->lock);
> + return -EINVAL;
> + }
> +
> trigger_data->carrier_link_up = false;
> if (trigger_data->net_dev != NULL)
> trigger_data->carrier_link_up =
netif_carrier_ok(trigger_data->net_dev);
> @@ -159,7 +272,7 @@ static ssize_t netdev_led_attr_store(struct device *dev,
> const char *buf, size_t size, enum led_trigger_netdev_modes attr)
> {
> struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> - unsigned long state;
> + unsigned long state, old_mode = trigger_data->mode;
> int ret;
> int bit;
>
> @@ -184,6 +297,12 @@ static ssize_t netdev_led_attr_store(struct device
> *dev, const char *buf, else
> clear_bit(bit, &trigger_data->mode);
>
> + if (!validate_baseline_state(trigger_data)) {
> + /* Restore old mode on validation fail */
> + trigger_data->mode = old_mode;
> + return -EINVAL;
> + }
> +
> set_baseline_state(trigger_data);
>
> return size;
> @@ -220,6 +339,8 @@ static ssize_t interval_store(struct device *dev,
> size_t size)
> {
> struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> + int old_interval = atomic_read(&trigger_data->interval);
> + u32 old_mode = trigger_data->mode;
> unsigned long value;
> int ret;
>
> @@ -228,13 +349,22 @@ static ssize_t interval_store(struct device *dev,
> return ret;
>
> /* impose some basic bounds on the timer interval */
> - if (value >= 5 && value <= 10000) {
> - cancel_delayed_work_sync(&trigger_data->work);
> + if (value < 5 || value > 10000)
> + return -EINVAL;
> +
> + cancel_delayed_work_sync(&trigger_data->work);
> +
> + atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
>
> - atomic_set(&trigger_data->interval,
msecs_to_jiffies(value));
> - set_baseline_state(trigger_data); /* resets timer
*/
> + if (!validate_baseline_state(trigger_data)) {
> + /* Restore old interval on validation error */
> + atomic_set(&trigger_data->interval, old_interval);
> + trigger_data->mode = old_mode;
> + return -EINVAL;
> }
>
> + set_baseline_state(trigger_data); /* resets timer */
> +
> return size;
> }
>
> @@ -368,13 +498,25 @@ static int netdev_trig_activate(struct led_classdev
> *led_cdev) trigger_data->mode = 0;
> atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
> trigger_data->last_activity = 0;
> + if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> + /* With hw mode enabled reset any rule set by default */
> + if (led_cdev->hw_control_status(led_cdev)) {
> + rc = led_cdev->hw_control_configure(led_cdev,
BIT(TRIGGER_NETDEV_LINK),
> +
BLINK_MODE_ZERO);
> + if (rc)
> + goto err;
> + }
> + }
>
> led_set_trigger_data(led_cdev, trigger_data);
>
> rc = register_netdevice_notifier(&trigger_data->notifier);
> if (rc)
> - kfree(trigger_data);
> + goto err;
>
> + return 0;
> +err:
> + kfree(trigger_data);
> return rc;
> }
>
> @@ -394,6 +536,7 @@ static void netdev_trig_deactivate(struct led_classdev
> *led_cdev)
>
> static struct led_trigger netdev_led_trigger = {
> .name = "netdev",
> + .supported_blink_modes = SOFTWARE_HARDWARE,
> .activate = netdev_trig_activate,
> .deactivate = netdev_trig_deactivate,
> .groups = netdev_trig_groups,
On Thu, Dec 15, 2022 at 12:54:33AM +0100, Christian Marangi wrote:
> Add hardware control support for the Netdev trigger.
> The trigger on config change will check if the requested trigger can set
> to blink mode using LED hardware mode and if every blink mode is supported,
> the trigger will enable hardware mode with the requested configuration.
> If there is at least one trigger that is not supported and can't run in
> hardware mode, then software mode will be used instead.
> A validation is done on every value change and on fail the old value is
> restored and -EINVAL is returned.
>
> Signed-off-by: Christian Marangi <[email protected]>
> ---
> drivers/leds/trigger/ledtrig-netdev.c | 155 +++++++++++++++++++++++++-
> 1 file changed, 149 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> index dd63cadb896e..ed019cb5867c 100644
> --- a/drivers/leds/trigger/ledtrig-netdev.c
> +++ b/drivers/leds/trigger/ledtrig-netdev.c
> @@ -37,6 +37,7 @@
> */
>
> struct led_netdev_data {
> + enum led_blink_modes blink_mode;
> spinlock_t lock;
>
> struct delayed_work work;
> @@ -53,11 +54,105 @@ struct led_netdev_data {
> bool carrier_link_up;
> };
>
> +struct netdev_led_attr_detail {
> + char *name;
> + bool hardware_only;
> + enum led_trigger_netdev_modes bit;
> +};
> +
> +static struct netdev_led_attr_detail attr_details[] = {
> + { .name = "link", .bit = TRIGGER_NETDEV_LINK},
> + { .name = "tx", .bit = TRIGGER_NETDEV_TX},
> + { .name = "rx", .bit = TRIGGER_NETDEV_RX},
> +};
> +
> +static bool validate_baseline_state(struct led_netdev_data *trigger_data)
> +{
> + struct led_classdev *led_cdev = trigger_data->led_cdev;
> + struct netdev_led_attr_detail *detail;
> + u32 hw_blink_mode_supported = 0;
> + bool force_sw = false;
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> + detail = &attr_details[i];
> +
> + /* Mode not active, skip */
> + if (!test_bit(detail->bit, &trigger_data->mode))
> + continue;
> +
> + /* Hardware only mode enabled on software controlled led */
> + if (led_cdev->blink_mode == SOFTWARE_CONTROLLED &&
> + detail->hardware_only)
> + return false;
> +
> + /* Check if the mode supports hardware mode */
> + if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> + /* With a net dev set, force software mode.
> + * With modes are handled by hardware, led will blink
> + * based on his own events and will ignore any event
> + * from the provided dev.
> + */
> + if (trigger_data->net_dev) {
> + force_sw = true;
> + continue;
> + }
> +
> + /* With empty dev, check if the mode is supported */
> + if (led_trigger_blink_mode_is_supported(led_cdev, detail->bit))
> + hw_blink_mode_supported |= BIT(detail->bit);
> + }
> + }
> +
> + /* We can't run modes handled by both software and hardware.
> + * Check if we run hardware modes and check if all the modes
> + * can be handled by hardware.
> + */
> + if (hw_blink_mode_supported && hw_blink_mode_supported != trigger_data->mode)
> + return false;
> +
> + /* Modes are valid. Decide now the running mode to later
> + * set the baseline.
> + * Software mode is enforced with net_dev set. With an empty
> + * one hardware mode is selected by default (if supported).
> + */
> + if (force_sw || led_cdev->blink_mode == SOFTWARE_CONTROLLED)
> + trigger_data->blink_mode = SOFTWARE_CONTROLLED;
> + else
> + trigger_data->blink_mode = HARDWARE_CONTROLLED;
> +
> + return true;
> +}
> +
> static void set_baseline_state(struct led_netdev_data *trigger_data)
> {
> + int i;
> int current_brightness;
> + struct netdev_led_attr_detail *detail;
> struct led_classdev *led_cdev = trigger_data->led_cdev;
>
> + /* Modes already validated. Directly apply hw trigger modes */
> + if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
> + /* We are refreshing the blink modes. Reset them */
> + led_cdev->hw_control_configure(led_cdev, BIT(TRIGGER_NETDEV_LINK),
> + BLINK_MODE_ZERO);
> +
> + for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> + detail = &attr_details[i];
> +
> + if (!test_bit(detail->bit, &trigger_data->mode))
> + continue;
> +
> + led_cdev->hw_control_configure(led_cdev, BIT(detail->bit),
> + BLINK_MODE_ENABLE);
> + }
> +
> + led_cdev->hw_control_start(led_cdev);
> +
> + return;
> + }
> +
> + /* Handle trigger modes by software */
> current_brightness = led_cdev->brightness;
> if (current_brightness)
> led_cdev->blink_brightness = current_brightness;
> @@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev,
> size_t size)
> {
> struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> + struct net_device *old_net = trigger_data->net_dev;
> + char old_device_name[IFNAMSIZ];
>
> if (size >= IFNAMSIZ)
> return -EINVAL;
>
> + /* Backup old device name */
> + memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> +
> cancel_delayed_work_sync(&trigger_data->work);
>
> spin_lock_bh(&trigger_data->lock);
> @@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev,
> trigger_data->net_dev =
> dev_get_by_name(&init_net, trigger_data->device_name);
>
> + if (!validate_baseline_state(trigger_data)) {
> + /* Restore old net_dev and device_name */
> + if (trigger_data->net_dev)
> + dev_put(trigger_data->net_dev);
> +
> + dev_hold(old_net);
> + trigger_data->net_dev = old_net;
> + memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ);
> +
> + spin_unlock_bh(&trigger_data->lock);
> + return -EINVAL;
I'm not sure this is the best way... putting the net_dev but holding a
reference, to leter regain the reference via dev_hold() just feels
wrong. Also, I wonder what happens if two threads try to change the
netdev together - will the read of the old device name be potentially
corrupted (since we're not holding the trigger's lock?)
Maybe instead:
+ struct net_device *old_net;
...
- if (trigger_data->net_dev) {
- dev_put(trigger_data->net_dev);
- trigger_data->net_dev = NULL;
- }
+ old_net = trigger_data->net_dev;
+ trigger_data->net_dev = NULL;
+ memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
...
... extract out the setup of trigger_data->device_name
...
+ if (!validate_baseline_state(trigger_data)) {
+ if (trigger_data->net_dev)
+ dev_put(trigger_data->net_dev);
+
+ /* Restore device settings */
+ trigger_data->net_dev = old_dev;
+ memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ);
+ spin_unlock_bh(&trigger_data->lock);
+ return -EINVAL;
+ } else {
+ dev_put(old_net);
+ }
would be safer all round?
One thought on this approach though - if one has a PHY that supports
"activity" but not independent "rx" and "tx" activity indications
and it doesn't support software control, how would one enable activity
mode? There isn't a way to simultaneously enable both at the same
time... However, I need to check whether there are any PHYs that fall
into this category.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Dec 15, 2022 at 04:27:17PM +0100, Alexander Stein wrote:
> Hi,
>
> thanks for the v7 series.
>
> Am Donnerstag, 15. Dezember 2022, 00:54:33 CET schrieb Christian Marangi:
> > Add hardware control support for the Netdev trigger.
> > The trigger on config change will check if the requested trigger can set
> > to blink mode using LED hardware mode and if every blink mode is supported,
> > the trigger will enable hardware mode with the requested configuration.
> > If there is at least one trigger that is not supported and can't run in
> > hardware mode, then software mode will be used instead.
> > A validation is done on every value change and on fail the old value is
> > restored and -EINVAL is returned.
> >
> > Signed-off-by: Christian Marangi <[email protected]>
> > ---
> > drivers/leds/trigger/ledtrig-netdev.c | 155 +++++++++++++++++++++++++-
> > 1 file changed, 149 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/leds/trigger/ledtrig-netdev.c
> > b/drivers/leds/trigger/ledtrig-netdev.c index dd63cadb896e..ed019cb5867c
> > 100644
> > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > @@ -37,6 +37,7 @@
> > */
> >
> > struct led_netdev_data {
> > + enum led_blink_modes blink_mode;
> > spinlock_t lock;
> >
> > struct delayed_work work;
> > @@ -53,11 +54,105 @@ struct led_netdev_data {
> > bool carrier_link_up;
> > };
> >
> > +struct netdev_led_attr_detail {
> > + char *name;
> > + bool hardware_only;
> > + enum led_trigger_netdev_modes bit;
> > +};
> > +
> > +static struct netdev_led_attr_detail attr_details[] = {
> > + { .name = "link", .bit = TRIGGER_NETDEV_LINK},
> > + { .name = "tx", .bit = TRIGGER_NETDEV_TX},
> > + { .name = "rx", .bit = TRIGGER_NETDEV_RX},
> > +};
> > +
> > +static bool validate_baseline_state(struct led_netdev_data *trigger_data)
> > +{
> > + struct led_classdev *led_cdev = trigger_data->led_cdev;
> > + struct netdev_led_attr_detail *detail;
> > + u32 hw_blink_mode_supported = 0;
> > + bool force_sw = false;
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> > + detail = &attr_details[i];
> > +
> > + /* Mode not active, skip */
> > + if (!test_bit(detail->bit, &trigger_data->mode))
> > + continue;
> > +
> > + /* Hardware only mode enabled on software controlled led
> */
> > + if (led_cdev->blink_mode == SOFTWARE_CONTROLLED &&
> > + detail->hardware_only)
> > + return false;
> > +
> > + /* Check if the mode supports hardware mode */
> > + if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> > + /* With a net dev set, force software mode.
> > + * With modes are handled by hardware, led will
> blink
> > + * based on his own events and will ignore any
> event
> > + * from the provided dev.
> > + */
> > + if (trigger_data->net_dev) {
> > + force_sw = true;
> > + continue;
> > + }
> > +
> > + /* With empty dev, check if the mode is
> supported */
> > + if
> (led_trigger_blink_mode_is_supported(led_cdev, detail->bit))
> > + hw_blink_mode_supported |= BIT(detail-
> >bit);
>
> Shouldn't this be BIT(detail->bit)?
>
I think I didn't understand?
> > + }
> > + }
> > +
> > + /* We can't run modes handled by both software and hardware.
> > + * Check if we run hardware modes and check if all the modes
> > + * can be handled by hardware.
> > + */
> > + if (hw_blink_mode_supported && hw_blink_mode_supported !=
> > trigger_data->mode) + return false;
> > +
> > + /* Modes are valid. Decide now the running mode to later
> > + * set the baseline.
> > + * Software mode is enforced with net_dev set. With an empty
> > + * one hardware mode is selected by default (if supported).
> > + */
> > + if (force_sw || led_cdev->blink_mode == SOFTWARE_CONTROLLED)
>
> IMHO '|| !hw_blink_mode_supported' should be added here for blink_modes. This
> might happen if a PHY LED is SOFTWARE_HARDWARE_CONTROLLED, but some blink mode
> is not supported by hardware, thus hw_blink_mode_supported=0.
>
Will check this and report back.
> Best regards,
> Alexander
>
> > + trigger_data->blink_mode = SOFTWARE_CONTROLLED;
> > + else
> > + trigger_data->blink_mode = HARDWARE_CONTROLLED;
> > +
> > + return true;
> > +}
> > +
> > static void set_baseline_state(struct led_netdev_data *trigger_data)
> > {
> > + int i;
> > int current_brightness;
> > + struct netdev_led_attr_detail *detail;
> > struct led_classdev *led_cdev = trigger_data->led_cdev;
> >
> > + /* Modes already validated. Directly apply hw trigger modes */
> > + if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
> > + /* We are refreshing the blink modes. Reset them */
> > + led_cdev->hw_control_configure(led_cdev,
> BIT(TRIGGER_NETDEV_LINK),
> > + BLINK_MODE_ZERO);
> > +
> > + for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> > + detail = &attr_details[i];
> > +
> > + if (!test_bit(detail->bit, &trigger_data->mode))
> > + continue;
> > +
> > + led_cdev->hw_control_configure(led_cdev,
> BIT(detail->bit),
> > +
> BLINK_MODE_ENABLE);
>
> Shouldn't this be BIT(detail->bit)?
>
> > + }
> > +
> > + led_cdev->hw_control_start(led_cdev);
> > +
> > + return;
> > + }
> > +
> > + /* Handle trigger modes by software */
> > current_brightness = led_cdev->brightness;
> > if (current_brightness)
> > led_cdev->blink_brightness = current_brightness;
> > @@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev,
> > size_t size)
> > {
> > struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> > + struct net_device *old_net = trigger_data->net_dev;
> > + char old_device_name[IFNAMSIZ];
> >
> > if (size >= IFNAMSIZ)
> > return -EINVAL;
> >
> > + /* Backup old device name */
> > + memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> > +
> > cancel_delayed_work_sync(&trigger_data->work);
> >
> > spin_lock_bh(&trigger_data->lock);
> > @@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev,
> > trigger_data->net_dev =
> > dev_get_by_name(&init_net, trigger_data->device_name);
> >
> > + if (!validate_baseline_state(trigger_data)) {
> > + /* Restore old net_dev and device_name */
> > + if (trigger_data->net_dev)
> > + dev_put(trigger_data->net_dev);
> > +
> > + dev_hold(old_net);
> > + trigger_data->net_dev = old_net;
> > + memcpy(trigger_data->device_name, old_device_name,
> IFNAMSIZ);
> > +
> > + spin_unlock_bh(&trigger_data->lock);
> > + return -EINVAL;
> > + }
> > +
> > trigger_data->carrier_link_up = false;
> > if (trigger_data->net_dev != NULL)
> > trigger_data->carrier_link_up =
> netif_carrier_ok(trigger_data->net_dev);
> > @@ -159,7 +272,7 @@ static ssize_t netdev_led_attr_store(struct device *dev,
> > const char *buf, size_t size, enum led_trigger_netdev_modes attr)
> > {
> > struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> > - unsigned long state;
> > + unsigned long state, old_mode = trigger_data->mode;
> > int ret;
> > int bit;
> >
> > @@ -184,6 +297,12 @@ static ssize_t netdev_led_attr_store(struct device
> > *dev, const char *buf, else
> > clear_bit(bit, &trigger_data->mode);
> >
> > + if (!validate_baseline_state(trigger_data)) {
> > + /* Restore old mode on validation fail */
> > + trigger_data->mode = old_mode;
> > + return -EINVAL;
> > + }
> > +
> > set_baseline_state(trigger_data);
> >
> > return size;
> > @@ -220,6 +339,8 @@ static ssize_t interval_store(struct device *dev,
> > size_t size)
> > {
> > struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> > + int old_interval = atomic_read(&trigger_data->interval);
> > + u32 old_mode = trigger_data->mode;
> > unsigned long value;
> > int ret;
> >
> > @@ -228,13 +349,22 @@ static ssize_t interval_store(struct device *dev,
> > return ret;
> >
> > /* impose some basic bounds on the timer interval */
> > - if (value >= 5 && value <= 10000) {
> > - cancel_delayed_work_sync(&trigger_data->work);
> > + if (value < 5 || value > 10000)
> > + return -EINVAL;
> > +
> > + cancel_delayed_work_sync(&trigger_data->work);
> > +
> > + atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
> >
> > - atomic_set(&trigger_data->interval,
> msecs_to_jiffies(value));
> > - set_baseline_state(trigger_data); /* resets timer
> */
> > + if (!validate_baseline_state(trigger_data)) {
> > + /* Restore old interval on validation error */
> > + atomic_set(&trigger_data->interval, old_interval);
> > + trigger_data->mode = old_mode;
> > + return -EINVAL;
> > }
> >
> > + set_baseline_state(trigger_data); /* resets timer */
> > +
> > return size;
> > }
> >
> > @@ -368,13 +498,25 @@ static int netdev_trig_activate(struct led_classdev
> > *led_cdev) trigger_data->mode = 0;
> > atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
> > trigger_data->last_activity = 0;
> > + if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> > + /* With hw mode enabled reset any rule set by default */
> > + if (led_cdev->hw_control_status(led_cdev)) {
> > + rc = led_cdev->hw_control_configure(led_cdev,
> BIT(TRIGGER_NETDEV_LINK),
> > +
> BLINK_MODE_ZERO);
> > + if (rc)
> > + goto err;
> > + }
> > + }
> >
> > led_set_trigger_data(led_cdev, trigger_data);
> >
> > rc = register_netdevice_notifier(&trigger_data->notifier);
> > if (rc)
> > - kfree(trigger_data);
> > + goto err;
> >
> > + return 0;
> > +err:
> > + kfree(trigger_data);
> > return rc;
> > }
> >
> > @@ -394,6 +536,7 @@ static void netdev_trig_deactivate(struct led_classdev
> > *led_cdev)
> >
> > static struct led_trigger netdev_led_trigger = {
> > .name = "netdev",
> > + .supported_blink_modes = SOFTWARE_HARDWARE,
> > .activate = netdev_trig_activate,
> > .deactivate = netdev_trig_deactivate,
> > .groups = netdev_trig_groups,
>
>
>
>
--
Ansuel
On Thu, Dec 15, 2022 at 05:07:31PM +0000, Russell King (Oracle) wrote:
> On Thu, Dec 15, 2022 at 12:54:33AM +0100, Christian Marangi wrote:
> > Add hardware control support for the Netdev trigger.
> > The trigger on config change will check if the requested trigger can set
> > to blink mode using LED hardware mode and if every blink mode is supported,
> > the trigger will enable hardware mode with the requested configuration.
> > If there is at least one trigger that is not supported and can't run in
> > hardware mode, then software mode will be used instead.
> > A validation is done on every value change and on fail the old value is
> > restored and -EINVAL is returned.
> >
> > Signed-off-by: Christian Marangi <[email protected]>
> > ---
> > drivers/leds/trigger/ledtrig-netdev.c | 155 +++++++++++++++++++++++++-
> > 1 file changed, 149 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
> > index dd63cadb896e..ed019cb5867c 100644
> > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > @@ -37,6 +37,7 @@
> > */
> >
> > struct led_netdev_data {
> > + enum led_blink_modes blink_mode;
> > spinlock_t lock;
> >
> > struct delayed_work work;
> > @@ -53,11 +54,105 @@ struct led_netdev_data {
> > bool carrier_link_up;
> > };
> >
> > +struct netdev_led_attr_detail {
> > + char *name;
> > + bool hardware_only;
> > + enum led_trigger_netdev_modes bit;
> > +};
> > +
> > +static struct netdev_led_attr_detail attr_details[] = {
> > + { .name = "link", .bit = TRIGGER_NETDEV_LINK},
> > + { .name = "tx", .bit = TRIGGER_NETDEV_TX},
> > + { .name = "rx", .bit = TRIGGER_NETDEV_RX},
> > +};
> > +
> > +static bool validate_baseline_state(struct led_netdev_data *trigger_data)
> > +{
> > + struct led_classdev *led_cdev = trigger_data->led_cdev;
> > + struct netdev_led_attr_detail *detail;
> > + u32 hw_blink_mode_supported = 0;
> > + bool force_sw = false;
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> > + detail = &attr_details[i];
> > +
> > + /* Mode not active, skip */
> > + if (!test_bit(detail->bit, &trigger_data->mode))
> > + continue;
> > +
> > + /* Hardware only mode enabled on software controlled led */
> > + if (led_cdev->blink_mode == SOFTWARE_CONTROLLED &&
> > + detail->hardware_only)
> > + return false;
> > +
> > + /* Check if the mode supports hardware mode */
> > + if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> > + /* With a net dev set, force software mode.
> > + * With modes are handled by hardware, led will blink
> > + * based on his own events and will ignore any event
> > + * from the provided dev.
> > + */
> > + if (trigger_data->net_dev) {
> > + force_sw = true;
> > + continue;
> > + }
> > +
> > + /* With empty dev, check if the mode is supported */
> > + if (led_trigger_blink_mode_is_supported(led_cdev, detail->bit))
> > + hw_blink_mode_supported |= BIT(detail->bit);
> > + }
> > + }
> > +
> > + /* We can't run modes handled by both software and hardware.
> > + * Check if we run hardware modes and check if all the modes
> > + * can be handled by hardware.
> > + */
> > + if (hw_blink_mode_supported && hw_blink_mode_supported != trigger_data->mode)
> > + return false;
> > +
> > + /* Modes are valid. Decide now the running mode to later
> > + * set the baseline.
> > + * Software mode is enforced with net_dev set. With an empty
> > + * one hardware mode is selected by default (if supported).
> > + */
> > + if (force_sw || led_cdev->blink_mode == SOFTWARE_CONTROLLED)
> > + trigger_data->blink_mode = SOFTWARE_CONTROLLED;
> > + else
> > + trigger_data->blink_mode = HARDWARE_CONTROLLED;
> > +
> > + return true;
> > +}
> > +
> > static void set_baseline_state(struct led_netdev_data *trigger_data)
> > {
> > + int i;
> > int current_brightness;
> > + struct netdev_led_attr_detail *detail;
> > struct led_classdev *led_cdev = trigger_data->led_cdev;
> >
> > + /* Modes already validated. Directly apply hw trigger modes */
> > + if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
> > + /* We are refreshing the blink modes. Reset them */
> > + led_cdev->hw_control_configure(led_cdev, BIT(TRIGGER_NETDEV_LINK),
> > + BLINK_MODE_ZERO);
> > +
> > + for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> > + detail = &attr_details[i];
> > +
> > + if (!test_bit(detail->bit, &trigger_data->mode))
> > + continue;
> > +
> > + led_cdev->hw_control_configure(led_cdev, BIT(detail->bit),
> > + BLINK_MODE_ENABLE);
> > + }
> > +
> > + led_cdev->hw_control_start(led_cdev);
> > +
> > + return;
> > + }
> > +
> > + /* Handle trigger modes by software */
> > current_brightness = led_cdev->brightness;
> > if (current_brightness)
> > led_cdev->blink_brightness = current_brightness;
> > @@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device *dev,
> > size_t size)
> > {
> > struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> > + struct net_device *old_net = trigger_data->net_dev;
> > + char old_device_name[IFNAMSIZ];
> >
> > if (size >= IFNAMSIZ)
> > return -EINVAL;
> >
> > + /* Backup old device name */
> > + memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> > +
> > cancel_delayed_work_sync(&trigger_data->work);
> >
> > spin_lock_bh(&trigger_data->lock);
> > @@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device *dev,
> > trigger_data->net_dev =
> > dev_get_by_name(&init_net, trigger_data->device_name);
> >
> > + if (!validate_baseline_state(trigger_data)) {
> > + /* Restore old net_dev and device_name */
> > + if (trigger_data->net_dev)
> > + dev_put(trigger_data->net_dev);
> > +
> > + dev_hold(old_net);
> > + trigger_data->net_dev = old_net;
> > + memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ);
> > +
> > + spin_unlock_bh(&trigger_data->lock);
> > + return -EINVAL;
>
> I'm not sure this is the best way... putting the net_dev but holding a
> reference, to leter regain the reference via dev_hold() just feels
> wrong. Also, I wonder what happens if two threads try to change the
> netdev together - will the read of the old device name be potentially
> corrupted (since we're not holding the trigger's lock?)
>
> Maybe instead:
>
> + struct net_device *old_net;
> ...
> - if (trigger_data->net_dev) {
> - dev_put(trigger_data->net_dev);
> - trigger_data->net_dev = NULL;
> - }
> + old_net = trigger_data->net_dev;
> + trigger_data->net_dev = NULL;
> + memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> ...
> ... extract out the setup of trigger_data->device_name
> ...
> + if (!validate_baseline_state(trigger_data)) {
> + if (trigger_data->net_dev)
> + dev_put(trigger_data->net_dev);
> +
> + /* Restore device settings */
> + trigger_data->net_dev = old_dev;
> + memcpy(trigger_data->device_name, old_device_name, IFNAMSIZ);
> + spin_unlock_bh(&trigger_data->lock);
> + return -EINVAL;
> + } else {
> + dev_put(old_net);
> + }
>
> would be safer all round?
Need to check but if I'm not wrong all this thing was to handle the very
corner case where net can be removed while we are changing trigger and
something goes wrong down the line... Holding that means it won't get
actually removed till everything is ok.
>
> One thought on this approach though - if one has a PHY that supports
> "activity" but not independent "rx" and "tx" activity indications
> and it doesn't support software control, how would one enable activity
> mode? There isn't a way to simultaneously enable both at the same
> time... However, I need to check whether there are any PHYs that fall
> into this category.
>
Problem is that for such feature and to have at least something working
we need to face compromise. We really can't support each switch feature
and have a generic API for everything. My original idea was to have
something VERY dynamic with a totally dedicated and dumb trigger... But
that was NACK as netdev was the correct way to handle these stuff...
But adapting everything to netdev trigger is hard since you have just
another generic abstraction layer. My idea at times was that in such
case the trigger rule will be rejected and only enabled if both tx and
rx were enabled. An alternative is to add another flag for activity
rule. (for switch supporting independent tx and rx with activity rule
enable both tx and rx event are enabled. for switch not supporting
independent tx and rx just fallback to sw and say that the mode is not
suported.)
I already had the idea of Documenting all this case but if we decide to
follow this approach then creating a schema file is a must at this
point. (but wanted to introduce that later if and ever this feature will
be accepted to permit to set trigger rules directly in DT following
something like linux,default-trigger.
--
Ansuel
> > One thought on this approach though - if one has a PHY that supports
> > "activity" but not independent "rx" and "tx" activity indications
> > and it doesn't support software control, how would one enable activity
> > mode? There isn't a way to simultaneously enable both at the same
> > time... However, I need to check whether there are any PHYs that fall
> > into this category.
> >
>
> Problem is that for such feature and to have at least something working
> we need to face compromise. We really can't support each switch feature
> and have a generic API for everything.
I agree we need to make compromises. We cannot support every LED
feature of every PHY, they are simply too diverse. Hopefully we can
support some features of every PHY. In the worst case, a PHY simply
cannot be controlled via this method, which is the current state
today. So it is not worse off.
Andrew
On Wed, Dec 21, 2022 at 12:59:55AM +0100, Andrew Lunn wrote:
> > > One thought on this approach though - if one has a PHY that supports
> > > "activity" but not independent "rx" and "tx" activity indications
> > > and it doesn't support software control, how would one enable activity
> > > mode? There isn't a way to simultaneously enable both at the same
> > > time... However, I need to check whether there are any PHYs that fall
> > > into this category.
> > >
> >
> > Problem is that for such feature and to have at least something working
> > we need to face compromise. We really can't support each switch feature
> > and have a generic API for everything.
>
> I agree we need to make compromises. We cannot support every LED
> feature of every PHY, they are simply too diverse. Hopefully we can
> support some features of every PHY. In the worst case, a PHY simply
> cannot be controlled via this method, which is the current state
> today. So it is not worse off.
... and that compromise is that it's not going to be possible to enable
activity mode on 88e151x with how the code stands and with the
independent nature of "rx" and "tx" activity control currently in the
netdev trigger... making this whole approach somewhat useless for
Marvell PHYs.
We really need to see a working implementation for this code for more
than just one PHY to prove that it is actually possible for it to
support other PHYs. If not, it isn't actually solving the problem,
and we're going to continue getting custom implementations to configure
the LED settings.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Wed, Dec 21, 2022 at 09:54:43AM +0000, Russell King (Oracle) wrote:
> On Wed, Dec 21, 2022 at 12:59:55AM +0100, Andrew Lunn wrote:
> > > > One thought on this approach though - if one has a PHY that supports
> > > > "activity" but not independent "rx" and "tx" activity indications
> > > > and it doesn't support software control, how would one enable activity
> > > > mode? There isn't a way to simultaneously enable both at the same
> > > > time... However, I need to check whether there are any PHYs that fall
> > > > into this category.
> > > >
> > >
> > > Problem is that for such feature and to have at least something working
> > > we need to face compromise. We really can't support each switch feature
> > > and have a generic API for everything.
> >
> > I agree we need to make compromises. We cannot support every LED
> > feature of every PHY, they are simply too diverse. Hopefully we can
> > support some features of every PHY. In the worst case, a PHY simply
> > cannot be controlled via this method, which is the current state
> > today. So it is not worse off.
>
> ... and that compromise is that it's not going to be possible to enable
> activity mode on 88e151x with how the code stands and with the
> independent nature of "rx" and "tx" activity control currently in the
> netdev trigger... making this whole approach somewhat useless for
> Marvell PHYs.
Again we can consider adding an activity mode. It seems logical that
some switch may only support global traffic instead of independend tx or
rx... The feature are not mutually exclusive. One include the other 2.
We already a simple workaround for the link mode where on the current
driver, if the link mode is enabled just all rule for 10 100 and 1000
mbps are enabled simulating a global link event.
>
> We really need to see a working implementation for this code for more
> than just one PHY to prove that it is actually possible for it to
> support other PHYs. If not, it isn't actually solving the problem,
> and we're going to continue getting custom implementations to configure
> the LED settings.
>
Agree that we need other user for this to catch some problem in the
implementation of this generic API.
--
Ansuel
> > > I agree we need to make compromises. We cannot support every LED
> > > feature of every PHY, they are simply too diverse. Hopefully we can
> > > support some features of every PHY. In the worst case, a PHY simply
> > > cannot be controlled via this method, which is the current state
> > > today. So it is not worse off.
> >
> > ... and that compromise is that it's not going to be possible to enable
> > activity mode on 88e151x with how the code stands and with the
> > independent nature of "rx" and "tx" activity control currently in the
> > netdev trigger... making this whole approach somewhat useless for
> > Marvell PHYs.
>
> Again we can consider adding an activity mode. It seems logical that
> some switch may only support global traffic instead of independend tx or
> rx... The feature are not mutually exclusive. One include the other 2.
Looking at the software trigger, adding NETDEV_LED_RXTX looks simple
to do. I also suspect it will be used by more than Marvell.
> > We really need to see a working implementation for this code for more
> > than just one PHY to prove that it is actually possible for it to
> > support other PHYs. If not, it isn't actually solving the problem,
> > and we're going to continue getting custom implementations to configure
> > the LED settings.
> >
>
> Agree that we need other user for this to catch some problem in the
> implementation of this generic API.
We need a PHY driver implementation. The phylib core needs to be
involved, the cled code needs to call generic phylib functions which
take the phydev->lock before calling into the PHY driver. Probably the
phylib core can do all the memory allocation, and registration of the
LED to the LED core. If it is not too ugly, i would also do the DT
binding parsing in the core, so we don't end up with subtle
differences.
Andrew
Am Freitag, 16. Dezember 2022, 18:00:45 CET schrieb Christian Marangi:
> On Thu, Dec 15, 2022 at 04:27:17PM +0100, Alexander Stein wrote:
> > Hi,
> >
> > thanks for the v7 series.
> >
> > Am Donnerstag, 15. Dezember 2022, 00:54:33 CET schrieb Christian Marangi:
> > > Add hardware control support for the Netdev trigger.
> > > The trigger on config change will check if the requested trigger can set
> > > to blink mode using LED hardware mode and if every blink mode is
> > > supported,
> > > the trigger will enable hardware mode with the requested configuration.
> > > If there is at least one trigger that is not supported and can't run in
> > > hardware mode, then software mode will be used instead.
> > > A validation is done on every value change and on fail the old value is
> > > restored and -EINVAL is returned.
> > >
> > > Signed-off-by: Christian Marangi <[email protected]>
> > > ---
> > >
> > > drivers/leds/trigger/ledtrig-netdev.c | 155 +++++++++++++++++++++++++-
> > > 1 file changed, 149 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/leds/trigger/ledtrig-netdev.c
> > > b/drivers/leds/trigger/ledtrig-netdev.c index dd63cadb896e..ed019cb5867c
> > > 100644
> > > --- a/drivers/leds/trigger/ledtrig-netdev.c
> > > +++ b/drivers/leds/trigger/ledtrig-netdev.c
> > > @@ -37,6 +37,7 @@
> > >
> > > */
> > >
> > > struct led_netdev_data {
> > >
> > > + enum led_blink_modes blink_mode;
> > >
> > > spinlock_t lock;
> > >
> > > struct delayed_work work;
> > >
> > > @@ -53,11 +54,105 @@ struct led_netdev_data {
> > >
> > > bool carrier_link_up;
> > >
> > > };
> > >
> > > +struct netdev_led_attr_detail {
> > > + char *name;
> > > + bool hardware_only;
> > > + enum led_trigger_netdev_modes bit;
> > > +};
> > > +
> > > +static struct netdev_led_attr_detail attr_details[] = {
> > > + { .name = "link", .bit = TRIGGER_NETDEV_LINK},
> > > + { .name = "tx", .bit = TRIGGER_NETDEV_TX},
> > > + { .name = "rx", .bit = TRIGGER_NETDEV_RX},
> > > +};
> > > +
> > > +static bool validate_baseline_state(struct led_netdev_data
> > > *trigger_data)
> > > +{
> > > + struct led_classdev *led_cdev = trigger_data->led_cdev;
> > > + struct netdev_led_attr_detail *detail;
> > > + u32 hw_blink_mode_supported = 0;
> > > + bool force_sw = false;
> > > + int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> > > + detail = &attr_details[i];
> > > +
> > > + /* Mode not active, skip */
> > > + if (!test_bit(detail->bit, &trigger_data->mode))
> > > + continue;
> > > +
> > > + /* Hardware only mode enabled on software controlled led
> >
> > */
> >
> > > + if (led_cdev->blink_mode == SOFTWARE_CONTROLLED &&
> > > + detail->hardware_only)
> > > + return false;
> > > +
> > > + /* Check if the mode supports hardware mode */
> > > + if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> > > + /* With a net dev set, force software mode.
> > > + * With modes are handled by hardware, led will
> >
> > blink
> >
> > > + * based on his own events and will ignore any
> >
> > event
> >
> > > + * from the provided dev.
> > > + */
> > > + if (trigger_data->net_dev) {
> > > + force_sw = true;
> > > + continue;
> > > + }
> > > +
> > > + /* With empty dev, check if the mode is
> >
> > supported */
> >
> > > + if
> >
> > (led_trigger_blink_mode_is_supported(led_cdev, detail->bit))
> >
> > > + hw_blink_mode_supported |= BIT(detail-
> > >
> > >bit);
> >
> > Shouldn't this be BIT(detail->bit)?
>
> I think I didn't understand?
The name 'bit' indicates this is a single bit number rather than a bitmask.
AFAICS the value (detail->bit) passed to led_trigger_blink_mode_is_supported
is eventually used within test_bit inside dp83867_parse_netdev. I assume you
have to actually pass the bitmask with this single bit set, not the bit number
itself.
Best regards,
Alexander
> > > + }
> > > + }
> > > +
> > > + /* We can't run modes handled by both software and hardware.
> > > + * Check if we run hardware modes and check if all the modes
> > > + * can be handled by hardware.
> > > + */
> > > + if (hw_blink_mode_supported && hw_blink_mode_supported !=
> > > trigger_data->mode) + return false;
> > > +
> > > + /* Modes are valid. Decide now the running mode to later
> > > + * set the baseline.
> > > + * Software mode is enforced with net_dev set. With an empty
> > > + * one hardware mode is selected by default (if supported).
> > > + */
> > > + if (force_sw || led_cdev->blink_mode == SOFTWARE_CONTROLLED)
> >
> > IMHO '|| !hw_blink_mode_supported' should be added here for blink_modes.
> > This might happen if a PHY LED is SOFTWARE_HARDWARE_CONTROLLED, but some
> > blink mode is not supported by hardware, thus hw_blink_mode_supported=0.
>
> Will check this and report back.
>
> > Best regards,
> > Alexander
> >
> > > + trigger_data->blink_mode = SOFTWARE_CONTROLLED;
> > > + else
> > > + trigger_data->blink_mode = HARDWARE_CONTROLLED;
> > > +
> > > + return true;
> > > +}
> > > +
> > >
> > > static void set_baseline_state(struct led_netdev_data *trigger_data)
> > > {
> > >
> > > + int i;
> > >
> > > int current_brightness;
> > >
> > > + struct netdev_led_attr_detail *detail;
> > >
> > > struct led_classdev *led_cdev = trigger_data->led_cdev;
> > >
> > > + /* Modes already validated. Directly apply hw trigger modes */
> > > + if (trigger_data->blink_mode == HARDWARE_CONTROLLED) {
> > > + /* We are refreshing the blink modes. Reset them */
> > > + led_cdev->hw_control_configure(led_cdev,
> >
> > BIT(TRIGGER_NETDEV_LINK),
> >
> > > + BLINK_MODE_ZERO);
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(attr_details); i++) {
> > > + detail = &attr_details[i];
> > > +
> > > + if (!test_bit(detail->bit, &trigger_data->mode))
> > > + continue;
> > > +
> > > + led_cdev->hw_control_configure(led_cdev,
> >
> > BIT(detail->bit),
> >
> > > +
> >
> > BLINK_MODE_ENABLE);
> >
> > Shouldn't this be BIT(detail->bit)?
> >
> > > + }
> > > +
> > > + led_cdev->hw_control_start(led_cdev);
> > > +
> > > + return;
> > > + }
> > > +
> > > + /* Handle trigger modes by software */
> > >
> > > current_brightness = led_cdev->brightness;
> > > if (current_brightness)
> > >
> > > led_cdev->blink_brightness = current_brightness;
> > >
> > > @@ -100,10 +195,15 @@ static ssize_t device_name_store(struct device
> > > *dev,
> > >
> > > size_t size)
> > >
> > > {
> > >
> > > struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> > >
> > > + struct net_device *old_net = trigger_data->net_dev;
> > > + char old_device_name[IFNAMSIZ];
> > >
> > > if (size >= IFNAMSIZ)
> > >
> > > return -EINVAL;
> > >
> > > + /* Backup old device name */
> > > + memcpy(old_device_name, trigger_data->device_name, IFNAMSIZ);
> > > +
> > >
> > > cancel_delayed_work_sync(&trigger_data->work);
> > >
> > > spin_lock_bh(&trigger_data->lock);
> > >
> > > @@ -122,6 +222,19 @@ static ssize_t device_name_store(struct device
> > > *dev,
> > >
> > > trigger_data->net_dev =
> > >
> > > dev_get_by_name(&init_net, trigger_data->device_name);
> > >
> > > + if (!validate_baseline_state(trigger_data)) {
> > > + /* Restore old net_dev and device_name */
> > > + if (trigger_data->net_dev)
> > > + dev_put(trigger_data->net_dev);
> > > +
> > > + dev_hold(old_net);
> > > + trigger_data->net_dev = old_net;
> > > + memcpy(trigger_data->device_name, old_device_name,
> >
> > IFNAMSIZ);
> >
> > > +
> > > + spin_unlock_bh(&trigger_data->lock);
> > > + return -EINVAL;
> > > + }
> > > +
> > >
> > > trigger_data->carrier_link_up = false;
> > > if (trigger_data->net_dev != NULL)
> > >
> > > trigger_data->carrier_link_up =
> >
> > netif_carrier_ok(trigger_data->net_dev);
> >
> > > @@ -159,7 +272,7 @@ static ssize_t netdev_led_attr_store(struct device
> > > *dev, const char *buf, size_t size, enum led_trigger_netdev_modes attr)
> > >
> > > {
> > >
> > > struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> > >
> > > - unsigned long state;
> > > + unsigned long state, old_mode = trigger_data->mode;
> > >
> > > int ret;
> > > int bit;
> > >
> > > @@ -184,6 +297,12 @@ static ssize_t netdev_led_attr_store(struct device
> > > *dev, const char *buf, else
> > >
> > > clear_bit(bit, &trigger_data->mode);
> > >
> > > + if (!validate_baseline_state(trigger_data)) {
> > > + /* Restore old mode on validation fail */
> > > + trigger_data->mode = old_mode;
> > > + return -EINVAL;
> > > + }
> > > +
> > >
> > > set_baseline_state(trigger_data);
> > >
> > > return size;
> > >
> > > @@ -220,6 +339,8 @@ static ssize_t interval_store(struct device *dev,
> > >
> > > size_t size)
> > >
> > > {
> > >
> > > struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
> > >
> > > + int old_interval = atomic_read(&trigger_data->interval);
> > > + u32 old_mode = trigger_data->mode;
> > >
> > > unsigned long value;
> > > int ret;
> > >
> > > @@ -228,13 +349,22 @@ static ssize_t interval_store(struct device *dev,
> > >
> > > return ret;
> > >
> > > /* impose some basic bounds on the timer interval */
> > >
> > > - if (value >= 5 && value <= 10000) {
> > > - cancel_delayed_work_sync(&trigger_data->work);
> > > + if (value < 5 || value > 10000)
> > > + return -EINVAL;
> > > +
> > > + cancel_delayed_work_sync(&trigger_data->work);
> > > +
> > > + atomic_set(&trigger_data->interval, msecs_to_jiffies(value));
> > >
> > > - atomic_set(&trigger_data->interval,
> >
> > msecs_to_jiffies(value));
> >
> > > - set_baseline_state(trigger_data); /* resets timer
> >
> > */
> >
> > > + if (!validate_baseline_state(trigger_data)) {
> > > + /* Restore old interval on validation error */
> > > + atomic_set(&trigger_data->interval, old_interval);
> > > + trigger_data->mode = old_mode;
> > > + return -EINVAL;
> > >
> > > }
> > >
> > > + set_baseline_state(trigger_data); /* resets timer */
> > > +
> > >
> > > return size;
> > >
> > > }
> > >
> > > @@ -368,13 +498,25 @@ static int netdev_trig_activate(struct
> > > led_classdev
> > > *led_cdev) trigger_data->mode = 0;
> > >
> > > atomic_set(&trigger_data->interval, msecs_to_jiffies(50));
> > > trigger_data->last_activity = 0;
> > >
> > > + if (led_cdev->blink_mode != SOFTWARE_CONTROLLED) {
> > > + /* With hw mode enabled reset any rule set by default */
> > > + if (led_cdev->hw_control_status(led_cdev)) {
> > > + rc = led_cdev->hw_control_configure(led_cdev,
> >
> > BIT(TRIGGER_NETDEV_LINK),
> >
> > > +
> >
> > BLINK_MODE_ZERO);
> >
> > > + if (rc)
> > > + goto err;
> > > + }
> > > + }
> > >
> > > led_set_trigger_data(led_cdev, trigger_data);
> > >
> > > rc = register_netdevice_notifier(&trigger_data->notifier);
> > > if (rc)
> > >
> > > - kfree(trigger_data);
> > > + goto err;
> > >
> > > + return 0;
> > > +err:
> > > + kfree(trigger_data);
> > >
> > > return rc;
> > >
> > > }
> > >
> > > @@ -394,6 +536,7 @@ static void netdev_trig_deactivate(struct
> > > led_classdev
> > > *led_cdev)
> > >
> > > static struct led_trigger netdev_led_trigger = {
> > >
> > > .name = "netdev",
> > >
> > > + .supported_blink_modes = SOFTWARE_HARDWARE,
> > >
> > > .activate = netdev_trig_activate,
> > > .deactivate = netdev_trig_deactivate,
> > > .groups = netdev_trig_groups,