[PATCH leds v1 0/5] Add support for offloading netdev trigger to HW

[PATCH leds v1 0/5] Add support for offloading netdev trigger to HW

[Marek Behún]

Hello,

I am sending the first non-RFC version of the series adding support
for offloading LED triggers to HW, with the netdev trigger being the
first user.

I have addressed the issues with the RFC version from October.

This version only implements the offloading API and adds support for
offloading to the netdev trigger. In the RFC I also added one user of
this API (Marvell ethernet PHY driver). I plan to continue the work on
those patches once the offloading API is finally merged.

Also, there is now other possible user for this API: leds-nuc driver
by Mauro.

Changes since RFC:
- split the patch adding HW offloading support to netdev trigger into
  several separate patches (suggested by Pavel):
  1. move trigger data structure to include/linux/ledtrig.h
  2. support HW offloading
  3. change spinlock to mutex
- fixed bug where the .offloaded variable was not set to false when
  offloading was disabled (suggested by Pavel)
- removed the code saving one call to set_baseline_state() on the
  NETDEV_CHANGE event. It is not needed, the trigger_offload() method
  can handle this situation on its own (suggested by Pavel)
- documentation now explicitly says that when offloading is being
  disabled, the function must return 0 (no error) (suggested by Pavel)

Marek Behún (5):
  leds: trigger: netdev: don't explicitly zero kzalloced data
  leds: trigger: add API for HW offloading of triggers
  leds: trigger: netdev: move trigger data structure to global include
    dir
  leds: trigger: netdev: support HW offloading
  leds: trigger: netdev: change spinlock to mutex

 Documentation/leds/leds-class.rst     | 22 +++++++++++++
 drivers/leds/led-triggers.c           |  1 +
 drivers/leds/trigger/ledtrig-netdev.c | 47 ++++++++-------------------
 include/linux/leds.h                  | 29 +++++++++++++++++
 include/linux/ledtrig.h               | 40 +++++++++++++++++++++++
 5 files changed, 105 insertions(+), 34 deletions(-)
 create mode 100644 include/linux/ledtrig.h

-- 
2.26.3

[PATCH leds v1 1/5] leds: trigger: netdev: don't explicitly zero kzalloced data

[Marek Behún]

The trigger_data struct is allocated with kzalloc, so we do not need to
explicitly set members to zero.

Signed-off-by: Marek Behún <kabel@kernel.org>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/leds/trigger/ledtrig-netdev.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index d5e774d83021..4f6b73e3b491 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -406,12 +406,8 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 	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);
 
-- 
2.26.3

[PATCH leds v1 2/5] leds: trigger: add API for HW offloading of triggers

[Marek Behún]

Add method trigger_offload() and member variable `offloaded` to struct
led_classdev. Add helper functions led_trigger_offload() and
led_trigger_offload_stop().

The trigger_offload() method, when implemented by the LED driver, should
be called (via led_trigger_offload() function) from trigger code wanting
to be offloaded at the moment when configuration of the trigger changes.

If the trigger is successfully offloaded, this method returns 0 and the
trigger does not have to blink the LED in software.

If the trigger with given configuration cannot be offloaded, the method
should return -EOPNOTSUPP, in which case the trigger must blink the LED
in SW.

The second argument to trigger_offload() being false means that the
offloading is being disabled. In this case the function must return 0,
errors are not permitted.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 Documentation/leds/leds-class.rst | 22 ++++++++++++++++++++++
 drivers/leds/led-triggers.c       |  1 +
 include/linux/leds.h              | 29 +++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+)

diff --git a/Documentation/leds/leds-class.rst b/Documentation/leds/leds-class.rst
index cd155ead8703..ebda64768e9d 100644
--- a/Documentation/leds/leds-class.rst
+++ b/Documentation/leds/leds-class.rst
@@ -169,6 +169,28 @@ Setting the brightness to zero with brightness_set() callback function
 should completely turn off the LED and cancel the previously programmed
 hardware blinking function, if any.
 
+Hardware offloading of LED triggers
+===================================
+
+Some LEDs can offload SW triggers to hardware (for example a LED connected to
+an ethernet PHY or an ethernet switch can be configured to blink on activity on
+the network, which in software is done by the netdev trigger).
+
+To do such offloading, both the trigger code and LED driver must support this.
+The LED must implement the trigger_offload() method and the trigger code must
+try to call this method (via led_trigger_offload() function) when configuration
+of the trigger (trigger_data) changes.
+
+The implementation of the trigger_offload() method by the LED driver must return
+0 if the offload is successful and -EOPNOTSUPP if the requested trigger
+configuration is not supported and the trigger should be executed in software.
+If trigger_offload() returns negative value, the trigger will be done in
+software, so any active offloading must also be disabled.
+
+If the second argument (enable) to the trigger_offload() method is false, any
+active HW offloading must be deactivated. In this case errors are not permitted
+in the trigger_offload() method.
+
 
 Known Issues
 ============
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 4e7b78a84149..372980791b87 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -177,6 +177,7 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
 			flags);
 		cancel_work_sync(&led_cdev->set_brightness_work);
 		led_stop_software_blink(led_cdev);
+		led_trigger_offload_stop(led_cdev);
 		if (led_cdev->trigger->deactivate)
 			led_cdev->trigger->deactivate(led_cdev);
 		device_remove_groups(led_cdev->dev, led_cdev->trigger->groups);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 329fd914cf24..b331e7bceac3 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -148,6 +148,11 @@ struct led_classdev {
 
 	/* LEDs that have private triggers have this set */
 	struct led_hw_trigger_type	*trigger_type;
+
+	/* some LEDs may be able to offload some SW triggers to HW */
+	int		(*trigger_offload)(struct led_classdev *led_cdev,
+					   bool enable);
+	bool			offloaded;
 #endif
 
 #ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
@@ -403,6 +408,30 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
 	return led_cdev->trigger_data;
 }
 
+static inline int led_trigger_offload(struct led_classdev *led_cdev)
+{
+	int ret;
+
+	if (!led_cdev->trigger_offload)
+		return -EOPNOTSUPP;
+
+	ret = led_cdev->trigger_offload(led_cdev, true);
+	led_cdev->offloaded = !ret;
+
+	return ret;
+}
+
+static inline void led_trigger_offload_stop(struct led_classdev *led_cdev)
+{
+	if (!led_cdev->trigger_offload)
+		return;
+
+	if (led_cdev->offloaded) {
+		led_cdev->trigger_offload(led_cdev, false);
+		led_cdev->offloaded = false;
+	}
+}
+
 /**
  * led_trigger_rename_static - rename a trigger
  * @name: the new trigger name
-- 
2.26.3

[PATCH leds v1 3/5] leds: trigger: netdev: move trigger data structure to global include dir

[Marek Behún]

In preparation for HW offloading of netdev trigger, move struct
led_trigger_data into global include directory, into file
linux/ledtrig.h, so that drivers wanting to offload the trigger can see
the requested settings.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/trigger/ledtrig-netdev.c | 23 +---------------
 include/linux/ledtrig.h               | 38 +++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 22 deletions(-)
 create mode 100644 include/linux/ledtrig.h

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index 4f6b73e3b491..a611ad755036 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -10,17 +10,16 @@
 //  Copyright 2005-2006 Openedhand Ltd.
 //  Author: Richard Purdie <rpurdie@openedhand.com>
 
-#include <linux/atomic.h>
 #include <linux/ctype.h>
 #include <linux/device.h>
 #include <linux/init.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/leds.h>
+#include <linux/ledtrig.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
-#include <linux/spinlock.h>
 #include <linux/timer.h>
 #include "../leds.h"
 
@@ -36,26 +35,6 @@
  *
  */
 
-struct led_netdev_data {
-	spinlock_t lock;
-
-	struct delayed_work work;
-	struct notifier_block notifier;
-
-	struct led_classdev *led_cdev;
-	struct net_device *net_dev;
-
-	char device_name[IFNAMSIZ];
-	atomic_t interval;
-	unsigned int last_activity;
-
-	unsigned long mode;
-#define NETDEV_LED_LINK	0
-#define NETDEV_LED_TX	1
-#define NETDEV_LED_RX	2
-#define NETDEV_LED_MODE_LINKUP	3
-};
-
 enum netdev_led_attr {
 	NETDEV_ATTR_LINK,
 	NETDEV_ATTR_TX,
diff --git a/include/linux/ledtrig.h b/include/linux/ledtrig.h
new file mode 100644
index 000000000000..1cb7f03e6c16
--- /dev/null
+++ b/include/linux/ledtrig.h
@@ -0,0 +1,38 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * LED trigger shared structures
+ */
+
+#ifndef __LINUX_LEDTRIG_H__
+#define __LINUX_LEDTRIG_H__
+
+#include <linux/atomic.h>
+#include <linux/leds.h>
+#include <linux/netdevice.h>
+#include <linux/spinlock.h>
+
+#if IS_ENABLED(CONFIG_LEDS_TRIGGER_NETDEV)
+
+struct led_netdev_data {
+	spinlock_t lock;
+
+	struct delayed_work work;
+	struct notifier_block notifier;
+
+	struct led_classdev *led_cdev;
+	struct net_device *net_dev;
+
+	char device_name[IFNAMSIZ];
+	atomic_t interval;
+	unsigned int last_activity;
+
+	unsigned long mode;
+#define NETDEV_LED_LINK		0
+#define NETDEV_LED_TX		1
+#define NETDEV_LED_RX		2
+#define NETDEV_LED_MODE_LINKUP	3
+};
+
+#endif /* IS_ENABLED(CONFIG_LEDS_TRIGGER_NETDEV) */
+
+#endif /* __LINUX_LEDTRIG_H__ */
-- 
2.26.3

[PATCH leds v1 4/5] leds: trigger: netdev: support HW offloading

[Marek Behún]

Add support for HW offloading of the netdev trigger.

We need to export the netdev_led_trigger variable so that drivers may
check whether the LED is set to this trigger.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/trigger/ledtrig-netdev.c | 6 +++++-
 include/linux/ledtrig.h               | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index a611ad755036..b6d51b24c213 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -52,6 +52,9 @@ static void set_baseline_state(struct led_netdev_data *trigger_data)
 	if (!led_cdev->blink_brightness)
 		led_cdev->blink_brightness = led_cdev->max_brightness;
 
+	if (!led_trigger_offload(led_cdev))
+		return;
+
 	if (!test_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode))
 		led_set_brightness(led_cdev, LED_OFF);
 	else {
@@ -411,12 +414,13 @@ static void netdev_trig_deactivate(struct led_classdev *led_cdev)
 	kfree(trigger_data);
 }
 
-static struct led_trigger netdev_led_trigger = {
+struct led_trigger netdev_led_trigger = {
 	.name = "netdev",
 	.activate = netdev_trig_activate,
 	.deactivate = netdev_trig_deactivate,
 	.groups = netdev_trig_groups,
 };
+EXPORT_SYMBOL_GPL(netdev_led_trigger);
 
 static int __init netdev_trig_init(void)
 {
diff --git a/include/linux/ledtrig.h b/include/linux/ledtrig.h
index 1cb7f03e6c16..a6a813bb154a 100644
--- a/include/linux/ledtrig.h
+++ b/include/linux/ledtrig.h
@@ -33,6 +33,8 @@ struct led_netdev_data {
 #define NETDEV_LED_MODE_LINKUP	3
 };
 
+extern struct led_trigger netdev_led_trigger;
+
 #endif /* IS_ENABLED(CONFIG_LEDS_TRIGGER_NETDEV) */
 
 #endif /* __LINUX_LEDTRIG_H__ */
-- 
2.26.3

[PATCH leds v1 5/5] leds: trigger: netdev: change spinlock to mutex

[Marek Behún]

Using spinlocks requires that the trigger_offload() method cannot sleep.
This can be problematic for some hardware, since access to registers may
sleep.

We can change the spinlock to mutex because, according to Jacek:
  register_netdevice_notifier() registers raw notifier chain,
  whose callbacks are not called from atomic context and there are
  no restrictions on callbacks. See include/linux/notifier.h.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 drivers/leds/trigger/ledtrig-netdev.c | 14 +++++++-------
 include/linux/ledtrig.h               |  4 ++--
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index b6d51b24c213..7073cc6b8ca2 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -79,9 +79,9 @@ static ssize_t device_name_show(struct device *dev,
 	struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
 	ssize_t len;
 
-	spin_lock_bh(&trigger_data->lock);
+	mutex_lock(&trigger_data->lock);
 	len = sprintf(buf, "%s\n", trigger_data->device_name);
-	spin_unlock_bh(&trigger_data->lock);
+	mutex_unlock(&trigger_data->lock);
 
 	return len;
 }
@@ -97,7 +97,7 @@ static ssize_t device_name_store(struct device *dev,
 
 	cancel_delayed_work_sync(&trigger_data->work);
 
-	spin_lock_bh(&trigger_data->lock);
+	mutex_lock(&trigger_data->lock);
 
 	if (trigger_data->net_dev) {
 		dev_put(trigger_data->net_dev);
@@ -121,7 +121,7 @@ static ssize_t device_name_store(struct device *dev,
 	trigger_data->last_activity = 0;
 
 	set_baseline_state(trigger_data);
-	spin_unlock_bh(&trigger_data->lock);
+	mutex_unlock(&trigger_data->lock);
 
 	return size;
 }
@@ -295,7 +295,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
 
 	cancel_delayed_work_sync(&trigger_data->work);
 
-	spin_lock_bh(&trigger_data->lock);
+	mutex_lock(&trigger_data->lock);
 
 	clear_bit(NETDEV_LED_MODE_LINKUP, &trigger_data->mode);
 	switch (evt) {
@@ -319,7 +319,7 @@ static int netdev_trig_notify(struct notifier_block *nb,
 
 	set_baseline_state(trigger_data);
 
-	spin_unlock_bh(&trigger_data->lock);
+	mutex_unlock(&trigger_data->lock);
 
 	return NOTIFY_DONE;
 }
@@ -380,7 +380,7 @@ static int netdev_trig_activate(struct led_classdev *led_cdev)
 	if (!trigger_data)
 		return -ENOMEM;
 
-	spin_lock_init(&trigger_data->lock);
+	mutex_init(&trigger_data->lock);
 
 	trigger_data->notifier.notifier_call = netdev_trig_notify;
 	trigger_data->notifier.priority = 10;
diff --git a/include/linux/ledtrig.h b/include/linux/ledtrig.h
index a6a813bb154a..bd8cdcfaf232 100644
--- a/include/linux/ledtrig.h
+++ b/include/linux/ledtrig.h
@@ -8,13 +8,13 @@
 
 #include <linux/atomic.h>
 #include <linux/leds.h>
+#include <linux/mutex.h>
 #include <linux/netdevice.h>
-#include <linux/spinlock.h>
 
 #if IS_ENABLED(CONFIG_LEDS_TRIGGER_NETDEV)
 
 struct led_netdev_data {
-	spinlock_t lock;
+	struct mutex lock;
 
 	struct delayed_work work;
 	struct notifier_block notifier;
-- 
2.26.3

Re: [PATCH leds v1 1/5] leds: trigger: netdev: don't explicitly zero kzalloced data

[Andrew Lunn]

On Wed, May 26, 2021 at 08:00:16PM +0200, Marek Behún wrote:
> The trigger_data struct is allocated with kzalloc, so we do not need to
> explicitly set members to zero.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Acked-by: Pavel Machek <pavel@ucw.cz>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH leds v1 3/5] leds: trigger: netdev: move trigger data structure to global include dir

[Andrew Lunn]

On Wed, May 26, 2021 at 08:00:18PM +0200, Marek Behún wrote:
> In preparation for HW offloading of netdev trigger, move struct
> led_trigger_data into global include directory, into file
> linux/ledtrig.h, so that drivers wanting to offload the trigger can see
> the requested settings.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 23 +---------------
>  include/linux/ledtrig.h               | 38 +++++++++++++++++++++++++++

I'm wondering how this is going to scale, if we have a lot of triggers
which can be offloaded. Rather than try to pack them all into
one header, would it make more sense to add

include/linux/led/ledtrig-netdev.h

	Andrew

Re: [PATCH leds v1 4/5] leds: trigger: netdev: support HW offloading

[Andrew Lunn]

On Wed, May 26, 2021 at 08:00:19PM +0200, Marek Behún wrote:
> Add support for HW offloading of the netdev trigger.
> 
> We need to export the netdev_led_trigger variable so that drivers may
> check whether the LED is set to this trigger.

Without seeing the driver side, it is not obvious to me why this is
needed. Please add the driver changes to this patchset, so we can
fully see how the API works.

> -static struct led_trigger netdev_led_trigger = {
> +struct led_trigger netdev_led_trigger = {
>  	.name = "netdev",
>  	.activate = netdev_trig_activate,
>  	.deactivate = netdev_trig_deactivate,
>  	.groups = netdev_trig_groups,
>  };
> +EXPORT_SYMBOL_GPL(netdev_led_trigger);

If these are going to be exported, maybe they should be made const to
protect them a bit?

	Andrew

Re: [PATCH leds v1 3/5] leds: trigger: netdev: move trigger data structure to global include dir

[Marek Behún]

On Thu, 27 May 2021 18:48:21 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Wed, May 26, 2021 at 08:00:18PM +0200, Marek Behún wrote:
> > In preparation for HW offloading of netdev trigger, move struct
> > led_trigger_data into global include directory, into file
> > linux/ledtrig.h, so that drivers wanting to offload the trigger can
> > see the requested settings.
> > 
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> >  drivers/leds/trigger/ledtrig-netdev.c | 23 +---------------
> >  include/linux/ledtrig.h               | 38
> > +++++++++++++++++++++++++++  
> 
> I'm wondering how this is going to scale, if we have a lot of triggers
> which can be offloaded. Rather than try to pack them all into
> one header, would it make more sense to add
> 
> include/linux/led/ledtrig-netdev.h
> 
> 	Andrew

Hmm, I guess you are right. Also when looking at a LED controller
driver we could immediately see which triggers this driver can offload,
when looking at headers included.

Re: [PATCH leds v1 4/5] leds: trigger: netdev: support HW offloading

[Marek Behún]

On Thu, 27 May 2021 18:57:17 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Wed, May 26, 2021 at 08:00:19PM +0200, Marek Behún wrote:
> > Add support for HW offloading of the netdev trigger.
> > 
> > We need to export the netdev_led_trigger variable so that drivers
> > may check whether the LED is set to this trigger.  
> 
> Without seeing the driver side, it is not obvious to me why this is
> needed. Please add the driver changes to this patchset, so we can
> fully see how the API works.

OK, I will send an implementation for leds-turris-omnia with v2.

The idea is that the trigger_offload() method should check which
trigger it should offload. A potential LED controller may be configured
to link the LED on net activity, or on SATA activity. So the method
should do something like this:

  static int my_trigger_offload(struct led_classdev *cdev, bool enable)
  {
    if (!enable)
      return my_disable_hw_triggering(cdev);
	
    if (cdev->trigger == &netdev_led_trigger)
      return my_offload_netdev_triggering(cdev);
    else if (cdev->trigger == &blkdev_led_trigger)
      return my_offload_blkdev_triggering(cdev);
    else
      return -EOPNOTSUPP;
  }

> > -static struct led_trigger netdev_led_trigger = {
> > +struct led_trigger netdev_led_trigger = {
> >  	.name = "netdev",
> >  	.activate = netdev_trig_activate,
> >  	.deactivate = netdev_trig_deactivate,
> >  	.groups = netdev_trig_groups,
> >  };
> > +EXPORT_SYMBOL_GPL(netdev_led_trigger);  
> 
> If these are going to be exported, maybe they should be made const to
> protect them a bit?

The trigger structure must be defined writable, for the code holds
a list of LEDs that have this trigger activated in the structure, among
other data. I don't think if it can be declared as const and then
defined non-const.

Marek

Re: [PATCH leds v1 4/5] leds: trigger: netdev: support HW offloading

[Andrew Lunn]

On Fri, May 28, 2021 at 08:45:56AM +0200, Marek Behún wrote:
> On Thu, 27 May 2021 18:57:17 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > On Wed, May 26, 2021 at 08:00:19PM +0200, Marek Behún wrote:
> > > Add support for HW offloading of the netdev trigger.
> > > 
> > > We need to export the netdev_led_trigger variable so that drivers
> > > may check whether the LED is set to this trigger.  
> > 
> > Without seeing the driver side, it is not obvious to me why this is
> > needed. Please add the driver changes to this patchset, so we can
> > fully see how the API works.
> 
> OK, I will send an implementation for leds-turris-omnia with v2.
> 
> The idea is that the trigger_offload() method should check which
> trigger it should offload. A potential LED controller may be configured
> to link the LED on net activity, or on SATA activity. So the method
> should do something like this:
> 
>   static int my_trigger_offload(struct led_classdev *cdev, bool enable)
>   {
>     if (!enable)
>       return my_disable_hw_triggering(cdev);
> 	
>     if (cdev->trigger == &netdev_led_trigger)
>       return my_offload_netdev_triggering(cdev);
>     else if (cdev->trigger == &blkdev_led_trigger)
>       return my_offload_blkdev_triggering(cdev);
>     else
>       return -EOPNOTSUPP;
>   }

So the hardware driver does not need the contents of the trigger? It
never manipulates the trigger. Maybe to keep the abstraction cleaner,
an enum can be added to the trigger to identify it. The code then
becomes:

static int my_trigger_offload(struct led_classdev *cdev, bool enable)
{
	if (!enable)
        	return my_disable_hw_triggering(cdev);
 	
	switch(cdev->trigger->trigger) {
	case TRIGGER_NETDEV:
	       return my_offload_netdev_triggering(cdev);
	case TRIGGER_BLKDEV:
	       return my_offload_blkdev_triggering(cdev);
	default:
	       return -EOPNOTSUPP;
}	

	Andrew

Re: [PATCH leds v1 4/5] leds: trigger: netdev: support HW offloading

[Marek Behún]

On Fri, 28 May 2021 15:50:08 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Fri, May 28, 2021 at 08:45:56AM +0200, Marek Behún wrote:
> > On Thu, 27 May 2021 18:57:17 +0200
> > Andrew Lunn <andrew@lunn.ch> wrote:
> >   
> > > On Wed, May 26, 2021 at 08:00:19PM +0200, Marek Behún wrote:  
> > > > Add support for HW offloading of the netdev trigger.
> > > > 
> > > > We need to export the netdev_led_trigger variable so that
> > > > drivers may check whether the LED is set to this trigger.    
> > > 
> > > Without seeing the driver side, it is not obvious to me why this
> > > is needed. Please add the driver changes to this patchset, so we
> > > can fully see how the API works.  
> > 
> > OK, I will send an implementation for leds-turris-omnia with v2.
> > 
> > The idea is that the trigger_offload() method should check which
> > trigger it should offload. A potential LED controller may be
> > configured to link the LED on net activity, or on SATA activity. So
> > the method should do something like this:
> > 
> >   static int my_trigger_offload(struct led_classdev *cdev, bool
> > enable) {
> >     if (!enable)
> >       return my_disable_hw_triggering(cdev);
> > 	
> >     if (cdev->trigger == &netdev_led_trigger)
> >       return my_offload_netdev_triggering(cdev);
> >     else if (cdev->trigger == &blkdev_led_trigger)
> >       return my_offload_blkdev_triggering(cdev);
> >     else
> >       return -EOPNOTSUPP;
> >   }  
> 
> So the hardware driver does not need the contents of the trigger? It
> never manipulates the trigger. Maybe to keep the abstraction cleaner,
> an enum can be added to the trigger to identify it. The code then
> becomes:
> 
> static int my_trigger_offload(struct led_classdev *cdev, bool enable)
> {
> 	if (!enable)
>         	return my_disable_hw_triggering(cdev);
>  	
> 	switch(cdev->trigger->trigger) {
> 	case TRIGGER_NETDEV:
> 	       return my_offload_netdev_triggering(cdev);
> 	case TRIGGER_BLKDEV:
> 	       return my_offload_blkdev_triggering(cdev);
> 	default:
> 	       return -EOPNOTSUPP;
> }	

If we want to avoid exporting the symbol I would rather compare
  !strcmp(cdev->trigger->name, "netdev")
What do you think?