[RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Fabio Baltieri]

This patch implements the functions to add two LED triggers, named
<ifname>-tx and <ifname>-rx, to a canbus device driver.

Triggers are called from specific handlers by each CAN device driver and
can be disabled altogether with a Kconfig option.

The implementation keeps the LED on when the interface is UP and blinks
the LED on network activity at a configurable rate.

This only supports can-dev based drivers, as it uses some support field
in the can_priv structure.

Supported drivers should call can_led_init(), can_led_exit() and
can_led_event() as needed.

Supported events are:
- CAN_LED_EVENT_OPEN: turn on tx/rx LEDs
- CAN_LED_EVENT_STOP: turn off tx/rx LEDs
- CAN_LED_EVENT_TX: trigger tx LED blink
- CAN_LED_EVENT_RX: trigger tx LED blink

Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
---

Hello again!

So, this is the v2 of my can-led patch. I reworked the whole code based on the
v1 and all the comments from the list (thanks guys!).

This version takes a different approach and is just an implementation of three
functions - can_led_{init,exit,event} - to easily add tx/rx led trigger
support to a netlink-based canbus driver. An implementation on flexcan is on
the 2/2.

Working principle:

- the driver register the triggers on probe, free on exit and add events for
  open, close, tx, rx where appropriate
- both LEDs stay off when device is down, on when device is up
- each LED blinks for led_delay off + led_delay on on tx/rx event using a
  kernel timer (well, that's actually the opposite to keep critical path
  cleaner but don't tell anyone, k?).
- new tx/rx packets don't retrigger the timer until the blink sequence is over,
  so that on heavy traffic the LED keeps blinking at constate rate.  

Notes:

- I considered putting all structures in can_priv to skip a couple of kzallocs
  as suggested by Oliver, but those pointers are actually useful to check if
  triggers registered properly, so the code now uses an internal structure with
  timer, state and name, and add just two pointers into struct can_dev.

- The blink timer is implemented as:
  * keep on for delay_time
  * turn off and retrigger the timer
  * turn back on after delay
  so that the hot-path only contains a switch-case, some checks and a mod_timer.

This is tested on an x86 with a custom usb-can interface and on a flexcan-based
Freescale ARM board. I'll post some patch to implement support in other drivers
if anyone is interested into testing this one.

Any comments?

Regards,
Fabio

 drivers/net/can/Kconfig  |   13 ++++
 drivers/net/can/Makefile |    2 +
 drivers/net/can/led.c    |  143 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/can/dev.h  |    6 ++
 include/linux/can/led.h  |   46 +++++++++++++++
 5 files changed, 210 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/can/led.c
 create mode 100644 include/linux/can/led.h

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index bb709fd..74d6bfb 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -54,6 +54,19 @@ config CAN_CALC_BITTIMING
 	  arguments "tq", "prop_seg", "phase_seg1", "phase_seg2" and "sjw".
 	  If unsure, say Y.
 
+config CAN_LEDS
+	bool "Enable LED triggers for Netlink based drivers"
+	depends on CAN
+	depends on CAN_DEV
+	depends on LEDS_CLASS
+	select LEDS_TRIGGERS
+	---help---
+	  This option adds two LED triggers for packet receive and transmit
+	  events on each supported CAN device.
+
+	  Say Y here if you are working on a system with led-class supported
+	  LEDs and you want to use them as canbus activity indicators.
+
 config CAN_AT91
 	tristate "Atmel AT91 onchip CAN controller"
 	depends on CAN_DEV && (ARCH_AT91SAM9263 || ARCH_AT91SAM9X5)
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index 938be37..24ee98b 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -8,6 +8,8 @@ obj-$(CONFIG_CAN_SLCAN)		+= slcan.o
 obj-$(CONFIG_CAN_DEV)		+= can-dev.o
 can-dev-y			:= dev.o
 
+can-dev-$(CONFIG_CAN_LEDS)	+= led.o
+
 obj-y				+= usb/
 obj-y				+= softing/
 
diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
new file mode 100644
index 0000000..3479e38
--- /dev/null
+++ b/drivers/net/can/led.c
@@ -0,0 +1,143 @@
+/*
+ * Copyright 2012, Fabio Baltieri <fabio.baltieri@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/netdevice.h>
+#include <linux/can/dev.h>
+#include <linux/timer.h>
+
+#include <linux/can/led.h>
+
+static int led_delay = 50;
+module_param(led_delay, int, 0644);
+MODULE_PARM_DESC(led_delay,
+		"blink delay time for activity leds (msecs, default: 50).");
+
+static void can_led_timerfunc(unsigned long data)
+{
+	struct can_led_data *led_data = (struct can_led_data *)data;
+
+	if (led_data->phase == 0) {
+		led_trigger_event(&led_data->trig, LED_OFF);
+		led_data->phase = 1;
+
+		mod_timer(&led_data->timer,
+			jiffies + msecs_to_jiffies(led_delay));
+	} else {
+		led_trigger_event(&led_data->trig, LED_FULL);
+		led_data->phase = 0;
+	}
+}
+
+void can_led_event(struct net_device *netdev, enum can_led_event event)
+{
+	struct can_priv *priv = netdev_priv(netdev);
+	struct can_led_data *tx_led = priv->tx_led_data;
+	struct can_led_data *rx_led = priv->rx_led_data;
+
+	switch (event) {
+	case CAN_LED_EVENT_OPEN:
+		if (likely(tx_led))
+			led_trigger_event(&tx_led->trig, LED_FULL);
+		if (likely(rx_led))
+			led_trigger_event(&rx_led->trig, LED_FULL);
+		break;
+	case CAN_LED_EVENT_STOP:
+		if (likely(tx_led)) {
+			del_timer_sync(&tx_led->timer);
+			led_trigger_event(&tx_led->trig, LED_OFF);
+		}
+		if (likely(rx_led)) {
+			del_timer_sync(&rx_led->timer);
+			led_trigger_event(&rx_led->trig, LED_OFF);
+		}
+		break;
+	case CAN_LED_EVENT_TX:
+		if (led_delay && likely(tx_led) &&
+		    !timer_pending(&tx_led->timer))
+			mod_timer(&tx_led->timer,
+				jiffies + msecs_to_jiffies(led_delay));
+		break;
+	case CAN_LED_EVENT_RX:
+		if (led_delay && likely(rx_led) &&
+		    !timer_pending(&rx_led->timer))
+			mod_timer(&rx_led->timer,
+				jiffies + msecs_to_jiffies(led_delay));
+		break;
+	}
+}
+EXPORT_SYMBOL_GPL(can_led_event);
+
+void can_led_init(struct net_device *netdev)
+{
+	struct can_priv *priv = netdev_priv(netdev);
+
+	priv->tx_led_data = kzalloc(sizeof(struct can_led_data), GFP_KERNEL);
+	if (!priv->tx_led_data)
+		goto tx_led_data_failed;
+
+	priv->rx_led_data = kzalloc(sizeof(struct can_led_data), GFP_KERNEL);
+	if (!priv->rx_led_data)
+		goto rx_led_data_failed;
+
+	snprintf(priv->tx_led_data->name, sizeof(priv->tx_led_data->name),
+		"%s-tx", netdev->name);
+
+	snprintf(priv->rx_led_data->name, sizeof(priv->rx_led_data->name),
+		"%s-rx", netdev->name);
+
+	priv->tx_led_data->trig.name = priv->tx_led_data->name;
+	if (led_trigger_register(&priv->tx_led_data->trig)) {
+		kfree(priv->tx_led_data);
+		priv->tx_led_data = NULL;
+	}
+
+	if (priv->tx_led_data)
+		setup_timer(&priv->tx_led_data->timer,
+			can_led_timerfunc, (unsigned long)priv->tx_led_data);
+
+	priv->rx_led_data->trig.name = priv->rx_led_data->name;
+	if (led_trigger_register(&priv->rx_led_data->trig)) {
+		kfree(priv->rx_led_data);
+		priv->rx_led_data = NULL;
+	}
+
+	if (priv->rx_led_data)
+		setup_timer(&priv->rx_led_data->timer,
+			can_led_timerfunc, (unsigned long)priv->rx_led_data);
+	return;
+
+rx_led_data_failed:
+	kfree(priv->tx_led_data);
+	priv->tx_led_data = NULL;
+tx_led_data_failed:
+	return;
+}
+EXPORT_SYMBOL_GPL(can_led_init);
+
+void can_led_exit(struct net_device *netdev)
+{
+	struct can_priv *priv = netdev_priv(netdev);
+
+	if (priv->tx_led_data) {
+		del_timer_sync(&priv->tx_led_data->timer);
+
+		led_trigger_unregister(&priv->tx_led_data->trig);
+		kfree(priv->tx_led_data);
+	}
+
+	if (priv->rx_led_data) {
+		del_timer_sync(&priv->rx_led_data->timer);
+
+		led_trigger_unregister(&priv->rx_led_data->trig);
+		kfree(priv->rx_led_data);
+	}
+}
+EXPORT_SYMBOL_GPL(can_led_exit);
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 5d2efe7..a5a70bd 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -16,6 +16,7 @@
 #include <linux/can.h>
 #include <linux/can/netlink.h>
 #include <linux/can/error.h>
+#include <linux/can/led.h>
 
 /*
  * CAN mode
@@ -52,6 +53,11 @@ struct can_priv {
 
 	unsigned int echo_skb_max;
 	struct sk_buff **echo_skb;
+
+#ifdef CONFIG_CAN_LEDS
+	struct can_led_data *tx_led_data;
+	struct can_led_data *rx_led_data;
+#endif
 };
 
 /*
diff --git a/include/linux/can/led.h b/include/linux/can/led.h
new file mode 100644
index 0000000..6496db5
--- /dev/null
+++ b/include/linux/can/led.h
@@ -0,0 +1,46 @@
+/*
+ * Copyright 2012, Fabio Baltieri <fabio.baltieri@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef CAN_LED_H
+#define CAN_LED_H
+
+#include <linux/timer.h>
+#include <linux/leds.h>
+
+enum can_led_event {
+	CAN_LED_EVENT_OPEN,
+	CAN_LED_EVENT_STOP,
+	CAN_LED_EVENT_TX,
+	CAN_LED_EVENT_RX,
+};
+
+#ifdef CONFIG_CAN_LEDS
+struct can_led_data {
+	char name[16];
+	struct led_trigger trig;
+	struct timer_list timer;
+	unsigned int phase;
+};
+
+void can_led_event(struct net_device *netdev, enum can_led_event event);
+void can_led_init(struct net_device *netdev);
+void can_led_exit(struct net_device *netdev);
+#else
+static inline void can_led_event(struct net_device *netdev,
+				 enum can_led_event event)
+{
+}
+static inline void can_led_init(struct net_device *netdev)
+{
+}
+static inline void can_led_exit(struct net_device *netdev)
+{
+}
+#endif
+
+#endif
-- 
1.7.5.1

[RFC PATCH v2 2/2] can: flexcan: add LED trigger support

[Fabio Baltieri]

Add support for canbus activity led indicators on flexcan interfaces by
calling appropriate can_led_* functions.

These are only enabled when CONFIG_CAN_LEDS is Y, becomes no-op
otherwise.

Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
---
 drivers/net/can/flexcan.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 1efb083..0e55c4c 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -23,6 +23,7 @@
 #include <linux/can.h>
 #include <linux/can/dev.h>
 #include <linux/can/error.h>
+#include <linux/can/led.h>
 #include <linux/can/platform/flexcan.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
@@ -524,6 +525,8 @@ static int flexcan_read_frame(struct net_device *dev)
 	stats->rx_packets++;
 	stats->rx_bytes += cf->can_dlc;
 
+	can_led_event(dev, CAN_LED_EVENT_RX);
+
 	return 1;
 }
 
@@ -614,6 +617,8 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 		stats->tx_packets++;
 		flexcan_write((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
 		netif_wake_queue(dev);
+
+		can_led_event(dev, CAN_LED_EVENT_TX);
 	}
 
 	return IRQ_HANDLED;
@@ -813,6 +818,8 @@ static int flexcan_open(struct net_device *dev)
 	if (err)
 		goto out_close;
 
+	can_led_event(dev, CAN_LED_EVENT_OPEN);
+
 	/* start chip and queuing */
 	err = flexcan_chip_start(dev);
 	if (err)
@@ -838,6 +845,8 @@ static int flexcan_close(struct net_device *dev)
 	napi_disable(&priv->napi);
 	flexcan_chip_stop(dev);
 
+	can_led_event(dev, CAN_LED_EVENT_STOP);
+
 	free_irq(dev->irq, dev);
 	clk_disable_unprepare(priv->clk);
 
@@ -1004,6 +1013,8 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 		goto failed_register;
 	}
 
+	can_led_init(dev);
+
 	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
 		 priv->base, dev->irq);
 
@@ -1028,6 +1039,8 @@ static int __devexit flexcan_remove(struct platform_device *pdev)
 	struct flexcan_priv *priv = netdev_priv(dev);
 	struct resource *mem;
 
+	can_led_exit(dev);
+
 	unregister_flexcandev(dev);
 	platform_set_drvdata(pdev, NULL);
 	iounmap(priv->base);
-- 
1.7.5.1

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Oliver Hartkopp]

On 23.04.2012 23:02, Fabio Baltieri wrote:

> This patch implements the functions to add two LED triggers, named
> <ifname>-tx and <ifname>-rx, to a canbus device driver.


Hello Fabio,

to me it looks really good now. I understand everything of the idea at first
sight which is a good indicator for simple code ;-)

The only remark i currently have is ...

> 
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index bb709fd..74d6bfb 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -54,6 +54,19 @@ config CAN_CALC_BITTIMING
>  	  arguments "tq", "prop_seg", "phase_seg1", "phase_seg2" and "sjw".
>  	  If unsure, say Y.
>  
> +config CAN_LEDS
> +	bool "Enable LED triggers for Netlink based drivers"
> +	depends on CAN


CAN_DEV and this entire Kconfig content itself depends on CAN (see at the top
of this Kconfig) so you can omit that line.

> +	depends on CAN_DEV
> +	depends on LEDS_CLASS
> +	select LEDS_TRIGGERS
> +	---help---
> +	  This option adds two LED triggers for packet receive and transmit
> +	  events on each supported CAN device.
> +
> +	  Say Y here if you are working on a system with led-class supported
> +	  LEDs and you want to use them as canbus activity indicators.
> +
>  config CAN_AT91
>  	tristate "Atmel AT91 onchip CAN controller"
>  	depends on CAN_DEV && (ARCH_AT91SAM9263 || ARCH_AT91SAM9X5)


Regards,
Oliver

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Fabio Baltieri]

On Tue, Apr 24, 2012 at 07:16:38AM +0200, Oliver Hartkopp wrote:
> On 23.04.2012 23:02, Fabio Baltieri wrote:
> 
> > This patch implements the functions to add two LED triggers, named
> > <ifname>-tx and <ifname>-rx, to a canbus device driver.
> 
> 
> Hello Fabio,

Hi Oliver,

> to me it looks really good now. I understand everything of the idea at first
> sight which is a good indicator for simple code ;-)

Thanks!

> The only remark i currently have is ...
> 
> > 
> > diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> > index bb709fd..74d6bfb 100644
> > --- a/drivers/net/can/Kconfig
> > +++ b/drivers/net/can/Kconfig
> > @@ -54,6 +54,19 @@ config CAN_CALC_BITTIMING
> >  	  arguments "tq", "prop_seg", "phase_seg1", "phase_seg2" and "sjw".
> >  	  If unsure, say Y.
> >  
> > +config CAN_LEDS
> > +	bool "Enable LED triggers for Netlink based drivers"
> > +	depends on CAN
> 
> 
> CAN_DEV and this entire Kconfig content itself depends on CAN (see at the top
> of this Kconfig) so you can omit that line.

You're right, that was a leftover from the v1.

Removed for v3.

Regards,
Fabio

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Wolfgang Grandegger]

Hi Fabio,

On 04/23/2012 11:02 PM, Fabio Baltieri wrote:
> This patch implements the functions to add two LED triggers, named
> <ifname>-tx and <ifname>-rx, to a canbus device driver.
> 
> Triggers are called from specific handlers by each CAN device driver and
> can be disabled altogether with a Kconfig option.
> 
> The implementation keeps the LED on when the interface is UP and blinks
> the LED on network activity at a configurable rate.
> 
> This only supports can-dev based drivers, as it uses some support field
> in the can_priv structure.
> 
> Supported drivers should call can_led_init(), can_led_exit() and
> can_led_event() as needed.
> 
> Supported events are:
> - CAN_LED_EVENT_OPEN: turn on tx/rx LEDs
> - CAN_LED_EVENT_STOP: turn off tx/rx LEDs
> - CAN_LED_EVENT_TX: trigger tx LED blink
> - CAN_LED_EVENT_RX: trigger tx LED blink
> 
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> ---
> 
> Hello again!
> 
> So, this is the v2 of my can-led patch. I reworked the whole code based on the
> v1 and all the comments from the list (thanks guys!).
> 
> This version takes a different approach and is just an implementation of three
> functions - can_led_{init,exit,event} - to easily add tx/rx led trigger
> support to a netlink-based canbus driver. An implementation on flexcan is on
> the 2/2.
> 
> Working principle:
> 
> - the driver register the triggers on probe, free on exit and add events for
>   open, close, tx, rx where appropriate
> - both LEDs stay off when device is down, on when device is up
> - each LED blinks for led_delay off + led_delay on on tx/rx event using a
>   kernel timer (well, that's actually the opposite to keep critical path
>   cleaner but don't tell anyone, k?).
> - new tx/rx packets don't retrigger the timer until the blink sequence is over,
>   so that on heavy traffic the LED keeps blinking at constate rate.  
> 
> Notes:
> 
> - I considered putting all structures in can_priv to skip a couple of kzallocs
>   as suggested by Oliver, but those pointers are actually useful to check if
>   triggers registered properly, so the code now uses an internal structure with
>   timer, state and name, and add just two pointers into struct can_dev.
> 
> - The blink timer is implemented as:
>   * keep on for delay_time
>   * turn off and retrigger the timer
>   * turn back on after delay
>   so that the hot-path only contains a switch-case, some checks and a mod_timer.
> 
> This is tested on an x86 with a custom usb-can interface and on a flexcan-based
> Freescale ARM board. I'll post some patch to implement support in other drivers
> if anyone is interested into testing this one.
> 
> Any comments?

I still think that the blinking support should go to the timer class to
avoid duplicated code. Any good reason against? Apart from that the
patches look good.

Wolfgang.

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Oliver Hartkopp]

On 24.04.2012 08:46, Wolfgang Grandegger wrote:


>> - The blink timer is implemented as:
>>   * keep on for delay_time
>>   * turn off and retrigger the timer
>>   * turn back on after delay
>>   so that the hot-path only contains a switch-case, some checks and a mod_timer.
>>
>> This is tested on an x86 with a custom usb-can interface and on a flexcan-based
>> Freescale ARM board. I'll post some patch to implement support in other drivers
>> if anyone is interested into testing this one.
>>
>> Any comments?
> 
> I still think that the blinking support should go to the timer class to
> avoid duplicated code. Any good reason against? Apart from that the
> patches look good.


Hello Wolfgang,

what exactly do you mean with "timer class" ??

To me the current implementation looks like standard jiffie-based timer usage.

IMO hrtimers are not needed here - or what are you thinking of?

Regards,
Oliver

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Wolfgang Grandegger]

On 04/24/2012 05:41 PM, Oliver Hartkopp wrote:
> On 24.04.2012 08:46, Wolfgang Grandegger wrote:
> 
> 
>>> - The blink timer is implemented as:
>>>   * keep on for delay_time
>>>   * turn off and retrigger the timer
>>>   * turn back on after delay
>>>   so that the hot-path only contains a switch-case, some checks and a mod_timer.
>>>
>>> This is tested on an x86 with a custom usb-can interface and on a flexcan-based
>>> Freescale ARM board. I'll post some patch to implement support in other drivers
>>> if anyone is interested into testing this one.
>>>
>>> Any comments?
>>
>> I still think that the blinking support should go to the timer class to
>> avoid duplicated code. Any good reason against? Apart from that the
>> patches look good.
> 
> 
> Hello Wolfgang,
> 
> what exactly do you mean with "timer class" ??
> 
> To me the current implementation looks like standard jiffie-based timer usage.
> 
> IMO hrtimers are not needed here - or what are you thinking of?

I mean that the code doing that kind of blinking should go to the timer
class as generic function/feature, which other drivers could then use as
well, similar to led_trigger_event or led_trigger_blink. Should not be a
big deal, I think. See also my previous mail:

  http://marc.info/?l=linux-can&m=133425656510148&w=2

Wolfgang.

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Oliver Hartkopp]

On 24.04.2012 20:08, Wolfgang Grandegger wrote:

> On 04/24/2012 05:41 PM, Oliver Hartkopp wrote:
>> On 24.04.2012 08:46, Wolfgang Grandegger wrote:
>>
>>
>>>> - The blink timer is implemented as:
>>>>   * keep on for delay_time
>>>>   * turn off and retrigger the timer
>>>>   * turn back on after delay
>>>>   so that the hot-path only contains a switch-case, some checks and a mod_timer.
>>>>
>>>> This is tested on an x86 with a custom usb-can interface and on a flexcan-based
>>>> Freescale ARM board. I'll post some patch to implement support in other drivers
>>>> if anyone is interested into testing this one.
>>>>
>>>> Any comments?
>>>
>>> I still think that the blinking support should go to the timer class to
>>> avoid duplicated code. Any good reason against? Apart from that the
>>> patches look good.
>>
>>
>> Hello Wolfgang,
>>
>> what exactly do you mean with "timer class" ??
>>
>> To me the current implementation looks like standard jiffie-based timer usage.
>>
>> IMO hrtimers are not needed here - or what are you thinking of?
> 
> I mean that the code doing that kind of blinking should go to the timer
> class as generic function/feature, which other drivers could then use as
> well, similar to led_trigger_event or led_trigger_blink. Should not be a
> big deal, I think. See also my previous mail:
> 
>   http://marc.info/?l=linux-can&m=133425656510148&w=2
> 


Hm - as i see from the two examples there, the LEDs have a different
behaviour. E.g. on which use-case specific events they blink or set the LEDs
to fixed values.

I can't see any real benefit for these LED users to implement a generic LED
blinking framework. IMO using timers and the led_trigger_event stuff is the
appropriate abstraction level.

If you look into other networking drivers (e.g. the wireless) - they do it
with delayed_work:

http://lxr.linux.no/#linux+v3.3.1/drivers/net/wireless/ath/carl9170/led.c#L105

And everyone defines a specific struct like this one:

http://lxr.linux.no/#linux+v3.3.1/drivers/net/wireless/ath/carl9170/carl9170.h#L205

I think it's an enormous effort to unify all the LED users ...

Regards,
Oliver

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Wolfgang Grandegger]

On 04/24/2012 08:57 PM, Oliver Hartkopp wrote:
> On 24.04.2012 20:08, Wolfgang Grandegger wrote:
> 
>> On 04/24/2012 05:41 PM, Oliver Hartkopp wrote:
>>> On 24.04.2012 08:46, Wolfgang Grandegger wrote:
>>>
>>>
>>>>> - The blink timer is implemented as:
>>>>>   * keep on for delay_time
>>>>>   * turn off and retrigger the timer
>>>>>   * turn back on after delay
>>>>>   so that the hot-path only contains a switch-case, some checks and a mod_timer.
>>>>>
>>>>> This is tested on an x86 with a custom usb-can interface and on a flexcan-based
>>>>> Freescale ARM board. I'll post some patch to implement support in other drivers
>>>>> if anyone is interested into testing this one.
>>>>>
>>>>> Any comments?
>>>>
>>>> I still think that the blinking support should go to the timer class to
>>>> avoid duplicated code. Any good reason against? Apart from that the
>>>> patches look good.
>>>
>>>
>>> Hello Wolfgang,
>>>
>>> what exactly do you mean with "timer class" ??
>>>
>>> To me the current implementation looks like standard jiffie-based timer usage.
>>>
>>> IMO hrtimers are not needed here - or what are you thinking of?
>>
>> I mean that the code doing that kind of blinking should go to the timer
>> class as generic function/feature, which other drivers could then use as
>> well, similar to led_trigger_event or led_trigger_blink. Should not be a
>> big deal, I think. See also my previous mail:
>>
>>   http://marc.info/?l=linux-can&m=133425656510148&w=2
>>
> 
> 
> Hm - as i see from the two examples there, the LEDs have a different
> behaviour. E.g. on which use-case specific events they blink or set the LEDs
> to fixed values.
> 
> I can't see any real benefit for these LED users to implement a generic LED
> blinking framework. IMO using timers and the led_trigger_event stuff is the
> appropriate abstraction level.

It's not the task of the CAN driver to let LEDs blink! Or what should
the LED class then be good for?

> If you look into other networking drivers (e.g. the wireless) - they do it
> with delayed_work:
> 
> http://lxr.linux.no/#linux+v3.3.1/drivers/net/wireless/ath/carl9170/led.c#L105
> 
> And everyone defines a specific struct like this one:
> 
> http://lxr.linux.no/#linux+v3.3.1/drivers/net/wireless/ath/carl9170/carl9170.h#L205
> 
> I think it's an enormous effort to unify all the LED users ...

Yes, they do that because there is no generic support. I believe that
the "one-shot-blinking" is useful in general which new drivers may use.
I did not ask for adapting old drivers.

Wolfgang.

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Fabio Baltieri]

Hi Wolfgang,

On Tue, Apr 24, 2012 at 08:46:29AM +0200, Wolfgang Grandegger wrote:
> I still think that the blinking support should go to the timer class to
> avoid duplicated code. Any good reason against? Apart from that the
> patches look good.
> 
> Wolfgang.

I can see you point and I considered your note about adding the
one-shot-blink function to the led-class framework (sorry for not
mentioning it in my first post).  Still, I ended up with this code for
a couple of reasons:

- I think that the led_blink_set function is primarily used to configure
  leds with hardware blinking (like i2c led drivers).  While it would be
  possible to extend the function to get one-shot behavior and always
  fallback on software blink, I think that that's out of the purpose of
  the led-class, which should just translate on-off requests to
  underlaying hardware.

- I think that different drivers may want to obtain different on-off
  behavior depending on the application.  For example in the ide-disk
  case the user expects to see a steady-on LED on constant activity,
  and that's how it's implemented, while in this case the on-if-up
  keep-blinking-on-activity off-if-down makes much more sense. So I think
  that even if a generic blink function were available, people would
  still be using custom functions because they want to fine tune the
  behavior for the application.  Also, maybe a function too generic may
  impact on performance in critical paths.

- in this case, it looks to me like the implementation is as optimized
  as it can be, in the sense that the hot-path does really only some
  essential check and engage the timer and the timer function itself is
  really short.  Also the final blinking effect is nice IMO :-)

Regards,
Fabio

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Wolfgang Grandegger]

Hi Fabio,

On 04/24/2012 09:02 PM, Fabio Baltieri wrote:
> Hi Wolfgang,
> 
> On Tue, Apr 24, 2012 at 08:46:29AM +0200, Wolfgang Grandegger wrote:
>> I still think that the blinking support should go to the timer class to
>> avoid duplicated code. Any good reason against? Apart from that the
>> patches look good.
>>
>> Wolfgang.
> 
> I can see you point and I considered your note about adding the
> one-shot-blink function to the led-class framework (sorry for not
> mentioning it in my first post).  Still, I ended up with this code for
> a couple of reasons:
> 
> - I think that the led_blink_set function is primarily used to configure
>   leds with hardware blinking (like i2c led drivers).  While it would be
>   possible to extend the function to get one-shot behavior and always
>   fallback on software blink, I think that that's out of the purpose of
>   the led-class, which should just translate on-off requests to
>   underlaying hardware.

Why, there is already led_tigger_event() and led_trigger_blink(). Why
should led_trigger_blink_once() then do not make sense?

> - I think that different drivers may want to obtain different on-off
>   behavior depending on the application.  For example in the ide-disk
>   case the user expects to see a steady-on LED on constant activity,
>   and that's how it's implemented, while in this case the on-if-up
>   keep-blinking-on-activity off-if-down makes much more sense. So I think
>   that even if a generic blink function were available, people would
>   still be using custom functions because they want to fine tune the
>   behavior for the application.  Also, maybe a function too generic may
>   impact on performance in critical paths.

The blinking could be specified with led_trigger_blink_once().

> - in this case, it looks to me like the implementation is as optimized
>   as it can be, in the sense that the hot-path does really only some
>   essential check and engage the timer and the timer function itself is
>   really short.  Also the final blinking effect is nice IMO :-)

For me it still does make sense to provide a generic
led_trigger_blink_once support. But well, go ahead if I'm the only one
with that opinion.

Wolfgang.

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2726 bytes --]

On 04/25/2012 09:26 AM, Wolfgang Grandegger wrote:
> Hi Fabio,
> 
> On 04/24/2012 09:02 PM, Fabio Baltieri wrote:
>> Hi Wolfgang,
>>
>> On Tue, Apr 24, 2012 at 08:46:29AM +0200, Wolfgang Grandegger wrote:
>>> I still think that the blinking support should go to the timer class to
>>> avoid duplicated code. Any good reason against? Apart from that the
>>> patches look good.
>>>
>>> Wolfgang.
>>
>> I can see you point and I considered your note about adding the
>> one-shot-blink function to the led-class framework (sorry for not
>> mentioning it in my first post).  Still, I ended up with this code for
>> a couple of reasons:
>>
>> - I think that the led_blink_set function is primarily used to configure
>>   leds with hardware blinking (like i2c led drivers).  While it would be
>>   possible to extend the function to get one-shot behavior and always
>>   fallback on software blink, I think that that's out of the purpose of
>>   the led-class, which should just translate on-off requests to
>>   underlaying hardware.
> 
> Why, there is already led_tigger_event() and led_trigger_blink(). Why
> should led_trigger_blink_once() then do not make sense?
> 
>> - I think that different drivers may want to obtain different on-off
>>   behavior depending on the application.  For example in the ide-disk
>>   case the user expects to see a steady-on LED on constant activity,
>>   and that's how it's implemented, while in this case the on-if-up
>>   keep-blinking-on-activity off-if-down makes much more sense. So I think
>>   that even if a generic blink function were available, people would
>>   still be using custom functions because they want to fine tune the
>>   behavior for the application.  Also, maybe a function too generic may
>>   impact on performance in critical paths.
> 
> The blinking could be specified with led_trigger_blink_once().
> 
>> - in this case, it looks to me like the implementation is as optimized
>>   as it can be, in the sense that the hot-path does really only some
>>   essential check and engage the timer and the timer function itself is
>>   really short.  Also the final blinking effect is nice IMO :-)
> 
> For me it still does make sense to provide a generic
> led_trigger_blink_once support. But well, go ahead if I'm the only one
> with that opinion.

+1
At least start a discussion on the LED list (I don't know which list
that is).

Marc


-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Wolfgang Grandegger]

On 04/25/2012 09:41 AM, Marc Kleine-Budde wrote:
> On 04/25/2012 09:26 AM, Wolfgang Grandegger wrote:
>> Hi Fabio,
>>
>> On 04/24/2012 09:02 PM, Fabio Baltieri wrote:
>>> Hi Wolfgang,
>>>
>>> On Tue, Apr 24, 2012 at 08:46:29AM +0200, Wolfgang Grandegger wrote:
>>>> I still think that the blinking support should go to the timer class to
>>>> avoid duplicated code. Any good reason against? Apart from that the
>>>> patches look good.
>>>>
>>>> Wolfgang.
>>>
>>> I can see you point and I considered your note about adding the
>>> one-shot-blink function to the led-class framework (sorry for not
>>> mentioning it in my first post).  Still, I ended up with this code for
>>> a couple of reasons:
>>>
>>> - I think that the led_blink_set function is primarily used to configure
>>>   leds with hardware blinking (like i2c led drivers).  While it would be
>>>   possible to extend the function to get one-shot behavior and always
>>>   fallback on software blink, I think that that's out of the purpose of
>>>   the led-class, which should just translate on-off requests to
>>>   underlaying hardware.
>>
>> Why, there is already led_tigger_event() and led_trigger_blink(). Why
>> should led_trigger_blink_once() then do not make sense?
>>
>>> - I think that different drivers may want to obtain different on-off
>>>   behavior depending on the application.  For example in the ide-disk
>>>   case the user expects to see a steady-on LED on constant activity,
>>>   and that's how it's implemented, while in this case the on-if-up
>>>   keep-blinking-on-activity off-if-down makes much more sense. So I think
>>>   that even if a generic blink function were available, people would
>>>   still be using custom functions because they want to fine tune the
>>>   behavior for the application.  Also, maybe a function too generic may
>>>   impact on performance in critical paths.
>>
>> The blinking could be specified with led_trigger_blink_once().
>>
>>> - in this case, it looks to me like the implementation is as optimized
>>>   as it can be, in the sense that the hot-path does really only some
>>>   essential check and engage the timer and the timer function itself is
>>>   really short.  Also the final blinking effect is nice IMO :-)
>>
>> For me it still does make sense to provide a generic
>> led_trigger_blink_once support. But well, go ahead if I'm the only one
>> with that opinion.
> 
> +1
> At least start a discussion on the LED list (I don't know which list
> that is).

The MAINAINERS file does not list a mailing list, therefore the LKML
should be used, with a CC to Richard Purdie, I think. While checking if
LED support is discussed, I found related mails. One shot timer LED
support seems to be a frequently discussed issue:

  http://marc.info/?t=133489487700001&r=1&w=2
  http://marc.info/?l=linux-kernel&m=133331013422467&w=4

As usual, it's better to send a patch than to ask a (general) questions.
I think we would get rather quick response.

Wolfgang.

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Fabio Baltieri]

Hello everyone,

I'm back on the activity-trigger subject.

On Wed, Apr 25, 2012 at 12:04:56PM +0200, Wolfgang Grandegger wrote:
> >>> On Tue, Apr 24, 2012 at 08:46:29AM +0200, Wolfgang Grandegger wrote:
> >>>> I still think that the blinking support should go to the timer class to
> >>>> avoid duplicated code. Any good reason against? Apart from that the
> >>>> patches look good.
> >>>>
> >> For me it still does make sense to provide a generic
> >> led_trigger_blink_once support. But well, go ahead if I'm the only one
> >> with that opinion.
> > 
> > +1
> > At least start a discussion on the LED list (I don't know which list
> > that is).
> 
> The MAINAINERS file does not list a mailing list, therefore the LKML
> should be used, with a CC to Richard Purdie, I think. While checking if
> LED support is discussed, I found related mails. One shot timer LED
> support seems to be a frequently discussed issue:
> 
>   http://marc.info/?t=133489487700001&r=1&w=2
>   http://marc.info/?l=linux-kernel&m=133331013422467&w=4
> 
> As usual, it's better to send a patch than to ask a (general) questions.
> I think we would get rather quick response.

I spent last couple of days trying to come up with a generic one-shot
blink patch clean enough to get some chance of integration, but what I
ended up with was these considerations in favor of framework-specific
code:

The only clean way I see to do that is to implement the function as an
extension of the actual soft-blink code.

That functions are used as fallback if no hw-blink is available and are
just blindly toggling values on-off, so they have to be modified quite
heavily to add necessary state information to support the behaviour of
the actual code (like when a trigger is added for an already up
interface) and the feature required by other usage cases (like the
trigger currently discussed in the list - which is going in the way of a
trigger-specific implemetation anyway).  This would involve adding flags
and cases in both trigger and timer code.

So, I'm taking a break on this and trying to feed this argument: why the
socketcan code would benefit from a specific implementation rather than
a generic one?

Because overall code is *much* more simple and clean!

- the timer function body is just 12 lines of code, whithout special
  cases, that's faster to execute and easy to understand (and debug).

- the event function for TX and RX paths is just some checks and a
  mod_timer, without list iterations or locking, and as it's going to be
  called in hard-irq context, I think this is a *strong* plus for the
  implementation (the most important thing here is to get out of the way
  as fast as possible, right? A generic implementation would only make
  latencies worse here).

So, my point, do you still think that this feature would benefit of a
generic triggering implementation?

(of course, I'm posting a v3 with just Oliver's Kconfig notes if you
agree with my point :-) )

Regards,
Fabio

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Oliver Hartkopp]

On 03.05.2012 23:49, Fabio Baltieri wrote:

> Hello everyone,
> 
> I'm back on the activity-trigger subject.
> 
> On Wed, Apr 25, 2012 at 12:04:56PM +0200, Wolfgang Grandegger wrote:
>>>>> On Tue, Apr 24, 2012 at 08:46:29AM +0200, Wolfgang Grandegger wrote:
>>>>>> I still think that the blinking support should go to the timer class to
>>>>>> avoid duplicated code. Any good reason against? Apart from that the
>>>>>> patches look good.
>>>>>>
>>>> For me it still does make sense to provide a generic
>>>> led_trigger_blink_once support. But well, go ahead if I'm the only one
>>>> with that opinion.
>>>
>>> +1
>>> At least start a discussion on the LED list (I don't know which list
>>> that is).
>>
>> The MAINAINERS file does not list a mailing list, therefore the LKML
>> should be used, with a CC to Richard Purdie, I think. While checking if
>> LED support is discussed, I found related mails. One shot timer LED
>> support seems to be a frequently discussed issue:
>>
>>   http://marc.info/?t=133489487700001&r=1&w=2
>>   http://marc.info/?l=linux-kernel&m=133331013422467&w=4
>>
>> As usual, it's better to send a patch than to ask a (general) questions.
>> I think we would get rather quick response.
> 
> I spent last couple of days trying to come up with a generic one-shot
> blink patch clean enough to get some chance of integration, but what I
> ended up with was these considerations in favor of framework-specific
> code:
> 
> The only clean way I see to do that is to implement the function as an
> extension of the actual soft-blink code.
> 
> That functions are used as fallback if no hw-blink is available and are
> just blindly toggling values on-off, so they have to be modified quite
> heavily to add necessary state information to support the behaviour of
> the actual code (like when a trigger is added for an already up
> interface) and the feature required by other usage cases (like the
> trigger currently discussed in the list - which is going in the way of a
> trigger-specific implemetation anyway).  This would involve adding flags
> and cases in both trigger and timer code.
> 
> So, I'm taking a break on this and trying to feed this argument: why the
> socketcan code would benefit from a specific implementation rather than
> a generic one?
> 
> Because overall code is *much* more simple and clean!
> 
> - the timer function body is just 12 lines of code, whithout special
>   cases, that's faster to execute and easy to understand (and debug).
> 
> - the event function for TX and RX paths is just some checks and a
>   mod_timer, without list iterations or locking, and as it's going to be
>   called in hard-irq context, I think this is a *strong* plus for the
>   implementation (the most important thing here is to get out of the way
>   as fast as possible, right? A generic implementation would only make
>   latencies worse here).
> 
> So, my point, do you still think that this feature would benefit of a
> generic triggering implementation?
> 
> (of course, I'm posting a v3 with just Oliver's Kconfig notes if you
> agree with my point :-) )
> 


Hi Fabio,

as the former implementation was fine to me there's no need to come up with
additional remarks for me ... btw. i think your implementation is clean,
simple and efficient. Therefore i would vote for it too.

I don't know if we should guide anyone to implement a generic led blinking
framework covering all the (existing) cases in the kernel. Adding your patches
would probably make the creation of such a framework more challenging to a
volunteer in terms of creating a solution which is that simple and efficient.

just my two cents ...

Regards,
Oliver

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Wolfgang Grandegger]

Hi Fabio,

On 05/03/2012 11:49 PM, Fabio Baltieri wrote:
> Hello everyone,
> 
> I'm back on the activity-trigger subject.
> 
> On Wed, Apr 25, 2012 at 12:04:56PM +0200, Wolfgang Grandegger wrote:
>>>>> On Tue, Apr 24, 2012 at 08:46:29AM +0200, Wolfgang Grandegger wrote:
>>>>>> I still think that the blinking support should go to the timer class to
>>>>>> avoid duplicated code. Any good reason against? Apart from that the
>>>>>> patches look good.
>>>>>>
>>>> For me it still does make sense to provide a generic
>>>> led_trigger_blink_once support. But well, go ahead if I'm the only one
>>>> with that opinion.

You mean it does not make sense because it's difficult to implement? At
least it sounds like.

>>> +1
>>> At least start a discussion on the LED list (I don't know which list
>>> that is).
>>
>> The MAINAINERS file does not list a mailing list, therefore the LKML
>> should be used, with a CC to Richard Purdie, I think. While checking if
>> LED support is discussed, I found related mails. One shot timer LED
>> support seems to be a frequently discussed issue:
>>
>>   http://marc.info/?t=133489487700001&r=1&w=2
>>   http://marc.info/?l=linux-kernel&m=133331013422467&w=4
>>
>> As usual, it's better to send a patch than to ask a (general) questions.
>> I think we would get rather quick response.
> 
> I spent last couple of days trying to come up with a generic one-shot
> blink patch clean enough to get some chance of integration, but what I
> ended up with was these considerations in favor of framework-specific
> code:
> 
> The only clean way I see to do that is to implement the function as an
> extension of the actual soft-blink code.

Yes, hardware support seems not to make sense.

> That functions are used as fallback if no hw-blink is available and are
> just blindly toggling values on-off, so they have to be modified quite
> heavily to add necessary state information to support the behaviour of
> the actual code (like when a trigger is added for an already up
> interface) and the feature required by other usage cases (like the
> trigger currently discussed in the list - which is going in the way of a
> trigger-specific implemetation anyway).  This would involve adding flags
> and cases in both trigger and timer code.
> 
> So, I'm taking a break on this and trying to feed this argument: why the
> socketcan code would benefit from a specific implementation rather than
> a generic one?

The kernel will benefit from a generic implementation, I believe.

> Because overall code is *much* more simple and clean!
> 
> - the timer function body is just 12 lines of code, whithout special
>   cases, that's faster to execute and easy to understand (and debug).

That's also true for a LED class based implementation.

> - the event function for TX and RX paths is just some checks and a
>   mod_timer, without list iterations or locking, and as it's going to be
>   called in hard-irq context, I think this is a *strong* plus for the
>   implementation (the most important thing here is to get out of the way
>   as fast as possible, right? A generic implementation would only make
>   latencies worse here).

I don't think so, at least not for a simple software driven one-shot
blinking similar to yours.

> So, my point, do you still think that this feature would benefit of a
> generic triggering implementation?

Yes. What I would do is to prepare a simple software driven one-shot
blinking similar to your existing code and similar to led_trigger_blink.
Send the patch to the LKML and see how the community and the maintainer
react. I think there is no hurry to get LED blinking supported by the
mainline kernel.

Wolfgang.

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Kurt Van Dijck]

On Mon, Apr 23, 2012 at 11:02:45PM +0200, Fabio Baltieri wrote:
> This patch implements the functions to add two LED triggers, named
> <ifname>-tx and <ifname>-rx, to a canbus device driver.
> 
[...]

If I understand well, this function is subject to maximal optimization.
> +void can_led_event(struct net_device *netdev, enum can_led_event event)
> +{
> +	struct can_priv *priv = netdev_priv(netdev);
If the assignment is postponed to withing the differenct cases, thereby
repeating this statement 3 times (not 4!), both for RX & TX path a single
assignment could be saved.
For stack usage, I'm not an expert, but I suspect a little less stack use.
I'm not sure wether it's worth the pain of extra lines of code.
I tried to illustrate my point here:
> +++	struct can_led_data *tx_led, *rx_led;
> +
> +	switch (event) {
> +	case CAN_LED_EVENT_OPEN:
> +++		tx_led = priv->tx_led_data;
> +		if (likely(tx_led))
> +			led_trigger_event(&tx_led->trig, LED_FULL);
> +++		rx_led = priv->rx_led_data;
> +		if (likely(rx_led))
> +			led_trigger_event(&rx_led->trig, LED_FULL);
> +		break;
> +	case CAN_LED_EVENT_STOP:
> +++		tx_led = priv->tx_led_data;
> +		if (likely(tx_led)) {
> +			del_timer_sync(&tx_led->timer);
> +			led_trigger_event(&tx_led->trig, LED_OFF);
> +		}
> +++		rx_led = priv->rx_led_data;
> +		if (likely(rx_led)) {
> +			del_timer_sync(&rx_led->timer);
> +			led_trigger_event(&rx_led->trig, LED_OFF);
> +		}
> +		break;
> +	case CAN_LED_EVENT_TX:
> +++		tx_led = priv->tx_led_data;
> +		if (led_delay && likely(tx_led) &&
> +		    !timer_pending(&tx_led->timer))
> +			mod_timer(&tx_led->timer,
> +				jiffies + msecs_to_jiffies(led_delay));
> +		break;
> +	case CAN_LED_EVENT_RX:
> +++		rx_led = priv->rx_led_data;
> +		if (led_delay && likely(rx_led) &&
> +		    !timer_pending(&rx_led->timer))
> +			mod_timer(&rx_led->timer,
> +				jiffies + msecs_to_jiffies(led_delay));
> +		break;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(can_led_event);

Apart from this tiny question, the patch looks very clean, as Wolfgang already
mentioned.

Kind regards,
Kurt

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Fabio Baltieri]

On Tue, Apr 24, 2012 at 10:38:06AM +0200, Kurt Van Dijck wrote:
> On Mon, Apr 23, 2012 at 11:02:45PM +0200, Fabio Baltieri wrote:
> > This patch implements the functions to add two LED triggers, named
> > <ifname>-tx and <ifname>-rx, to a canbus device driver.
> > 
> [...]

Hello Kurt,

> If I understand well, this function is subject to maximal optimization.
> > +void can_led_event(struct net_device *netdev, enum can_led_event event)
> > +{
> > +	struct can_priv *priv = netdev_priv(netdev);
> If the assignment is postponed to withing the differenct cases, thereby
> repeating this statement 3 times (not 4!), both for RX & TX path a single
> assignment could be saved.
> For stack usage, I'm not an expert, but I suspect a little less stack use.
> I'm not sure wether it's worth the pain of extra lines of code.
> I tried to illustrate my point here:

I'm looking at the disassembled function for both cases on ARM.  Your
version uses some less instructions but I think the optimizer is
crunching the whoole thing in some funny way and I'm not sure if it's
going to make a difference at all in the end.

Do you know if there are any other part of the kernel which are using
similar optimiziations?

> > +++	struct can_led_data *tx_led, *rx_led;
> > +
> > +	switch (event) {
> > +	case CAN_LED_EVENT_OPEN:
> > +++		tx_led = priv->tx_led_data;
> > +		if (likely(tx_led))
> > +			led_trigger_event(&tx_led->trig, LED_FULL);
> > +++		rx_led = priv->rx_led_data;
> > +		if (likely(rx_led))
> > +			led_trigger_event(&rx_led->trig, LED_FULL);
> > +		break;
> > +	case CAN_LED_EVENT_STOP:
> > +++		tx_led = priv->tx_led_data;
> > +		if (likely(tx_led)) {
> > +			del_timer_sync(&tx_led->timer);
> > +			led_trigger_event(&tx_led->trig, LED_OFF);
> > +		}
> > +++		rx_led = priv->rx_led_data;
> > +		if (likely(rx_led)) {
> > +			del_timer_sync(&rx_led->timer);
> > +			led_trigger_event(&rx_led->trig, LED_OFF);
> > +		}
> > +		break;
> > +	case CAN_LED_EVENT_TX:
> > +++		tx_led = priv->tx_led_data;
> > +		if (led_delay && likely(tx_led) &&
> > +		    !timer_pending(&tx_led->timer))
> > +			mod_timer(&tx_led->timer,
> > +				jiffies + msecs_to_jiffies(led_delay));
> > +		break;
> > +	case CAN_LED_EVENT_RX:
> > +++		rx_led = priv->rx_led_data;
> > +		if (led_delay && likely(rx_led) &&
> > +		    !timer_pending(&rx_led->timer))
> > +			mod_timer(&rx_led->timer,
> > +				jiffies + msecs_to_jiffies(led_delay));
> > +		break;
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(can_led_event);
> 
> Apart from this tiny question, the patch looks very clean, as Wolfgang already
> mentioned.

Thanks.

Regards,
Fabio

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Kurt Van Dijck]

On Tue, Apr 24, 2012 at 10:22:00PM +0200, Fabio Baltieri wrote:
> On Tue, Apr 24, 2012 at 10:38:06AM +0200, Kurt Van Dijck wrote:
> > On Mon, Apr 23, 2012 at 11:02:45PM +0200, Fabio Baltieri wrote:
> > > This patch implements the functions to add two LED triggers, named
> > > <ifname>-tx and <ifname>-rx, to a canbus device driver.
> > > 
> > [...]
> 
> Hello Kurt,
> 
> > If I understand well, this function is subject to maximal optimization.
> > > +void can_led_event(struct net_device *netdev, enum can_led_event event)
> > > +{
> > > +	struct can_priv *priv = netdev_priv(netdev);
> > If the assignment is postponed to withing the differenct cases, thereby
> > repeating this statement 3 times (not 4!), both for RX & TX path a single
> > assignment could be saved.
> > For stack usage, I'm not an expert, but I suspect a little less stack use.
> > I'm not sure wether it's worth the pain of extra lines of code.
> > I tried to illustrate my point here:
> 
> I'm looking at the disassembled function for both cases on ARM.  Your
> version uses some less instructions but I think the optimizer is
> crunching the whoole thing in some funny way and I'm not sure if it's
> going to make a difference at all in the end.

If the benefit cannot be proven, I'd stick to the original, as the code
is much cleaner :-)

> Do you know if there are any other part of the kernel which are using
> similar optimiziations?

No, I'm not an expert on code optimization ...

Kurt

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Kurt Van Dijck]

On Mon, Apr 23, 2012 at 11:02:45PM +0200, Fabio Baltieri wrote:
> This patch implements the functions to add two LED triggers, named
> <ifname>-tx and <ifname>-rx, to a canbus device driver.
> 

> +void can_led_event(struct net_device *netdev, enum can_led_event event)

> +	case CAN_LED_EVENT_TX:
> +		if (led_delay && likely(tx_led) &&
> +		    !timer_pending(&tx_led->timer))
> +			mod_timer(&tx_led->timer,
I think that the latter 2 statements need threadsafe protection
_OR_ strict serialization from the caller (much more likely).
I don't think the code needs change, but it might be worth mentioning
this serialization requirement in the header.

Kind regards,
Kurt

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Fabio Baltieri]

On Tue, Apr 24, 2012 at 10:45:16AM +0200, Kurt Van Dijck wrote:
> On Mon, Apr 23, 2012 at 11:02:45PM +0200, Fabio Baltieri wrote:
> > This patch implements the functions to add two LED triggers, named
> > <ifname>-tx and <ifname>-rx, to a canbus device driver.
> > 
> 
> > +void can_led_event(struct net_device *netdev, enum can_led_event event)
> 
> > +	case CAN_LED_EVENT_TX:
> > +		if (led_delay && likely(tx_led) &&
> > +		    !timer_pending(&tx_led->timer))
> > +			mod_timer(&tx_led->timer,
> I think that the latter 2 statements need threadsafe protection
> _OR_ strict serialization from the caller (much more likely).
> I don't think the code needs change, but it might be worth mentioning
> this serialization requirement in the header.

Sure? Each device is going to have individual timer structures, and
each timer_list is going to be accessed only from the event function and
by the timer handler itself for retriggering.

Also, mod_timer() by itself should be safe.

http://lxr.free-electrons.com/source/kernel/timer.c#L833

Regards,
Fabio

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Kurt Van Dijck]

On Tue, Apr 24, 2012 at 10:34:57PM +0200, Fabio Baltieri wrote:
> On Tue, Apr 24, 2012 at 10:45:16AM +0200, Kurt Van Dijck wrote:
> > On Mon, Apr 23, 2012 at 11:02:45PM +0200, Fabio Baltieri wrote:
> > > This patch implements the functions to add two LED triggers, named
> > > <ifname>-tx and <ifname>-rx, to a canbus device driver.
> > > 
> > 
> > > +void can_led_event(struct net_device *netdev, enum can_led_event event)
> > 
> > > +	case CAN_LED_EVENT_TX:
> > > +		if (led_delay && likely(tx_led) &&
> > > +		    !timer_pending(&tx_led->timer))
> > > +			mod_timer(&tx_led->timer,
> > I think that the latter 2 statements need threadsafe protection
> > _OR_ strict serialization from the caller (much more likely).
> > I don't think the code needs change, but it might be worth mentioning
> > this serialization requirement in the header.
> 
> Sure? Each device is going to have individual timer structures, and
> each timer_list is going to be accessed only from the event function and
> by the timer handler itself for retriggering.
> 
> Also, mod_timer() by itself should be safe.

_between_ timer_pending() & mod_timer(), which are both
safe I assume, an interrupt may happen.
In such case, it's important that can_led_event(CAN_LED_EVENT_TX)
is only called from either some IRQ function, or the network subsystem's softirq,
but NOT from both places.
Any good design will be compliant to this.
Since this depends on actual drivers, I think it may be good
to avoid bad implimentations by mentioning this driver
requirement in the header ...

Kurt

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Fabio Baltieri]

On Wed, Apr 25, 2012 at 10:00:04AM +0200, Kurt Van Dijck wrote:
> On Tue, Apr 24, 2012 at 10:34:57PM +0200, Fabio Baltieri wrote:
> > On Tue, Apr 24, 2012 at 10:45:16AM +0200, Kurt Van Dijck wrote:
> > > On Mon, Apr 23, 2012 at 11:02:45PM +0200, Fabio Baltieri wrote:
> > > > This patch implements the functions to add two LED triggers, named
> > > > <ifname>-tx and <ifname>-rx, to a canbus device driver.
> > > > 
> > > 
> > > > +void can_led_event(struct net_device *netdev, enum can_led_event event)
> > > 
> > > > +	case CAN_LED_EVENT_TX:
> > > > +		if (led_delay && likely(tx_led) &&
> > > > +		    !timer_pending(&tx_led->timer))
> > > > +			mod_timer(&tx_led->timer,
> > > I think that the latter 2 statements need threadsafe protection
> > > _OR_ strict serialization from the caller (much more likely).
> > > I don't think the code needs change, but it might be worth mentioning
> > > this serialization requirement in the header.
> > 
> > Sure? Each device is going to have individual timer structures, and
> > each timer_list is going to be accessed only from the event function and
> > by the timer handler itself for retriggering.
> > 
> > Also, mod_timer() by itself should be safe.
> 
> _between_ timer_pending() & mod_timer(), which are both
> safe I assume, an interrupt may happen.
> In such case, it's important that can_led_event(CAN_LED_EVENT_TX)
> is only called from either some IRQ function, or the network subsystem's softirq,
> but NOT from both places.

Ok - but I can't think of a case where you would call this one outside
hardirq/softirq context.  Either way, I think that the only thing which
could happen without locks is to schedule the timer two times, which as
I see from mod_timer implementation is already optimized out, so I think
that no locking is needed on this one. Am I missing something?

Regards,
Fabio

Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Kurt Van Dijck]

On Wed, Apr 25, 2012 at 10:39:48PM +0200, Fabio Baltieri wrote:
> On Wed, Apr 25, 2012 at 10:00:04AM +0200, Kurt Van Dijck wrote:
> > On Tue, Apr 24, 2012 at 10:34:57PM +0200, Fabio Baltieri wrote:
> > > On Tue, Apr 24, 2012 at 10:45:16AM +0200, Kurt Van Dijck wrote:
> > > > On Mon, Apr 23, 2012 at 11:02:45PM +0200, Fabio Baltieri wrote:
> > > > > This patch implements the functions to add two LED triggers, named
> > > > > <ifname>-tx and <ifname>-rx, to a canbus device driver.
> > > > > 
> > > > 
> > > > > +void can_led_event(struct net_device *netdev, enum can_led_event event)
> > > > 
> > > > > +	case CAN_LED_EVENT_TX:
> > > > > +		if (led_delay && likely(tx_led) &&
> > > > > +		    !timer_pending(&tx_led->timer))
> > > > > +			mod_timer(&tx_led->timer,
> > > > I think that the latter 2 statements need threadsafe protection
> > > > _OR_ strict serialization from the caller (much more likely).
> > > > I don't think the code needs change, but it might be worth mentioning
> > > > this serialization requirement in the header.
> > > 
> > > Sure? Each device is going to have individual timer structures, and
> > > each timer_list is going to be accessed only from the event function and
> > > by the timer handler itself for retriggering.
> > > 
> > > Also, mod_timer() by itself should be safe.
> > 
> > _between_ timer_pending() & mod_timer(), which are both
> > safe I assume, an interrupt may happen.
> > In such case, it's important that can_led_event(CAN_LED_EVENT_TX)
> > is only called from either some IRQ function, or the network subsystem's softirq,
> > but NOT from both places.
> 
> Ok - but I can't think of a case where you would call this one outside
> hardirq/softirq context.  Either way, I think that the only thing which
> could happen without locks is to schedule the timer two times, which as
> I see from mod_timer implementation is already optimized out, so I think
> that no locking is needed on this one.

I neither think that locking is needed, for reasons you also mentioned.
I did think the proposed way of using can_led_event() is worth describing
in the header, so that it's clear that no locking is needed.
But maybe I'm mistaken here...

> Am I missing something?
no.

Kind regards,
Kurt