[PATCH can-next v3 1/2] can: add tx/rx LED trigger support

[PATCH can-next v3 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

Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Wolfgang Grandegger <wg@grandegger.com>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
---

Hi all!

This is the v3 of my CAN LED trigger patch.  It's a major refactoring of the v2
that was discussed about three months ago concluding with the idea that
implementing the oneshot triggering code in the LED framework would be a better
solution.

This is the old thread for reference:
http://marc.info/?l=linux-can&m=133521499002898&w=2

So, generic oneshot trigger code is now merged in mainline (see 5bb629c), and
these are the changes in the v3:

- use the new led_trigger_blink_oneshot() function for LED triggering
- use kasprintf() and led_trigger_{un}register_simple for LED allocation
- add some usage note in the comments

The resulting code is quite simple now and - I think - a bit less intrusive.
Still, I hope on some feedback from the community as I don't have that much
hardware to test it - this version has been tested mainly on an x86 with a
custom usb-can interface.

In 2/2 there is a sample implementation for the flexcan driver, which is
basically unchanged from the old version.

Any comments?
Fabio

 drivers/net/can/Kconfig  | 12 ++++++
 drivers/net/can/Makefile |  2 +
 drivers/net/can/led.c    | 99 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/can/dev.h  |  8 ++++
 include/linux/can/led.h  | 38 +++++++++++++++++++
 5 files changed, 159 insertions(+)
 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..19dec19 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -54,6 +54,18 @@ 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_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..68c4f30
--- /dev/null
+++ b/drivers/net/can/led.c
@@ -0,0 +1,99 @@
+/*
+ * 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/can/led.h>
+
+static unsigned long led_delay = 50;
+module_param(led_delay, ulong, 0644);
+MODULE_PARM_DESC(led_delay,
+		"blink delay time for activity leds (msecs, default: 50).");
+
+/*
+ * Trigger a LED event in response to a CAN device event
+ */
+void can_led_event(struct net_device *netdev, enum can_led_event event)
+{
+	struct can_priv *priv = netdev_priv(netdev);
+
+	switch (event) {
+	case CAN_LED_EVENT_OPEN:
+		led_trigger_event(priv->tx_led_trig, LED_FULL);
+		led_trigger_event(priv->rx_led_trig, LED_FULL);
+		break;
+	case CAN_LED_EVENT_STOP:
+		led_trigger_event(priv->tx_led_trig, LED_OFF);
+		led_trigger_event(priv->rx_led_trig, LED_OFF);
+		break;
+	case CAN_LED_EVENT_TX:
+		if (led_delay)
+			led_trigger_blink_oneshot(priv->tx_led_trig,
+						  &led_delay, &led_delay, 1);
+		break;
+	case CAN_LED_EVENT_RX:
+		if (led_delay)
+			led_trigger_blink_oneshot(priv->rx_led_trig,
+						  &led_delay, &led_delay, 1);
+		break;
+	}
+}
+EXPORT_SYMBOL_GPL(can_led_event);
+
+/*
+ * Register CAN LED triggers for a CAN device
+ *
+ * This is normally called from a driver's probe function
+ */
+void can_led_init(struct net_device *netdev)
+{
+	struct can_priv *priv = netdev_priv(netdev);
+
+	priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);
+	if (!priv->tx_led_trig_name)
+		goto tx_led_failed;
+
+	priv->rx_led_trig_name = kasprintf(GFP_KERNEL, "%s-rx", netdev->name);
+	if (!priv->rx_led_trig_name)
+		goto rx_led_failed;
+
+	led_trigger_register_simple(priv->tx_led_trig_name,
+				    &priv->tx_led_trig);
+	led_trigger_register_simple(priv->rx_led_trig_name,
+				    &priv->rx_led_trig);
+
+	return;
+
+rx_led_failed:
+	kfree(priv->tx_led_trig_name);
+	priv->tx_led_trig_name = NULL;
+tx_led_failed:
+	return;
+}
+EXPORT_SYMBOL_GPL(can_led_init);
+
+/*
+ * Unregister CAN LED triggers for a CAN device
+ *
+ * This is normally called from a driver's remove function
+ */
+void can_led_exit(struct net_device *netdev)
+{
+	struct can_priv *priv = netdev_priv(netdev);
+
+	led_trigger_unregister_simple(priv->tx_led_trig);
+	led_trigger_unregister_simple(priv->rx_led_trig);
+
+	kfree(priv->tx_led_trig_name);
+	kfree(priv->rx_led_trig_name);
+}
+EXPORT_SYMBOL_GPL(can_led_exit);
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 2b2fc34..167b04a 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,13 @@ struct can_priv {
 
 	unsigned int echo_skb_max;
 	struct sk_buff **echo_skb;
+
+#ifdef CONFIG_CAN_LEDS
+	struct led_trigger *tx_led_trig;
+	char *tx_led_trig_name;
+	struct led_trigger *rx_led_trig;
+	char *rx_led_trig_name;
+#endif
 };
 
 /*
diff --git a/include/linux/can/led.h b/include/linux/can/led.h
new file mode 100644
index 0000000..274c7fd
--- /dev/null
+++ b/include/linux/can/led.h
@@ -0,0 +1,38 @@
+/*
+ * 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/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
+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.11.rc1.9.gf623ca1.dirty

[PATCH can-next v3 2/2] can: flexcan: add LED trigger support

[Fabio Baltieri]

Add support for canbus activity led indicators on flexcan devices 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 file changed, 13 insertions(+)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index c5f1431..4816ed0 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>
@@ -547,6 +548,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;
 }
 
@@ -635,6 +638,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
 		stats->tx_bytes += can_get_echo_skb(dev, 0);
 		stats->tx_packets++;
+		can_led_event(dev, CAN_LED_EVENT_TX);
 		flexcan_write((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
 		netif_wake_queue(dev);
 	}
@@ -844,6 +848,9 @@ static int flexcan_open(struct net_device *dev)
 	err = flexcan_chip_start(dev);
 	if (err)
 		goto out_close;
+
+	can_led_event(dev, CAN_LED_EVENT_OPEN);
+
 	napi_enable(&priv->napi);
 	netif_start_queue(dev);
 
@@ -872,6 +879,8 @@ static int flexcan_close(struct net_device *dev)
 
 	close_candev(dev);
 
+	can_led_event(dev, CAN_LED_EVENT_STOP);
+
 	return 0;
 }
 
@@ -1068,6 +1077,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);
 
@@ -1091,6 +1102,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.11.rc1.9.gf623ca1.dirty

Re: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support

[Marc Kleine-Budde]

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

On 07/30/2012 09:20 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
> 
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> ---
> 
> Hi all!
> 
> This is the v3 of my CAN LED trigger patch.  It's a major refactoring of the v2
> that was discussed about three months ago concluding with the idea that
> implementing the oneshot triggering code in the LED framework would be a better
> solution.
> 
> This is the old thread for reference:
> http://marc.info/?l=linux-can&m=133521499002898&w=2
> 
> So, generic oneshot trigger code is now merged in mainline (see 5bb629c), and
> these are the changes in the v3:
> 
> - use the new led_trigger_blink_oneshot() function for LED triggering
> - use kasprintf() and led_trigger_{un}register_simple for LED allocation
> - add some usage note in the comments
> 
> The resulting code is quite simple now and - I think - a bit less intrusive.
> Still, I hope on some feedback from the community as I don't have that much
> hardware to test it - this version has been tested mainly on an x86 with a
> custom usb-can interface.
> 
> In 2/2 there is a sample implementation for the flexcan driver, which is
> basically unchanged from the old version.
> 
> Any comments?
> Fabio

Looks quite clean now. Can you provide a devm implementation, too?

Marc
> 
>  drivers/net/can/Kconfig  | 12 ++++++
>  drivers/net/can/Makefile |  2 +
>  drivers/net/can/led.c    | 99 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/can/dev.h  |  8 ++++
>  include/linux/can/led.h  | 38 +++++++++++++++++++
>  5 files changed, 159 insertions(+)
>  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..19dec19 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -54,6 +54,18 @@ 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_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..68c4f30
> --- /dev/null
> +++ b/drivers/net/can/led.c
> @@ -0,0 +1,99 @@
> +/*
> + * 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/can/led.h>
> +
> +static unsigned long led_delay = 50;
> +module_param(led_delay, ulong, 0644);
> +MODULE_PARM_DESC(led_delay,
> +		"blink delay time for activity leds (msecs, default: 50).");
> +
> +/*
> + * Trigger a LED event in response to a CAN device event
> + */
> +void can_led_event(struct net_device *netdev, enum can_led_event event)
> +{
> +	struct can_priv *priv = netdev_priv(netdev);
> +
> +	switch (event) {
> +	case CAN_LED_EVENT_OPEN:
> +		led_trigger_event(priv->tx_led_trig, LED_FULL);
> +		led_trigger_event(priv->rx_led_trig, LED_FULL);
> +		break;
> +	case CAN_LED_EVENT_STOP:
> +		led_trigger_event(priv->tx_led_trig, LED_OFF);
> +		led_trigger_event(priv->rx_led_trig, LED_OFF);
> +		break;
> +	case CAN_LED_EVENT_TX:
> +		if (led_delay)
> +			led_trigger_blink_oneshot(priv->tx_led_trig,
> +						  &led_delay, &led_delay, 1);
> +		break;
> +	case CAN_LED_EVENT_RX:
> +		if (led_delay)
> +			led_trigger_blink_oneshot(priv->rx_led_trig,
> +						  &led_delay, &led_delay, 1);
> +		break;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(can_led_event);
> +
> +/*
> + * Register CAN LED triggers for a CAN device
> + *
> + * This is normally called from a driver's probe function
> + */
> +void can_led_init(struct net_device *netdev)
> +{
> +	struct can_priv *priv = netdev_priv(netdev);
> +
> +	priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);
> +	if (!priv->tx_led_trig_name)
> +		goto tx_led_failed;
> +
> +	priv->rx_led_trig_name = kasprintf(GFP_KERNEL, "%s-rx", netdev->name);
> +	if (!priv->rx_led_trig_name)
> +		goto rx_led_failed;
> +
> +	led_trigger_register_simple(priv->tx_led_trig_name,
> +				    &priv->tx_led_trig);
> +	led_trigger_register_simple(priv->rx_led_trig_name,
> +				    &priv->rx_led_trig);
> +
> +	return;
> +
> +rx_led_failed:
> +	kfree(priv->tx_led_trig_name);
> +	priv->tx_led_trig_name = NULL;
> +tx_led_failed:
> +	return;
> +}
> +EXPORT_SYMBOL_GPL(can_led_init);

Can you provide a devm implementation for can_led?

> +
> +/*
> + * Unregister CAN LED triggers for a CAN device
> + *
> + * This is normally called from a driver's remove function
> + */
> +void can_led_exit(struct net_device *netdev)
> +{
> +	struct can_priv *priv = netdev_priv(netdev);
> +
> +	led_trigger_unregister_simple(priv->tx_led_trig);
> +	led_trigger_unregister_simple(priv->rx_led_trig);
> +
> +	kfree(priv->tx_led_trig_name);
> +	kfree(priv->rx_led_trig_name);
> +}
> +EXPORT_SYMBOL_GPL(can_led_exit);
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 2b2fc34..167b04a 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,13 @@ struct can_priv {
>  
>  	unsigned int echo_skb_max;
>  	struct sk_buff **echo_skb;
> +
> +#ifdef CONFIG_CAN_LEDS
> +	struct led_trigger *tx_led_trig;
> +	char *tx_led_trig_name;
> +	struct led_trigger *rx_led_trig;
> +	char *rx_led_trig_name;
> +#endif
>  };
>  
>  /*
> diff --git a/include/linux/can/led.h b/include/linux/can/led.h
> new file mode 100644
> index 0000000..274c7fd
> --- /dev/null
> +++ b/include/linux/can/led.h
> @@ -0,0 +1,38 @@
> +/*
> + * 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/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
> +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
> 


-- 
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: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support

[Fabio Baltieri]

Hi Marc,

On Mon, Jul 30, 2012 at 11:17:53PM +0200, Marc Kleine-Budde wrote:
> On 07/30/2012 09:20 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
> > 
> > Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> > Cc: Wolfgang Grandegger <wg@grandegger.com>
> > Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> > Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> > ---
> > 
> > Hi all!
> > 
> > This is the v3 of my CAN LED trigger patch.  It's a major refactoring of the v2
> > that was discussed about three months ago concluding with the idea that
> > implementing the oneshot triggering code in the LED framework would be a better
> > solution.
> > 
> > This is the old thread for reference:
> > http://marc.info/?l=linux-can&m=133521499002898&w=2
> > 
> > So, generic oneshot trigger code is now merged in mainline (see 5bb629c), and
> > these are the changes in the v3:
> > 
> > - use the new led_trigger_blink_oneshot() function for LED triggering
> > - use kasprintf() and led_trigger_{un}register_simple for LED allocation
> > - add some usage note in the comments
> > 
> > The resulting code is quite simple now and - I think - a bit less intrusive.
> > Still, I hope on some feedback from the community as I don't have that much
> > hardware to test it - this version has been tested mainly on an x86 with a
> > custom usb-can interface.
> > 
> > In 2/2 there is a sample implementation for the flexcan driver, which is
> > basically unchanged from the old version.
> > 
> > Any comments?
> > Fabio
> 
> Looks quite clean now. Can you provide a devm implementation, too?
> 
> Marc
> > 
> >  drivers/net/can/Kconfig  | 12 ++++++
> >  drivers/net/can/Makefile |  2 +
> >  drivers/net/can/led.c    | 99 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/can/dev.h  |  8 ++++
> >  include/linux/can/led.h  | 38 +++++++++++++++++++
> >  5 files changed, 159 insertions(+)
> >  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..19dec19 100644
> > --- a/drivers/net/can/Kconfig
> > +++ b/drivers/net/can/Kconfig
> > @@ -54,6 +54,18 @@ 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_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..68c4f30
> > --- /dev/null
> > +++ b/drivers/net/can/led.c
> > @@ -0,0 +1,99 @@
> > +/*
> > + * 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/can/led.h>
> > +
> > +static unsigned long led_delay = 50;
> > +module_param(led_delay, ulong, 0644);
> > +MODULE_PARM_DESC(led_delay,
> > +		"blink delay time for activity leds (msecs, default: 50).");
> > +
> > +/*
> > + * Trigger a LED event in response to a CAN device event
> > + */
> > +void can_led_event(struct net_device *netdev, enum can_led_event event)
> > +{
> > +	struct can_priv *priv = netdev_priv(netdev);
> > +
> > +	switch (event) {
> > +	case CAN_LED_EVENT_OPEN:
> > +		led_trigger_event(priv->tx_led_trig, LED_FULL);
> > +		led_trigger_event(priv->rx_led_trig, LED_FULL);
> > +		break;
> > +	case CAN_LED_EVENT_STOP:
> > +		led_trigger_event(priv->tx_led_trig, LED_OFF);
> > +		led_trigger_event(priv->rx_led_trig, LED_OFF);
> > +		break;
> > +	case CAN_LED_EVENT_TX:
> > +		if (led_delay)
> > +			led_trigger_blink_oneshot(priv->tx_led_trig,
> > +						  &led_delay, &led_delay, 1);
> > +		break;
> > +	case CAN_LED_EVENT_RX:
> > +		if (led_delay)
> > +			led_trigger_blink_oneshot(priv->rx_led_trig,
> > +						  &led_delay, &led_delay, 1);
> > +		break;
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(can_led_event);
> > +
> > +/*
> > + * Register CAN LED triggers for a CAN device
> > + *
> > + * This is normally called from a driver's probe function
> > + */
> > +void can_led_init(struct net_device *netdev)
> > +{
> > +	struct can_priv *priv = netdev_priv(netdev);
> > +
> > +	priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);
> > +	if (!priv->tx_led_trig_name)
> > +		goto tx_led_failed;
> > +
> > +	priv->rx_led_trig_name = kasprintf(GFP_KERNEL, "%s-rx", netdev->name);
> > +	if (!priv->rx_led_trig_name)
> > +		goto rx_led_failed;
> > +
> > +	led_trigger_register_simple(priv->tx_led_trig_name,
> > +				    &priv->tx_led_trig);
> > +	led_trigger_register_simple(priv->rx_led_trig_name,
> > +				    &priv->rx_led_trig);
> > +
> > +	return;
> > +
> > +rx_led_failed:
> > +	kfree(priv->tx_led_trig_name);
> > +	priv->tx_led_trig_name = NULL;
> > +tx_led_failed:
> > +	return;
> > +}
> > +EXPORT_SYMBOL_GPL(can_led_init);
> 
> Can you provide a devm implementation for can_led?

Sounds reasonable, you mean like a devm_kasprintf implementation to
remove kfree and unwinding code?

Fabio

> > +
> > +/*
> > + * Unregister CAN LED triggers for a CAN device
> > + *
> > + * This is normally called from a driver's remove function
> > + */
> > +void can_led_exit(struct net_device *netdev)
> > +{
> > +	struct can_priv *priv = netdev_priv(netdev);
> > +
> > +	led_trigger_unregister_simple(priv->tx_led_trig);
> > +	led_trigger_unregister_simple(priv->rx_led_trig);
> > +
> > +	kfree(priv->tx_led_trig_name);
> > +	kfree(priv->rx_led_trig_name);
> > +}
> > +EXPORT_SYMBOL_GPL(can_led_exit);
> > diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> > index 2b2fc34..167b04a 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,13 @@ struct can_priv {
> >  
> >  	unsigned int echo_skb_max;
> >  	struct sk_buff **echo_skb;
> > +
> > +#ifdef CONFIG_CAN_LEDS
> > +	struct led_trigger *tx_led_trig;
> > +	char *tx_led_trig_name;
> > +	struct led_trigger *rx_led_trig;
> > +	char *rx_led_trig_name;
> > +#endif
> >  };
> >  
> >  /*
> > diff --git a/include/linux/can/led.h b/include/linux/can/led.h
> > new file mode 100644
> > index 0000000..274c7fd
> > --- /dev/null
> > +++ b/include/linux/can/led.h
> > @@ -0,0 +1,38 @@
> > +/*
> > + * 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/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
> > +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
> > 
> 
> 
> -- 
> 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   |
> 

Re: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support

[Marc Kleine-Budde]

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

On 07/31/2012 08:57 AM, Fabio Baltieri wrote:
> Hi Marc,
> 
> On Mon, Jul 30, 2012 at 11:17:53PM +0200, Marc Kleine-Budde wrote:
>> On 07/30/2012 09:20 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
>>>
>>> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
>>> Cc: Wolfgang Grandegger <wg@grandegger.com>
>>> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
>>> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
>>> ---
>>>
>>> Hi all!
>>>
>>> This is the v3 of my CAN LED trigger patch.  It's a major refactoring of the v2
>>> that was discussed about three months ago concluding with the idea that
>>> implementing the oneshot triggering code in the LED framework would be a better
>>> solution.
>>>
>>> This is the old thread for reference:
>>> http://marc.info/?l=linux-can&m=133521499002898&w=2
>>>
>>> So, generic oneshot trigger code is now merged in mainline (see 5bb629c), and
>>> these are the changes in the v3:
>>>
>>> - use the new led_trigger_blink_oneshot() function for LED triggering
>>> - use kasprintf() and led_trigger_{un}register_simple for LED allocation
>>> - add some usage note in the comments
>>>
>>> The resulting code is quite simple now and - I think - a bit less intrusive.
>>> Still, I hope on some feedback from the community as I don't have that much
>>> hardware to test it - this version has been tested mainly on an x86 with a
>>> custom usb-can interface.
>>>
>>> In 2/2 there is a sample implementation for the flexcan driver, which is
>>> basically unchanged from the old version.
>>>
>>> Any comments?
>>> Fabio
>>
>> Looks quite clean now. Can you provide a devm implementation, too?
>>
>> Marc
>>>
>>>  drivers/net/can/Kconfig  | 12 ++++++
>>>  drivers/net/can/Makefile |  2 +
>>>  drivers/net/can/led.c    | 99 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/can/dev.h  |  8 ++++
>>>  include/linux/can/led.h  | 38 +++++++++++++++++++
>>>  5 files changed, 159 insertions(+)
>>>  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..19dec19 100644
>>> --- a/drivers/net/can/Kconfig
>>> +++ b/drivers/net/can/Kconfig
>>> @@ -54,6 +54,18 @@ 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_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..68c4f30
>>> --- /dev/null
>>> +++ b/drivers/net/can/led.c
>>> @@ -0,0 +1,99 @@
>>> +/*
>>> + * 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/can/led.h>
>>> +
>>> +static unsigned long led_delay = 50;
>>> +module_param(led_delay, ulong, 0644);
>>> +MODULE_PARM_DESC(led_delay,
>>> +		"blink delay time for activity leds (msecs, default: 50).");
>>> +
>>> +/*
>>> + * Trigger a LED event in response to a CAN device event
>>> + */
>>> +void can_led_event(struct net_device *netdev, enum can_led_event event)
>>> +{
>>> +	struct can_priv *priv = netdev_priv(netdev);
>>> +
>>> +	switch (event) {
>>> +	case CAN_LED_EVENT_OPEN:
>>> +		led_trigger_event(priv->tx_led_trig, LED_FULL);
>>> +		led_trigger_event(priv->rx_led_trig, LED_FULL);
>>> +		break;
>>> +	case CAN_LED_EVENT_STOP:
>>> +		led_trigger_event(priv->tx_led_trig, LED_OFF);
>>> +		led_trigger_event(priv->rx_led_trig, LED_OFF);
>>> +		break;
>>> +	case CAN_LED_EVENT_TX:
>>> +		if (led_delay)
>>> +			led_trigger_blink_oneshot(priv->tx_led_trig,
>>> +						  &led_delay, &led_delay, 1);
>>> +		break;
>>> +	case CAN_LED_EVENT_RX:
>>> +		if (led_delay)
>>> +			led_trigger_blink_oneshot(priv->rx_led_trig,
>>> +						  &led_delay, &led_delay, 1);
>>> +		break;
>>> +	}
>>> +}
>>> +EXPORT_SYMBOL_GPL(can_led_event);
>>> +
>>> +/*
>>> + * Register CAN LED triggers for a CAN device
>>> + *
>>> + * This is normally called from a driver's probe function
>>> + */
>>> +void can_led_init(struct net_device *netdev)
>>> +{
>>> +	struct can_priv *priv = netdev_priv(netdev);
>>> +
>>> +	priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);
>>> +	if (!priv->tx_led_trig_name)
>>> +		goto tx_led_failed;
>>> +
>>> +	priv->rx_led_trig_name = kasprintf(GFP_KERNEL, "%s-rx", netdev->name);
>>> +	if (!priv->rx_led_trig_name)
>>> +		goto rx_led_failed;
>>> +
>>> +	led_trigger_register_simple(priv->tx_led_trig_name,
>>> +				    &priv->tx_led_trig);
>>> +	led_trigger_register_simple(priv->rx_led_trig_name,
>>> +				    &priv->rx_led_trig);
>>> +
>>> +	return;
>>> +
>>> +rx_led_failed:
>>> +	kfree(priv->tx_led_trig_name);
>>> +	priv->tx_led_trig_name = NULL;
>>> +tx_led_failed:
>>> +	return;
>>> +}
>>> +EXPORT_SYMBOL_GPL(can_led_init);
>>
>> Can you provide a devm implementation for can_led?
> 
> Sounds reasonable, you mean like a devm_kasprintf implementation to
> remove kfree and unwinding code?

IMHO it would be sufficient if you implement the devm cleanup functions
here.

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: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support

[Fabio Baltieri]

On Tue, Jul 31, 2012 at 09:10:00AM +0200, Marc Kleine-Budde wrote:
> >>> +/*
> >>> + * Register CAN LED triggers for a CAN device
> >>> + *
> >>> + * This is normally called from a driver's probe function
> >>> + */
> >>> +void can_led_init(struct net_device *netdev)
> >>> +{
> >>> +	struct can_priv *priv = netdev_priv(netdev);
> >>> +
> >>> +	priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);
> >>> +	if (!priv->tx_led_trig_name)
> >>> +		goto tx_led_failed;
> >>> +
> >>> +	priv->rx_led_trig_name = kasprintf(GFP_KERNEL, "%s-rx", netdev->name);
> >>> +	if (!priv->rx_led_trig_name)
> >>> +		goto rx_led_failed;
> >>> +
> >>> +	led_trigger_register_simple(priv->tx_led_trig_name,
> >>> +				    &priv->tx_led_trig);
> >>> +	led_trigger_register_simple(priv->rx_led_trig_name,
> >>> +				    &priv->rx_led_trig);
> >>> +
> >>> +	return;
> >>> +
> >>> +rx_led_failed:
> >>> +	kfree(priv->tx_led_trig_name);
> >>> +	priv->tx_led_trig_name = NULL;
> >>> +tx_led_failed:
> >>> +	return;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(can_led_init);
> >>
> >> Can you provide a devm implementation for can_led?
> > 
> > Sounds reasonable, you mean like a devm_kasprintf implementation to
> > remove kfree and unwinding code?
> 
> IMHO it would be sufficient if you implement the devm cleanup functions
> here.

Uh - can you be more specific?  I mean, are you suggesting to just
convert the code to something like:

unsigned int len;
char *p;

len = snprintf(NULL, 0, "%s-tx", netdev->name);
p = devm_kzalloc(&netdev->dev, len + 1, GFP_KERNEL);
if (!p)
return -ENOMEM;
sprintf(p, len + 1, "%s-tx", netdev->name);

or to implement something with devres_alloc() and a specific
release function?

Fabio

Re: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support

[Marc Kleine-Budde]

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

On 07/31/2012 01:57 PM, Fabio Baltieri wrote:
[...]

>>>> Can you provide a devm implementation for can_led?
>>>
>>> Sounds reasonable, you mean like a devm_kasprintf implementation to
>>> remove kfree and unwinding code?
>>
>> IMHO it would be sufficient if you implement the devm cleanup functions
>> here.
> 
> Uh - can you be more specific?  I mean, are you suggesting to just
> convert the code to something like:
> 
> unsigned int len;
> char *p;
> 
> len = snprintf(NULL, 0, "%s-tx", netdev->name);
> p = devm_kzalloc(&netdev->dev, len + 1, GFP_KERNEL);
> if (!p)
> return -ENOMEM;
> sprintf(p, len + 1, "%s-tx", netdev->name);

This would work, if you just have to free both names, but...

> or to implement something with devres_alloc() and a specific
> release function?

...you have to call led_trigger_unregister_simple(), so we need a custom
release function.

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 --]

[PATCH can-next v4] 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() and can_led_event() as
needed.

Cleanup is handled automatically by devres, so no *_exit function is
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

Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Wolfgang Grandegger <wg@grandegger.com>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
---

Hi!

So, here is the v4.

Changes against v3:
- add a netdev_err message in kasprintf error path
- implement a devres_alloc release function to de-allocate strings and trigger
- can_led_exit is now unneeded and gone

Is that what you had in mind Marc?

I like the devres implementation, as callers don't have to care about exit
function anymore... still it looks like a bit of an hack to me.

Also, I made an initial version with the four pointers in a separate can_led
struct to be allocated as a payload of devres_alloc - but I think this one
looks cleaner and is more cache-friendly.

What you think about this?

Fabio

 drivers/net/can/Kconfig  |  12 ++++++
 drivers/net/can/Makefile |   2 +
 drivers/net/can/led.c    | 102 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/can/dev.h  |   8 ++++
 include/linux/can/led.h  |  34 ++++++++++++++++
 5 files changed, 158 insertions(+)
 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..19dec19 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -54,6 +54,18 @@ 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_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..4fa8a47
--- /dev/null
+++ b/drivers/net/can/led.c
@@ -0,0 +1,102 @@
+/*
+ * 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/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/netdevice.h>
+#include <linux/can/dev.h>
+
+#include <linux/can/led.h>
+
+static unsigned long led_delay = 50;
+module_param(led_delay, ulong, 0644);
+MODULE_PARM_DESC(led_delay,
+		"blink delay time for activity leds (msecs, default: 50).");
+
+/*
+ * Trigger a LED event in response to a CAN device event
+ */
+void can_led_event(struct net_device *netdev, enum can_led_event event)
+{
+	struct can_priv *priv = netdev_priv(netdev);
+
+	switch (event) {
+	case CAN_LED_EVENT_OPEN:
+		led_trigger_event(priv->tx_led_trig, LED_FULL);
+		led_trigger_event(priv->rx_led_trig, LED_FULL);
+		break;
+	case CAN_LED_EVENT_STOP:
+		led_trigger_event(priv->tx_led_trig, LED_OFF);
+		led_trigger_event(priv->rx_led_trig, LED_OFF);
+		break;
+	case CAN_LED_EVENT_TX:
+		if (led_delay)
+			led_trigger_blink_oneshot(priv->tx_led_trig,
+						  &led_delay, &led_delay, 1);
+		break;
+	case CAN_LED_EVENT_RX:
+		if (led_delay)
+			led_trigger_blink_oneshot(priv->rx_led_trig,
+						  &led_delay, &led_delay, 1);
+		break;
+	}
+}
+EXPORT_SYMBOL_GPL(can_led_event);
+
+static void can_led_release(struct device *gendev, void *res)
+{
+	struct can_priv *priv = netdev_priv(to_net_dev(gendev));
+
+	led_trigger_unregister_simple(priv->tx_led_trig);
+	led_trigger_unregister_simple(priv->rx_led_trig);
+
+	kfree(priv->tx_led_trig_name);
+	kfree(priv->rx_led_trig_name);
+}
+
+/*
+ * Register CAN LED triggers for a CAN device
+ *
+ * This is normally called from a driver's probe function
+ */
+void can_led_init(struct net_device *netdev)
+{
+	struct can_priv *priv = netdev_priv(netdev);
+	void *res;
+
+	res = devres_alloc(can_led_release, 0, GFP_KERNEL);
+	if (!res)
+		goto err_out;
+
+	priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);
+	if (!priv->tx_led_trig_name)
+		goto err_out;
+
+	priv->rx_led_trig_name = kasprintf(GFP_KERNEL, "%s-rx", netdev->name);
+	if (!priv->rx_led_trig_name)
+		goto err_out_rx;
+
+	led_trigger_register_simple(priv->tx_led_trig_name,
+				    &priv->tx_led_trig);
+	led_trigger_register_simple(priv->rx_led_trig_name,
+				    &priv->rx_led_trig);
+
+	devres_add(&netdev->dev, res);
+
+	return;
+
+err_out_rx:
+	kfree(priv->tx_led_trig_name);
+	priv->tx_led_trig_name = NULL;
+err_out:
+	netdev_err(netdev, "cannot register LED triggers\n");
+	devres_free(res);
+}
+EXPORT_SYMBOL_GPL(can_led_init);
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 2b2fc34..167b04a 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,13 @@ struct can_priv {
 
 	unsigned int echo_skb_max;
 	struct sk_buff **echo_skb;
+
+#ifdef CONFIG_CAN_LEDS
+	struct led_trigger *tx_led_trig;
+	char *tx_led_trig_name;
+	struct led_trigger *rx_led_trig;
+	char *rx_led_trig_name;
+#endif
 };
 
 /*
diff --git a/include/linux/can/led.h b/include/linux/can/led.h
new file mode 100644
index 0000000..b91982c
--- /dev/null
+++ b/include/linux/can/led.h
@@ -0,0 +1,34 @@
+/*
+ * 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/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
+void can_led_event(struct net_device *netdev, enum can_led_event event);
+void can_led_init(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)
+{
+}
+#endif
+
+#endif
-- 
1.7.11.rc1.9.gf623ca1.dirty

Re: [PATCH can-next v4] can: add tx/rx LED trigger support

[Marc Kleine-Budde]

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

On 08/01/2012 12:05 AM, 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() and can_led_event() as
> needed.
> 
> Cleanup is handled automatically by devres, so no *_exit function is
> 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
> 
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> ---
> 
> Hi!
> 
> So, here is the v4.
> 
> Changes against v3:
> - add a netdev_err message in kasprintf error path
> - implement a devres_alloc release function to de-allocate strings and trigger
> - can_led_exit is now unneeded and gone
> 
> Is that what you had in mind Marc?

Yes, that's it. Can you add a devm_ prefix to can_led_init() please.

> I like the devres implementation, as callers don't have to care about exit
> function anymore... still it looks like a bit of an hack to me.

No :)

> Also, I made an initial version with the four pointers in a separate can_led
> struct to be allocated as a payload of devres_alloc - but I think this one
> looks cleaner and is more cache-friendly.

If you allocate the 4 pointers in a separate struct you only save 3
points, as you need the pointer to the struct anyways.

> What you think about this?

Looks good. See comment inline,

Marc

>  drivers/net/can/Kconfig  |  12 ++++++
>  drivers/net/can/Makefile |   2 +
>  drivers/net/can/led.c    | 102 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/can/dev.h  |   8 ++++
>  include/linux/can/led.h  |  34 ++++++++++++++++
>  5 files changed, 158 insertions(+)
>  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..19dec19 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -54,6 +54,18 @@ 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_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..4fa8a47
> --- /dev/null
> +++ b/drivers/net/can/led.c
> @@ -0,0 +1,102 @@
> +/*
> + * 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/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/netdevice.h>
> +#include <linux/can/dev.h>
> +
> +#include <linux/can/led.h>
> +
> +static unsigned long led_delay = 50;
> +module_param(led_delay, ulong, 0644);
> +MODULE_PARM_DESC(led_delay,
> +		"blink delay time for activity leds (msecs, default: 50).");
> +
> +/*
> + * Trigger a LED event in response to a CAN device event
> + */
> +void can_led_event(struct net_device *netdev, enum can_led_event event)
> +{
> +	struct can_priv *priv = netdev_priv(netdev);
> +
> +	switch (event) {
> +	case CAN_LED_EVENT_OPEN:
> +		led_trigger_event(priv->tx_led_trig, LED_FULL);
> +		led_trigger_event(priv->rx_led_trig, LED_FULL);
> +		break;
> +	case CAN_LED_EVENT_STOP:
> +		led_trigger_event(priv->tx_led_trig, LED_OFF);
> +		led_trigger_event(priv->rx_led_trig, LED_OFF);
> +		break;
> +	case CAN_LED_EVENT_TX:
> +		if (led_delay)
> +			led_trigger_blink_oneshot(priv->tx_led_trig,
> +						  &led_delay, &led_delay, 1);
> +		break;
> +	case CAN_LED_EVENT_RX:
> +		if (led_delay)
> +			led_trigger_blink_oneshot(priv->rx_led_trig,
> +						  &led_delay, &led_delay, 1);
> +		break;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(can_led_event);
> +
> +static void can_led_release(struct device *gendev, void *res)
> +{
> +	struct can_priv *priv = netdev_priv(to_net_dev(gendev));
> +
> +	led_trigger_unregister_simple(priv->tx_led_trig);
> +	led_trigger_unregister_simple(priv->rx_led_trig);
> +
> +	kfree(priv->tx_led_trig_name);
> +	kfree(priv->rx_led_trig_name);
> +}
> +
> +/*
> + * Register CAN LED triggers for a CAN device
> + *
> + * This is normally called from a driver's probe function
> + */
> +void can_led_init(struct net_device *netdev)
> +{
> +	struct can_priv *priv = netdev_priv(netdev);
> +	void *res;
> +
> +	res = devres_alloc(can_led_release, 0, GFP_KERNEL);
                                            ^
I'm not really sure if this is working. For example, pinctrl [1]
allocates a double pointer here. The res pointer here and in
can_led_release simply points to invalid memory. But as long as you
don't dereference it, it should work.

[1] http://lxr.free-electrons.com/source/drivers/pinctrl/core.c#L862

> +	if (!res)
> +		goto err_out;
> +
> +	priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);
> +	if (!priv->tx_led_trig_name)
> +		goto err_out;
> +
> +	priv->rx_led_trig_name = kasprintf(GFP_KERNEL, "%s-rx", netdev->name);
> +	if (!priv->rx_led_trig_name)
> +		goto err_out_rx;
> +
> +	led_trigger_register_simple(priv->tx_led_trig_name,
> +				    &priv->tx_led_trig);
> +	led_trigger_register_simple(priv->rx_led_trig_name,
> +				    &priv->rx_led_trig);
> +
> +	devres_add(&netdev->dev, res);
> +
> +	return;
> +
> +err_out_rx:
> +	kfree(priv->tx_led_trig_name);
> +	priv->tx_led_trig_name = NULL;
> +err_out:
> +	netdev_err(netdev, "cannot register LED triggers\n");
> +	devres_free(res);
> +}
> +EXPORT_SYMBOL_GPL(can_led_init);

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: [PATCH can-next v4] can: add tx/rx LED trigger support

[Marc Kleine-Budde]

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

On 08/01/2012 11:36 AM, Marc Kleine-Budde wrote:
[...]

>> +/*
>> + * Register CAN LED triggers for a CAN device
>> + *
>> + * This is normally called from a driver's probe function
>> + */
>> +void can_led_init(struct net_device *netdev)
>> +{
>> +	struct can_priv *priv = netdev_priv(netdev);
>> +	void *res;
>> +
>> +	res = devres_alloc(can_led_release, 0, GFP_KERNEL);
>                                             ^
> I'm not really sure if this is working. For example, pinctrl [1]
> allocates a double pointer here. The res pointer here and in
> can_led_release simply points to invalid memory. But as long as you
> don't dereference it, it should work.
> 
> [1] http://lxr.free-electrons.com/source/drivers/pinctrl/core.c#L862

The pinctrl usecase if different, pinctrl needs that extra memory
because they cannot get a reference to their pinctrl they have to put.

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: [PATCH can-next v4] can: add tx/rx LED trigger support

[Fabio Baltieri]

On Wed, Aug 01, 2012 at 11:36:51AM +0200, Marc Kleine-Budde wrote:
> On 08/01/2012 12:05 AM, 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() and can_led_event() as
> > needed.
> > 
> > Cleanup is handled automatically by devres, so no *_exit function is
> > 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
> > 
> > Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> > Cc: Wolfgang Grandegger <wg@grandegger.com>
> > Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> > Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> > ---
> > 
> > Hi!
> > 
> > So, here is the v4.
> > 
> > Changes against v3:
> > - add a netdev_err message in kasprintf error path
> > - implement a devres_alloc release function to de-allocate strings and trigger
> > - can_led_exit is now unneeded and gone
> > 
> > Is that what you had in mind Marc?
> 
> Yes, that's it. Can you add a devm_ prefix to can_led_init() please.

Okay.

> 
> > I like the devres implementation, as callers don't have to care about exit
> > function anymore... still it looks like a bit of an hack to me.
> 
> No :)

Fair enough :)

> > Also, I made an initial version with the four pointers in a separate can_led
> > struct to be allocated as a payload of devres_alloc - but I think this one
> > looks cleaner and is more cache-friendly.
> 
> If you allocate the 4 pointers in a separate struct you only save 3
> points, as you need the pointer to the struct anyways.
> 
> > What you think about this?
> 
> Looks good. See comment inline,
> 
> Marc
> 
> > +/*
> > + * Register CAN LED triggers for a CAN device
> > + *
> > + * This is normally called from a driver's probe function
> > + */
> > +void can_led_init(struct net_device *netdev)
> > +{
> > +	struct can_priv *priv = netdev_priv(netdev);
> > +	void *res;
> > +
> > +	res = devres_alloc(can_led_release, 0, GFP_KERNEL);
>                                             ^
> I'm not really sure if this is working. For example, pinctrl [1]
> allocates a double pointer here. The res pointer here and in
> can_led_release simply points to invalid memory. But as long as you
> don't dereference it, it should work.
> 
> [1] http://lxr.free-electrons.com/source/drivers/pinctrl/core.c#L862

Actually that's also used by libata-core (not in lxr yet) and dma-mapping:

http://lxr.free-electrons.com/source/drivers/base/dma-mapping.c#L193

actually, res should point at the end of some internal devres structure,
and is only used as return value in this case.

Of course, in this case the release function can only use the struct
device pointer.

I've run some fail test and ftraced the whole thing and it seems to work
pretty good!  I'll send a v5 with the rename later.

Thanks,
Fabio

Re: [PATCH can-next v4] can: add tx/rx LED trigger support

[Marc Kleine-Budde]

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

On 08/01/2012 12:30 PM, Fabio Baltieri wrote:
[...]
>>> +void can_led_init(struct net_device *netdev)
>>> +{
>>> +	struct can_priv *priv = netdev_priv(netdev);
>>> +	void *res;
>>> +
>>> +	res = devres_alloc(can_led_release, 0, GFP_KERNEL);
>>                                             ^
>> I'm not really sure if this is working. For example, pinctrl [1]
>> allocates a double pointer here. The res pointer here and in
>> can_led_release simply points to invalid memory. But as long as you
>> don't dereference it, it should work.
>>
>> [1] http://lxr.free-electrons.com/source/drivers/pinctrl/core.c#L862
> 
> Actually that's also used by libata-core (not in lxr yet) and dma-mapping:
> 
> http://lxr.free-electrons.com/source/drivers/base/dma-mapping.c#L193
> 
> actually, res should point at the end of some internal devres structure,
> and is only used as return value in this case.
> 
> Of course, in this case the release function can only use the struct
> device pointer.
> 
> I've run some fail test and ftraced the whole thing and it seems to work
> pretty good!  I'll send a v5 with the rename later.

Thanks for checking. I just noticed, the pinctrl usecase is a bit different.

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 --]

[PATCH can-next v5 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 devm_can_led_init() and can_led_event() as
needed.

Cleanup is handled automatically by devres, so no *_exit function is
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

Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Wolfgang Grandegger <wg@grandegger.com>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
---

Hello,

so, v5, renamed can_led_init to devm_can_led_init like other devres based
functions.

Also, I'm reposting implementation into flexcan in 2/2... I have a small
patchset for others embedded can drivers to be tested which I can post in
another thread if this patch is accepted.

Fabio

 drivers/net/can/Kconfig  |  12 ++++++
 drivers/net/can/Makefile |   2 +
 drivers/net/can/led.c    | 102 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/can/dev.h  |   8 ++++
 include/linux/can/led.h  |  34 ++++++++++++++++
 5 files changed, 158 insertions(+)
 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..19dec19 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -54,6 +54,18 @@ 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_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..8b4f6a9
--- /dev/null
+++ b/drivers/net/can/led.c
@@ -0,0 +1,102 @@
+/*
+ * 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/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/netdevice.h>
+#include <linux/can/dev.h>
+
+#include <linux/can/led.h>
+
+static unsigned long led_delay = 50;
+module_param(led_delay, ulong, 0644);
+MODULE_PARM_DESC(led_delay,
+		"blink delay time for activity leds (msecs, default: 50).");
+
+/*
+ * Trigger a LED event in response to a CAN device event
+ */
+void can_led_event(struct net_device *netdev, enum can_led_event event)
+{
+	struct can_priv *priv = netdev_priv(netdev);
+
+	switch (event) {
+	case CAN_LED_EVENT_OPEN:
+		led_trigger_event(priv->tx_led_trig, LED_FULL);
+		led_trigger_event(priv->rx_led_trig, LED_FULL);
+		break;
+	case CAN_LED_EVENT_STOP:
+		led_trigger_event(priv->tx_led_trig, LED_OFF);
+		led_trigger_event(priv->rx_led_trig, LED_OFF);
+		break;
+	case CAN_LED_EVENT_TX:
+		if (led_delay)
+			led_trigger_blink_oneshot(priv->tx_led_trig,
+						  &led_delay, &led_delay, 1);
+		break;
+	case CAN_LED_EVENT_RX:
+		if (led_delay)
+			led_trigger_blink_oneshot(priv->rx_led_trig,
+						  &led_delay, &led_delay, 1);
+		break;
+	}
+}
+EXPORT_SYMBOL_GPL(can_led_event);
+
+static void can_led_release(struct device *gendev, void *res)
+{
+	struct can_priv *priv = netdev_priv(to_net_dev(gendev));
+
+	led_trigger_unregister_simple(priv->tx_led_trig);
+	led_trigger_unregister_simple(priv->rx_led_trig);
+
+	kfree(priv->tx_led_trig_name);
+	kfree(priv->rx_led_trig_name);
+}
+
+/*
+ * Register CAN LED triggers for a CAN device
+ *
+ * This is normally called from a driver's probe function
+ */
+void devm_can_led_init(struct net_device *netdev)
+{
+	struct can_priv *priv = netdev_priv(netdev);
+	void *res;
+
+	res = devres_alloc(can_led_release, 0, GFP_KERNEL);
+	if (!res)
+		goto err_out;
+
+	priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);
+	if (!priv->tx_led_trig_name)
+		goto err_out;
+
+	priv->rx_led_trig_name = kasprintf(GFP_KERNEL, "%s-rx", netdev->name);
+	if (!priv->rx_led_trig_name)
+		goto err_out_rx;
+
+	led_trigger_register_simple(priv->tx_led_trig_name,
+				    &priv->tx_led_trig);
+	led_trigger_register_simple(priv->rx_led_trig_name,
+				    &priv->rx_led_trig);
+
+	devres_add(&netdev->dev, res);
+
+	return;
+
+err_out_rx:
+	kfree(priv->tx_led_trig_name);
+	priv->tx_led_trig_name = NULL;
+err_out:
+	netdev_err(netdev, "cannot register LED triggers\n");
+	devres_free(res);
+}
+EXPORT_SYMBOL_GPL(devm_can_led_init);
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 2b2fc34..167b04a 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,13 @@ struct can_priv {
 
 	unsigned int echo_skb_max;
 	struct sk_buff **echo_skb;
+
+#ifdef CONFIG_CAN_LEDS
+	struct led_trigger *tx_led_trig;
+	char *tx_led_trig_name;
+	struct led_trigger *rx_led_trig;
+	char *rx_led_trig_name;
+#endif
 };
 
 /*
diff --git a/include/linux/can/led.h b/include/linux/can/led.h
new file mode 100644
index 0000000..946059c
--- /dev/null
+++ b/include/linux/can/led.h
@@ -0,0 +1,34 @@
+/*
+ * 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/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
+void can_led_event(struct net_device *netdev, enum can_led_event event);
+void devm_can_led_init(struct net_device *netdev);
+#else
+static inline void can_led_event(struct net_device *netdev,
+				 enum can_led_event event)
+{
+}
+static inline void devm_can_led_init(struct net_device *netdev)
+{
+}
+#endif
+
+#endif
-- 
1.7.11.rc1.9.gf623ca1.dirty

[PATCH can-next v5 2/2] can: flexcan: add LED trigger support

[Fabio Baltieri]

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

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

Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Wolfgang Grandegger <wg@grandegger.com>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
---
 drivers/net/can/flexcan.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index c5f1431..6467fc1 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>
@@ -547,6 +548,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;
 }
 
@@ -635,6 +638,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
 	if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
 		stats->tx_bytes += can_get_echo_skb(dev, 0);
 		stats->tx_packets++;
+		can_led_event(dev, CAN_LED_EVENT_TX);
 		flexcan_write((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
 		netif_wake_queue(dev);
 	}
@@ -844,6 +848,9 @@ static int flexcan_open(struct net_device *dev)
 	err = flexcan_chip_start(dev);
 	if (err)
 		goto out_close;
+
+	can_led_event(dev, CAN_LED_EVENT_OPEN);
+
 	napi_enable(&priv->napi);
 	netif_start_queue(dev);
 
@@ -872,6 +879,8 @@ static int flexcan_close(struct net_device *dev)
 
 	close_candev(dev);
 
+	can_led_event(dev, CAN_LED_EVENT_STOP);
+
 	return 0;
 }
 
@@ -1068,6 +1077,8 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
 		goto failed_register;
 	}
 
+	devm_can_led_init(dev);
+
 	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
 		 priv->base, dev->irq);
 
-- 
1.7.11.rc1.9.gf623ca1.dirty

Re: [PATCH can-next v5 2/2] can: flexcan: add LED trigger support

[Marc Kleine-Budde]

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

On 08/01/2012 01:49 PM, Fabio Baltieri wrote:
> Add support for canbus activity led indicators on flexcan devices by
> calling appropriate can_led_* functions.
> 
> These are only enabled when CONFIG_CAN_LEDS is Y, becomes no-op
> otherwise.
> 
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> ---
>  drivers/net/can/flexcan.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index c5f1431..6467fc1 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>
> @@ -547,6 +548,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;
>  }
>  
> @@ -635,6 +638,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>  	if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
>  		stats->tx_bytes += can_get_echo_skb(dev, 0);
>  		stats->tx_packets++;
> +		can_led_event(dev, CAN_LED_EVENT_TX);

Should the led blink on TX or TX completion interrupt?

>  		flexcan_write((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
>  		netif_wake_queue(dev);
>  	}
> @@ -844,6 +848,9 @@ static int flexcan_open(struct net_device *dev)
>  	err = flexcan_chip_start(dev);
>  	if (err)
>  		goto out_close;
> +
> +	can_led_event(dev, CAN_LED_EVENT_OPEN);
> +
>  	napi_enable(&priv->napi);
>  	netif_start_queue(dev);
>  
> @@ -872,6 +879,8 @@ static int flexcan_close(struct net_device *dev)
>  
>  	close_candev(dev);
>  
> +	can_led_event(dev, CAN_LED_EVENT_STOP);
> +
>  	return 0;
>  }
>  
> @@ -1068,6 +1077,8 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
>  		goto failed_register;
>  	}
>  
> +	devm_can_led_init(dev);
> +
>  	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
>  		 priv->base, dev->irq);
>  
> 

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: [PATCH can-next v5 2/2] can: flexcan: add LED trigger support

[Fabio Baltieri]

On Wed, Aug 01, 2012 at 01:53:22PM +0200, Marc Kleine-Budde wrote:
[...]
> > @@ -635,6 +638,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> >  	if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
> >  		stats->tx_bytes += can_get_echo_skb(dev, 0);
> >  		stats->tx_packets++;
> > +		can_led_event(dev, CAN_LED_EVENT_TX);
> 
> Should the led blink on TX or TX completion interrupt?

I'd say on complention interrupt, together with can_get_echo_skb().

That was briefly discussed with Oliver in my first patch:

http://article.gmane.org/gmane.linux.can/1007

Fabio

Re: [PATCH can-next v5 2/2] can: flexcan: add LED trigger support

[Marc Kleine-Budde]

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

On 08/01/2012 02:24 PM, Fabio Baltieri wrote:
> On Wed, Aug 01, 2012 at 01:53:22PM +0200, Marc Kleine-Budde wrote:
> [...]
>>> @@ -635,6 +638,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>>>  	if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
>>>  		stats->tx_bytes += can_get_echo_skb(dev, 0);
>>>  		stats->tx_packets++;
>>> +		can_led_event(dev, CAN_LED_EVENT_TX);
>>
>> Should the led blink on TX or TX completion interrupt?
> 
> I'd say on complention interrupt, together with can_get_echo_skb().

Yes, good that we already agreed on this :D

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: [PATCH can-next v5 2/2] can: flexcan: add LED trigger support

[Marc Kleine-Budde]

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

On 08/01/2012 01:49 PM, Fabio Baltieri wrote:
> Add support for canbus activity led indicators on flexcan devices by
> calling appropriate can_led_* functions.
> 
> These are only enabled when CONFIG_CAN_LEDS is Y, becomes no-op
> otherwise.
> 
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> ---

Applied to can-next/master

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: [PATCH can-next v5 1/2] can: add tx/rx LED trigger support

[Marc Kleine-Budde]

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

On 08/01/2012 01:49 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 devm_can_led_init() and can_led_event() as
> needed.
> 
> Cleanup is handled automatically by devres, so no *_exit function is
> 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
> 
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>

Applied to can-next/master.

But as net-next is still closed, feel free to send Acked-by or
Tested-by. Or even code improvements.

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: [PATCH can-next v5 1/2] can: add tx/rx LED trigger support

[Oliver Hartkopp]

Sorry for this potentially mangled mail from my webmail access ...

Fabio Baltieri <fabio.baltieri@gmail.com> hat am 1. August 2012 um 13:49
geschrieben:

> +void devm_can_led_init(struct net_device *netdev)
> +{
> +  struct can_priv *priv = netdev_priv(netdev);
> +  void *res;
> +
> +  res = devres_alloc(can_led_release, 0, GFP_KERNEL);
> +  if (!res)
> +          goto err_out;
> +
> +  priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);

IMO putting a string with 8 or 9 bytes into a separate kmalloc memory sniplet is
pretty oversized.
Ok - these functions provide to hide the complexitiy for allocating and storing
strings, which is
definitely fine for path names and these kind of strings that are not known in
length and probably
more than 100 bytes long.

But in this case i would suggest to allocate a fixed space in can_priv, as we
know the string length
very good (IFNAMSZ + strlen("-tx")) and there's no reason to get all the
overhead from three kmallocs
instead of one for that small memory allocations.

So i would suggest:

> @@ -52,6 +53,13 @@ struct can_priv {
> 
>    unsigned int echo_skb_max;
>    struct sk_buff **echo_skb;
> +
> +#ifdef CONFIG_CAN_LEDS
> +  struct led_trigger *tx_led_trig;
> +  char *tx_led_trig_name;

char tx_led_trig_name[IFNAMSZ+4];

> +  struct led_trigger *rx_led_trig;
> +  char *rx_led_trig_name;

char rx_led_trig_name[IFNAMSZ+4];

> +#endif
>  };
> 

Just my two cents as i was in CC here :-)

Thanks for the cool LED support & best regards
Oliver

Re: [PATCH can-next v5 1/2] can: add tx/rx LED trigger support

[Marc Kleine-Budde]

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

On 08/01/2012 02:06 PM, Oliver Hartkopp wrote:
> Sorry for this potentially mangled mail from my webmail access ...
> 
> Fabio Baltieri <fabio.baltieri@gmail.com> hat am 1. August 2012 um 13:49
> geschrieben:
> 
>> +void devm_can_led_init(struct net_device *netdev)
>> +{
>> +  struct can_priv *priv = netdev_priv(netdev);
>> +  void *res;
>> +
>> +  res = devres_alloc(can_led_release, 0, GFP_KERNEL);
>> +  if (!res)
>> +          goto err_out;
>> +
>> +  priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);
> 
> IMO putting a string with 8 or 9 bytes into a separate kmalloc memory sniplet is
> pretty oversized.
> Ok - these functions provide to hide the complexitiy for allocating and storing
> strings, which is
> definitely fine for path names and these kind of strings that are not known in
> length and probably
> more than 100 bytes long.
> 
> But in this case i would suggest to allocate a fixed space in can_priv, as we
> know the string length
> very good (IFNAMSZ + strlen("-tx")) and there's no reason to get all the
> overhead from three kmallocs
> instead of one for that small memory allocations.
> 
> So i would suggest:
> 
>> @@ -52,6 +53,13 @@ struct can_priv {
>>
>>    unsigned int echo_skb_max;
>>    struct sk_buff **echo_skb;
>> +
>> +#ifdef CONFIG_CAN_LEDS
>> +  struct led_trigger *tx_led_trig;
>> +  char *tx_led_trig_name;
> 
> char tx_led_trig_name[IFNAMSZ+4];
> 
>> +  struct led_trigger *rx_led_trig;
>> +  char *rx_led_trig_name;
> 
> char rx_led_trig_name[IFNAMSZ+4];
> 
>> +#endif
>>  };

Sounds reasonable. Please Introduce a #define for "IFNAMSZ + 4" and
document where the 4 comes from. Send a v6 patch, I'll force update the
git repo, after you have Oliver's Acked-by.

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 --]

[PATCH can-next v6] 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 devm_can_led_init() and can_led_event() as
needed.

Cleanup is handled automatically by devres, so no *_exit function is
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

Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Wolfgang Grandegger <wg@grandegger.com>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
---

Hi all,

so, v6, change trigger names for fixed size allocation capped to (IFNAMSIZ + 4)
and removed kasprintf as suggested by Oliver (thanks!).

This also has the side effect of reducing the error path to just one check to
devres_alloc return value... nice!

I've put CAN_LED_NAME_SZ definition with the active function declaration,
but used sizeof(priv->tx_led_trig_name) as snprintf length argument in the
code, as it looks cleaner to me.

I'm not reposting the flexcan patch as it's not affected by the change.

Fabio

 drivers/net/can/Kconfig  | 12 +++++++
 drivers/net/can/Makefile |  2 ++
 drivers/net/can/led.c    | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/can/dev.h  |  8 +++++
 include/linux/can/led.h  | 42 +++++++++++++++++++++++
 5 files changed, 153 insertions(+)
 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..19dec19 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -54,6 +54,18 @@ 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_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..eaa14ac
--- /dev/null
+++ b/drivers/net/can/led.c
@@ -0,0 +1,89 @@
+/*
+ * 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/device.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/netdevice.h>
+#include <linux/can/dev.h>
+
+#include <linux/can/led.h>
+
+static unsigned long led_delay = 50;
+module_param(led_delay, ulong, 0644);
+MODULE_PARM_DESC(led_delay,
+		"blink delay time for activity leds (msecs, default: 50).");
+
+/*
+ * Trigger a LED event in response to a CAN device event
+ */
+void can_led_event(struct net_device *netdev, enum can_led_event event)
+{
+	struct can_priv *priv = netdev_priv(netdev);
+
+	switch (event) {
+	case CAN_LED_EVENT_OPEN:
+		led_trigger_event(priv->tx_led_trig, LED_FULL);
+		led_trigger_event(priv->rx_led_trig, LED_FULL);
+		break;
+	case CAN_LED_EVENT_STOP:
+		led_trigger_event(priv->tx_led_trig, LED_OFF);
+		led_trigger_event(priv->rx_led_trig, LED_OFF);
+		break;
+	case CAN_LED_EVENT_TX:
+		if (led_delay)
+			led_trigger_blink_oneshot(priv->tx_led_trig,
+						  &led_delay, &led_delay, 1);
+		break;
+	case CAN_LED_EVENT_RX:
+		if (led_delay)
+			led_trigger_blink_oneshot(priv->rx_led_trig,
+						  &led_delay, &led_delay, 1);
+		break;
+	}
+}
+EXPORT_SYMBOL_GPL(can_led_event);
+
+static void can_led_release(struct device *gendev, void *res)
+{
+	struct can_priv *priv = netdev_priv(to_net_dev(gendev));
+
+	led_trigger_unregister_simple(priv->tx_led_trig);
+	led_trigger_unregister_simple(priv->rx_led_trig);
+}
+
+/*
+ * Register CAN LED triggers for a CAN device
+ *
+ * This is normally called from a driver's probe function
+ */
+void devm_can_led_init(struct net_device *netdev)
+{
+	struct can_priv *priv = netdev_priv(netdev);
+	void *res;
+
+	res = devres_alloc(can_led_release, 0, GFP_KERNEL);
+	if (!res) {
+		netdev_err(netdev, "cannot register LED triggers\n");
+		return;
+	}
+
+	snprintf(priv->tx_led_trig_name, sizeof(priv->tx_led_trig_name),
+		 "%s-tx", netdev->name);
+	snprintf(priv->rx_led_trig_name, sizeof(priv->rx_led_trig_name),
+		 "%s-rx", netdev->name);
+
+	led_trigger_register_simple(priv->tx_led_trig_name,
+				    &priv->tx_led_trig);
+	led_trigger_register_simple(priv->rx_led_trig_name,
+				    &priv->rx_led_trig);
+
+	devres_add(&netdev->dev, res);
+}
+EXPORT_SYMBOL_GPL(devm_can_led_init);
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 2b2fc34..7747d9b 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,13 @@ struct can_priv {
 
 	unsigned int echo_skb_max;
 	struct sk_buff **echo_skb;
+
+#ifdef CONFIG_CAN_LEDS
+	struct led_trigger *tx_led_trig;
+	char tx_led_trig_name[CAN_LED_NAME_SZ];
+	struct led_trigger *rx_led_trig;
+	char rx_led_trig_name[CAN_LED_NAME_SZ];
+#endif
 };
 
 /*
diff --git a/include/linux/can/led.h b/include/linux/can/led.h
new file mode 100644
index 0000000..12d5549
--- /dev/null
+++ b/include/linux/can/led.h
@@ -0,0 +1,42 @@
+/*
+ * 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/if.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
+
+/* keep space for interface name + "-tx"/"-rx" suffix and null terminator */
+#define CAN_LED_NAME_SZ (IFNAMSIZ + 4)
+
+void can_led_event(struct net_device *netdev, enum can_led_event event);
+void devm_can_led_init(struct net_device *netdev);
+
+#else
+
+static inline void can_led_event(struct net_device *netdev,
+				 enum can_led_event event)
+{
+}
+static inline void devm_can_led_init(struct net_device *netdev)
+{
+}
+
+#endif
+
+#endif
-- 
1.7.11.rc1.9.gf623ca1.dirty

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Marc Kleine-Budde]

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

On 08/01/2012 08:21 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 devm_can_led_init() and can_led_event() as
> needed.
> 
> Cleanup is handled automatically by devres, so no *_exit function is
> 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
> 
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> ---
> 
> Hi all,
> 
> so, v6, change trigger names for fixed size allocation capped to (IFNAMSIZ + 4)
> and removed kasprintf as suggested by Oliver (thanks!).
> 
> This also has the side effect of reducing the error path to just one check to
> devres_alloc return value... nice!
> 
> I've put CAN_LED_NAME_SZ definition with the active function declaration,
> but used sizeof(priv->tx_led_trig_name) as snprintf length argument in the
> code, as it looks cleaner to me.
> 
> I'm not reposting the flexcan patch as it's not affected by the change.

Pushed to can-next/master, it even compiles now, as David has included
some upstream branches.

I'm still taking Tested- and Acked-by for these patches.

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: [PATCH can-next v6] can: add tx/rx LED trigger support

[Fabio Baltieri]

On Wed, Aug 01, 2012 at 11:00:04PM +0200, Marc Kleine-Budde wrote:
> On 08/01/2012 08:21 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 devm_can_led_init() and can_led_event() as
> > needed.
> > 
> > Cleanup is handled automatically by devres, so no *_exit function is
> > 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
> > 
> > Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> > Cc: Wolfgang Grandegger <wg@grandegger.com>
> > Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> > Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> > ---
> > 
> > Hi all,
> > 
> > so, v6, change trigger names for fixed size allocation capped to (IFNAMSIZ + 4)
> > and removed kasprintf as suggested by Oliver (thanks!).
> > 
> > This also has the side effect of reducing the error path to just one check to
> > devres_alloc return value... nice!
> > 
> > I've put CAN_LED_NAME_SZ definition with the active function declaration,
> > but used sizeof(priv->tx_led_trig_name) as snprintf length argument in the
> > code, as it looks cleaner to me.
> > 
> > I'm not reposting the flexcan patch as it's not affected by the change.
> 
> Pushed to can-next/master, it even compiles now, as David has included
> some upstream branches.
> 
> I'm still taking Tested- and Acked-by for these patches.

Nice!  So, I'll start preparing some patch for other embedded CAN
controllers for test/review by developers who have access to the
actual hardware.

In the meantime, thanks to everyone on the list for reviews and ideas!

Cheers!
Fabio

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Oliver Hartkopp]

Fabio Baltieri <fabio.baltieri@gmail.com> hat am 1. August 2012 um 20:21
geschrieben:

> so, v6, change trigger names for fixed size allocation capped to (IFNAMSIZ +
> 4)
> and removed kasprintf as suggested by Oliver (thanks!).
>
> This also has the side effect of reducing the error path to just one check to
> devres_alloc return value... nice!
>
> I've put CAN_LED_NAME_SZ definition with the active function declaration,
> but used sizeof(priv->tx_led_trig_name) as snprintf length argument in the
> code, as it looks cleaner to me.

Yes. Nice improvement.

Thanks Fabio!

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Kurt Van Dijck]

Hello,

I find the CAN led triggers an interesting thing.

And then, this scenario fell crossed my mind:
Imagine I do:
[insert CAN device: can0]
$ ip link set can0 name helga
[insert another CAN device: again 'can0']

Registering 'can0-tx' led trigger will fail for the second CAN device,
since that led trigger name is already reserved for CAN device 'helga'.

I'm not sure how to fix such.
If 'rx' & 'tx' may be combined, reusing the netdev name may be possible?
Just wild thinking ...

Kind regards,
Kurt

On Wed, Aug 01, 2012 at 08:21:38PM +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.
> 
> 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 devm_can_led_init() and can_led_event() as
> needed.
> 
> Cleanup is handled automatically by devres, so no *_exit function is
> 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
> 
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> ---
> 
> Hi all,
> 
> so, v6, change trigger names for fixed size allocation capped to (IFNAMSIZ + 4)
> and removed kasprintf as suggested by Oliver (thanks!).
> 
> This also has the side effect of reducing the error path to just one check to
> devres_alloc return value... nice!
> 
> I've put CAN_LED_NAME_SZ definition with the active function declaration,
> but used sizeof(priv->tx_led_trig_name) as snprintf length argument in the
> code, as it looks cleaner to me.
> 
> I'm not reposting the flexcan patch as it's not affected by the change.
> 
> Fabio
> 
>  drivers/net/can/Kconfig  | 12 +++++++
>  drivers/net/can/Makefile |  2 ++
>  drivers/net/can/led.c    | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/can/dev.h  |  8 +++++
>  include/linux/can/led.h  | 42 +++++++++++++++++++++++
>  5 files changed, 153 insertions(+)
>  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..19dec19 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -54,6 +54,18 @@ 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_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..eaa14ac
> --- /dev/null
> +++ b/drivers/net/can/led.c
> @@ -0,0 +1,89 @@
> +/*
> + * 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/device.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/netdevice.h>
> +#include <linux/can/dev.h>
> +
> +#include <linux/can/led.h>
> +
> +static unsigned long led_delay = 50;
> +module_param(led_delay, ulong, 0644);
> +MODULE_PARM_DESC(led_delay,
> +		"blink delay time for activity leds (msecs, default: 50).");
> +
> +/*
> + * Trigger a LED event in response to a CAN device event
> + */
> +void can_led_event(struct net_device *netdev, enum can_led_event event)
> +{
> +	struct can_priv *priv = netdev_priv(netdev);
> +
> +	switch (event) {
> +	case CAN_LED_EVENT_OPEN:
> +		led_trigger_event(priv->tx_led_trig, LED_FULL);
> +		led_trigger_event(priv->rx_led_trig, LED_FULL);
> +		break;
> +	case CAN_LED_EVENT_STOP:
> +		led_trigger_event(priv->tx_led_trig, LED_OFF);
> +		led_trigger_event(priv->rx_led_trig, LED_OFF);
> +		break;
> +	case CAN_LED_EVENT_TX:
> +		if (led_delay)
> +			led_trigger_blink_oneshot(priv->tx_led_trig,
> +						  &led_delay, &led_delay, 1);
> +		break;
> +	case CAN_LED_EVENT_RX:
> +		if (led_delay)
> +			led_trigger_blink_oneshot(priv->rx_led_trig,
> +						  &led_delay, &led_delay, 1);
> +		break;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(can_led_event);
> +
> +static void can_led_release(struct device *gendev, void *res)
> +{
> +	struct can_priv *priv = netdev_priv(to_net_dev(gendev));
> +
> +	led_trigger_unregister_simple(priv->tx_led_trig);
> +	led_trigger_unregister_simple(priv->rx_led_trig);
> +}
> +
> +/*
> + * Register CAN LED triggers for a CAN device
> + *
> + * This is normally called from a driver's probe function
> + */
> +void devm_can_led_init(struct net_device *netdev)
> +{
> +	struct can_priv *priv = netdev_priv(netdev);
> +	void *res;
> +
> +	res = devres_alloc(can_led_release, 0, GFP_KERNEL);
> +	if (!res) {
> +		netdev_err(netdev, "cannot register LED triggers\n");
> +		return;
> +	}
> +
> +	snprintf(priv->tx_led_trig_name, sizeof(priv->tx_led_trig_name),
> +		 "%s-tx", netdev->name);
> +	snprintf(priv->rx_led_trig_name, sizeof(priv->rx_led_trig_name),
> +		 "%s-rx", netdev->name);
> +
> +	led_trigger_register_simple(priv->tx_led_trig_name,
> +				    &priv->tx_led_trig);
> +	led_trigger_register_simple(priv->rx_led_trig_name,
> +				    &priv->rx_led_trig);
> +
> +	devres_add(&netdev->dev, res);
> +}
> +EXPORT_SYMBOL_GPL(devm_can_led_init);
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 2b2fc34..7747d9b 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,13 @@ struct can_priv {
>  
>  	unsigned int echo_skb_max;
>  	struct sk_buff **echo_skb;
> +
> +#ifdef CONFIG_CAN_LEDS
> +	struct led_trigger *tx_led_trig;
> +	char tx_led_trig_name[CAN_LED_NAME_SZ];
> +	struct led_trigger *rx_led_trig;
> +	char rx_led_trig_name[CAN_LED_NAME_SZ];
> +#endif
>  };
>  
>  /*
> diff --git a/include/linux/can/led.h b/include/linux/can/led.h
> new file mode 100644
> index 0000000..12d5549
> --- /dev/null
> +++ b/include/linux/can/led.h
> @@ -0,0 +1,42 @@
> +/*
> + * 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/if.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
> +
> +/* keep space for interface name + "-tx"/"-rx" suffix and null terminator */
> +#define CAN_LED_NAME_SZ (IFNAMSIZ + 4)
> +
> +void can_led_event(struct net_device *netdev, enum can_led_event event);
> +void devm_can_led_init(struct net_device *netdev);
> +
> +#else
> +
> +static inline void can_led_event(struct net_device *netdev,
> +				 enum can_led_event event)
> +{
> +}
> +static inline void devm_can_led_init(struct net_device *netdev)
> +{
> +}
> +
> +#endif
> +
> +#endif
> -- 
> 1.7.11.rc1.9.gf623ca1.dirty
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Kurt Van Dijck
GRAMMER EiA ELECTRONICS
http://www.eia.be
kurt.van.dijck@eia.be
+32-38708534

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Marc Kleine-Budde]

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

On 08/24/2012 07:10 AM, Kurt Van Dijck wrote:
> Hello,
> 
> I find the CAN led triggers an interesting thing.
> 
> And then, this scenario fell crossed my mind:
> Imagine I do:
> [insert CAN device: can0]
> $ ip link set can0 name helga
> [insert another CAN device: again 'can0']
> 
> Registering 'can0-tx' led trigger will fail for the second CAN device,
> since that led trigger name is already reserved for CAN device 'helga'.

Good point.

> I'm not sure how to fix such.
> If 'rx' & 'tx' may be combined, reusing the netdev name may be possible?
> Just wild thinking ...

I think the device's name (not netdev) is unique in the system and
cannot be changed.

On my device tree enabled mx28 I'm talking about the "80032000.can" in:

> dmesg |grep flexcan
> flexcan 80032000.can: device registered (reg_base=f5032000, irq=8)
> flexcan 80034000.can: device registered (reg_base=f5034000, irq=9)

> $ ls /sys/bus/platform/devices/*can* -d
> /sys/bus/platform/devices/80032000.can
> /sys/bus/platform/devices/80034000.can

regards, 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: [PATCH can-next v6] can: add tx/rx LED trigger support

[Kurt Van Dijck]

On Fri, Aug 24, 2012 at 01:28:16PM +0200, Marc Kleine-Budde wrote:
> On 08/24/2012 07:10 AM, Kurt Van Dijck wrote:
> > Hello,
> > 
> > I find the CAN led triggers an interesting thing.
> > 
> > And then, this scenario fell crossed my mind:
> > Imagine I do:
> > [insert CAN device: can0]
> > $ ip link set can0 name helga
> > [insert another CAN device: again 'can0']
> > 
> > Registering 'can0-tx' led trigger will fail for the second CAN device,
> > since that led trigger name is already reserved for CAN device 'helga'.
> 
> Good point.
> 
> > I'm not sure how to fix such.
> > If 'rx' & 'tx' may be combined, reusing the netdev name may be possible?
> > Just wild thinking ...
> 
> I think the device's name (not netdev) is unique in the system and
> cannot be changed.

but may contain several netdev's ...

> 
> On my device tree enabled mx28 I'm talking about the "80032000.can" in:

You idea triggered another thougt: since control is put in device drivers,
why putting the name in the generic can_dev struct?
A more flexible approach to assign names is the key to success here.
The correct 'works in all conditions' approach is not yet in my sight :-(

Kurt
> 
> regards, 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   |
> 



-- 
Kurt Van Dijck
GRAMMER EiA ELECTRONICS
http://www.eia.be
kurt.van.dijck@eia.be
+32-38708534

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Fabio Baltieri]

Hello Kurt,

On Fri, Aug 24, 2012 at 02:42:48PM +0200, Kurt Van Dijck wrote:
> On Fri, Aug 24, 2012 at 01:28:16PM +0200, Marc Kleine-Budde wrote:
> > On 08/24/2012 07:10 AM, Kurt Van Dijck wrote:
> > > Hello,
> > > 
> > > I find the CAN led triggers an interesting thing.
> > > 
> > > And then, this scenario fell crossed my mind:
> > > Imagine I do:
> > > [insert CAN device: can0]
> > > $ ip link set can0 name helga
> > > [insert another CAN device: again 'can0']
> > > 
> > > Registering 'can0-tx' led trigger will fail for the second CAN device,
> > > since that led trigger name is already reserved for CAN device 'helga'.
> > Good point.

Yep, thanks for pointing that out!

Interface renaming was something I considered when I first wrote the
code and I had the mac80211-led driver in mind, as that driver uses the
phy name and not the netdev one for its triggers.

The reason why I did not care that much in the end is that on SoC based
systems trigger-led association is made at probe time, based on data
either from platform_data or devicetree, so I imagined that once the
kernel is ported to the board and default triggers are set correctly at
boot time, the userspace is free to rename CAN interfaces and nobody
should notice... :^)

The thing I did not consider are hot-plug interfaces mixed with
renaming, such as in the case you pointed out - it's probably not really
common but still possible.

> > > I'm not sure how to fix such.
> > > If 'rx' & 'tx' may be combined, reusing the netdev name may be possible?
> > > Just wild thinking ...
> > 
> > I think the device's name (not netdev) is unique in the system and
> > cannot be changed.
>
> but may contain several netdev's ...

Ouch.

> 
> > 
> > On my device tree enabled mx28 I'm talking about the "80032000.can" in:
> 
> You idea triggered another thougt: since control is put in device drivers,
> why putting the name in the generic can_dev struct?

Why not?  That makes the API easy.

> A more flexible approach to assign names is the key to success here.
> The correct 'works in all conditions' approach is not yet in my sight :-(

Agreed.

What about using a combination of device name + an optional port index
specified in devm_can_led_init()? (something like to platform_device names)
Of course that would require changing the API for libraries like
register_sja1000dev(), to add a port index.

Fabio

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Kurt Van Dijck]

On Sat, Aug 25, 2012 at 12:01:42AM +0200, Fabio Baltieri wrote:
> Hello Kurt,
> 
> On Fri, Aug 24, 2012 at 02:42:48PM +0200, Kurt Van Dijck wrote:
> > On Fri, Aug 24, 2012 at 01:28:16PM +0200, Marc Kleine-Budde wrote:
> > > On 08/24/2012 07:10 AM, Kurt Van Dijck wrote:
> > > > Hello,
> > > > 
> > > > I find the CAN led triggers an interesting thing.
> > > > 
> > > > And then, this scenario fell crossed my mind:
> > > > Imagine I do:
> > > > [insert CAN device: can0]
> > > > $ ip link set can0 name helga
> > > > [insert another CAN device: again 'can0']
> > > > 
> > > > Registering 'can0-tx' led trigger will fail for the second CAN device,
> > > > since that led trigger name is already reserved for CAN device 'helga'.
> > > Good point.
> 
> Yep, thanks for pointing that out!
> 
> Interface renaming was something I considered when I first wrote the
> code and I had the mac80211-led driver in mind, as that driver uses the
> phy name and not the netdev one for its triggers.
> 
> The reason why I did not care that much in the end is that on SoC based
> systems trigger-led association is made at probe time, based on data
> either from platform_data or devicetree, so I imagined that once the
> kernel is ported to the board and default triggers are set correctly at
> boot time, the userspace is free to rename CAN interfaces and nobody
> should notice... :^)
> 
> The thing I did not consider are hot-plug interfaces mixed with
> renaming, such as in the case you pointed out - it's probably not really
> common but still possible.
> 
> > > > I'm not sure how to fix such.
> > > > If 'rx' & 'tx' may be combined, reusing the netdev name may be possible?
> > > > Just wild thinking ...
> > > 
> > > I think the device's name (not netdev) is unique in the system and
> > > cannot be changed.
> >
> > but may contain several netdev's ...
> 
> Ouch.
> 
> > 
> > > 
> > > On my device tree enabled mx28 I'm talking about the "80032000.can" in:
> > 
> > You idea triggered another thougt: since control is put in device drivers,
> > why putting the name in the generic can_dev struct?
> 
> Why not?  That makes the API easy.

The code is not next to the data?
Anyway, I said the above because I think the led names need a per-device approach,
but the data is in common parts. That's where things start being complicated.
I understand the common part now because you mainly addressed the SoC based drivers.

> 
> > A more flexible approach to assign names is the key to success here.
> > The correct 'works in all conditions' approach is not yet in my sight :-(
> 
> Agreed.
> 
> What about using a combination of device name + an optional port index
> specified in devm_can_led_init()? (something like to platform_device names)
> Of course that would require changing the API for libraries like
> register_sja1000dev(), to add a port index.
> 
> Fabio
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Kurt Van Dijck
GRAMMER EiA ELECTRONICS
http://www.eia.be
kurt.van.dijck@eia.be
+32-38708534

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Marc Kleine-Budde]

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

On 08/25/2012 12:01 AM, Fabio Baltieri wrote:
> Hello Kurt,
> 
> On Fri, Aug 24, 2012 at 02:42:48PM +0200, Kurt Van Dijck wrote:
>> On Fri, Aug 24, 2012 at 01:28:16PM +0200, Marc Kleine-Budde wrote:
>>> On 08/24/2012 07:10 AM, Kurt Van Dijck wrote:
>>>> Hello,
>>>>
>>>> I find the CAN led triggers an interesting thing.
>>>>
>>>> And then, this scenario fell crossed my mind:
>>>> Imagine I do:
>>>> [insert CAN device: can0]
>>>> $ ip link set can0 name helga
>>>> [insert another CAN device: again 'can0']
>>>>
>>>> Registering 'can0-tx' led trigger will fail for the second CAN device,
>>>> since that led trigger name is already reserved for CAN device 'helga'.
>>> Good point.
> 
> Yep, thanks for pointing that out!
> 
> Interface renaming was something I considered when I first wrote the
> code and I had the mac80211-led driver in mind, as that driver uses the
> phy name and not the netdev one for its triggers.
> 
> The reason why I did not care that much in the end is that on SoC based
> systems trigger-led association is made at probe time, based on data
> either from platform_data or devicetree, so I imagined that once the
> kernel is ported to the board and default triggers are set correctly at
> boot time, the userspace is free to rename CAN interfaces and nobody
> should notice... :^)
> 
> The thing I did not consider are hot-plug interfaces mixed with
> renaming, such as in the case you pointed out - it's probably not really
> common but still possible.
> 
>>>> I'm not sure how to fix such.
>>>> If 'rx' & 'tx' may be combined, reusing the netdev name may be possible?
>>>> Just wild thinking ...
>>>
>>> I think the device's name (not netdev) is unique in the system and
>>> cannot be changed.
>>
>> but may contain several netdev's ...
> 
> Ouch.

The net->ifindex is unique. But it's only an integer. Usually can0 has a
ifindex != 0, so a simple can%d is contra productive here.

Some pointers to related code:
http://lxr.free-electrons.com/source/drivers/base/core.c#L1847
http://lxr.free-electrons.com/source/drivers/base/core.c#L73
http://lxr.free-electrons.com/source/include/linux/device.h#L695

comments?

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: 259 bytes --]

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Kurt Van Dijck]

On Mon, Sep 03, 2012 at 02:40:39PM +0200, Marc Kleine-Budde wrote:
> On 08/25/2012 12:01 AM, Fabio Baltieri wrote:
> > Hello Kurt,
> > 
> > On Fri, Aug 24, 2012 at 02:42:48PM +0200, Kurt Van Dijck wrote:
> >> On Fri, Aug 24, 2012 at 01:28:16PM +0200, Marc Kleine-Budde wrote:
> >>> On 08/24/2012 07:10 AM, Kurt Van Dijck wrote:
> >>>> Hello,
> >>>>
> >>>> I find the CAN led triggers an interesting thing.
> >>>>
> >>>> And then, this scenario fell crossed my mind:
> >>>> Imagine I do:
> >>>> [insert CAN device: can0]
> >>>> $ ip link set can0 name helga
> >>>> [insert another CAN device: again 'can0']
> >>>>
> >>>> Registering 'can0-tx' led trigger will fail for the second CAN device,
> >>>> since that led trigger name is already reserved for CAN device 'helga'.
> >>> Good point.
> > 
> > Yep, thanks for pointing that out!
> > 
> > Interface renaming was something I considered when I first wrote the
> > code and I had the mac80211-led driver in mind, as that driver uses the
> > phy name and not the netdev one for its triggers.
> > 
> > The reason why I did not care that much in the end is that on SoC based
> > systems trigger-led association is made at probe time, based on data
> > either from platform_data or devicetree, so I imagined that once the
> > kernel is ported to the board and default triggers are set correctly at
> > boot time, the userspace is free to rename CAN interfaces and nobody
> > should notice... :^)
> > 
> > The thing I did not consider are hot-plug interfaces mixed with
> > renaming, such as in the case you pointed out - it's probably not really
> > common but still possible.
> > 
> >>>> I'm not sure how to fix such.
> >>>> If 'rx' & 'tx' may be combined, reusing the netdev name may be possible?
> >>>> Just wild thinking ...
> >>>
> >>> I think the device's name (not netdev) is unique in the system and
> >>> cannot be changed.
> >>
> >> but may contain several netdev's ...
> > 
> > Ouch.
> 
> The net->ifindex is unique. But it's only an integer. Usually can0 has a
> ifindex != 0, so a simple can%d is contra productive here.
> 
> Some pointers to related code:
> http://lxr.free-electrons.com/source/drivers/base/core.c#L1847
> http://lxr.free-electrons.com/source/drivers/base/core.c#L73
> http://lxr.free-electrons.com/source/include/linux/device.h#L695
> 
> comments?

a very recent idea: something with netdevice notifiers and NETDEV_CHANGENAME ...
http://lxr.free-electrons.com/source/net/core/dev.c#L1030

you could: rename the trigger, or if we think it's usefull,
block the netdev rename when its triggers are in use.

Kurt

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Fabio Baltieri]

On Mon, Sep 03, 2012 at 08:13:35PM +0200, Kurt Van Dijck wrote:
> On Mon, Sep 03, 2012 at 02:40:39PM +0200, Marc Kleine-Budde wrote:
> > The net->ifindex is unique. But it's only an integer. Usually can0 has a
> > ifindex != 0, so a simple can%d is contra productive here.
> > 
> > Some pointers to related code:
> > http://lxr.free-electrons.com/source/drivers/base/core.c#L1847
> > http://lxr.free-electrons.com/source/drivers/base/core.c#L73
> > http://lxr.free-electrons.com/source/include/linux/device.h#L695
> > 
> > comments?

That would probabily makes really hard to choose the right
default_trigger for led devices to get to the appropriate CAN LED in
embedded systems, as trigger name would depend from other network
devices and probing order (correct me if I'm wrong).

Something with device name would probaily be more appropriate here.

> 
> a very recent idea: something with netdevice notifiers and NETDEV_CHANGENAME ...
> http://lxr.free-electrons.com/source/net/core/dev.c#L1030
> 
> you could: rename the trigger, or if we think it's usefull,
> block the netdev rename when its triggers are in use.

Blocking the rename looks overkill to me, what about using device name
with an optional "port id" appended to it?  Sounds simpler...

Fabio

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Oliver Hartkopp]

On 03.09.2012 20:29, Fabio Baltieri wrote:

> On Mon, Sep 03, 2012 at 08:13:35PM +0200, Kurt Van Dijck wrote:
>> On Mon, Sep 03, 2012 at 02:40:39PM +0200, Marc Kleine-Budde wrote:
>>> The net->ifindex is unique. But it's only an integer. Usually can0 has a
>>> ifindex != 0, so a simple can%d is contra productive here.
>>>
>>> Some pointers to related code:
>>> http://lxr.free-electrons.com/source/drivers/base/core.c#L1847
>>> http://lxr.free-electrons.com/source/drivers/base/core.c#L73
>>> http://lxr.free-electrons.com/source/include/linux/device.h#L695
>>>
>>> comments?
> 
> That would probabily makes really hard to choose the right
> default_trigger for led devices to get to the appropriate CAN LED in
> embedded systems, as trigger name would depend from other network
> devices and probing order (correct me if I'm wrong).
> 
> Something with device name would probaily be more appropriate here.
> 
>>
>> a very recent idea: something with netdevice notifiers and NETDEV_CHANGENAME ...
>> http://lxr.free-electrons.com/source/net/core/dev.c#L1030
>>
>> you could: rename the trigger, or if we think it's usefull,
>> block the netdev rename when its triggers are in use.
> 
> Blocking the rename looks overkill to me, what about using device name
> with an optional "port id" appended to it?  Sounds simpler...


The name of the device can only be changed when the interface is down.
Is it possible to put some scripting around it to detach and attach the leds
to the interfaces on ifup/ifdown triggers?

Regards,
Oliver

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Kurt Van Dijck]

On Mon, Sep 03, 2012 at 10:54:49PM +0200, Oliver Hartkopp wrote:
> On 03.09.2012 20:29, Fabio Baltieri wrote:
> 
> > On Mon, Sep 03, 2012 at 08:13:35PM +0200, Kurt Van Dijck wrote:
> >> On Mon, Sep 03, 2012 at 02:40:39PM +0200, Marc Kleine-Budde wrote:
> >>> The net->ifindex is unique. But it's only an integer. Usually can0 has a
> >>> ifindex != 0, so a simple can%d is contra productive here.
> >>>
> >>> Some pointers to related code:
> >>> http://lxr.free-electrons.com/source/drivers/base/core.c#L1847
> >>> http://lxr.free-electrons.com/source/drivers/base/core.c#L73
> >>> http://lxr.free-electrons.com/source/include/linux/device.h#L695
> >>>
> >>> comments?
> > 
> > That would probabily makes really hard to choose the right
> > default_trigger for led devices to get to the appropriate CAN LED in
> > embedded systems, as trigger name would depend from other network
> > devices and probing order (correct me if I'm wrong).
> > 
> > Something with device name would probaily be more appropriate here.
> > 
> >>
> >> a very recent idea: something with netdevice notifiers and NETDEV_CHANGENAME ...
> >> http://lxr.free-electrons.com/source/net/core/dev.c#L1030
> >>
> >> you could: rename the trigger, or if we think it's usefull,
> >> block the netdev rename when its triggers are in use.
> > 
> > Blocking the rename looks overkill to me,

renaming a netdev _after_ first attaching led triggers looks stupid to me
anyway.

> > what about using device name
> > with an optional "port id" appended to it?  Sounds simpler...
> 
> 
> The name of the device can only be changed when the interface is down.
> Is it possible to put some scripting around it to detach and attach the leds
> to the interfaces on ifup/ifdown triggers?

Are the led triggers available for using while the netdev is down then?

Kurt

[PATCH] can: rename LED trigger name on netdev renames

[Kurt Van Dijck]

The LED trigger name for CAN devices is based on the initial
CAN device name, but does never change. The LED trigger name
is not guaranteed to be unique in case of hotplugging CAN devices.

This patch tries to address this problem by modifying the
LED trigger name according to the CAN device name when
the latter changes.

This patch is meant as illustration only.
In case of VCAN device rename, a segmentation fault will occur.

Signed-off-by: Kurt Van Dijck <kurt.van.dijck@eia.be>
---
 drivers/net/can/led.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
index eaa14ac..f62f908 100644
--- a/drivers/net/can/led.c
+++ b/drivers/net/can/led.c
@@ -12,6 +12,7 @@
 #include <linux/slab.h>
 #include <linux/netdevice.h>
 #include <linux/can/dev.h>
+#include <linux/if_arp.h>
 
 #include <linux/can/led.h>
 
@@ -87,3 +88,59 @@ void devm_can_led_init(struct net_device *netdev)
 	devres_add(&netdev->dev, res);
 }
 EXPORT_SYMBOL_GPL(devm_can_led_init);
+
+/*
+ * NETDEV rename notifier to rename the associated led triggers too
+ */
+static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
+			void *data)
+{
+	struct net_device *netdev = (struct net_device *)data;
+	struct can_priv *priv = netdev_priv(netdev);
+	int busy = 0;
+
+	if (!net_eq(dev_net(netdev), &init_net))
+		return NOTIFY_DONE;
+
+	if (netdev->type != ARPHRD_CAN)
+		return NOTIFY_DONE;
+
+	if (msg != NETDEV_CHANGENAME)
+		return NOTIFY_DONE;
+
+	read_lock(&priv->tx_led_trig->leddev_list_lock);
+	if (!list_empty(&priv->tx_led_trig->led_cdevs))
+		++busy;
+	read_unlock(&priv->tx_led_trig->leddev_list_lock);
+	read_lock(&priv->rx_led_trig->leddev_list_lock);
+	if (!list_empty(&priv->rx_led_trig->led_cdevs))
+		++busy;
+	read_unlock(&priv->rx_led_trig->leddev_list_lock);
+
+	if (busy)
+		return notifier_from_errno(-EBUSY);
+
+	snprintf(priv->tx_led_trig_name, sizeof(priv->tx_led_trig_name),
+		 "%s-tx", netdev->name);
+	snprintf(priv->rx_led_trig_name, sizeof(priv->rx_led_trig_name),
+		 "%s-rx", netdev->name);
+	return NOTIFY_DONE;
+}
+
+/* notifier block for netdevice event */
+static struct notifier_block can_netdev_notifier __read_mostly = {
+	.notifier_call = can_led_notifier,
+};
+
+static __init int can_led_init(void)
+{
+	return register_netdevice_notifier(&can_netdev_notifier);
+}
+
+static __exit void can_led_exit(void)
+{
+	unregister_netdevice_notifier(&can_netdev_notifier);
+}
+
+module_init(can_led_init);
+module_exit(can_led_exit);
-- 
1.7.10.4

Re: [PATCH] can: rename LED trigger name on netdev renames

[Fabio Baltieri]

Hi Kurt,

thanks for the patch!

On Tue, Sep 04, 2012 at 11:29:11AM +0200, Kurt Van Dijck wrote:
> The LED trigger name for CAN devices is based on the initial
> CAN device name, but does never change. The LED trigger name
> is not guaranteed to be unique in case of hotplugging CAN devices.
> 
> This patch tries to address this problem by modifying the
> LED trigger name according to the CAN device name when
> the latter changes.

That's an ideal solution, and I like the notifier implementation, but I
see a couple of problems with it.

> This patch is meant as illustration only.
> In case of VCAN device rename, a segmentation fault will occur.
> 
> Signed-off-by: Kurt Van Dijck <kurt.van.dijck@eia.be>
> ---
>  drivers/net/can/led.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
> index eaa14ac..f62f908 100644
> --- a/drivers/net/can/led.c
> +++ b/drivers/net/can/led.c
> @@ -12,6 +12,7 @@
>  #include <linux/slab.h>
>  #include <linux/netdevice.h>
>  #include <linux/can/dev.h>
> +#include <linux/if_arp.h>
>  
>  #include <linux/can/led.h>
>  
> @@ -87,3 +88,59 @@ void devm_can_led_init(struct net_device *netdev)
>  	devres_add(&netdev->dev, res);
>  }
>  EXPORT_SYMBOL_GPL(devm_can_led_init);
> +
> +/*
> + * NETDEV rename notifier to rename the associated led triggers too
> + */
> +static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
> +			void *data)
> +{
> +	struct net_device *netdev = (struct net_device *)data;
> +	struct can_priv *priv = netdev_priv(netdev);
> +	int busy = 0;
> +
> +	if (!net_eq(dev_net(netdev), &init_net))
> +		return NOTIFY_DONE;
> +
> +	if (netdev->type != ARPHRD_CAN)
> +		return NOTIFY_DONE;
> +
> +	if (msg != NETDEV_CHANGENAME)
> +		return NOTIFY_DONE;

That's the main problem, which I also got stuck into when I did my first
can-led implementation.  As LED structures are in netdev's private data,
you can only use it if your driver is based on the can-dev API, and
there are no way to be sure of that if you get outside driver's code
itself.

This would give problems with vcan, slcan, and probabily other
non-mainlined drivers.

Is there any way to register a notifier which fires only for devices
registered with devm_can_led_init?

> +	read_lock(&priv->tx_led_trig->leddev_list_lock);
> +	if (!list_empty(&priv->tx_led_trig->led_cdevs))
> +		++busy;
> +	read_unlock(&priv->tx_led_trig->leddev_list_lock);
> +	read_lock(&priv->rx_led_trig->leddev_list_lock);
> +	if (!list_empty(&priv->rx_led_trig->led_cdevs))
> +		++busy;
> +	read_unlock(&priv->rx_led_trig->leddev_list_lock);
> +
> +	if (busy)
> +		return notifier_from_errno(-EBUSY);

As we were discussing in the other thread, I don't think that this is
necessary.

> +	snprintf(priv->tx_led_trig_name, sizeof(priv->tx_led_trig_name),
> +		 "%s-tx", netdev->name);
> +	snprintf(priv->rx_led_trig_name, sizeof(priv->rx_led_trig_name),
> +		 "%s-rx", netdev->name);

I'm afraid that this is going to race against other code using trigger
name, so that's probabily needs a down_write(&led_cdev->trigger_lock),
which is a bit of a problem beacause that's static in led-triggers.c.

> +	return NOTIFY_DONE;
> +}
> +
> +/* notifier block for netdevice event */
> +static struct notifier_block can_netdev_notifier __read_mostly = {
> +	.notifier_call = can_led_notifier,
> +};
> +
> +static __init int can_led_init(void)
> +{
> +	return register_netdevice_notifier(&can_netdev_notifier);
> +}
> +
> +static __exit void can_led_exit(void)
> +{
> +	unregister_netdevice_notifier(&can_netdev_notifier);
> +}
> +
> +module_init(can_led_init);
> +module_exit(can_led_exit);

This breaks the driver when built as a module, as led.c is actually
linked together with can-dev.  The solution is to get it on a module by
its own or just to call init/exit functions from can_dev.

Besides those points I like the idea of renaming the trigger if we can
find a clean way to do that, so I'll try to look at those point further
in the weekend.

Fabio

Re: [PATCH] can: rename LED trigger name on netdev renames

[Oliver Hartkopp]

On 06.09.2012 20:59, Fabio Baltieri wrote:

> Hi Kurt,
> 
> thanks for the patch!
> 
> On Tue, Sep 04, 2012 at 11:29:11AM +0200, Kurt Van Dijck wrote:
>> The LED trigger name for CAN devices is based on the initial
>> CAN device name, but does never change. The LED trigger name
>> is not guaranteed to be unique in case of hotplugging CAN devices.
>>
>> This patch tries to address this problem by modifying the
>> LED trigger name according to the CAN device name when
>> the latter changes.
> 
> That's an ideal solution, and I like the notifier implementation, but I
> see a couple of problems with it.
> 
>> This patch is meant as illustration only.
>> In case of VCAN device rename, a segmentation fault will occur.
>>
>> Signed-off-by: Kurt Van Dijck <kurt.van.dijck@eia.be>
>> ---
>>  drivers/net/can/led.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 57 insertions(+)
>>
>> diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
>> index eaa14ac..f62f908 100644
>> --- a/drivers/net/can/led.c
>> +++ b/drivers/net/can/led.c
>> @@ -12,6 +12,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/netdevice.h>
>>  #include <linux/can/dev.h>
>> +#include <linux/if_arp.h>
>>  
>>  #include <linux/can/led.h>
>>  
>> @@ -87,3 +88,59 @@ void devm_can_led_init(struct net_device *netdev)
>>  	devres_add(&netdev->dev, res);
>>  }
>>  EXPORT_SYMBOL_GPL(devm_can_led_init);
>> +
>> +/*
>> + * NETDEV rename notifier to rename the associated led triggers too
>> + */
>> +static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
>> +			void *data)
>> +{
>> +	struct net_device *netdev = (struct net_device *)data;
>> +	struct can_priv *priv = netdev_priv(netdev);
>> +	int busy = 0;
>> +
>> +	if (!net_eq(dev_net(netdev), &init_net))
>> +		return NOTIFY_DONE;
>> +
>> +	if (netdev->type != ARPHRD_CAN)
>> +		return NOTIFY_DONE;
>> +
>> +	if (msg != NETDEV_CHANGENAME)
>> +		return NOTIFY_DONE;
> 
> That's the main problem, which I also got stuck into when I did my first
> can-led implementation.  As LED structures are in netdev's private data,
> you can only use it if your driver is based on the can-dev API, and
> there are no way to be sure of that if you get outside driver's code
> itself.
> 
> This would give problems with vcan, slcan, and probabily other
> non-mainlined drivers.


Do you think, this is really a problem?

If a driver decides not to use the can-dev framework it has to implement own
solutions or just adopt can-dev.

Regards,
Oliver

> 
> Is there any way to register a notifier which fires only for devices
> registered with devm_can_led_init?
> 

Re: [PATCH] can: rename LED trigger name on netdev renames

[Fabio Baltieri]

On Thu, Sep 06, 2012 at 09:31:07PM +0200, Oliver Hartkopp wrote:
> >> +
> >> +/*
> >> + * NETDEV rename notifier to rename the associated led triggers too
> >> + */
> >> +static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
> >> +			void *data)
> >> +{
> >> +	struct net_device *netdev = (struct net_device *)data;
> >> +	struct can_priv *priv = netdev_priv(netdev);
> >> +	int busy = 0;
> >> +
> >> +	if (!net_eq(dev_net(netdev), &init_net))
> >> +		return NOTIFY_DONE;
> >> +
> >> +	if (netdev->type != ARPHRD_CAN)
> >> +		return NOTIFY_DONE;
> >> +
> >> +	if (msg != NETDEV_CHANGENAME)
> >> +		return NOTIFY_DONE;
> > 
> > That's the main problem, which I also got stuck into when I did my first
> > can-led implementation.  As LED structures are in netdev's private data,
> > you can only use it if your driver is based on the can-dev API, and
> > there are no way to be sure of that if you get outside driver's code
> > itself.
> > 
> > This would give problems with vcan, slcan, and probabily other
> > non-mainlined drivers.
> 
> 
> Do you think, this is really a problem?
> 
> If a driver decides not to use the can-dev framework it has to implement own
> solutions or just adopt can-dev.

Agreed, but this still means that we can't assume that
netdev_priv(netdev) to a netdev where netdev->type == ARPHRD_CAN points
to a struct can_priv, right?

Fabio

Re: [PATCH] can: rename LED trigger name on netdev renames

[Kurt Van Dijck]

On Thu, Sep 06, 2012 at 10:46:00PM +0200, Fabio Baltieri wrote:
> On Thu, Sep 06, 2012 at 09:31:07PM +0200, Oliver Hartkopp wrote:
> > >> +
> > >> +/*
> > >> + * NETDEV rename notifier to rename the associated led triggers too
> > >> + */
> > >> +static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
> > >> +			void *data)
> > >> +{
> > >> +	struct net_device *netdev = (struct net_device *)data;
> > >> +	struct can_priv *priv = netdev_priv(netdev);
> > > 
> > > That's the main problem, which I also got stuck into when I did my first
> > > can-led implementation.  As LED structures are in netdev's private data,
> > > you can only use it if your driver is based on the can-dev API, and
> > > there are no way to be sure of that if you get outside driver's code
> > > itself.
> > > 
> > > This would give problems with vcan, slcan, and probabily other
> > > non-mainlined drivers.
> > 
> > 
> > Do you think, this is really a problem?
> > 
> > If a driver decides not to use the can-dev framework it has to implement own
> > solutions or just adopt can-dev.
> 
> Agreed, but this still means that we can't assume that
> netdev_priv(netdev) to a netdev where netdev->type == ARPHRD_CAN points
> to a struct can_priv, right?

It remains a problem for vcan & slcan.
I tend to start looking at how netlink deals with it.
I see 2 options:
* Invent a new flag for netdev->features, like NETIF_F_CANDEV, to be assigned
  at http://lxr.free-electrons.com/source/drivers/net/can/dev.c#L464

* test for the rtnl_link_ops:
  We put a function can_priv_safe(netdev) in driver/net/can/dev.c that tests
  netdev->rtnl_link_ops == &can_link_ops, like in
  http://lxr.free-electrons.com/source/drivers/net/can/dev.c#L751
  and for candev based drivers, it returns a can_priv*, otherwise NULL.
  I'll prepare something early next week ...

Kind regards,
Kurt

[PATCH v2] can: rename LED trigger name on netdev renames

[Fabio Baltieri]

From: Kurt Van Dijck <kurt.van.dijck@eia.be>

The LED trigger name for CAN devices is based on the initial
CAN device name, but does never change. The LED trigger name
is not guaranteed to be unique in case of hotplugging CAN devices.

This patch tries to address this problem by modifying the
LED trigger name according to the CAN device name when
the latter changes.

This patch is meant as illustration only.
In case of VCAN device rename, a segmentation fault will occur.

v1 - Kurt Van Dijck
v2 - Fabio Baltieri
- remove rename blocking if trigger is bound
- use led-subsystem function for the actual rename (still WiP)
- call init/exit functions from dev.c

Signed-off-by: Kurt Van Dijck <kurt.van.dijck@eia.be>
Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
---

Hi Kurt,

I like the can_priv_safe() idea, so I allowed myself to write a v2 of your
patch (hope you are ok with that - let me know if you think I should just post
a diff) to address the other issues I found and bring back the discussion.

So, that's what I changed:

- remove device rename blocking if trigger is bound: I don't think it's needed
  as we can just rename the trigger and everyone should be happy.  Besides that
  I don't like the idea of using list lock outside led-triggers.

- use led_trigger_rename_static to rename the LED: the actual implementation is
  a locked strcpy to prevent races, I just posted the patch in the linux-leds
  mailing list, so this patch depends on wether that would be accepted.
  Link: http://marc.info/?l=linux-kernel&m=134720454513067&w=2

- call init/exit functions from dev.c: that fixes builds as a module.  I also
  renamed functions to can_led_notifier_{init,exit} to avoid confusion with
  devm_can_led_init.

- left a TODO marker for you for a candev-safe v3 version :-)

As this is starting to look like a jam of patches, I pushed my working branch,
based upon Marc's can-next/led-trigger on:

git://github.com/fabiobaltieri/linux.git can-leds-devel

Let me know what you think of it.

Fabio

 drivers/net/can/dev.c   |  5 +++++
 drivers/net/can/led.c   | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/can/led.h |  9 +++++++++
 3 files changed, 60 insertions(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 963e2cc..f738841 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -25,6 +25,7 @@
 #include <linux/can.h>
 #include <linux/can/dev.h>
 #include <linux/can/netlink.h>
+#include <linux/can/led.h>
 #include <net/rtnetlink.h>
 
 #define MOD_DESC "CAN device driver interface"
@@ -799,6 +800,8 @@ static __init int can_dev_init(void)
 {
 	int err;
 
+	can_led_notifier_init();
+
 	err = rtnl_link_register(&can_link_ops);
 	if (!err)
 		printk(KERN_INFO MOD_DESC "\n");
@@ -810,6 +813,8 @@ module_init(can_dev_init);
 static __exit void can_dev_exit(void)
 {
 	rtnl_link_unregister(&can_link_ops);
+
+	can_led_notifier_exit();
 }
 module_exit(can_dev_exit);
 
diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
index eaa14ac..43ebd47 100644
--- a/drivers/net/can/led.c
+++ b/drivers/net/can/led.c
@@ -12,6 +12,7 @@
 #include <linux/slab.h>
 #include <linux/netdevice.h>
 #include <linux/can/dev.h>
+#include <linux/if_arp.h>
 
 #include <linux/can/led.h>
 
@@ -87,3 +88,48 @@ void devm_can_led_init(struct net_device *netdev)
 	devres_add(&netdev->dev, res);
 }
 EXPORT_SYMBOL_GPL(devm_can_led_init);
+
+/*
+ * NETDEV rename notifier to rename the associated led triggers too
+ */
+static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
+			void *data)
+{
+	struct net_device *netdev = (struct net_device *)data;
+	struct can_priv *priv = netdev_priv(netdev);
+	char name[CAN_LED_NAME_SZ];
+
+	if (!net_eq(dev_net(netdev), &init_net))
+		return NOTIFY_DONE;
+
+	if (netdev->type != ARPHRD_CAN)
+		return NOTIFY_DONE;
+
+	if (msg != NETDEV_CHANGENAME)
+		return NOTIFY_DONE;
+
+	/* TODO: make sure we are can-dev safe */
+
+	snprintf(name, sizeof(name), "%s-tx", netdev->name);
+	led_trigger_rename_static(name, priv->tx_led_trig);
+
+	snprintf(name, sizeof(name), "%s-rx", netdev->name);
+	led_trigger_rename_static(name, priv->rx_led_trig);
+
+	return NOTIFY_DONE;
+}
+
+/* notifier block for netdevice event */
+static struct notifier_block can_netdev_notifier __read_mostly = {
+	.notifier_call = can_led_notifier,
+};
+
+int __init can_led_notifier_init(void)
+{
+	return register_netdevice_notifier(&can_netdev_notifier);
+}
+
+void __exit can_led_notifier_exit(void)
+{
+	unregister_netdevice_notifier(&can_netdev_notifier);
+}
diff --git a/include/linux/can/led.h b/include/linux/can/led.h
index 12d5549..9c1167b 100644
--- a/include/linux/can/led.h
+++ b/include/linux/can/led.h
@@ -26,6 +26,8 @@ enum can_led_event {
 
 void can_led_event(struct net_device *netdev, enum can_led_event event);
 void devm_can_led_init(struct net_device *netdev);
+int __init can_led_notifier_init(void);
+void __exit can_led_notifier_exit(void);
 
 #else
 
@@ -36,6 +38,13 @@ static inline void can_led_event(struct net_device *netdev,
 static inline void devm_can_led_init(struct net_device *netdev)
 {
 }
+static inline int can_led_notifier_init(void)
+{
+	return 0;
+}
+static inline void can_led_notifier_exit(void)
+{
+}
 
 #endif
 
-- 
1.7.11.rc1.9.gf623ca1.dirty

[PATCH] can: export a safe netdev_priv wrapper for candev

[Kurt Van Dijck]

can: export a safe netdev_priv wrapper for candev

In net_device notifier calls, it was impossible to determine
if a CAN device is based on candev in a safe way.
This patch adds such test in order to access candev storage
from within those notifiers.

Signed-off-by: Kurt Van Dijck <kurt.van.dijck@eia.be>

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 963e2cc..6c1e704 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -795,6 +795,18 @@ void unregister_candev(struct net_device *dev)
 }
 EXPORT_SYMBOL_GPL(unregister_candev);
 
+/*
+ * Test if a network device is a candev based device
+ * and return the can_priv* if so.
+ */
+struct can_priv *safe_candev_priv(struct net_device *dev)
+{
+	if ((dev->type != ARPHRD_CAN) || (dev->rtnl_link_ops != &can_link_ops))
+		return NULL;
+
+	return netdev_priv(dev);
+}
+
 static __init int can_dev_init(void)
 {
 	int err;
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 7747d9b..fb0ab65 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -106,6 +106,9 @@ u8 can_len2dlc(u8 len);
 struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);
 void free_candev(struct net_device *dev);
 
+/* a candev safe wrapper around netdev_priv */
+struct can_priv *safe_candev_priv(struct net_device *dev);
+
 int open_candev(struct net_device *dev);
 void close_candev(struct net_device *dev);
 

Re: [PATCH] can: export a safe netdev_priv wrapper for candev

[Oliver Hartkopp]

On 10.09.2012 16:25, Kurt Van Dijck wrote:

> can: export a safe netdev_priv wrapper for candev
> 
> In net_device notifier calls, it was impossible to determine
> if a CAN device is based on candev in a safe way.
> This patch adds such test in order to access candev storage
> from within those notifiers.
> 
> Signed-off-by: Kurt Van Dijck <kurt.van.dijck@eia.be>
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 963e2cc..6c1e704 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -795,6 +795,18 @@ void unregister_candev(struct net_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(unregister_candev);
>  
> +/*
> + * Test if a network device is a candev based device
> + * and return the can_priv* if so.
> + */
> +struct can_priv *safe_candev_priv(struct net_device *dev)
> +{
> +	if ((dev->type != ARPHRD_CAN) || (dev->rtnl_link_ops != &can_link_ops))
> +		return NULL;


Nice idea!

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

> +
> +	return netdev_priv(dev);
> +}
> +
>  static __init int can_dev_init(void)
>  {
>  	int err;
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 7747d9b..fb0ab65 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -106,6 +106,9 @@ u8 can_len2dlc(u8 len);
>  struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);
>  void free_candev(struct net_device *dev);
>  
> +/* a candev safe wrapper around netdev_priv */
> +struct can_priv *safe_candev_priv(struct net_device *dev);
> +
>  int open_candev(struct net_device *dev);
>  void close_candev(struct net_device *dev);
>  

Re: [PATCH] can: export a safe netdev_priv wrapper for candev

[Fabio Baltieri]

Hi Kurt,

On Mon, Sep 10, 2012 at 04:25:07PM +0200, Kurt Van Dijck wrote:
> can: export a safe netdev_priv wrapper for candev
> 
> In net_device notifier calls, it was impossible to determine
> if a CAN device is based on candev in a safe way.
> This patch adds such test in order to access candev storage
> from within those notifiers.
> 
> Signed-off-by: Kurt Van Dijck <kurt.van.dijck@eia.be>
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 963e2cc..6c1e704 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -795,6 +795,18 @@ void unregister_candev(struct net_device *dev)
>  }
>  EXPORT_SYMBOL_GPL(unregister_candev);
>  
> +/*
> + * Test if a network device is a candev based device
> + * and return the can_priv* if so.
> + */
> +struct can_priv *safe_candev_priv(struct net_device *dev)
> +{
> +	if ((dev->type != ARPHRD_CAN) || (dev->rtnl_link_ops != &can_link_ops))
> +		return NULL;
> +
> +	return netdev_priv(dev);
> +}
> +

No EXPORT_SYMBOL_GPL here?

Looks good to me a part from that.

Fabio

-- 
Fabio Baltieri

[PATCH v2] can: export a safe netdev_priv wrapper for candev

[Kurt Van Dijck]

can: export a safe netdev_priv wrapper for candev

In net_device notifier calls, it was impossible to determine
if a CAN device is based on candev in a safe way.
This patch adds such test in order to access candev storage
from within those notifiers.

Signed-off-by: Kurt Van Dijck <kurt.van.dijck@eia.be>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 963e2cc..5112cf3 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -795,6 +795,19 @@ void unregister_candev(struct net_device *dev)
 }
 EXPORT_SYMBOL_GPL(unregister_candev);
 
+/*
+ * Test if a network device is a candev based device
+ * and return the can_priv* if so.
+ */
+struct can_priv *safe_candev_priv(struct net_device *dev)
+{
+	if ((dev->type != ARPHRD_CAN) || (dev->rtnl_link_ops != &can_link_ops))
+		return NULL;
+
+	return netdev_priv(dev);
+}
+EXPORT_SYMBOL_GPL(safe_candev_priv);
+
 static __init int can_dev_init(void)
 {
 	int err;
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 7747d9b..fb0ab65 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -106,6 +106,9 @@ u8 can_len2dlc(u8 len);
 struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);
 void free_candev(struct net_device *dev);
 
+/* a candev safe wrapper around netdev_priv */
+struct can_priv *safe_candev_priv(struct net_device *dev);
+
 int open_candev(struct net_device *dev);
 void close_candev(struct net_device *dev);
 

[PATCH v3] can: rename LED trigger name on netdev renames

[Kurt Van Dijck]

can: rename LED trigger name on netdev renames

The LED trigger name for CAN devices is based on the initial
CAN device name, but does never change. The LED trigger name
is not guaranteed to be unique in case of hotplugging CAN devices.

This patch tries to address this problem by modifying the
LED trigger name according to the CAN device name when
the latter changes.

v1 - Kurt Van Dijck
v2 - Fabio Baltieri
- remove rename blocking if trigger is bound
- use led-subsystem function for the actual rename (still WiP)
- call init/exit functions from dev.c
v3 - Kurt Van Dijck
- safe operation for non-candev based devices (vcan, slcan)
	based on earlier patch

Signed-off-by: Kurt Van Dijck <kurt.van.dijck@eia.be>
Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
--

Fabio,

I followed all your contribution to this patch.

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 6c1e704..55ad320 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -25,6 +25,7 @@
 #include <linux/can.h>
 #include <linux/can/dev.h>
 #include <linux/can/netlink.h>
+#include <linux/can/led.h>
 #include <net/rtnetlink.h>
 
 #define MOD_DESC "CAN device driver interface"
@@ -811,6 +812,10 @@ static __init int can_dev_init(void)
 {
 	int err;
 
+	can_led_notifier_init();
+
+	can_led_notifier_init();
+
 	err = rtnl_link_register(&can_link_ops);
 	if (!err)
 		printk(KERN_INFO MOD_DESC "\n");
@@ -822,6 +827,8 @@ module_init(can_dev_init);
 static __exit void can_dev_exit(void)
 {
 	rtnl_link_unregister(&can_link_ops);
+
+	can_led_notifier_exit();
 }
 module_exit(can_dev_exit);
 
diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
index eaa14ac..1663b28 100644
--- a/drivers/net/can/led.c
+++ b/drivers/net/can/led.c
@@ -1,5 +1,6 @@
 /*
  * Copyright 2012, Fabio Baltieri <fabio.baltieri@gmail.com>
+ * Copyright 2012, Kurt Van Dijck <kurt.van.dijck@eia.be>
  *
  * 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
@@ -87,3 +88,39 @@ void devm_can_led_init(struct net_device *netdev)
 	devres_add(&netdev->dev, res);
 }
 EXPORT_SYMBOL_GPL(devm_can_led_init);
+
+/*
+ * NETDEV rename notifier to rename the associated led triggers too
+ */
+static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
+			void *data)
+{
+	struct net_device *netdev = data;
+	struct can_priv *priv = safe_candev_priv(netdev);
+	char name[CAN_LED_NAME_SZ];
+
+	if (msg == NETDEV_CHANGENAME) {
+		snprintf(name, sizeof(name), "%s-tx", netdev->name);
+		led_trigger_rename_static(name, priv->tx_led_trig);
+        
+		snprintf(name, sizeof(name), "%s-rx", netdev->name);
+		led_trigger_rename_static(name, priv->rx_led_trig);
+	}
+
+	return NOTIFY_DONE;
+}
+
+/* notifier block for netdevice event */
+static struct notifier_block can_netdev_notifier __read_mostly = {
+	.notifier_call = can_led_notifier,
+};
+
+int __init can_led_notifier_init(void)
+{
+	return register_netdevice_notifier(&can_netdev_notifier);
+}
+
+void __exit can_led_notifier_exit(void)
+{
+	unregister_netdevice_notifier(&can_netdev_notifier);
+}
diff --git a/include/linux/can/led.h b/include/linux/can/led.h
index 12d5549..9c1167b 100644
--- a/include/linux/can/led.h
+++ b/include/linux/can/led.h
@@ -26,6 +26,8 @@ enum can_led_event {
 
 void can_led_event(struct net_device *netdev, enum can_led_event event);
 void devm_can_led_init(struct net_device *netdev);
+int __init can_led_notifier_init(void);
+void __exit can_led_notifier_exit(void);
 
 #else
 
@@ -36,6 +38,13 @@ static inline void can_led_event(struct net_device *netdev,
 static inline void devm_can_led_init(struct net_device *netdev)
 {
 }
+static inline int can_led_notifier_init(void)
+{
+	return 0;
+}
+static inline void can_led_notifier_exit(void)
+{
+}
 
 #endif
 

Re: [PATCH v3] can: rename LED trigger name on netdev renames

[Oliver Hartkopp]

On 10.09.2012 16:28, Kurt Van Dijck wrote:

> can: rename LED trigger name on netdev renames
> 
> The LED trigger name for CAN devices is based on the initial
> CAN device name, but does never change. The LED trigger name
> is not guaranteed to be unique in case of hotplugging CAN devices.
> 
> This patch tries to address this problem by modifying the
> LED trigger name according to the CAN device name when
> the latter changes.
> 
> v1 - Kurt Van Dijck
> v2 - Fabio Baltieri
> - remove rename blocking if trigger is bound
> - use led-subsystem function for the actual rename (still WiP)
> - call init/exit functions from dev.c
> v3 - Kurt Van Dijck
> - safe operation for non-candev based devices (vcan, slcan)
> 	based on earlier patch
> 
> Signed-off-by: Kurt Van Dijck <kurt.van.dijck@eia.be>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> --
> 
> Fabio,
> 
> I followed all your contribution to this patch.
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 6c1e704..55ad320 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -25,6 +25,7 @@
>  #include <linux/can.h>
>  #include <linux/can/dev.h>
>  #include <linux/can/netlink.h>
> +#include <linux/can/led.h>
>  #include <net/rtnetlink.h>
>  
>  #define MOD_DESC "CAN device driver interface"
> @@ -811,6 +812,10 @@ static __init int can_dev_init(void)
>  {
>  	int err;
>  
> +	can_led_notifier_init();
> +
> +	can_led_notifier_init();
> +


two times?

>  	err = rtnl_link_register(&can_link_ops);
>  	if (!err)
>  		printk(KERN_INFO MOD_DESC "\n");
> @@ -822,6 +827,8 @@ module_init(can_dev_init);
>  static __exit void can_dev_exit(void)
>  {
>  	rtnl_link_unregister(&can_link_ops);
> +
> +	can_led_notifier_exit();
>  }
>  module_exit(can_dev_exit);
>  
> diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
> index eaa14ac..1663b28 100644
> --- a/drivers/net/can/led.c
> +++ b/drivers/net/can/led.c
> @@ -1,5 +1,6 @@
>  /*
>   * Copyright 2012, Fabio Baltieri <fabio.baltieri@gmail.com>
> + * Copyright 2012, Kurt Van Dijck <kurt.van.dijck@eia.be>
>   *
>   * 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
> @@ -87,3 +88,39 @@ void devm_can_led_init(struct net_device *netdev)
>  	devres_add(&netdev->dev, res);
>  }
>  EXPORT_SYMBOL_GPL(devm_can_led_init);
> +
> +/*
> + * NETDEV rename notifier to rename the associated led triggers too
> + */
> +static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
> +			void *data)
> +{
> +	struct net_device *netdev = data;
> +	struct can_priv *priv = safe_candev_priv(netdev);
> +	char name[CAN_LED_NAME_SZ];
> +


What about

if (!priv)
	return NOTIFY_DONE;

:-)

You created that nice function but you are accessing priv->tx_led_trig below
without checking the output of save_candev_priv() ...

Regards,
Oliver

> +	if (msg == NETDEV_CHANGENAME) {
> +		snprintf(name, sizeof(name), "%s-tx", netdev->name);
> +		led_trigger_rename_static(name, priv->tx_led_trig);
> +        
> +		snprintf(name, sizeof(name), "%s-rx", netdev->name);
> +		led_trigger_rename_static(name, priv->rx_led_trig);
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +/* notifier block for netdevice event */
> +static struct notifier_block can_netdev_notifier __read_mostly = {
> +	.notifier_call = can_led_notifier,
> +};
> +
> +int __init can_led_notifier_init(void)
> +{
> +	return register_netdevice_notifier(&can_netdev_notifier);
> +}
> +
> +void __exit can_led_notifier_exit(void)
> +{
> +	unregister_netdevice_notifier(&can_netdev_notifier);
> +}
> diff --git a/include/linux/can/led.h b/include/linux/can/led.h
> index 12d5549..9c1167b 100644
> --- a/include/linux/can/led.h
> +++ b/include/linux/can/led.h
> @@ -26,6 +26,8 @@ enum can_led_event {
>  
>  void can_led_event(struct net_device *netdev, enum can_led_event event);
>  void devm_can_led_init(struct net_device *netdev);
> +int __init can_led_notifier_init(void);
> +void __exit can_led_notifier_exit(void);
>  
>  #else
>  
> @@ -36,6 +38,13 @@ static inline void can_led_event(struct net_device *netdev,
>  static inline void devm_can_led_init(struct net_device *netdev)
>  {
>  }
> +static inline int can_led_notifier_init(void)
> +{
> +	return 0;
> +}
> +static inline void can_led_notifier_exit(void)
> +{
> +}
>  
>  #endif
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] can: rename LED trigger name on netdev renames

[Fabio Baltieri]

On Mon, Sep 10, 2012 at 08:25:50PM +0200, Oliver Hartkopp wrote:
> On 10.09.2012 16:28, Kurt Van Dijck wrote:
> > Fabio,
> > 
> > I followed all your contribution to this patch.

Thanks!

> > diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> > index 6c1e704..55ad320 100644
> > --- a/drivers/net/can/dev.c
> > +++ b/drivers/net/can/dev.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/can.h>
> >  #include <linux/can/dev.h>
> >  #include <linux/can/netlink.h>
> > +#include <linux/can/led.h>
> >  #include <net/rtnetlink.h>
> >  
> >  #define MOD_DESC "CAN device driver interface"
> > @@ -811,6 +812,10 @@ static __init int can_dev_init(void)
> >  {
> >  	int err;
> >  
> > +	can_led_notifier_init();
> > +
> > +	can_led_notifier_init();
> > +
> 
> 
> two times?
> 
> >  	err = rtnl_link_register(&can_link_ops);
> >  	if (!err)
> >  		printk(KERN_INFO MOD_DESC "\n");
> > @@ -822,6 +827,8 @@ module_init(can_dev_init);
> >  static __exit void can_dev_exit(void)
> >  {
> >  	rtnl_link_unregister(&can_link_ops);
> > +
> > +	can_led_notifier_exit();
> >  }
> >  module_exit(can_dev_exit);
> >  
> > diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
> > index eaa14ac..1663b28 100644
> > --- a/drivers/net/can/led.c
> > +++ b/drivers/net/can/led.c
> > @@ -1,5 +1,6 @@
> >  /*
> >   * Copyright 2012, Fabio Baltieri <fabio.baltieri@gmail.com>
> > + * Copyright 2012, Kurt Van Dijck <kurt.van.dijck@eia.be>
> >   *
> >   * 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
> > @@ -87,3 +88,39 @@ void devm_can_led_init(struct net_device *netdev)
> >  	devres_add(&netdev->dev, res);
> >  }
> >  EXPORT_SYMBOL_GPL(devm_can_led_init);
> > +
> > +/*
> > + * NETDEV rename notifier to rename the associated led triggers too
> > + */
> > +static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
> > +			void *data)
> > +{
> > +	struct net_device *netdev = data;
> > +	struct can_priv *priv = safe_candev_priv(netdev);
> > +	char name[CAN_LED_NAME_SZ];
> > +
> 
> 
> What about
> 
> if (!priv)
> 	return NOTIFY_DONE;
> 
> :-)
> 
> You created that nice function but you are accessing priv->tx_led_trig below
> without checking the output of save_candev_priv() ...

Also, I noticed you removed the namespace check... wasn't it needed?
(just asking, I have no idea).

> > +	if (msg == NETDEV_CHANGENAME) {
> > +		snprintf(name, sizeof(name), "%s-tx", netdev->name);
> > +		led_trigger_rename_static(name, priv->tx_led_trig);
> > +        

whitespaces here? (checkpatch)

Fabio

> > +		snprintf(name, sizeof(name), "%s-rx", netdev->name);
> > +		led_trigger_rename_static(name, priv->rx_led_trig);
> > +	}
> > +
> > +	return NOTIFY_DONE;
> > +}
> > +
> > +/* notifier block for netdevice event */
> > +static struct notifier_block can_netdev_notifier __read_mostly = {
> > +	.notifier_call = can_led_notifier,
> > +};
> > +
> > +int __init can_led_notifier_init(void)
> > +{
> > +	return register_netdevice_notifier(&can_netdev_notifier);
> > +}
> > +
> > +void __exit can_led_notifier_exit(void)
> > +{
> > +	unregister_netdevice_notifier(&can_netdev_notifier);
> > +}
> > diff --git a/include/linux/can/led.h b/include/linux/can/led.h
> > index 12d5549..9c1167b 100644
> > --- a/include/linux/can/led.h
> > +++ b/include/linux/can/led.h
> > @@ -26,6 +26,8 @@ enum can_led_event {
> >  
> >  void can_led_event(struct net_device *netdev, enum can_led_event event);
> >  void devm_can_led_init(struct net_device *netdev);
> > +int __init can_led_notifier_init(void);
> > +void __exit can_led_notifier_exit(void);
> >  
> >  #else
> >  
> > @@ -36,6 +38,13 @@ static inline void can_led_event(struct net_device *netdev,
> >  static inline void devm_can_led_init(struct net_device *netdev)
> >  {
> >  }
> > +static inline int can_led_notifier_init(void)
> > +{
> > +	return 0;
> > +}
> > +static inline void can_led_notifier_exit(void)
> > +{
> > +}
> >  
> >  #endif

-- 
Fabio Baltieri

Re: [PATCH v3] can: rename LED trigger name on netdev renames

[Oliver Hartkopp]

On 10.09.2012 20:40, Fabio Baltieri wrote:

> On Mon, Sep 10, 2012 at 08:25:50PM +0200, Oliver Hartkopp wrote:
>> On 10.09.2012 16:28, Kurt Van Dijck wrote:
>>> Fabio,
>>>
>>> I followed all your contribution to this patch.
> 
> Thanks!
> 
>>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>>> index 6c1e704..55ad320 100644
>>> --- a/drivers/net/can/dev.c
>>> +++ b/drivers/net/can/dev.c
>>> @@ -25,6 +25,7 @@
>>>  #include <linux/can.h>
>>>  #include <linux/can/dev.h>
>>>  #include <linux/can/netlink.h>
>>> +#include <linux/can/led.h>
>>>  #include <net/rtnetlink.h>
>>>  
>>>  #define MOD_DESC "CAN device driver interface"
>>> @@ -811,6 +812,10 @@ static __init int can_dev_init(void)
>>>  {
>>>  	int err;
>>>  
>>> +	can_led_notifier_init();
>>> +
>>> +	can_led_notifier_init();
>>> +
>>
>>
>> two times?
>>
>>>  	err = rtnl_link_register(&can_link_ops);
>>>  	if (!err)
>>>  		printk(KERN_INFO MOD_DESC "\n");
>>> @@ -822,6 +827,8 @@ module_init(can_dev_init);
>>>  static __exit void can_dev_exit(void)
>>>  {
>>>  	rtnl_link_unregister(&can_link_ops);
>>> +
>>> +	can_led_notifier_exit();
>>>  }
>>>  module_exit(can_dev_exit);
>>>  
>>> diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
>>> index eaa14ac..1663b28 100644
>>> --- a/drivers/net/can/led.c
>>> +++ b/drivers/net/can/led.c
>>> @@ -1,5 +1,6 @@
>>>  /*
>>>   * Copyright 2012, Fabio Baltieri <fabio.baltieri@gmail.com>
>>> + * Copyright 2012, Kurt Van Dijck <kurt.van.dijck@eia.be>
>>>   *
>>>   * 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
>>> @@ -87,3 +88,39 @@ void devm_can_led_init(struct net_device *netdev)
>>>  	devres_add(&netdev->dev, res);
>>>  }
>>>  EXPORT_SYMBOL_GPL(devm_can_led_init);
>>> +
>>> +/*
>>> + * NETDEV rename notifier to rename the associated led triggers too
>>> + */
>>> +static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
>>> +			void *data)
>>> +{
>>> +	struct net_device *netdev = data;
>>> +	struct can_priv *priv = safe_candev_priv(netdev);
>>> +	char name[CAN_LED_NAME_SZ];
>>> +
>>
>>
>> What about
>>
>> if (!priv)
>> 	return NOTIFY_DONE;
>>
>> :-)
>>
>> You created that nice function but you are accessing priv->tx_led_trig below
>> without checking the output of save_candev_priv() ...
> 
> Also, I noticed you removed the namespace check... wasn't it needed?
> (just asking, I have no idea).
> 
>>> +	if (msg == NETDEV_CHANGENAME) {


I do have a second remark.

What about always using the notifiers to name the LED triggers?

Looking into

http://lxr.linux.no/#linux+v3.5.3/include/linux/netdevice.h#L1526

we have NETDEV_CHANGENAME but what about

	NETDEV_REGISTER

???

When changing and updating the LED trigger name always inside the notifier, we
should never run into any locking or race conditions ...

What do you think about this idea?

Regards,
Oliver

>>> +		snprintf(name, sizeof(name), "%s-tx", netdev->name);
>>> +		led_trigger_rename_static(name, priv->tx_led_trig);
>>> +        
> 
> whitespaces here? (checkpatch)
> 
> Fabio
> 
>>> +		snprintf(name, sizeof(name), "%s-rx", netdev->name);
>>> +		led_trigger_rename_static(name, priv->rx_led_trig);
>>> +	}
>>> +
>>> +	return NOTIFY_DONE;
>>> +}
>>> +
>>> +/* notifier block for netdevice event */
>>> +static struct notifier_block can_netdev_notifier __read_mostly = {
>>> +	.notifier_call = can_led_notifier,
>>> +};
>>> +
>>> +int __init can_led_notifier_init(void)
>>> +{
>>> +	return register_netdevice_notifier(&can_netdev_notifier);
>>> +}
>>> +
>>> +void __exit can_led_notifier_exit(void)
>>> +{
>>> +	unregister_netdevice_notifier(&can_netdev_notifier);
>>> +}
>>> diff --git a/include/linux/can/led.h b/include/linux/can/led.h
>>> index 12d5549..9c1167b 100644
>>> --- a/include/linux/can/led.h
>>> +++ b/include/linux/can/led.h
>>> @@ -26,6 +26,8 @@ enum can_led_event {
>>>  
>>>  void can_led_event(struct net_device *netdev, enum can_led_event event);
>>>  void devm_can_led_init(struct net_device *netdev);
>>> +int __init can_led_notifier_init(void);
>>> +void __exit can_led_notifier_exit(void);
>>>  
>>>  #else
>>>  
>>> @@ -36,6 +38,13 @@ static inline void can_led_event(struct net_device *netdev,
>>>  static inline void devm_can_led_init(struct net_device *netdev)
>>>  {
>>>  }
>>> +static inline int can_led_notifier_init(void)
>>> +{
>>> +	return 0;
>>> +}
>>> +static inline void can_led_notifier_exit(void)
>>> +{
>>> +}
>>>  
>>>  #endif
> 

Re: [PATCH v3] can: rename LED trigger name on netdev renames

[Kurt Van Dijck]

On Mon, Sep 10, 2012 at 09:01:21PM +0200, Oliver Hartkopp wrote:
> On 10.09.2012 20:40, Fabio Baltieri wrote:
> 
> > On Mon, Sep 10, 2012 at 08:25:50PM +0200, Oliver Hartkopp wrote:
> >> On 10.09.2012 16:28, Kurt Van Dijck wrote:
> >>> Fabio,
> >>>
> >>> I followed all your contribution to this patch.
> > 
> > Thanks!
> > 
> >>
> >> You created that nice function but you are accessing priv->tx_led_trig below
> >> without checking the output of save_candev_priv() ...
> > 
> > Also, I noticed you removed the namespace check... wasn't it needed?
> > (just asking, I have no idea).

I don't actually know.
Anyone?

> > 
> >>> +	if (msg == NETDEV_CHANGENAME) {
> 
> 
> I do have a second remark.
> 
> What about always using the notifiers to name the LED triggers?
> 
> Looking into
> 
> http://lxr.linux.no/#linux+v3.5.3/include/linux/netdevice.h#L1526
> 
> we have NETDEV_CHANGENAME but what about
> 
> 	NETDEV_REGISTER
> 
> ???
> 
> When changing and updating the LED trigger name always inside the notifier, we
> should never run into any locking or race conditions ...
> 
> What do you think about this idea?
> 
But the initial name must have been set properly already in that case...

Kurt

Re: [PATCH v3] can: rename LED trigger name on netdev renames

[Oliver Hartkopp]

On 10.09.2012 22:08, Kurt Van Dijck wrote:

> On Mon, Sep 10, 2012 at 09:01:21PM +0200, Oliver Hartkopp wrote:

>>>>> +	if (msg == NETDEV_CHANGENAME) {
>>
>>
>> I do have a second remark.
>>
>> What about always using the notifiers to name the LED triggers?
>>
>> Looking into
>>
>> http://lxr.linux.no/#linux+v3.5.3/include/linux/netdevice.h#L1526
>>
>> we have NETDEV_CHANGENAME but what about
>>
>> 	NETDEV_REGISTER
>>
>> ???
>>
>> When changing and updating the LED trigger name always inside the notifier, we
>> should never run into any locking or race conditions ...
>>
>> What do you think about this idea?
>>
> But the initial name must have been set properly already in that case...
> 


Yes it is.

See register_netdevice()

http://lxr.linux.no/#linux+v3.5.3/net/core/dev.c#L5453


at the end NETDEV_REGISTER is notified:

http://lxr.linux.no/#linux+v3.5.3/net/core/dev.c#L5552

So it would be pretty elegant to create the LED trigger name in the notifier.
This would allow to remove that code at the current init function too.

Regards,
Oliver

Re: [PATCH v3] can: rename LED trigger name on netdev renames

[Fabio Baltieri]

Hi Oliver,

On Tue, Sep 11, 2012 at 07:42:19AM +0200, Oliver Hartkopp wrote:
> On 10.09.2012 22:08, Kurt Van Dijck wrote:
> > But the initial name must have been set properly already in that case...
> 
> Yes it is.
> 
> See register_netdevice()
> 
> http://lxr.linux.no/#linux+v3.5.3/net/core/dev.c#L5453
> 
> 
> at the end NETDEV_REGISTER is notified:
> 
> http://lxr.linux.no/#linux+v3.5.3/net/core/dev.c#L5552
> 
> So it would be pretty elegant to create the LED trigger name in the notifier.
> This would allow to remove that code at the current init function too.

That would indeed be elegant, but in the current implementation
triggers only works for supported drivers, while putting registration in
the notifier would imply to use it also in unsupported ones, where you
would get a trigger which never triggers.

Is that what you had in mind?

Of course that would simplify another bit trigger integration to the
driver... if the target is to have support for all drivers that's a good
thing, through I think that triggers are mostly useful on embedded drivers.

Cheers,
Fabio

-- 
Fabio Baltieri

Re: [PATCH v3] can: rename LED trigger name on netdev renames

[Kurt Van Dijck]

On Tue, Sep 11, 2012 at 09:13:14AM +0200, Fabio Baltieri wrote:
> Hi Oliver,
> 
> On Tue, Sep 11, 2012 at 07:42:19AM +0200, Oliver Hartkopp wrote:
> > On 10.09.2012 22:08, Kurt Van Dijck wrote:
> > > But the initial name must have been set properly already in that case...
> > 
> > Yes it is.
> > 
> > See register_netdevice()
> > 
> > http://lxr.linux.no/#linux+v3.5.3/net/core/dev.c#L5453
> > 
> > 
> > at the end NETDEV_REGISTER is notified:
> > 
> > http://lxr.linux.no/#linux+v3.5.3/net/core/dev.c#L5552
> > 
> > So it would be pretty elegant to create the LED trigger name in the notifier.
> > This would allow to remove that code at the current init function too.
> 
> That would indeed be elegant, but in the current implementation
> triggers only works for supported drivers, while putting registration in
> the notifier would imply to use it also in unsupported ones, where you
> would get a trigger which never triggers.

I note here another problem: any candev rename causes the notifier to
rename its led triggers, but some have it never registered.

Oliver, your approach is the better. It took a bit for me to see ...
but this requires a bit of reverting in the drivers.

Kind regards,
Kurt

Re: [PATCH v3] can: rename LED trigger name on netdev renames

[Kurt Van Dijck]

On Mon, Sep 10, 2012 at 10:08:44PM +0200, Kurt Van Dijck wrote:
> On Mon, Sep 10, 2012 at 09:01:21PM +0200, Oliver Hartkopp wrote:
> > On 10.09.2012 20:40, Fabio Baltieri wrote:
> > 
> > > On Mon, Sep 10, 2012 at 08:25:50PM +0200, Oliver Hartkopp wrote:
> > >> On 10.09.2012 16:28, Kurt Van Dijck wrote:
> > >>> Fabio,
> > >>>
> > > 
> > > Also, I noticed you removed the namespace check... wasn't it needed?
> > > (just asking, I have no idea).
> 

Moreover, the function tests if the net_device has an associated can_priv.
The namespace it's in is irrelevant then?
I think those tests may be removed in af_can too.

Kurt

[PATCH v4] can: rename LED trigger name on netdev renames

[Kurt Van Dijck]

can: rename LED trigger name on netdev renames

The LED trigger name for CAN devices is based on the initial
CAN device name, but does never change. The LED trigger name
is not guaranteed to be unique in case of hotplugging CAN devices.

This patch tries to address this problem by modifying the
LED trigger name according to the CAN device name when
the latter changes.

v1 - Kurt Van Dijck
v2 - Fabio Baltieri
- remove rename blocking if trigger is bound
- use led-subsystem function for the actual rename (still WiP)
- call init/exit functions from dev.c
v3 - Kurt Van Dijck
- safe operation for non-candev based devices (vcan, slcan)
	based on earlier patch
v4 - Kurt Van Dijck
- trivial patch mistakes fixed

Signed-off-by: Kurt Van Dijck <kurt.van.dijck@eia.be>
Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 5112cf3..df87c28 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -25,6 +25,7 @@
 #include <linux/can.h>
 #include <linux/can/dev.h>
 #include <linux/can/netlink.h>
+#include <linux/can/led.h>
 #include <net/rtnetlink.h>
 
 #define MOD_DESC "CAN device driver interface"
@@ -812,6 +813,8 @@ static __init int can_dev_init(void)
 {
 	int err;
 
+	can_led_notifier_init();
+
 	err = rtnl_link_register(&can_link_ops);
 	if (!err)
 		printk(KERN_INFO MOD_DESC "\n");
@@ -823,6 +826,8 @@ module_init(can_dev_init);
 static __exit void can_dev_exit(void)
 {
 	rtnl_link_unregister(&can_link_ops);
+
+	can_led_notifier_exit();
 }
 module_exit(can_dev_exit);
 
diff --git a/drivers/net/can/led.c b/drivers/net/can/led.c
index eaa14ac..227d359 100644
--- a/drivers/net/can/led.c
+++ b/drivers/net/can/led.c
@@ -1,5 +1,6 @@
 /*
  * Copyright 2012, Fabio Baltieri <fabio.baltieri@gmail.com>
+ * Copyright 2012, Kurt Van Dijck <kurt.van.dijck@eia.be>
  *
  * 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
@@ -87,3 +88,42 @@ void devm_can_led_init(struct net_device *netdev)
 	devres_add(&netdev->dev, res);
 }
 EXPORT_SYMBOL_GPL(devm_can_led_init);
+
+/*
+ * NETDEV rename notifier to rename the associated led triggers too
+ */
+static int can_led_notifier(struct notifier_block *nb, unsigned long msg,
+			void *data)
+{
+	struct net_device *netdev = data;
+	struct can_priv *priv = safe_candev_priv(netdev);
+	char name[CAN_LED_NAME_SZ];
+
+	if (!priv)
+		return NOTIFY_DONE;
+
+	if (msg == NETDEV_CHANGENAME) {
+		snprintf(name, sizeof(name), "%s-tx", netdev->name);
+		led_trigger_rename_static(name, priv->tx_led_trig);
+
+		snprintf(name, sizeof(name), "%s-rx", netdev->name);
+		led_trigger_rename_static(name, priv->rx_led_trig);
+	}
+
+	return NOTIFY_DONE;
+}
+
+/* notifier block for netdevice event */
+static struct notifier_block can_netdev_notifier __read_mostly = {
+	.notifier_call = can_led_notifier,
+};
+
+int __init can_led_notifier_init(void)
+{
+	return register_netdevice_notifier(&can_netdev_notifier);
+}
+
+void __exit can_led_notifier_exit(void)
+{
+	unregister_netdevice_notifier(&can_netdev_notifier);
+}
diff --git a/include/linux/can/led.h b/include/linux/can/led.h
index 12d5549..9c1167b 100644
--- a/include/linux/can/led.h
+++ b/include/linux/can/led.h
@@ -26,6 +26,8 @@ enum can_led_event {
 
 void can_led_event(struct net_device *netdev, enum can_led_event event);
 void devm_can_led_init(struct net_device *netdev);
+int __init can_led_notifier_init(void);
+void __exit can_led_notifier_exit(void);
 
 #else
 
@@ -36,6 +38,13 @@ static inline void can_led_event(struct net_device *netdev,
 static inline void devm_can_led_init(struct net_device *netdev)
 {
 }
+static inline int can_led_notifier_init(void)
+{
+	return 0;
+}
+static inline void can_led_notifier_exit(void)
+{
+}
 
 #endif
 

Re: [PATCH v4] can: rename LED trigger name on netdev renames

[Fabio Baltieri]

Hi Kurt,

On Mon, Sep 10, 2012 at 10:06:48PM +0200, Kurt Van Dijck wrote:
> can: rename LED trigger name on netdev renames
> 
> The LED trigger name for CAN devices is based on the initial
> CAN device name, but does never change. The LED trigger name
> is not guaranteed to be unique in case of hotplugging CAN devices.
> 
> This patch tries to address this problem by modifying the
> LED trigger name according to the CAN device name when
> the latter changes.
> 
> v1 - Kurt Van Dijck
> v2 - Fabio Baltieri
> - remove rename blocking if trigger is bound
> - use led-subsystem function for the actual rename (still WiP)
> - call init/exit functions from dev.c
> v3 - Kurt Van Dijck
> - safe operation for non-candev based devices (vcan, slcan)
> 	based on earlier patch
> v4 - Kurt Van Dijck
> - trivial patch mistakes fixed

Well done, I'm testing the last patches (my can-leds-devel branch on
github) and seems to work good (tested rename, disconnect-reconnect,
vcan rename, rename from udev rule).

Now I think we just have to wait for Bryan for the LED rename patch.

Fabio

-- 
Fabio Baltieri

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Fabio Baltieri]

On Tue, Sep 04, 2012 at 09:11:28AM +0200, Kurt Van Dijck wrote:
> On Mon, Sep 03, 2012 at 10:54:49PM +0200, Oliver Hartkopp wrote:
> > On 03.09.2012 20:29, Fabio Baltieri wrote:
> > 
> > > On Mon, Sep 03, 2012 at 08:13:35PM +0200, Kurt Van Dijck wrote:
> > >> On Mon, Sep 03, 2012 at 02:40:39PM +0200, Marc Kleine-Budde wrote:
> > >>> The net->ifindex is unique. But it's only an integer. Usually can0 has a
> > >>> ifindex != 0, so a simple can%d is contra productive here.
> > >>>
> > >>> Some pointers to related code:
> > >>> http://lxr.free-electrons.com/source/drivers/base/core.c#L1847
> > >>> http://lxr.free-electrons.com/source/drivers/base/core.c#L73
> > >>> http://lxr.free-electrons.com/source/include/linux/device.h#L695
> > >>>
> > >>> comments?
> > > 
> > > That would probabily makes really hard to choose the right
> > > default_trigger for led devices to get to the appropriate CAN LED in
> > > embedded systems, as trigger name would depend from other network
> > > devices and probing order (correct me if I'm wrong).
> > > 
> > > Something with device name would probaily be more appropriate here.
> > > 
> > >>
> > >> a very recent idea: something with netdevice notifiers and NETDEV_CHANGENAME ...
> > >> http://lxr.free-electrons.com/source/net/core/dev.c#L1030
> > >>
> > >> you could: rename the trigger, or if we think it's usefull,
> > >> block the netdev rename when its triggers are in use.
> > > 
> > > Blocking the rename looks overkill to me,
> 
> renaming a netdev _after_ first attaching led triggers looks stupid to me
> anyway.
> 
> > > what about using device name
> > > with an optional "port id" appended to it?  Sounds simpler...
> > 
> > 
> > The name of the device can only be changed when the interface is down.
> > Is it possible to put some scripting around it to detach and attach the leds
> > to the interfaces on ifup/ifdown triggers?
> 
> Are the led triggers available for using while the netdev is down then?

Sure!  On embedded systems triggers are usually attached to actual LEDs
at probe time using default_trigger field of struct led_classdev, and
that can be specified both in machine files or in device tree.

See

http://lxr.free-electrons.com/source/arch/powerpc/boot/dts/mpc8315erdb.dts#L477
http://lxr.free-electrons.com/source/arch/arm/mach-pxa/tosa.c#L576

that's why the trigger name should be predictable, at least at
boot/probe time.

The actual CAN LED trigger also reflects the actual state of the
interface (off if down, on if up, blink on activity).

Fabio

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Kurt Van Dijck]

On Tue, Sep 04, 2012 at 10:15:53PM +0200, Fabio Baltieri wrote:
> On Tue, Sep 04, 2012 at 09:11:28AM +0200, Kurt Van Dijck wrote:
> > On Mon, Sep 03, 2012 at 10:54:49PM +0200, Oliver Hartkopp wrote:
> > > On 03.09.2012 20:29, Fabio Baltieri wrote:
> > > 
> > > 
[...]
> > > The name of the device can only be changed when the interface is down.
> > > Is it possible to put some scripting around it to detach and attach the leds
> > > to the interfaces on ifup/ifdown triggers?
> > 
> > Are the led triggers available for using while the netdev is down then?
> 
> Sure!  On embedded systems triggers are usually attached to actual LEDs
> at probe time using default_trigger field of struct led_classdev, and
> that can be specified both in machine files or in device tree.

I also think that led triggers should be available.
I asked the question because detach & attach leds to interfaces would
indeed break that.

btw, I tried to send a patch tuesday (my first $ git send-email) using
netdev notifiers: did you receive it, and what do you think of it?

Kurt

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Fabio Baltieri]

Hi Kurt,

On Thu, Sep 6, 2012 at 12:33 PM, Kurt Van Dijck <kurt.van.dijck@eia.be> wrote:
> On Tue, Sep 04, 2012 at 10:15:53PM +0200, Fabio Baltieri wrote:
>> On Tue, Sep 04, 2012 at 09:11:28AM +0200, Kurt Van Dijck wrote:
> [...]
>> > > The name of the device can only be changed when the interface is down.
>> > > Is it possible to put some scripting around it to detach and attach the leds
>> > > to the interfaces on ifup/ifdown triggers?
>> >
>> > Are the led triggers available for using while the netdev is down then?
>>
>> Sure!  On embedded systems triggers are usually attached to actual LEDs
>> at probe time using default_trigger field of struct led_classdev, and
>> that can be specified both in machine files or in device tree.
>
> I also think that led triggers should be available.

Right, that's why I think the only way is to use device name.

> I asked the question because detach & attach leds to interfaces would
> indeed break that.

Sure? I think that the trigger would be set again on reattach, as
default_trigger is checked both in led_cdev probe and
trigger_register, see:

http://lxr.free-electrons.com/source/drivers/leds/led-triggers.c#L180

I'll try that tonight.

> btw, I tried to send a patch tuesday (my first $ git send-email) using
> netdev notifiers: did you receive it, and what do you think of it?

Sure, I got it!  I was planning to try that this weekend but I can
give you some comments earlier tonight... sorry for the dealy!

Fabio

-- 
Fabio Baltieri

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Kurt Van Dijck]

On Thu, Sep 06, 2012 at 01:17:39PM +0200, Fabio Baltieri wrote:
> Hi Kurt,
> 
> On Thu, Sep 6, 2012 at 12:33 PM, Kurt Van Dijck <kurt.van.dijck@eia.be> wrote:
> > On Tue, Sep 04, 2012 at 10:15:53PM +0200, Fabio Baltieri wrote:
> >> On Tue, Sep 04, 2012 at 09:11:28AM +0200, Kurt Van Dijck wrote:
> > [...]
> >> > > The name of the device can only be changed when the interface is down.
> >> > > Is it possible to put some scripting around it to detach and attach the leds
> >> > > to the interfaces on ifup/ifdown triggers?
> >> >
> >> > Are the led triggers available for using while the netdev is down then?
> >>
> >> Sure!  On embedded systems triggers are usually attached to actual LEDs
> >> at probe time using default_trigger field of struct led_classdev, and
> >> that can be specified both in machine files or in device tree.
> >
> > I also think that led triggers should be available.
> 
> Right, that's why I think the only way is to use device name.

yes, but it has 2 disadvantages:
* inconvenient. I like 'can0-tx' much more than any device name
  (Sorry I can't get any real example, I'm not at home now).
  And for usecases where the actual device of the CAN iface is
  important, such setup requires udev to assign the proper iface names.
* extends existing function calls like sja1000

> 
> > I asked the question because detach & attach leds to interfaces would
> > indeed break that.
> 
> Sure? I think that the trigger would be set again on reattach, as
> default_trigger is checked both in led_cdev probe and
> trigger_register, see:
> 
> http://lxr.free-electrons.com/source/drivers/leds/led-triggers.c#L180
> 
> I'll try that tonight.

It looks even more inconvenient. It only works as expected when you don't
change the trigger afterwards, but still it is possible (as should be),
so the design of trigger is ... wrong.
example:
When you put another trigger to a led, and have a proper sequence of
'ip link set xxx up' and 'ip link set xxx down', you will end up
with the default_trigger again.
I realize I'm looking at unusual scenario's.

> 
> > btw, I tried to send a patch tuesday (my first $ git send-email) using
> > netdev notifiers: did you receive it, and what do you think of it?
> 
> Sure, I got it!
Ok, well done for me then :-)
> I was planning to try that this weekend but I can
> give you some comments earlier tonight... sorry for the dealy!
no hurry, I'm already satified that I got git setup right.

Kurt

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Fabio Baltieri]

On Thu, Sep 06, 2012 at 05:11:58PM +0200, Kurt Van Dijck wrote:
> > > I also think that led triggers should be available.
> > 
> > Right, that's why I think the only way is to use device name.
> 
> yes, but it has 2 disadvantages:
> * inconvenient. I like 'can0-tx' much more than any device name
>   (Sorry I can't get any real example, I'm not at home now).
>   And for usecases where the actual device of the CAN iface is
>   important, such setup requires udev to assign the proper iface names.

Can't argue with that... I'm trying to see how it comes but names like
"can-3-2:1.0-tx" doesn't looks that friendly to me...

> * extends existing function calls like sja1000

Indeed.

> > > I asked the question because detach & attach leds to interfaces would
> > > indeed break that.
> > 
> > Sure? I think that the trigger would be set again on reattach, as
> > default_trigger is checked both in led_cdev probe and
> > trigger_register, see:
> > 
> > http://lxr.free-electrons.com/source/drivers/leds/led-triggers.c#L180
> > 
> > I'll try that tonight.
> 
> It looks even more inconvenient. It only works as expected when you don't
> change the trigger afterwards, but still it is possible (as should be),
> so the design of trigger is ... wrong.
> example:
> When you put another trigger to a led, and have a proper sequence of
> 'ip link set xxx up' and 'ip link set xxx down', you will end up
> with the default_trigger again.
> I realize I'm looking at unusual scenario's.

That's not correct, default trigger is going to be set only if there are
no other trigger assigned to the LED, and only on led probe and trigger
probe.

So, the LED framework is not going to change the trigger if you manually
changed it to something else, and in any case default_trigger assignment
only happens at probe/exit, not when interface is set up/down.

> > > btw, I tried to send a patch tuesday (my first $ git send-email) using
> > > netdev notifiers: did you receive it, and what do you think of it?
> > 
> > Sure, I got it!
> Ok, well done for me then :-)
> > I was planning to try that this weekend but I can
> > give you some comments earlier tonight... sorry for the dealy!
> no hurry, I'm already satified that I got git setup right.

Way to go! :-)

Fabio

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Kurt Van Dijck]

On Thu, Sep 06, 2012 at 10:57:28PM +0200, Fabio Baltieri wrote:
> On Thu, Sep 06, 2012 at 05:11:58PM +0200, Kurt Van Dijck wrote:
> > > > I also think that led triggers should be available.
> > > 
> > > Right, that's why I think the only way is to use device name.
> > 
> > yes, but it has 2 disadvantages:
> > * inconvenient. I like 'can0-tx' much more than any device name
> >   (Sorry I can't get any real example, I'm not at home now).
> >   And for usecases where the actual device of the CAN iface is
> >   important, such setup requires udev to assign the proper iface names.
> 
> Can't argue with that... I'm trying to see how it comes but names like
> "can-3-2:1.0-tx" doesn't looks that friendly to me...

"can-3-2:1.0" is an iface name?
We must keep default iface names can%d. Only in the usecase that someone
wants to name its ifaces according device names, he/she should manually
adjust udev rules on his/her system ...

And if the iface is names "can-3-2:1.0", "can-3-2:1.0-tx" _is_
a friendly trigger name.

> 
> > * extends existing function calls like sja1000
> 
> Indeed.
> 
> > > > I asked the question because detach & attach leds to interfaces would
> > > > indeed break that.
> > > 
> > > Sure? I think that the trigger would be set again on reattach, as
> > > default_trigger is checked both in led_cdev probe and
> > > trigger_register, see:
> > > 
> > > http://lxr.free-electrons.com/source/drivers/leds/led-triggers.c#L180
> > > 
> > > I'll try that tonight.
> > 
> > It looks even more inconvenient. It only works as expected when you don't
> > change the trigger afterwards, but still it is possible (as should be),
> > so the design of trigger is ... wrong.
> > example:
> > When you put another trigger to a led, and have a proper sequence of
> > 'ip link set xxx up' and 'ip link set xxx down', you will end up
> > with the default_trigger again.
> > I realize I'm looking at unusual scenario's.
> 
> That's not correct, default trigger is going to be set only if there are
> no other trigger assigned to the LED, and only on led probe and trigger
> probe.
> 
> So, the LED framework is not going to change the trigger if you manually
> changed it to something else, and in any case default_trigger assignment
> only happens at probe/exit, not when interface is set up/down.

I think you're also arguing against a register/unregister during iface up/down.
We agree.

Kurt

Re: [PATCH can-next v6] can: add tx/rx LED trigger support

[Fabio Baltieri]

On Fri, Sep 07, 2012 at 09:04:19AM +0200, Kurt Van Dijck wrote:
> On Thu, Sep 06, 2012 at 10:57:28PM +0200, Fabio Baltieri wrote:
> > On Thu, Sep 06, 2012 at 05:11:58PM +0200, Kurt Van Dijck wrote:
> > Can't argue with that... I'm trying to see how it comes but names like
> > "can-3-2:1.0-tx" doesn't looks that friendly to me...
> 
> "can-3-2:1.0" is an iface name?

No, I was trying some combination using device name instead of netdev
one (which remains can0), here 3-2:1.0 is the usb path, looks really bad
to be used for dev name if netdev name is still can0 - no way of
determine the link between the two.

> We must keep default iface names can%d. Only in the usecase that someone
> wants to name its ifaces according device names, he/she should manually
> adjust udev rules on his/her system ...
> 
> And if the iface is names "can-3-2:1.0", "can-3-2:1.0-tx" _is_
> a friendly trigger name.

Agreed.

> > > It looks even more inconvenient. It only works as expected when you don't
> > > change the trigger afterwards, but still it is possible (as should be),
> > > so the design of trigger is ... wrong.
> > > example:
> > > When you put another trigger to a led, and have a proper sequence of
> > > 'ip link set xxx up' and 'ip link set xxx down', you will end up
> > > with the default_trigger again.
> > > I realize I'm looking at unusual scenario's.
> > 
> > That's not correct, default trigger is going to be set only if there are
> > no other trigger assigned to the LED, and only on led probe and trigger
> > probe.
> > 
> > So, the LED framework is not going to change the trigger if you manually
> > changed it to something else, and in any case default_trigger assignment
> > only happens at probe/exit, not when interface is set up/down.
> 
> I think you're also arguing against a register/unregister during iface up/down.
> We agree.

Uh - yes! :-)

Fabio

Re: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support

[Wolfgang Grandegger]

Hi Fabio,

On 07/30/2012 09:20 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
> 
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> ---
> 
> Hi all!
> 
> This is the v3 of my CAN LED trigger patch.  It's a major refactoring of the v2
> that was discussed about three months ago concluding with the idea that
> implementing the oneshot triggering code in the LED framework would be a better
> solution.
> 
> This is the old thread for reference:
> http://marc.info/?l=linux-can&m=133521499002898&w=2
> 
> So, generic oneshot trigger code is now merged in mainline (see 5bb629c), and
> these are the changes in the v3:

Nice, thanks for your patience (and not giving up).

> 
> - use the new led_trigger_blink_oneshot() function for LED triggering
> - use kasprintf() and led_trigger_{un}register_simple for LED allocation
> - add some usage note in the comments
> 
> The resulting code is quite simple now and - I think - a bit less intrusive.
> Still, I hope on some feedback from the community as I don't have that much
> hardware to test it - this version has been tested mainly on an x86 with a
> custom usb-can interface.
> 
> In 2/2 there is a sample implementation for the flexcan driver, which is
> basically unchanged from the old version.
> 
> Any comments?
> Fabio
> 
>  drivers/net/can/Kconfig  | 12 ++++++
>  drivers/net/can/Makefile |  2 +
>  drivers/net/can/led.c    | 99 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/can/dev.h  |  8 ++++
>  include/linux/can/led.h  | 38 +++++++++++++++++++
>  5 files changed, 159 insertions(+)
>  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..19dec19 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -54,6 +54,18 @@ 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_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..68c4f30
> --- /dev/null
> +++ b/drivers/net/can/led.c
> @@ -0,0 +1,99 @@
> +/*
> + * 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/can/led.h>
> +
> +static unsigned long led_delay = 50;
> +module_param(led_delay, ulong, 0644);
> +MODULE_PARM_DESC(led_delay,
> +		"blink delay time for activity leds (msecs, default: 50).");
> +
> +/*
> + * Trigger a LED event in response to a CAN device event
> + */
> +void can_led_event(struct net_device *netdev, enum can_led_event event)
> +{
> +	struct can_priv *priv = netdev_priv(netdev);
> +
> +	switch (event) {
> +	case CAN_LED_EVENT_OPEN:
> +		led_trigger_event(priv->tx_led_trig, LED_FULL);
> +		led_trigger_event(priv->rx_led_trig, LED_FULL);
> +		break;
> +	case CAN_LED_EVENT_STOP:
> +		led_trigger_event(priv->tx_led_trig, LED_OFF);
> +		led_trigger_event(priv->rx_led_trig, LED_OFF);
> +		break;
> +	case CAN_LED_EVENT_TX:
> +		if (led_delay)
> +			led_trigger_blink_oneshot(priv->tx_led_trig,
> +						  &led_delay, &led_delay, 1);
> +		break;
> +	case CAN_LED_EVENT_RX:
> +		if (led_delay)
> +			led_trigger_blink_oneshot(priv->rx_led_trig,
> +						  &led_delay, &led_delay, 1);
> +		break;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(can_led_event);
> +
> +/*
> + * Register CAN LED triggers for a CAN device
> + *
> + * This is normally called from a driver's probe function
> + */
> +void can_led_init(struct net_device *netdev)
> +{
> +	struct can_priv *priv = netdev_priv(netdev);
> +
> +	priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);
> +	if (!priv->tx_led_trig_name)
> +		goto tx_led_failed;

Just return here?

> +	priv->rx_led_trig_name = kasprintf(GFP_KERNEL, "%s-rx", netdev->name);
> +	if (!priv->rx_led_trig_name)
> +		goto rx_led_failed;
> +
> +	led_trigger_register_simple(priv->tx_led_trig_name,
> +				    &priv->tx_led_trig);
> +	led_trigger_register_simple(priv->rx_led_trig_name,
> +				    &priv->rx_led_trig);
> +
> +	return;
> +
> +rx_led_failed:
> +	kfree(priv->tx_led_trig_name);
> +	priv->tx_led_trig_name = NULL;
> +tx_led_failed:
> +	return;

In case of error the function returns silently. Is this by purpose? What
happens if CAN_LEDS is enabled but no LEDs are assigned?

> +}
> +EXPORT_SYMBOL_GPL(can_led_init);
> +
> +/*
> + * Unregister CAN LED triggers for a CAN device
> + *
> + * This is normally called from a driver's remove function
> + */
> +void can_led_exit(struct net_device *netdev)
> +{
> +	struct can_priv *priv = netdev_priv(netdev);
> +
> +	led_trigger_unregister_simple(priv->tx_led_trig);
> +	led_trigger_unregister_simple(priv->rx_led_trig);
> +
> +	kfree(priv->tx_led_trig_name);
> +	kfree(priv->rx_led_trig_name);
> +}
> +EXPORT_SYMBOL_GPL(can_led_exit);
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 2b2fc34..167b04a 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,13 @@ struct can_priv {
>  
>  	unsigned int echo_skb_max;
>  	struct sk_buff **echo_skb;
> +
> +#ifdef CONFIG_CAN_LEDS
> +	struct led_trigger *tx_led_trig;
> +	char *tx_led_trig_name;
> +	struct led_trigger *rx_led_trig;
> +	char *rx_led_trig_name;
> +#endif

Do we need to store the names?

Wolfgang.

Re: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support

[Marc Kleine-Budde]

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

On 07/31/2012 10:46 AM, Wolfgang Grandegger wrote:
> Hi Fabio,
> 
> On 07/30/2012 09:20 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
>>
>> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
>> Cc: Wolfgang Grandegger <wg@grandegger.com>
>> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
>> Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
>> ---
>>
>> Hi all!
>>
>> This is the v3 of my CAN LED trigger patch.  It's a major refactoring of the v2
>> that was discussed about three months ago concluding with the idea that
>> implementing the oneshot triggering code in the LED framework would be a better
>> solution.
>>
>> This is the old thread for reference:
>> http://marc.info/?l=linux-can&m=133521499002898&w=2
>>
>> So, generic oneshot trigger code is now merged in mainline (see 5bb629c), and
>> these are the changes in the v3:
> 
> Nice, thanks for your patience (and not giving up).
> 
>>
>> - use the new led_trigger_blink_oneshot() function for LED triggering
>> - use kasprintf() and led_trigger_{un}register_simple for LED allocation
>> - add some usage note in the comments
>>
>> The resulting code is quite simple now and - I think - a bit less intrusive.
>> Still, I hope on some feedback from the community as I don't have that much
>> hardware to test it - this version has been tested mainly on an x86 with a
>> custom usb-can interface.
>>
>> In 2/2 there is a sample implementation for the flexcan driver, which is
>> basically unchanged from the old version.
>>
>> Any comments?
>> Fabio
>>
>>  drivers/net/can/Kconfig  | 12 ++++++
>>  drivers/net/can/Makefile |  2 +
>>  drivers/net/can/led.c    | 99 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/can/dev.h  |  8 ++++
>>  include/linux/can/led.h  | 38 +++++++++++++++++++
>>  5 files changed, 159 insertions(+)
>>  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..19dec19 100644
>> --- a/drivers/net/can/Kconfig
>> +++ b/drivers/net/can/Kconfig
>> @@ -54,6 +54,18 @@ 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_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..68c4f30
>> --- /dev/null
>> +++ b/drivers/net/can/led.c
>> @@ -0,0 +1,99 @@
>> +/*
>> + * 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/can/led.h>
>> +
>> +static unsigned long led_delay = 50;
>> +module_param(led_delay, ulong, 0644);
>> +MODULE_PARM_DESC(led_delay,
>> +		"blink delay time for activity leds (msecs, default: 50).");
>> +
>> +/*
>> + * Trigger a LED event in response to a CAN device event
>> + */
>> +void can_led_event(struct net_device *netdev, enum can_led_event event)
>> +{
>> +	struct can_priv *priv = netdev_priv(netdev);
>> +
>> +	switch (event) {
>> +	case CAN_LED_EVENT_OPEN:
>> +		led_trigger_event(priv->tx_led_trig, LED_FULL);
>> +		led_trigger_event(priv->rx_led_trig, LED_FULL);
>> +		break;
>> +	case CAN_LED_EVENT_STOP:
>> +		led_trigger_event(priv->tx_led_trig, LED_OFF);
>> +		led_trigger_event(priv->rx_led_trig, LED_OFF);
>> +		break;
>> +	case CAN_LED_EVENT_TX:
>> +		if (led_delay)
>> +			led_trigger_blink_oneshot(priv->tx_led_trig,
>> +						  &led_delay, &led_delay, 1);
>> +		break;
>> +	case CAN_LED_EVENT_RX:
>> +		if (led_delay)
>> +			led_trigger_blink_oneshot(priv->rx_led_trig,
>> +						  &led_delay, &led_delay, 1);
>> +		break;
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(can_led_event);
>> +
>> +/*
>> + * Register CAN LED triggers for a CAN device
>> + *
>> + * This is normally called from a driver's probe function
>> + */
>> +void can_led_init(struct net_device *netdev)
>> +{
>> +	struct can_priv *priv = netdev_priv(netdev);
>> +
>> +	priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);
>> +	if (!priv->tx_led_trig_name)
>> +		goto tx_led_failed;
> 
> Just return here?
> 
>> +	priv->rx_led_trig_name = kasprintf(GFP_KERNEL, "%s-rx", netdev->name);
>> +	if (!priv->rx_led_trig_name)
>> +		goto rx_led_failed;
>> +
>> +	led_trigger_register_simple(priv->tx_led_trig_name,
>> +				    &priv->tx_led_trig);
>> +	led_trigger_register_simple(priv->rx_led_trig_name,
>> +				    &priv->rx_led_trig);
>> +
>> +	return;
>> +
>> +rx_led_failed:
>> +	kfree(priv->tx_led_trig_name);
>> +	priv->tx_led_trig_name = NULL;
>> +tx_led_failed:
>> +	return;
> 
> In case of error the function returns silently. Is this by purpose? What
> happens if CAN_LEDS is enabled but no LEDs are assigned?

It's a bit strange, but led_trigger_register_simple() can fail silently,
too. Or better it has no return value, but does a warning printk in case
of an error.

> 
>> +}
>> +EXPORT_SYMBOL_GPL(can_led_init);
>> +
>> +/*
>> + * Unregister CAN LED triggers for a CAN device
>> + *
>> + * This is normally called from a driver's remove function
>> + */
>> +void can_led_exit(struct net_device *netdev)
>> +{
>> +	struct can_priv *priv = netdev_priv(netdev);
>> +
>> +	led_trigger_unregister_simple(priv->tx_led_trig);
>> +	led_trigger_unregister_simple(priv->rx_led_trig);
>> +
>> +	kfree(priv->tx_led_trig_name);
>> +	kfree(priv->rx_led_trig_name);
>> +}
>> +EXPORT_SYMBOL_GPL(can_led_exit);
>> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
>> index 2b2fc34..167b04a 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,13 @@ struct can_priv {
>>  
>>  	unsigned int echo_skb_max;
>>  	struct sk_buff **echo_skb;
>> +
>> +#ifdef CONFIG_CAN_LEDS
>> +	struct led_trigger *tx_led_trig;
>> +	char *tx_led_trig_name;
>> +	struct led_trigger *rx_led_trig;
>> +	char *rx_led_trig_name;
>> +#endif
> 
> Do we need to store the names?

Yes, Seems, so the name is not copied:

http://lxr.free-electrons.com/source/drivers/leds/led-triggers.c#L253

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: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support

[Fabio Baltieri]

On Tue, Jul 31, 2012 at 12:12:59PM +0200, Marc Kleine-Budde wrote:
> >> +/*
> >> + * Register CAN LED triggers for a CAN device
> >> + *
> >> + * This is normally called from a driver's probe function
> >> + */
> >> +void can_led_init(struct net_device *netdev)
> >> +{
> >> +	struct can_priv *priv = netdev_priv(netdev);
> >> +
> >> +	priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);
> >> +	if (!priv->tx_led_trig_name)
> >> +		goto tx_led_failed;
> > 
> > Just return here?

Right.

> >> +	priv->rx_led_trig_name = kasprintf(GFP_KERNEL, "%s-rx", netdev->name);
> >> +	if (!priv->rx_led_trig_name)
> >> +		goto rx_led_failed;
> >> +
> >> +	led_trigger_register_simple(priv->tx_led_trig_name,
> >> +				    &priv->tx_led_trig);
> >> +	led_trigger_register_simple(priv->rx_led_trig_name,
> >> +				    &priv->rx_led_trig);
> >> +
> >> +	return;
> >> +
> >> +rx_led_failed:
> >> +	kfree(priv->tx_led_trig_name);
> >> +	priv->tx_led_trig_name = NULL;
> >> +tx_led_failed:
> >> +	return;
> > 
> > In case of error the function returns silently. Is this by purpose? What
> > happens if CAN_LEDS is enabled but no LEDs are assigned?
> 
> It's a bit strange, but led_trigger_register_simple() can fail silently,
> too. Or better it has no return value, but does a warning printk in case
> of an error.

Well, that's in line with the behviour of leds trigger registration in
other subsystems out there (mac80211 and power_supply for instance) but
now that you pointed it out, I agree that this is not really nice to the
user.  led_trigger_register_simple already has a printk to KERN_ERR, I
may add another one in the error path (if we keep the kasprintf).

> 
> > 
> >> +}
> >> +EXPORT_SYMBOL_GPL(can_led_init);
> >> +
> >> +/*
> >> + * Unregister CAN LED triggers for a CAN device
> >> + *
> >> + * This is normally called from a driver's remove function
> >> + */
> >> +void can_led_exit(struct net_device *netdev)
> >> +{
> >> +	struct can_priv *priv = netdev_priv(netdev);
> >> +
> >> +	led_trigger_unregister_simple(priv->tx_led_trig);
> >> +	led_trigger_unregister_simple(priv->rx_led_trig);
> >> +
> >> +	kfree(priv->tx_led_trig_name);
> >> +	kfree(priv->rx_led_trig_name);
> >> +}
> >> +EXPORT_SYMBOL_GPL(can_led_exit);
> >> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> >> index 2b2fc34..167b04a 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,13 @@ struct can_priv {
> >>  
> >>  	unsigned int echo_skb_max;
> >>  	struct sk_buff **echo_skb;
> >> +
> >> +#ifdef CONFIG_CAN_LEDS
> >> +	struct led_trigger *tx_led_trig;
> >> +	char *tx_led_trig_name;
> >> +	struct led_trigger *rx_led_trig;
> >> +	char *rx_led_trig_name;
> >> +#endif
> > 
> > Do we need to store the names?
> 
> Yes, Seems, so the name is not copied:
> 
> http://lxr.free-electrons.com/source/drivers/leds/led-triggers.c#L253
> 
> Marc

Actually we may try to exploit struct led_trigger to get back the
pointers, but then we have to free the names before calling
led_trigger_unregister, and that's going to be race against
led_trigger_show().  Anyway, those pointers would go away using a
devm-based allocation, so I'll keep that in mind.

Thanks,
Fabio

Re: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support

[Fabio Baltieri]

On Tue, Jul 31, 2012 at 10:46:57AM +0200, Wolfgang Grandegger wrote:
> Hi Fabio,
> 
> On 07/30/2012 09:20 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
> > 
> > Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> > Cc: Wolfgang Grandegger <wg@grandegger.com>
> > Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> > Signed-off-by: Fabio Baltieri <fabio.baltieri@gmail.com>
> > ---
> > 
> > Hi all!
> > 
> > This is the v3 of my CAN LED trigger patch.  It's a major refactoring of the v2
> > that was discussed about three months ago concluding with the idea that
> > implementing the oneshot triggering code in the LED framework would be a better
> > solution.
> > 
> > This is the old thread for reference:
> > http://marc.info/?l=linux-can&m=133521499002898&w=2
> > 
> > So, generic oneshot trigger code is now merged in mainline (see 5bb629c), and
> > these are the changes in the v3:
> 
> Nice, thanks for your patience (and not giving up).

Thanks to you for the review... no intention of giving up, I'm learning
a lot from this! ;-)

Fabio