diff mbox series

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

Message ID 20210526180020.13557-5-kabel@kernel.org (mailing list archive)
State Not Applicable
Headers show
Series Add support for offloading netdev trigger to HW | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Marek Behún May 26, 2021, 6 p.m. UTC
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(-)

Comments

Andrew Lunn May 27, 2021, 4:57 p.m. UTC | #1
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
Marek Behún May 28, 2021, 6:45 a.m. UTC | #2
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
Andrew Lunn May 28, 2021, 1:50 p.m. UTC | #3
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
Marek Behún May 28, 2021, 1:57 p.m. UTC | #4
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?
diff mbox series

Patch

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__ */