[RFC PATCH] can: add tx/rx led trigger support

[RFC PATCH] can: add tx/rx led trigger support

[Fabio Baltieri]

This patch adds two led triggers, named <ifname>-tx and <ifname>-rx to
each registered canbus interface.

Triggers are called from can_send() and can_rcv() functions in af_can.h,
and can be disabled with a Kconfig option.

The implementation lights up the LED when a packet is transmitted or
received and turn it off after a configurable time using a timer.

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

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

this is a try to add generic tx/rx LED triggers for canbus interfaces, somthing
I think is very useful in embedded systems where canbus devices often have
status leds associated with them, maybe on the bus connector itself, like
ethernet interfaces. I saw a couple of hardware implementation to drive status
leds from tx/rx lines but these were not as effective as software ones.

The implementation is similar to the MAC80211_LEDS one, and takes quite a lot
of inspiration and some code from it.

In this case, however, tx and rx events are trapped from the af_can source file
as there are no generic tx/rx functions in can/dev.c. This also required an
additional counter to discard rx events for looped frames.

Also, as all the support data are in the can_priv structure, this only supports
can-dev based drivers. All the others are ignored by checking the
netdev->rtnl_link_ops->kind string. This actually excludes only vcan and slcan
drivers.

The implementation should be quite unintrusive on existing code and can be disabled
altogether with a config option.

This has been tested tested on x86 and a powerpc with a custom USB-CAN
interface (which I hope to publish as open-hardware soon BTW) and i2c-based
leds.

Any thoughts? Do you think this can be merged?

Thanks,
Fabio


 include/linux/can/dev.h |    9 +++
 net/can/Kconfig         |   10 +++
 net/can/Makefile        |    2 +
 net/can/af_can.c        |   14 ++++-
 net/can/led.c           |  154 +++++++++++++++++++++++++++++++++++++++++++++++
 net/can/led.h           |   29 +++++++++
 6 files changed, 217 insertions(+), 1 deletions(-)
 create mode 100644 net/can/led.c
 create mode 100644 net/can/led.h

diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 5d2efe7..76eb70c 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -16,6 +16,8 @@
 #include <linux/can.h>
 #include <linux/can/netlink.h>
 #include <linux/can/error.h>
+#include <linux/timer.h>
+#include <linux/leds.h>
 
 /*
  * CAN mode
@@ -52,6 +54,13 @@ struct can_priv {
 
 	unsigned int echo_skb_max;
 	struct sk_buff **echo_skb;
+
+#ifdef CONFIG_CAN_LEDS
+	struct timer_list tx_off_timer, rx_off_timer;
+	struct led_trigger *tx_led, *rx_led;
+	char tx_led_name[32], rx_led_name[32];
+	atomic_t led_discard_count;
+#endif
 };
 
 /*
diff --git a/net/can/Kconfig b/net/can/Kconfig
index 0320069..55894b7 100644
--- a/net/can/Kconfig
+++ b/net/can/Kconfig
@@ -52,4 +52,14 @@ config CAN_GW
 	  They can be modified with AND/OR/XOR/SET operations as configured
 	  by the netlink configuration interface known e.g. from iptables.
 
+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 enables two LED triggers for packet receive and transmit
+	  events on each CAN device based on the can-dev framework.
+
 source "drivers/net/can/Kconfig"
diff --git a/net/can/Makefile b/net/can/Makefile
index cef49eb..fc6b286 100644
--- a/net/can/Makefile
+++ b/net/can/Makefile
@@ -5,6 +5,8 @@
 obj-$(CONFIG_CAN)	+= can.o
 can-y			:= af_can.o proc.o
 
+can-$(CONFIG_CAN_LEDS)	+= led.o
+
 obj-$(CONFIG_CAN_RAW)	+= can-raw.o
 can-raw-y		:= raw.o
 
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 0ce2ad0..e0c17cf 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -61,6 +61,7 @@
 #include <net/sock.h>
 
 #include "af_can.h"
+#include "led.h"
 
 static __initdata const char banner[] = KERN_INFO
 	"can: controller area network core (" CAN_VERSION_STRING ")\n";
@@ -282,6 +283,8 @@ int can_send(struct sk_buff *skb, int loop)
 		skb->pkt_type = PACKET_HOST;
 	}
 
+	can_led_tx(skb->dev, loop);
+
 	/* send to netdevice */
 	err = dev_queue_xmit(skb);
 	if (err > 0)
@@ -674,6 +677,8 @@ static int can_rcv(struct sk_buff *skb, struct net_device *dev,
 		can_stats.matches_delta++;
 	}
 
+	can_led_rx(dev);
+
 	return NET_RX_SUCCESS;
 
 drop:
@@ -776,6 +781,8 @@ static int can_notifier(struct notifier_block *nb, unsigned long msg,
 		BUG_ON(dev->ml_priv);
 		dev->ml_priv = d;
 
+		can_led_init(dev);
+
 		break;
 
 	case NETDEV_UNREGISTER:
@@ -795,6 +802,8 @@ static int can_notifier(struct notifier_block *nb, unsigned long msg,
 
 		spin_unlock(&can_rcvlists_lock);
 
+		can_led_free(dev);
+
 		break;
 	}
 
@@ -867,7 +876,7 @@ static __exit void can_exit(void)
 	/* remove created dev_rcv_lists from still registered CAN devices */
 	rcu_read_lock();
 	for_each_netdev_rcu(&init_net, dev) {
-		if (dev->type == ARPHRD_CAN && dev->ml_priv){
+		if (dev->type == ARPHRD_CAN && dev->ml_priv) {
 
 			struct dev_rcv_lists *d = dev->ml_priv;
 
@@ -875,6 +884,9 @@ static __exit void can_exit(void)
 			kfree(d);
 			dev->ml_priv = NULL;
 		}
+
+		if (dev->type == ARPHRD_CAN)
+			can_led_free(dev);
 	}
 	rcu_read_unlock();
 
diff --git a/net/can/led.c b/net/can/led.c
new file mode 100644
index 0000000..33e34e8
--- /dev/null
+++ b/net/can/led.c
@@ -0,0 +1,154 @@
+/*
+ * Copyright 2012, Fabio Baltieri <fabio.baltieri@gmail.com>
+ *
+ * Implementation inspired by mac80211_leds driver.
+ *
+ * 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/slab.h>
+#include <linux/netdevice.h>
+#include <linux/can/dev.h>
+#include <linux/timer.h>
+
+#include <net/rtnetlink.h>
+
+#include "led.h"
+
+static int led_off_delay = 20;
+module_param(led_off_delay, int, 0644);
+MODULE_PARM_DESC(led_off_delay,
+		"turn-off delay time for activity leds (msecs, default: 20).");
+
+static void tx_off_function(unsigned long data)
+{
+	struct can_priv *priv = (struct can_priv *)data;
+
+	led_trigger_event(priv->tx_led, LED_OFF);
+}
+
+static void rx_off_function(unsigned long data)
+{
+	struct can_priv *priv = (struct can_priv *)data;
+
+	led_trigger_event(priv->rx_led, LED_OFF);
+}
+
+/* This is used to ignore devices not based on the can-dev framework */
+static int rtnl_link_kind_is(struct net_device *netdev, const char *kind)
+{
+	if (netdev->rtnl_link_ops &&
+		strncmp(kind, netdev->rtnl_link_ops->kind, strlen(kind)) == 0)
+		return 1;
+	else
+		return 0;
+}
+
+void can_led_tx(struct net_device *netdev, int loop)
+{
+	struct can_priv *priv = netdev_priv(netdev);
+
+	if (!rtnl_link_kind_is(netdev, "can"))
+		return;
+
+	if (unlikely(!priv->tx_led))
+		return;
+
+	if (!timer_pending(&priv->tx_off_timer)) {
+		led_trigger_event(priv->tx_led, LED_FULL);
+
+		mod_timer(&priv->tx_off_timer,
+			jiffies + msecs_to_jiffies(led_off_delay));
+	}
+
+	if (loop)
+		atomic_dec(&priv->led_discard_count);
+}
+
+void can_led_rx(struct net_device *netdev)
+{
+	struct can_priv *priv = netdev_priv(netdev);
+
+	if (!rtnl_link_kind_is(netdev, "can"))
+		return;
+
+	if (unlikely(!priv->rx_led))
+		return;
+
+	/* discard echoed packets */
+	if (atomic_inc_not_zero(&priv->led_discard_count))
+		return;
+
+	if (!timer_pending(&priv->rx_off_timer)) {
+		led_trigger_event(priv->rx_led, LED_FULL);
+
+		mod_timer(&priv->rx_off_timer,
+			jiffies + msecs_to_jiffies(led_off_delay));
+	}
+}
+
+void can_led_init(struct net_device *netdev)
+{
+	struct can_priv *priv = netdev_priv(netdev);
+
+	if (!rtnl_link_kind_is(netdev, "can"))
+		return;
+
+	snprintf(priv->tx_led_name, sizeof(priv->tx_led_name),
+		"%s-tx", netdev->name);
+	snprintf(priv->rx_led_name, sizeof(priv->rx_led_name),
+		"%s-rx", netdev->name);
+
+	priv->tx_led = kzalloc(sizeof(struct led_trigger), GFP_KERNEL);
+	if (priv->tx_led) {
+		priv->tx_led->name = priv->tx_led_name;
+		if (led_trigger_register(priv->tx_led)) {
+			kfree(priv->tx_led);
+			priv->tx_led = NULL;
+		}
+	}
+
+	priv->rx_led = kzalloc(sizeof(struct led_trigger), GFP_KERNEL);
+	if (priv->rx_led) {
+		priv->rx_led->name = priv->rx_led_name;
+		if (led_trigger_register(priv->rx_led)) {
+			kfree(priv->rx_led);
+			priv->rx_led = NULL;
+		}
+	}
+
+	if (priv->tx_led)
+		setup_timer(&priv->tx_off_timer, tx_off_function,
+			(unsigned long)priv);
+
+	if (priv->rx_led)
+		setup_timer(&priv->rx_off_timer, rx_off_function,
+			(unsigned long)priv);
+
+	atomic_set(&priv->led_discard_count, 0);
+}
+
+void can_led_free(struct net_device *netdev)
+{
+	struct can_priv *priv = netdev_priv(netdev);
+
+	if (!rtnl_link_kind_is(netdev, "can"))
+		return;
+
+	if (priv->tx_led) {
+		del_timer_sync(&priv->tx_off_timer);
+
+		led_trigger_unregister(priv->tx_led);
+		kfree(priv->tx_led);
+	}
+
+	if (priv->rx_led) {
+		del_timer_sync(&priv->rx_off_timer);
+
+		led_trigger_unregister(priv->rx_led);
+		kfree(priv->rx_led);
+	}
+}
diff --git a/net/can/led.h b/net/can/led.h
new file mode 100644
index 0000000..d55151c
--- /dev/null
+++ b/net/can/led.h
@@ -0,0 +1,29 @@
+/*
+ * 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/leds.h>
+
+#ifdef CONFIG_CAN_LEDS
+void can_led_tx(struct net_device *netdev, int loop);
+void can_led_rx(struct net_device *netdev);
+void can_led_init(struct net_device *netdev);
+void can_led_free(struct net_device *netdev);
+#else
+static inline void can_led_tx(struct net_device *netdev, int loop)
+{
+}
+static inline void can_led_rx(struct net_device *netdev)
+{
+}
+static inline void can_led_init(struct net_device *netdev)
+{
+}
+static inline void can_led_free(struct net_device *netdev)
+{
+}
+#endif
-- 
1.7.5.1

Re: [RFC PATCH] can: add tx/rx led trigger support

[Oliver Hartkopp]

Hi Fabio,

at first i REALLY LIKE blinkenlights :-)

According to your implementation i have some remarks:

On 10.04.2012 23:39, Fabio Baltieri wrote:

> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 5d2efe7..76eb70c 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -16,6 +16,8 @@
>  #include <linux/can.h>
>  #include <linux/can/netlink.h>
>  #include <linux/can/error.h>
> +#include <linux/timer.h>
> +#include <linux/leds.h>
>  
>  /*
>   * CAN mode
> @@ -52,6 +54,13 @@ struct can_priv {
>  
>  	unsigned int echo_skb_max;
>  	struct sk_buff **echo_skb;
> +
> +#ifdef CONFIG_CAN_LEDS
> +	struct timer_list tx_off_timer, rx_off_timer;
> +	struct led_trigger *tx_led, *rx_led;
> +	char tx_led_name[32], rx_led_name[32];
> +	atomic_t led_discard_count;
> +#endif
>  };


Why don't you add struct led_trigger directly here, as it makes the two
kzalloc calls in  can_led_init() obsolete?

Struct can_priv is allocated anyway and therefore you do not take care of the
removal of your privately allocated memory.

Now looking into the tx path LEDs as an example:

> +/* This is used to ignore devices not based on the can-dev framework */
> +static int rtnl_link_kind_is(struct net_device *netdev, const char *kind)
> +{
> +	if (netdev->rtnl_link_ops &&
> +		strncmp(kind, netdev->rtnl_link_ops->kind, strlen(kind)) == 0)
> +		return 1;
> +	else
> +		return 0;
> +}


You do this costly compare with *each* tx/rx CAN frame in the hot path.

Not very nice :-(

> +
> +void can_led_tx(struct net_device *netdev, int loop)
> +{
> +	struct can_priv *priv = netdev_priv(netdev);
> +
> +	if (!rtnl_link_kind_is(netdev, "can"))
> +		return;


See here.

> +
> +	if (unlikely(!priv->tx_led))
> +		return;
> +
> +	if (!timer_pending(&priv->tx_off_timer)) {
> +		led_trigger_event(priv->tx_led, LED_FULL);
> +
> +		mod_timer(&priv->tx_off_timer,
> +			jiffies + msecs_to_jiffies(led_off_delay));
> +	}
> +
> +	if (loop)
> +		atomic_dec(&priv->led_discard_count);
> +}
> +


As you pointed out that only CAN devices are supported that use the can-dev
infrastructure you do some costly checks one layer above (in net/can) instead
of implementing it on the driver layer (in drivers/net/can).

The only correct trigger for a tx-LED can be retrieved from the TX done
interrupt in the interrupt function like here:

http://lxr.linux.no/#linux+v3.3.1/drivers/net/can/sja1000/sja1000.c#L507

where the echo skb is sent back to the system.

This makes sure, that the tx-LED is only triggered, when there was a really
successful transmission on the CAN bus.

I wonder if it makes more sense to add the LED trigger stuff into
drivers/net/can/dev.c ?!?

Of course the correct calling points to trigger the rx/tx-LEDs need to be
implemented inside *each* driver at the correct position.

And we should probably add a new CAN netlink command here

	http://lxr.linux.no/#linux+v3.3.1/drivers/net/can/dev.c#L577

to configure the LED timer per interface: 0 -> LEDs off (==default)

The question would be, if this code

> +	if (!timer_pending(&priv->tx_off_timer)) {
> +		led_trigger_event(priv->tx_led, LED_FULL);
> +
> +		mod_timer(&priv->tx_off_timer,
> +			jiffies + msecs_to_jiffies(led_off_delay));
> +	}


is ready to be used in hard IRQ context?

If not we may use a construct with a tasklet that can be triggered from hard
IRQ context, like here:

	http://lxr.linux.no/#linux+v3.3.1/net/can/bcm.c#L356

I think this part

> +
> +	if (loop)
> +		atomic_dec(&priv->led_discard_count);
> +}

is becoming obsolete anyway then, right?

Regards,
Oliver

Re: [RFC PATCH] can: add tx/rx led trigger support

[Alexander Stein]

Am Dienstag, 10. April 2012, 23:39:25 schrieb Fabio Baltieri:
> This patch adds two led triggers, named <ifname>-tx and <ifname>-rx to
> each registered canbus interface.

Does this mean, you can have either a tx or rx trigger, bot not both? The 
status LEDs I know from embedded system are triggered by both rx and tx.
Despite that, nice idea.

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
August-Bebel-Str. 29
D-07973 Greiz

Tel: +49-3661-6279-0, Fax: +49-3661-6279-99
eMail:    Alexander.Stein@systec-electronic.com
Internet: http://www.systec-electronic.com

Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Jena, HRB 205563

Re: [RFC PATCH] can: add tx/rx led trigger support

[Wolfgang Grandegger]

Hi Fabio,

On 04/10/2012 11:39 PM, Fabio Baltieri wrote:
> This patch adds two led triggers, named <ifname>-tx and <ifname>-rx to
> each registered canbus interface.
> 
> Triggers are called from can_send() and can_rcv() functions in af_can.h,
> and can be disabled with a Kconfig option.
> 
> The implementation lights up the LED when a packet is transmitted or
> received and turn it off after a configurable time using a timer.
> 
> This only supports can-dev based drivers, as it uses some support field
> in the can_priv structure.
> 
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> ---
> Hi all,
> 
> this is a try to add generic tx/rx LED triggers for canbus interfaces, somthing
> I think is very useful in embedded systems where canbus devices often have
> status leds associated with them, maybe on the bus connector itself, like

Please define "often"? So far only a few CAN devices need LED control by
software. Actually, I only remember the PEAK PCMCIA card.

> ethernet interfaces. I saw a couple of hardware implementation to drive status
> leds from tx/rx lines but these were not as effective as software ones.

Why? LEDs are ususally controlled by software only if the hardware does
not do it (properly?). But your LED support cannot be compared with the
LEDs on the CAN devices itself. It will allow to trigger user-defined
LEDs by RX or TX activity, right? ... which might be useful especially
for debugging, I assume.

> The implementation is similar to the MAC80211_LEDS one, and takes quite a lot
> of inspiration and some code from it.
> 
> In this case, however, tx and rx events are trapped from the af_can source file
> as there are no generic tx/rx functions in can/dev.c. This also required an
> additional counter to discard rx events for looped frames.

Well, controlling the LEDs from the protocol level seems wrong to me. If
at all, it should be handled by the CAN device interface.

> Also, as all the support data are in the can_priv structure, this only supports
> can-dev based drivers. All the others are ignored by checking the
> netdev->rtnl_link_ops->kind string. This actually excludes only vcan and slcan
> drivers.
> 
> The implementation should be quite unintrusive on existing code and can be disabled
> altogether with a config option.

I find it rather intrusive, especially when #ifdef's should be avoided.
Furthermore it introduces overhead in the RX and TX path and uses a lot
of space in the "struct can_priv".

> This has been tested tested on x86 and a powerpc with a custom USB-CAN
> interface (which I hope to publish as open-hardware soon BTW) and i2c-based
> leds.
> 
> Any thoughts? Do you think this can be merged?

LED interface might be useful, especially for debugging. Why not
enabling it just if "CONFIG_CAN_DEBUG_DEVICES=y". Also LEDs for error
states would make sense.

Some more comments inline...

>  include/linux/can/dev.h |    9 +++
>  net/can/Kconfig         |   10 +++
>  net/can/Makefile        |    2 +
>  net/can/af_can.c        |   14 ++++-
>  net/can/led.c           |  154 +++++++++++++++++++++++++++++++++++++++++++++++
>  net/can/led.h           |   29 +++++++++
>  6 files changed, 217 insertions(+), 1 deletions(-)
>  create mode 100644 net/can/led.c
>  create mode 100644 net/can/led.h
> 
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 5d2efe7..76eb70c 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -16,6 +16,8 @@
>  #include <linux/can.h>
>  #include <linux/can/netlink.h>
>  #include <linux/can/error.h>
> +#include <linux/timer.h>
> +#include <linux/leds.h>
>  
>  /*
>   * CAN mode
> @@ -52,6 +54,13 @@ struct can_priv {
>  
>  	unsigned int echo_skb_max;
>  	struct sk_buff **echo_skb;
> +
> +#ifdef CONFIG_CAN_LEDS
> +	struct timer_list tx_off_timer, rx_off_timer;
> +	struct led_trigger *tx_led, *rx_led;
> +	char tx_led_name[32], rx_led_name[32];
> +	atomic_t led_discard_count;
> +#endif

This requires a lot of space. Please add just *one* pointer here and
allocate the required space when needed.

>  };
>  
>  /*
> diff --git a/net/can/Kconfig b/net/can/Kconfig
> index 0320069..55894b7 100644
> --- a/net/can/Kconfig
> +++ b/net/can/Kconfig
> @@ -52,4 +52,14 @@ config CAN_GW
>  	  They can be modified with AND/OR/XOR/SET operations as configured
>  	  by the netlink configuration interface known e.g. from iptables.
>  
> +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 enables two LED triggers for packet receive and transmit
> +	  events on each CAN device based on the can-dev framework.
> +
>  source "drivers/net/can/Kconfig"
> diff --git a/net/can/Makefile b/net/can/Makefile
> index cef49eb..fc6b286 100644
> --- a/net/can/Makefile
> +++ b/net/can/Makefile
> @@ -5,6 +5,8 @@
>  obj-$(CONFIG_CAN)	+= can.o
>  can-y			:= af_can.o proc.o
>  
> +can-$(CONFIG_CAN_LEDS)	+= led.o
> +
>  obj-$(CONFIG_CAN_RAW)	+= can-raw.o
>  can-raw-y		:= raw.o

In this file you only find things for CAN protocol support. LEDs should
be handled by drivers/net/can/dev.c.

Wolfgang.

Re: [RFC PATCH] can: add tx/rx led trigger support

[Oliver Hartkopp]

On 11.04.2012 16:45, Wolfgang Grandegger wrote:


> On 04/10/2012 11:39 PM, Fabio Baltieri wrote:

>> @@ -52,6 +54,13 @@ struct can_priv {
>>  
>>  	unsigned int echo_skb_max;
>>  	struct sk_buff **echo_skb;
>> +
>> +#ifdef CONFIG_CAN_LEDS
>> +	struct timer_list tx_off_timer, rx_off_timer;
>> +	struct led_trigger *tx_led, *rx_led;
>> +	char tx_led_name[32], rx_led_name[32];
>> +	atomic_t led_discard_count;
>> +#endif
> 
> This requires a lot of space. Please add just *one* pointer here and
> allocate the required space when needed.


Hello Wolfgang,

my argumentation was the other way round.

When CONFIG_CAN_LEDS is set, why do additional kzallocs() at init time and
take care to remove them at exit time?

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

Where i also gave some more details where to implement the rx/tx triggers
which follows your remark too:

> In this file you only find things for CAN protocol support. LEDs should
> be handled by drivers/net/can/dev.c.


Regards,
Oliver

Re: [RFC PATCH] can: add tx/rx led trigger support

[Fabio Baltieri]

Hi Oliver,

thanks for the feedback!

You addressed all the concerns I had while writing the patch. See inline:

On Wed, Apr 11, 2012 at 8:14 AM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> at first i REALLY LIKE blinkenlights :-)

I know the feeling! :-)

>> @@ -52,6 +54,13 @@ struct can_priv {
>>
>>       unsigned int echo_skb_max;
>>       struct sk_buff **echo_skb;
>> +
>> +#ifdef CONFIG_CAN_LEDS
>> +     struct timer_list tx_off_timer, rx_off_timer;
>> +     struct led_trigger *tx_led, *rx_led;
>> +     char tx_led_name[32], rx_led_name[32];
>> +     atomic_t led_discard_count;
>> +#endif
>>  };
>
>
> Why don't you add struct led_trigger directly here, as it makes the two
> kzalloc calls in  can_led_init() obsolete?
>
> Struct can_priv is allocated anyway and therefore you do not take care of the
> removal of your privately allocated memory.

Sounds good to me, I'll keep that in mind for a v2.

> Now looking into the tx path LEDs as an example:
>
>> +/* This is used to ignore devices not based on the can-dev framework */
>> +static int rtnl_link_kind_is(struct net_device *netdev, const char *kind)
>> +{
>> +     if (netdev->rtnl_link_ops &&
>> +             strncmp(kind, netdev->rtnl_link_ops->kind, strlen(kind)) == 0)
>> +             return 1;
>> +     else
>> +             return 0;
>> +}
>
>
> You do this costly compare with *each* tx/rx CAN frame in the hot path.
>
> Not very nice :-(

Agreed, that was one point where I was interested to discuss for some
better solution.

>> +
>> +     if (unlikely(!priv->tx_led))
>> +             return;
>> +
>> +     if (!timer_pending(&priv->tx_off_timer)) {
>> +             led_trigger_event(priv->tx_led, LED_FULL);
>> +
>> +             mod_timer(&priv->tx_off_timer,
>> +                     jiffies + msecs_to_jiffies(led_off_delay));
>> +     }
>> +
>> +     if (loop)
>> +             atomic_dec(&priv->led_discard_count);
>> +}
>> +
>
>
> As you pointed out that only CAN devices are supported that use the can-dev
> infrastructure you do some costly checks one layer above (in net/can) instead
> of implementing it on the driver layer (in drivers/net/can).
>
> The only correct trigger for a tx-LED can be retrieved from the TX done
> interrupt in the interrupt function like here:
>
> http://lxr.linux.no/#linux+v3.3.1/drivers/net/can/sja1000/sja1000.c#L507
>
> where the echo skb is sent back to the system.
>
> This makes sure, that the tx-LED is only triggered, when there was a really
> successful transmission on the CAN bus.
>
> I wonder if it makes more sense to add the LED trigger stuff into
> drivers/net/can/dev.c ?!?

Of course that seems to be the obvious (=correct) place to trap this
stuff. The reason why I worked on upper layer is that I did not found
any generic place to trap tx/rx events in the dev file without
modifying every device driver, as while tx can be trapped in
can_get_echo_skb(), rx frames are handled directly to the network
layer.

> Of course the correct calling points to trigger the rx/tx-LEDs need to be
> implemented inside *each* driver at the correct position.

That's the point. So, do you think it would be better to add trigger
functions into dev.c and modify each driver to call them?

> And we should probably add a new CAN netlink command here
>
>        http://lxr.linux.no/#linux+v3.3.1/drivers/net/can/dev.c#L577
>
> to configure the LED timer per interface: 0 -> LEDs off (==default)

Nice... I'm taking notes here. :-)

> The question would be, if this code
>
>> +     if (!timer_pending(&priv->tx_off_timer)) {
>> +             led_trigger_event(priv->tx_led, LED_FULL);
>> +
>> +             mod_timer(&priv->tx_off_timer,
>> +                     jiffies + msecs_to_jiffies(led_off_delay));
>> +     }
>
>
> is ready to be used in hard IRQ context?
>
> If not we may use a construct with a tasklet that can be triggered from hard
> IRQ context, like here:
>
>        http://lxr.linux.no/#linux+v3.3.1/net/can/bcm.c#L356

I think that led_trigger_event is ok as long as the actual
led-<device>'s brigthness_set function is safe - for example
leds-gpio's one seems to make some check about it. See:

http://lxr.linux.no/#linux+v3.3.1/drivers/leds/leds-gpio.c#L70

Will think about it.

> I think this part
>
>> +
>> +     if (loop)
>> +             atomic_dec(&priv->led_discard_count);
>> +}
>
> is becoming obsolete anyway then, right?

Definitely.

Regards.

-- 
Fabio Baltieri

Re: [RFC PATCH] can: add tx/rx led trigger support

[Fabio Baltieri]

On Wed, Apr 11, 2012 at 8:29 AM, Alexander Stein
<alexander.stein@systec-electronic.com> wrote:
> Am Dienstag, 10. April 2012, 23:39:25 schrieb Fabio Baltieri:
>> This patch adds two led triggers, named <ifname>-tx and <ifname>-rx to
>> each registered canbus interface.
>
> Does this mean, you can have either a tx or rx trigger, bot not both? The
> status LEDs I know from embedded system are triggered by both rx and tx.

That's a point... My idea was to have a tx AND rx trigger - independents.

I also saw adapters with separated tx/rx/error LEDs. Anyway, once the
system is in place, adding a new trigger for a slightly different
behavior - like combined tx/rx - should be easy.

> Despite that, nice idea.

Thanks.

Regards.

-- 
Fabio Baltieri

Re: [RFC PATCH] can: add tx/rx led trigger support

[Oliver Hartkopp]

On 11.04.2012 19:58, Fabio Baltieri wrote:


> On Wed, Apr 11, 2012 at 8:14 AM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:

>> I wonder if it makes more sense to add the LED trigger stuff into
>> drivers/net/can/dev.c ?!?
> 
> Of course that seems to be the obvious (=correct) place to trap this
> stuff. The reason why I worked on upper layer is that I did not found
> any generic place to trap tx/rx events in the dev file without
> modifying every device driver, as while tx can be trapped in
> can_get_echo_skb(), rx frames are handled directly to the network
> layer.


I think that depends heavily on the driver and therefore adding the
can_led_tx() and can_led_rx() calls to the appropriate places is the most
transparent implementation.

IMO i would not(!) hide can_led_tx() in can_get_echo_skb() but place it next to
it in the interrupt function.

> 
>> Of course the correct calling points to trigger the rx/tx-LEDs need to be
>> implemented inside *each* driver at the correct position.
> 
> That's the point. So, do you think it would be better to add trigger
> functions into dev.c and modify each driver to call them?


Yes - as described above - something like this:

$ diff -U6 drivers/net/can/sja1000/sja1000.c drivers/net/can/sja1000/sja1000.c-new
--- drivers/net/can/sja1000/sja1000.c	2012-02-27 19:37:56.440594122 +0100
+++ drivers/net/can/sja1000/sja1000.c-new	2012-04-11 20:26:11.089344979 +0200
@@ -505,24 +505,26 @@
 			netdev_warn(dev, "wakeup interrupt\n");
 
 		if (isrc & IRQ_TI) {
 			/* transmission complete interrupt */
 			stats->tx_bytes += priv->read_reg(priv, REG_FI) & 0xf;
 			stats->tx_packets++;
+			can_led_tx(dev);
 			can_get_echo_skb(dev, 0);
 			netif_wake_queue(dev);
 		}
 		if (isrc & IRQ_RI) {
 			/* receive interrupt */
 			while (status & SR_RBS) {
 				sja1000_rx(dev);
 				status = priv->read_reg(priv, REG_SR);
 				/* check for absent controller */
 				if (status == 0xFF && sja1000_is_absent(priv))
 					return IRQ_NONE;
 			}
+			can_led_rx(dev);
 		}
 		if (isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
 			/* error interrupt */
 			if (sja1000_err(dev, isrc, status))
 				break;
 		}



>> The question would be, if this code
>>
>>> +     if (!timer_pending(&priv->tx_off_timer)) {
>>> +             led_trigger_event(priv->tx_led, LED_FULL);
>>> +
>>> +             mod_timer(&priv->tx_off_timer,
>>> +                     jiffies + msecs_to_jiffies(led_off_delay));
>>> +     }
>>
>>
>> is ready to be used in hard IRQ context?
>>
>> If not we may use a construct with a tasklet that can be triggered from hard
>> IRQ context, like here:
>>
>>        http://lxr.linux.no/#linux+v3.3.1/net/can/bcm.c#L356
> 
> I think that led_trigger_event is ok as long as the actual
> led-<device>'s brigthness_set function is safe - for example
> leds-gpio's one seems to make some check about it. See:
> 
> http://lxr.linux.no/#linux+v3.3.1/drivers/leds/leds-gpio.c#L70
> 
> Will think about it.


Fine. So i'm looking forward to v2 :-)

Regards,
Oliver

Re: [RFC PATCH] can: add tx/rx led trigger support

[Wolfgang Grandegger]

On 04/11/2012 08:29 PM, Oliver Hartkopp wrote:
> On 11.04.2012 19:58, Fabio Baltieri wrote:
> 
> 
>> On Wed, Apr 11, 2012 at 8:14 AM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> 
>>> I wonder if it makes more sense to add the LED trigger stuff into
>>> drivers/net/can/dev.c ?!?
>>
>> Of course that seems to be the obvious (=correct) place to trap this
>> stuff. The reason why I worked on upper layer is that I did not found
>> any generic place to trap tx/rx events in the dev file without
>> modifying every device driver, as while tx can be trapped in
>> can_get_echo_skb(), rx frames are handled directly to the network
>> layer.
> 
> 
> I think that depends heavily on the driver and therefore adding the
> can_led_tx() and can_led_rx() calls to the appropriate places is the most
> transparent implementation.

I would prefer to implement can_led_trigger (or can_led_event) passing a
mask defining the event (RX, TX or event something else).

Wolfgang.

Re: [RFC PATCH] can: add tx/rx led trigger support

[Oliver Hartkopp]

Two additional questions:

On 11.04.2012 19:58, Fabio Baltieri wrote:

> On Wed, Apr 11, 2012 at 8:14 AM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:

>> And we should probably add a new CAN netlink command here
>>
>>        http://lxr.linux.no/#linux+v3.3.1/drivers/net/can/dev.c#L577
>>
>> to configure the LED timer per interface: 0 -> LEDs off (==default)
> 
> Nice... I'm taking notes here. :-)


Hm - probably this is a bit heavy weighted suggestion from me.

Do you really think the timer value needs to be configurable or is a simple
on/off switch ok too?

The second question:

Is the LED "blinking" on traffic or is it just "on" for some time when a CAN
frame is processed?

Regards,
Oliver

Re: [RFC PATCH] can: add tx/rx led trigger support

[Fabio Baltieri]

Hello Wolfgang,

On Wed, Apr 11, 2012 at 4:45 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> this is a try to add generic tx/rx LED triggers for canbus interfaces, somthing
>> I think is very useful in embedded systems where canbus devices often have
>> status leds associated with them, maybe on the bus connector itself, like
>
> Please define "often"? So far only a few CAN devices need LED control by
> software. Actually, I only remember the PEAK PCMCIA card.

I'm actually referring to SoC-based boards with led-class supported
LEDs, which can be linked to kernel events (like
wifi-association/activity etc...).

>> ethernet interfaces. I saw a couple of hardware implementation to drive status
>> leds from tx/rx lines but these were not as effective as software ones.
>
> Why? LEDs are ususally controlled by software only if the hardware does
> not do it (properly?). But your LED support cannot be compared with the
> LEDs on the CAN devices itself.

I agree for "smart" CAN interfaces - like USB ones. What I had in mind
are embedded canbus controllers, which does not provide any activity
indication signal (as ethernet PHYs does). In that case I saw some
hardware implementation which try to drive LEDs from TX/RX signals of
the single-ended bus between controller and transceiver, and in that
case LEDs barely turns on for single packets and you can't have
individual tx/rx indication as the RX driver in the transceiver is
always active.

>> The implementation is similar to the MAC80211_LEDS one, and takes quite a lot
>> of inspiration and some code from it.
>>
>> In this case, however, tx and rx events are trapped from the af_can source file
>> as there are no generic tx/rx functions in can/dev.c. This also required an
>> additional counter to discard rx events for looped frames.
>
> Well, controlling the LEDs from the protocol level seems wrong to me. If
> at all, it should be handled by the CAN device interface.

Agreed, that was a try to keep it driver-independent. I'll rework the
code as suggested by Oliver.

>> Also, as all the support data are in the can_priv structure, this only supports
>> can-dev based drivers. All the others are ignored by checking the
>> netdev->rtnl_link_ops->kind string. This actually excludes only vcan and slcan
>> drivers.
>>
>> The implementation should be quite unintrusive on existing code and can be disabled
>> altogether with a config option.
>
> I find it rather intrusive, especially when #ifdef's should be avoided.

I only used one in "struct can_priv" and then redefined trap functions
to inline-noop. Is that a better way to do that?

> Furthermore it introduces overhead in the RX and TX path and uses a lot
> of space in the "struct can_priv".

Ok for tx/rx path overhead, I did not like the solution either.

>> This has been tested tested on x86 and a powerpc with a custom USB-CAN
>> interface (which I hope to publish as open-hardware soon BTW) and i2c-based
>> leds.
>>
>> Any thoughts? Do you think this can be merged?
>
> LED interface might be useful, especially for debugging. Why not
> enabling it just if "CONFIG_CAN_DEBUG_DEVICES=y". Also LEDs for error
> states would make sense.

I see that more useful for an installer of the system to see that the
bus is working than for debugging the driver, also because what I had
in mind was some kind of device with front-panel connector and status
leds.

Agreed for error state. Of course that have to be trapped in the driver too.

> Some more comments inline...
>
>>  include/linux/can/dev.h |    9 +++
>>  net/can/Kconfig         |   10 +++
>>  net/can/Makefile        |    2 +
>>  net/can/af_can.c        |   14 ++++-
>>  net/can/led.c           |  154 +++++++++++++++++++++++++++++++++++++++++++++++
>>  net/can/led.h           |   29 +++++++++
>>  6 files changed, 217 insertions(+), 1 deletions(-)
>>  create mode 100644 net/can/led.c
>>  create mode 100644 net/can/led.h
>>
>> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
>> index 5d2efe7..76eb70c 100644
>> --- a/include/linux/can/dev.h
>> +++ b/include/linux/can/dev.h
>> @@ -16,6 +16,8 @@
>>  #include <linux/can.h>
>>  #include <linux/can/netlink.h>
>>  #include <linux/can/error.h>
>> +#include <linux/timer.h>
>> +#include <linux/leds.h>
>>
>>  /*
>>   * CAN mode
>> @@ -52,6 +54,13 @@ struct can_priv {
>>
>>       unsigned int echo_skb_max;
>>       struct sk_buff **echo_skb;
>> +
>> +#ifdef CONFIG_CAN_LEDS
>> +     struct timer_list tx_off_timer, rx_off_timer;
>> +     struct led_trigger *tx_led, *rx_led;
>> +     char tx_led_name[32], rx_led_name[32];
>> +     atomic_t led_discard_count;
>> +#endif
>
> This requires a lot of space. Please add just *one* pointer here and
> allocate the required space when needed.

How would that help?

>>  };
>>
>>  /*
>> diff --git a/net/can/Kconfig b/net/can/Kconfig
>> index 0320069..55894b7 100644
>> --- a/net/can/Kconfig
>> +++ b/net/can/Kconfig
>> @@ -52,4 +52,14 @@ config CAN_GW
>>         They can be modified with AND/OR/XOR/SET operations as configured
>>         by the netlink configuration interface known e.g. from iptables.
>>
>> +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 enables two LED triggers for packet receive and transmit
>> +       events on each CAN device based on the can-dev framework.
>> +
>>  source "drivers/net/can/Kconfig"
>> diff --git a/net/can/Makefile b/net/can/Makefile
>> index cef49eb..fc6b286 100644
>> --- a/net/can/Makefile
>> +++ b/net/can/Makefile
>> @@ -5,6 +5,8 @@
>>  obj-$(CONFIG_CAN)    += can.o
>>  can-y                        := af_can.o proc.o
>>
>> +can-$(CONFIG_CAN_LEDS)       += led.o
>> +
>>  obj-$(CONFIG_CAN_RAW)        += can-raw.o
>>  can-raw-y            := raw.o
>
> In this file you only find things for CAN protocol support. LEDs should
> be handled by drivers/net/can/dev.c.

Agreed.

Thanks for the feedback.

Regards.

-- 
Fabio Baltieri

Re: [RFC PATCH] can: add tx/rx led trigger support

[Fabio Baltieri]

On Wed, Apr 11, 2012 at 9:02 PM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>> And we should probably add a new CAN netlink command here
>>>
>>>        http://lxr.linux.no/#linux+v3.3.1/drivers/net/can/dev.c#L577
>>>
>>> to configure the LED timer per interface: 0 -> LEDs off (==default)
>>
>> Nice... I'm taking notes here. :-)
>
>
> Hm - probably this is a bit heavy weighted suggestion from me.
>
> Do you really think the timer value needs to be configurable or is a simple
> on/off switch ok too?

I think that if the bus only has a light traffic a longer time would
be more helpful, otherwise any fixed time of some tenths of
milliseconds should just do the job (that's how ledtrig-ide-disk
works). A single module parameter looked like a decent compromise to
me.

I should add a check to skip it entirely if led_off_delay == 0 as a
run-time "optimization" if the feature is not used (triggers does not
provide a use-counter). Do you think it should be disabled by default?

> The second question:
>
> Is the LED "blinking" on traffic or is it just "on" for some time when a CAN
> frame is processed?

It stays on for some time after each frame. Above a certain
packet-rate (1/led_off_delay) LED appear to be constantly on. You can
tell periodic packets from bursts from it but nothing more.

Quick not-so-off-topic - I have a small userspace deamon on my router
which blinks an LED every 100kB of traffic on the WAN interface, which
is nice because you can almost tell the network load just by looking
at it! Much more useful than the standard activity LED. (sometimes I
think I'm taking this too seriously...)

Regards.

-- 
Fabio Baltieri

Re: [RFC PATCH] can: add tx/rx led trigger support

[Oliver Hartkopp]

On 11.04.2012 21:36, Fabio Baltieri wrote:

> On Wed, Apr 11, 2012 at 9:02 PM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:

> I should add a check to skip it entirely if led_off_delay == 0 as a
> run-time "optimization" if the feature is not used (triggers does not
> provide a use-counter).


Yes. That's a good idea.

> Do you think it should be disabled by default?


The LED triggers?

Yes. They should be disabled by default.

So far i assume there's a very limited number of hardware having extra LEDs.

> 
>> The second question:
>>
>> Is the LED "blinking" on traffic or is it just "on" for some time when a CAN
>> frame is processed?
> 
> It stays on for some time after each frame. Above a certain
> packet-rate (1/led_off_delay) LED appear to be constantly on. You can
> tell periodic packets from bursts from it but nothing more.


Ok. That's fine.

> Quick not-so-off-topic - I have a small userspace deamon on my router
> which blinks an LED every 100kB of traffic on the WAN interface, which
> is nice because you can almost tell the network load just by looking
> at it! Much more useful than the standard activity LED. (sometimes I
> think I'm taking this too seriously...)


Don't know if traffic counting is a good idea for the first step.

If we all find a really needed use-case we might think about it later.

Regards,
Oliver

Re: [RFC PATCH] can: add tx/rx led trigger support

[Oliver Hartkopp]

On 11.04.2012 20:58, Wolfgang Grandegger wrote:

> On 04/11/2012 08:29 PM, Oliver Hartkopp wrote:
>> On 11.04.2012 19:58, Fabio Baltieri wrote:
>>
>>
>>> On Wed, Apr 11, 2012 at 8:14 AM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>
>>>> I wonder if it makes more sense to add the LED trigger stuff into
>>>> drivers/net/can/dev.c ?!?
>>>
>>> Of course that seems to be the obvious (=correct) place to trap this
>>> stuff. The reason why I worked on upper layer is that I did not found
>>> any generic place to trap tx/rx events in the dev file without
>>> modifying every device driver, as while tx can be trapped in
>>> can_get_echo_skb(), rx frames are handled directly to the network
>>> layer.
>>
>>
>> I think that depends heavily on the driver and therefore adding the
>> can_led_tx() and can_led_rx() calls to the appropriate places is the most
>> transparent implementation.
> 
> I would prefer to implement can_led_trigger (or can_led_event) passing a
> mask defining the event (RX, TX or event something else).
> 


Yes. Good idea.

What about passing the pointer to some kind of can_led_handler struct, e.g.

struct can_led_handler {
	struct timer_list off_timer;
	struct led_trigger *led;
	char led_name[32];
}

which is part of struct can_priv then:

+#ifdef CONFIG_CAN_LEDS
+	struct can_led_handler can_tx_led, can_rx_lead;
+#endif

Then we could use in the interrupt:

(..)
	can_led_trigger(&priv->can_tx_led)
(..)

This would omit the check for flags and directly work on the correct LEDs.


Regards,
Oliver

Re: [RFC PATCH] can: add tx/rx led trigger support

[Alexander Stein]

Am Mittwoch, 11. April 2012, 21:36:05 schrieb Fabio Baltieri:
> On Wed, Apr 11, 2012 at 9:02 PM, Oliver Hartkopp <socketcan@hartkopp.net> 
wrote:
> >>> And we should probably add a new CAN netlink command here
> >>> 
> >>>        http://lxr.linux.no/#linux+v3.3.1/drivers/net/can/dev.c#L577
> >>> 
> >>> to configure the LED timer per interface: 0 -> LEDs off (==default)
> >> 
> >> Nice... I'm taking notes here. :-)
> > 
> > Hm - probably this is a bit heavy weighted suggestion from me.
> > 
> > Do you really think the timer value needs to be configurable or is a
> > simple
> > on/off switch ok too?
> 
> I think that if the bus only has a light traffic a longer time would
> be more helpful, otherwise any fixed time of some tenths of
> milliseconds should just do the job (that's how ledtrig-ide-disk
> works). A single module parameter looked like a decent compromise to
> me.
> 
> I should add a check to skip it entirely if led_off_delay == 0 as a
> run-time "optimization" if the feature is not used (triggers does not
> provide a use-counter). Do you think it should be disabled by default?
> 
> > The second question:
> > 
> > Is the LED "blinking" on traffic or is it just "on" for some time when a
> > CAN frame is processed?
> 
> It stays on for some time after each frame. Above a certain
> packet-rate (1/led_off_delay) LED appear to be constantly on. You can
> tell periodic packets from bursts from it but nothing more.

I think a Tx or Rx trigger should just "start" one LED blink, e.g. ON for 
100ms and OFF for 100ms no matter if other packets being processed. This way 
you will constantly see a LED blinking if a specific load is exceeded. If it 
is at lower rate the LED will blink more rarely.
You could even go one step further and activate a LED if the CAN interface is 
UP and turn it off for some time if there is some traffic. So the LED could 
inidicate if the interface is being used and/or if there is some traffic.

Regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
August-Bebel-Str. 29
D-07973 Greiz

Tel: +49-3661-6279-0, Fax: +49-3661-6279-99
eMail:    Alexander.Stein@systec-electronic.com
Internet: http://www.systec-electronic.com

Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Jena, HRB 205563

Re: [RFC PATCH] can: add tx/rx led trigger support

[Wolfgang Grandegger]

On 04/11/2012 09:36 PM, Fabio Baltieri wrote:
> On Wed, Apr 11, 2012 at 9:02 PM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>>> And we should probably add a new CAN netlink command here
>>>>
>>>>        http://lxr.linux.no/#linux+v3.3.1/drivers/net/can/dev.c#L577
>>>>
>>>> to configure the LED timer per interface: 0 -> LEDs off (==default)
>>>
>>> Nice... I'm taking notes here. :-)
>>
>>
>> Hm - probably this is a bit heavy weighted suggestion from me.
>>
>> Do you really think the timer value needs to be configurable or is a simple
>> on/off switch ok too?
> 
> I think that if the bus only has a light traffic a longer time would
> be more helpful, otherwise any fixed time of some tenths of
> milliseconds should just do the job (that's how ledtrig-ide-disk
> works). A single module parameter looked like a decent compromise to
> me.
> 
> I should add a check to skip it entirely if led_off_delay == 0 as a
> run-time "optimization" if the feature is not used (triggers does not
> provide a use-counter). Do you think it should be disabled by default?
> 
>> The second question:
>>
>> Is the LED "blinking" on traffic or is it just "on" for some time when a CAN
>> frame is processed?
> 
> It stays on for some time after each frame. Above a certain
> packet-rate (1/led_off_delay) LED appear to be constantly on. You can
> tell periodic packets from bursts from it but nothing more.

Concerning the LED control (on. off, blinking), could that not be
done/configured in the generic LED interface (via sysfs files)? That
seems the right place for me. Then the CAN LED software just needs to
send the trigger which would significantly simplify the code (without
timer).

Furthermore, if I understand your patch correctly, the LEDs with the
names "canX-rx" and "canX-tx" are directly assigned (hardwired). A more
flexible method would be appreciated, if it could be realized with just
a little more code.

Wolfgang.

Re: [RFC PATCH] can: add tx/rx led trigger support

[Martin Gysel]

Am 12.04.2012 09:37, schrieb Wolfgang Grandegger:
> Concerning the LED control (on. off, blinking), could that not be
> done/configured in the generic LED interface (via sysfs files)? That
> seems the right place for me. Then the CAN LED software just needs to
> send the trigger which would significantly simplify the code (without
> timer).
> 
> Furthermore, if I understand your patch correctly, the LEDs with the
> names "canX-rx" and "canX-tx" are directly assigned (hardwired). A more
> flexible method would be appreciated, if it could be realized with just
> a little more code.

maybe something familiar to the led netdev trigger (which is afaik
'still' not mainline...)

https://dev.openwrt.org/ticket/2776

/martin

Re: [RFC PATCH] can: add tx/rx led trigger support

[Oliver Hartkopp]

>>> Is the LED "blinking" on traffic or is it just "on" for some time when a
>>> CAN frame is processed?
>>
>> It stays on for some time after each frame. Above a certain
>> packet-rate (1/led_off_delay) LED appear to be constantly on. You can
>> tell periodic packets from bursts from it but nothing more.
> 
> I think a Tx or Rx trigger should just "start" one LED blink, e.g. ON for 
> 100ms and OFF for 100ms no matter if other packets being processed. This way 
> you will constantly see a LED blinking if a specific load is exceeded. If it 
> is at lower rate the LED will blink more rarely.
> You could even go one step further and activate a LED if the CAN interface is 
> UP and turn it off for some time if there is some traffic. So the LED could 
> inidicate if the interface is being used and/or if there is some traffic.


Hi Alexander,

the latter is implemented in my WIFI LED in my notebook:

Interface up -> LED is ON

Interface traffic -> LED is doing a off-on-sequence which leads to constant
blinking when downloading files ...

Nice idea.

Regards,
Oliver

Re: [RFC PATCH] can: add tx/rx led trigger support

[Oliver Hartkopp]

On 12.04.2012 13:07, Martin Gysel wrote:

> Am 12.04.2012 09:37, schrieb Wolfgang Grandegger:
>> Concerning the LED control (on. off, blinking), could that not be
>> done/configured in the generic LED interface (via sysfs files)? That
>> seems the right place for me. Then the CAN LED software just needs to
>> send the trigger which would significantly simplify the code (without
>> timer).
>>
>> Furthermore, if I understand your patch correctly, the LEDs with the
>> names "canX-rx" and "canX-tx" are directly assigned (hardwired). A more
>> flexible method would be appreciated, if it could be realized with just
>> a little more code.
> 
> maybe something familiar to the led netdev trigger (which is afaik
> 'still' not mainline...)
> 
> https://dev.openwrt.org/ticket/2776


Hm - "still not in mainline" is not really nice.

I think the original approach from Fabio was to follow the LED implementation
from mac80211:

"The implementation is similar to the MAC80211_LEDS one, and takes quite a lot
of inspiration and some code from it."

And IMHO we should follow this implementation as it is proved to be used in
high traffic netdev environments. I wonder if some 'generic gpio LED'
implementation will perform sufficiently.

Regards,
Oliver

Re: [RFC PATCH] can: add tx/rx led trigger support

[Wolfgang Grandegger]

On 04/12/2012 06:02 PM, Oliver Hartkopp wrote:
> On 12.04.2012 13:07, Martin Gysel wrote:
> 
>> Am 12.04.2012 09:37, schrieb Wolfgang Grandegger:
>>> Concerning the LED control (on. off, blinking), could that not be
>>> done/configured in the generic LED interface (via sysfs files)? That
>>> seems the right place for me. Then the CAN LED software just needs to
>>> send the trigger which would significantly simplify the code (without
>>> timer).
>>>
>>> Furthermore, if I understand your patch correctly, the LEDs with the
>>> names "canX-rx" and "canX-tx" are directly assigned (hardwired). A more
>>> flexible method would be appreciated, if it could be realized with just
>>> a little more code.
>>
>> maybe something familiar to the led netdev trigger (which is afaik
>> 'still' not mainline...)
>>
>> https://dev.openwrt.org/ticket/2776
> 
> 
> Hm - "still not in mainline" is not really nice.
> 
> I think the original approach from Fabio was to follow the LED implementation
> from mac80211:
> 
> "The implementation is similar to the MAC80211_LEDS one, and takes quite a lot
> of inspiration and some code from it."
> 
> And IMHO we should follow this implementation as it is proved to be used in
> high traffic netdev environments. I wonder if some 'generic gpio LED'
> implementation will perform sufficiently.

Yes, I agree. But that implementation does not use timers but toggles
the LED on/off for each new packet.

http://lxr.linux.no/#linux+v3.3.1/net/mac80211/led.c#L15

I would vote for that solution as well (because it's very lite and we
already have counters for rx and tx packets).

Wolfgang.

Re: [RFC PATCH] can: add tx/rx led trigger support

[Fabio Baltieri]

On Thu, Apr 12, 2012 at 6:13 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> I think the original approach from Fabio was to follow the LED implementation
>> from mac80211:
>>
>> "The implementation is similar to the MAC80211_LEDS one, and takes quite a lot
>> of inspiration and some code from it."
>>
>> And IMHO we should follow this implementation as it is proved to be used in
>> high traffic netdev environments. I wonder if some 'generic gpio LED'
>> implementation will perform sufficiently.
>
> Yes, I agree. But that implementation does not use timers but toggles
> the LED on/off for each new packet.
>
> http://lxr.linux.no/#linux+v3.3.1/net/mac80211/led.c#L15

That's correct, but I think it works for 80211 because there you
always have some traffic. In the canbus case if implemented as
"toggle" you'll end up with a led full-on when removing the cable with
50% probability - which I think is quite misleading.

> I would vote for that solution as well (because it's very lite and we
> already have counters for rx and tx packets).

There are other implementation (which I took as example) in critical
paths which blink leds on one-time events with timers:

ledtrig-ide-disk (with fixed 10ms timeout):
http://lxr.linux.no/#linux+v3.3.1/drivers/leds/ledtrig-ide-disk.c#L28

netfilter's xt_LED trigger:
http://lxr.linux.no/#linux+v3.3.1/net/netfilter/xt_LED.c#L69

I think that with the timer called with a reasonably high delay
(20-30ms) it should be quite lightweight. After all, most of the times
embedded system's leds are gpio-mapped and will be handled by a bunch
of function call and single memory write...

Regards.

-- 
Fabio Baltieri

Re: [RFC PATCH] can: add tx/rx led trigger support

[Fabio Baltieri]

On Thu, Apr 12, 2012 at 9:37 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> It stays on for some time after each frame. Above a certain
>> packet-rate (1/led_off_delay) LED appear to be constantly on. You can
>> tell periodic packets from bursts from it but nothing more.
>
> Concerning the LED control (on. off, blinking), could that not be
> done/configured in the generic LED interface (via sysfs files)? That
> seems the right place for me. Then the CAN LED software just needs to
> send the trigger which would significantly simplify the code (without
> timer).

Of course, that's possible, but I think a kernel implementation with
timers for blinking and some well placed trigger points would be more
effective that, for example, polling interface stats from userspace.
After all many other subsystems have similar triggers in kernel space
(netfilter, ide/ata with timers, mtd, mmc, mac80211 without triggers).

> Furthermore, if I understand your patch correctly, the LEDs with the
> names "canX-rx" and "canX-tx" are directly assigned (hardwired). A more
> flexible method would be appreciated, if it could be realized with just
> a little more code.

Well, they are just sprintf-ed from initial interface name, that's
also how mac80211 implementation works. Can you be more specific on
how should I change it? Like following interface rename or so?

Regards.

-- 
Fabio Baltieri

Re: [RFC PATCH] can: add tx/rx led trigger support

[Fabio Baltieri]

On Thu, Apr 12, 2012 at 5:52 PM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>>> Is the LED "blinking" on traffic or is it just "on" for some time when a
>>>> CAN frame is processed?
>>>
>>> It stays on for some time after each frame. Above a certain
>>> packet-rate (1/led_off_delay) LED appear to be constantly on. You can
>>> tell periodic packets from bursts from it but nothing more.
>>
>> I think a Tx or Rx trigger should just "start" one LED blink, e.g. ON for
>> 100ms and OFF for 100ms no matter if other packets being processed. This way
>> you will constantly see a LED blinking if a specific load is exceeded. If it
>> is at lower rate the LED will blink more rarely.
>> You could even go one step further and activate a LED if the CAN interface is
>> UP and turn it off for some time if there is some traffic. So the LED could
>> inidicate if the interface is being used and/or if there is some traffic.
>
>
> Hi Alexander,
>
> the latter is implemented in my WIFI LED in my notebook:
>
> Interface up -> LED is ON
>
> Interface traffic -> LED is doing a off-on-sequence which leads to constant
> blinking when downloading files ...

I like the idea too.

So, I'll try to recap the points posted for a v2 o the patch:
- move the trigger code away from the net layer and put it the drivers
one for direct use of the drivers, I'll keep it in a separate source
file with #ifdef to redefine functions as no-op, as it seems to be
what most other triggers do (and it looks clean to me).
- implement functions as led_can_init, led_can_exit, led_can_event
with an event-id for tx, rx, error, open, stop
- implement event call from some driver as reference (I can only test
on vcan, slcan and flexcan, that's probably enough to test it out).
- implement blink function as follows:
  - for tx/rx trigger, turn full-on on device open, off on device stop
and one cycle of "blink_delay off - blink_delay on" on event, other
frames are ignored until the off-on sequence is over.
  - for error trigger, just blink on-off on each error event.
- keep delay time as parameter and skip triggers altogether if 0 - or
- use the activate/deactivate functions of the trigger subsystem to do
that automatically, that would cost some atomic usage counters.
- shorten trigger names to 16 characters (32 bytes look i bit big now
that I think at it).

Am I missing anything?

Regards.

-- 
Fabio Baltieri

Re: [RFC PATCH] can: add tx/rx led trigger support

[Wolfgang Grandegger]

On 04/12/2012 07:28 PM, Fabio Baltieri wrote:
> On Thu, Apr 12, 2012 at 6:13 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>>> I think the original approach from Fabio was to follow the LED implementation
>>> from mac80211:
>>>
>>> "The implementation is similar to the MAC80211_LEDS one, and takes quite a lot
>>> of inspiration and some code from it."
>>>
>>> And IMHO we should follow this implementation as it is proved to be used in
>>> high traffic netdev environments. I wonder if some 'generic gpio LED'
>>> implementation will perform sufficiently.
>>
>> Yes, I agree. But that implementation does not use timers but toggles
>> the LED on/off for each new packet.
>>
>> http://lxr.linux.no/#linux+v3.3.1/net/mac80211/led.c#L15
> 
> That's correct, but I think it works for 80211 because there you
> always have some traffic. In the canbus case if implemented as
> "toggle" you'll end up with a led full-on when removing the cable with
> 50% probability - which I think is quite misleading.

Well, yes, I see that problem as well.

>> I would vote for that solution as well (because it's very lite and we
>> already have counters for rx and tx packets).
> 
> There are other implementation (which I took as example) in critical
> paths which blink leds on one-time events with timers:
> 
> ledtrig-ide-disk (with fixed 10ms timeout):
> http://lxr.linux.no/#linux+v3.3.1/drivers/leds/ledtrig-ide-disk.c#L28
> 
> netfilter's xt_LED trigger:
> http://lxr.linux.no/#linux+v3.3.1/net/netfilter/xt_LED.c#L69

So we will duplicate again almost the same code for CAN? No, please
don't...

> I think that with the timer called with a reasonably high delay
> (20-30ms) it should be quite lightweight. After all, most of the times
> embedded system's leds are gpio-mapped and will be handled by a bunch
> of function call and single memory write...

... but provide a patch for the LED class first implementing that
functionality, maybe even with a configurable delay. Note that there is
already a led_tigger_blink().

Wolfgang.

Re: [RFC PATCH] can: add tx/rx led trigger support

[Wolfgang Grandegger]

On 04/12/2012 07:46 PM, Fabio Baltieri wrote:
> On Thu, Apr 12, 2012 at 9:37 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>>> It stays on for some time after each frame. Above a certain
>>> packet-rate (1/led_off_delay) LED appear to be constantly on. You can
>>> tell periodic packets from bursts from it but nothing more.
>>
>> Concerning the LED control (on. off, blinking), could that not be
>> done/configured in the generic LED interface (via sysfs files)? That
>> seems the right place for me. Then the CAN LED software just needs to
>> send the trigger which would significantly simplify the code (without
>> timer).
> 
> Of course, that's possible, but I think a kernel implementation with
> timers for blinking and some well placed trigger points would be more
> effective that, for example, polling interface stats from userspace.
> After all many other subsystems have similar triggers in kernel space
> (netfilter, ide/ata with timers, mtd, mmc, mac80211 without triggers).

See my previous mail.

>> Furthermore, if I understand your patch correctly, the LEDs with the
>> names "canX-rx" and "canX-tx" are directly assigned (hardwired). A more
>> flexible method would be appreciated, if it could be realized with just
>> a little more code.
> 
> Well, they are just sprintf-ed from initial interface name, that's
> also how mac80211 implementation works. Can you be more specific on
> how should I change it? Like following interface rename or so?

The LED could be assigned when configuring the CAN interface:

  ip ... type can rx-led "the-can-rx-led"

But maybe it's overkill and we even misuse the ip tool. I still think
that the LED interface should be very slim.

Wolfgang.

Re: [RFC PATCH] can: add tx/rx led trigger support

[Oliver Hartkopp]

On 12.04.2012 20:30, Fabio Baltieri wrote:


>> the latter is implemented in my WIFI LED in my notebook:
>>
>> Interface up -> LED is ON
>>
>> Interface traffic -> LED is doing a off-on-sequence which leads to constant
>> blinking when downloading files ...
> 
> I like the idea too.
> 
> So, I'll try to recap the points posted for a v2 o the patch:
> - move the trigger code away from the net layer and put it the drivers
> one for direct use of the drivers, I'll keep it in a separate source
> file with #ifdef to redefine functions as no-op, as it seems to be
> what most other triggers do (and it looks clean to me).


ack.

> - implement functions as led_can_init, led_can_exit, led_can_event
> with an event-id for tx, rx, error, open, stop
> - implement event call from some driver as reference (I can only test
> on vcan, slcan and flexcan, that's probably enough to test it out).


Providing an example for flexcan is IMO the most relevant to demonstrate the
implementation.

> - implement blink function as follows:
>   - for tx/rx trigger, turn full-on on device open, off on device stop
> and one cycle of "blink_delay off - blink_delay on" on event, other
> frames are ignored until the off-on sequence is over.


ack.

>   - for error trigger, just blink on-off on each error event.


?

> - keep delay time as parameter and skip triggers altogether if 0 - or
> - use the activate/deactivate functions of the trigger subsystem to do
> that automatically, that would cost some atomic usage counters.
> - shorten trigger names to 16 characters (32 bytes look i bit big now
> that I think at it).
> 
> Am I missing anything?


Don't know - send a patch 8-)

Tnx,
Oliver