* [PATCH can-next v3 1/2] can: add tx/rx LED trigger support @ 2012-07-30 19:20 Fabio Baltieri 2012-07-30 19:20 ` [PATCH can-next v3 2/2] can: flexcan: add " Fabio Baltieri ` (2 more replies) 0 siblings, 3 replies; 67+ messages in thread From: Fabio Baltieri @ 2012-07-30 19:20 UTC (permalink / raw) To: linux-can Cc: linux-kernel, Fabio Baltieri, Oliver Hartkopp, Wolfgang Grandegger, Marc Kleine-Budde 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 ^ permalink raw reply related [flat|nested] 67+ messages in thread
* [PATCH can-next v3 2/2] can: flexcan: add LED trigger support 2012-07-30 19:20 [PATCH can-next v3 1/2] can: add tx/rx LED trigger support Fabio Baltieri @ 2012-07-30 19:20 ` Fabio Baltieri 2012-07-30 21:17 ` [PATCH can-next v3 1/2] can: add tx/rx " Marc Kleine-Budde 2012-07-31 8:46 ` [PATCH can-next v3 1/2] " Wolfgang Grandegger 2 siblings, 0 replies; 67+ messages in thread From: Fabio Baltieri @ 2012-07-30 19:20 UTC (permalink / raw) To: linux-can; +Cc: linux-kernel, 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), ®s->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 ^ permalink raw reply related [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support 2012-07-30 19:20 [PATCH can-next v3 1/2] can: add tx/rx LED trigger support Fabio Baltieri 2012-07-30 19:20 ` [PATCH can-next v3 2/2] can: flexcan: add " Fabio Baltieri @ 2012-07-30 21:17 ` Marc Kleine-Budde 2012-07-31 6:57 ` Fabio Baltieri 2012-07-31 8:46 ` [PATCH can-next v3 1/2] " Wolfgang Grandegger 2 siblings, 1 reply; 67+ messages in thread From: Marc Kleine-Budde @ 2012-07-30 21:17 UTC (permalink / raw) To: Fabio Baltieri Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger [-- 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 --] ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support 2012-07-30 21:17 ` [PATCH can-next v3 1/2] can: add tx/rx " Marc Kleine-Budde @ 2012-07-31 6:57 ` Fabio Baltieri 2012-07-31 7:10 ` Marc Kleine-Budde 0 siblings, 1 reply; 67+ messages in thread From: Fabio Baltieri @ 2012-07-31 6:57 UTC (permalink / raw) To: Marc Kleine-Budde Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger 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 | > ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support 2012-07-31 6:57 ` Fabio Baltieri @ 2012-07-31 7:10 ` Marc Kleine-Budde 2012-07-31 11:57 ` Fabio Baltieri 0 siblings, 1 reply; 67+ messages in thread From: Marc Kleine-Budde @ 2012-07-31 7:10 UTC (permalink / raw) To: Fabio Baltieri Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger [-- 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 --] ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support 2012-07-31 7:10 ` Marc Kleine-Budde @ 2012-07-31 11:57 ` Fabio Baltieri 2012-07-31 12:00 ` Marc Kleine-Budde 0 siblings, 1 reply; 67+ messages in thread From: Fabio Baltieri @ 2012-07-31 11:57 UTC (permalink / raw) To: Marc Kleine-Budde Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support 2012-07-31 11:57 ` Fabio Baltieri @ 2012-07-31 12:00 ` Marc Kleine-Budde 2012-07-31 22:05 ` [PATCH can-next v4] " Fabio Baltieri 0 siblings, 1 reply; 67+ messages in thread From: Marc Kleine-Budde @ 2012-07-31 12:00 UTC (permalink / raw) To: Fabio Baltieri Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger [-- 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 --] ^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH can-next v4] can: add tx/rx LED trigger support 2012-07-31 12:00 ` Marc Kleine-Budde @ 2012-07-31 22:05 ` Fabio Baltieri 2012-08-01 9:36 ` Marc Kleine-Budde 0 siblings, 1 reply; 67+ messages in thread From: Fabio Baltieri @ 2012-07-31 22:05 UTC (permalink / raw) To: linux-can Cc: linux-kernel, Fabio Baltieri, Oliver Hartkopp, Wolfgang Grandegger, Marc Kleine-Budde 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 ^ permalink raw reply related [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v4] can: add tx/rx LED trigger support 2012-07-31 22:05 ` [PATCH can-next v4] " Fabio Baltieri @ 2012-08-01 9:36 ` Marc Kleine-Budde 2012-08-01 10:07 ` Marc Kleine-Budde ` (2 more replies) 0 siblings, 3 replies; 67+ messages in thread From: Marc Kleine-Budde @ 2012-08-01 9:36 UTC (permalink / raw) To: Fabio Baltieri Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger [-- 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 --] ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v4] can: add tx/rx LED trigger support 2012-08-01 9:36 ` Marc Kleine-Budde @ 2012-08-01 10:07 ` Marc Kleine-Budde 2012-08-01 10:30 ` Fabio Baltieri 2012-08-01 11:49 ` [PATCH can-next v5 1/2] " Fabio Baltieri 2 siblings, 0 replies; 67+ messages in thread From: Marc Kleine-Budde @ 2012-08-01 10:07 UTC (permalink / raw) To: Fabio Baltieri Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger [-- 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 --] ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v4] can: add tx/rx LED trigger support 2012-08-01 9:36 ` Marc Kleine-Budde 2012-08-01 10:07 ` Marc Kleine-Budde @ 2012-08-01 10:30 ` Fabio Baltieri 2012-08-01 11:37 ` Marc Kleine-Budde 2012-08-01 11:49 ` [PATCH can-next v5 1/2] " Fabio Baltieri 2 siblings, 1 reply; 67+ messages in thread From: Fabio Baltieri @ 2012-08-01 10:30 UTC (permalink / raw) To: Marc Kleine-Budde Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v4] can: add tx/rx LED trigger support 2012-08-01 10:30 ` Fabio Baltieri @ 2012-08-01 11:37 ` Marc Kleine-Budde 0 siblings, 0 replies; 67+ messages in thread From: Marc Kleine-Budde @ 2012-08-01 11:37 UTC (permalink / raw) To: Fabio Baltieri Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger [-- 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 --] ^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH can-next v5 1/2] can: add tx/rx LED trigger support 2012-08-01 9:36 ` Marc Kleine-Budde 2012-08-01 10:07 ` Marc Kleine-Budde 2012-08-01 10:30 ` Fabio Baltieri @ 2012-08-01 11:49 ` Fabio Baltieri 2012-08-01 11:49 ` [PATCH can-next v5 2/2] can: flexcan: add " Fabio Baltieri ` (2 more replies) 2 siblings, 3 replies; 67+ messages in thread From: Fabio Baltieri @ 2012-08-01 11:49 UTC (permalink / raw) To: linux-can Cc: linux-kernel, Fabio Baltieri, Oliver Hartkopp, Wolfgang Grandegger, Marc Kleine-Budde 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 ^ permalink raw reply related [flat|nested] 67+ messages in thread
* [PATCH can-next v5 2/2] can: flexcan: add LED trigger support 2012-08-01 11:49 ` [PATCH can-next v5 1/2] " Fabio Baltieri @ 2012-08-01 11:49 ` Fabio Baltieri 2012-08-01 11:53 ` Marc Kleine-Budde 2012-08-01 21:02 ` Marc Kleine-Budde 2012-08-01 11:59 ` [PATCH can-next v5 1/2] can: add tx/rx " Marc Kleine-Budde 2012-08-01 12:06 ` Oliver Hartkopp 2 siblings, 2 replies; 67+ messages in thread From: Fabio Baltieri @ 2012-08-01 11:49 UTC (permalink / raw) To: linux-can Cc: linux-kernel, Fabio Baltieri, Oliver Hartkopp, Wolfgang Grandegger, Marc Kleine-Budde 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), ®s->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 ^ permalink raw reply related [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v5 2/2] can: flexcan: add LED trigger support 2012-08-01 11:49 ` [PATCH can-next v5 2/2] can: flexcan: add " Fabio Baltieri @ 2012-08-01 11:53 ` Marc Kleine-Budde 2012-08-01 12:24 ` Fabio Baltieri 2012-08-01 21:02 ` Marc Kleine-Budde 1 sibling, 1 reply; 67+ messages in thread From: Marc Kleine-Budde @ 2012-08-01 11:53 UTC (permalink / raw) To: Fabio Baltieri Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger [-- 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), ®s->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 --] ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v5 2/2] can: flexcan: add LED trigger support 2012-08-01 11:53 ` Marc Kleine-Budde @ 2012-08-01 12:24 ` Fabio Baltieri 2012-08-01 12:30 ` Marc Kleine-Budde 0 siblings, 1 reply; 67+ messages in thread From: Fabio Baltieri @ 2012-08-01 12:24 UTC (permalink / raw) To: Marc Kleine-Budde Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v5 2/2] can: flexcan: add LED trigger support 2012-08-01 12:24 ` Fabio Baltieri @ 2012-08-01 12:30 ` Marc Kleine-Budde 0 siblings, 0 replies; 67+ messages in thread From: Marc Kleine-Budde @ 2012-08-01 12:30 UTC (permalink / raw) To: Fabio Baltieri Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger [-- 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 --] ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v5 2/2] can: flexcan: add LED trigger support 2012-08-01 11:49 ` [PATCH can-next v5 2/2] can: flexcan: add " Fabio Baltieri 2012-08-01 11:53 ` Marc Kleine-Budde @ 2012-08-01 21:02 ` Marc Kleine-Budde 1 sibling, 0 replies; 67+ messages in thread From: Marc Kleine-Budde @ 2012-08-01 21:02 UTC (permalink / raw) To: Fabio Baltieri Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger [-- 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 --] ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v5 1/2] can: add tx/rx LED trigger support 2012-08-01 11:49 ` [PATCH can-next v5 1/2] " Fabio Baltieri 2012-08-01 11:49 ` [PATCH can-next v5 2/2] can: flexcan: add " Fabio Baltieri @ 2012-08-01 11:59 ` Marc Kleine-Budde 2012-08-01 12:06 ` Oliver Hartkopp 2 siblings, 0 replies; 67+ messages in thread From: Marc Kleine-Budde @ 2012-08-01 11:59 UTC (permalink / raw) To: Fabio Baltieri Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger [-- 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 --] ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v5 1/2] can: add tx/rx LED trigger support 2012-08-01 11:49 ` [PATCH can-next v5 1/2] " Fabio Baltieri 2012-08-01 11:49 ` [PATCH can-next v5 2/2] can: flexcan: add " Fabio Baltieri 2012-08-01 11:59 ` [PATCH can-next v5 1/2] can: add tx/rx " Marc Kleine-Budde @ 2012-08-01 12:06 ` Oliver Hartkopp 2012-08-01 12:18 ` Marc Kleine-Budde 2 siblings, 1 reply; 67+ messages in thread From: Oliver Hartkopp @ 2012-08-01 12:06 UTC (permalink / raw) To: linux-can, Fabio Baltieri Cc: linux-kernel, Wolfgang Grandegger, Marc Kleine-Budde 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v5 1/2] can: add tx/rx LED trigger support 2012-08-01 12:06 ` Oliver Hartkopp @ 2012-08-01 12:18 ` Marc Kleine-Budde 2012-08-01 18:21 ` [PATCH can-next v6] " Fabio Baltieri 0 siblings, 1 reply; 67+ messages in thread From: Marc Kleine-Budde @ 2012-08-01 12:18 UTC (permalink / raw) To: Oliver Hartkopp Cc: linux-can, Fabio Baltieri, linux-kernel, Wolfgang Grandegger [-- 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 --] ^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH can-next v6] can: add tx/rx LED trigger support 2012-08-01 12:18 ` Marc Kleine-Budde @ 2012-08-01 18:21 ` Fabio Baltieri 2012-08-01 21:00 ` Marc Kleine-Budde ` (2 more replies) 0 siblings, 3 replies; 67+ messages in thread From: Fabio Baltieri @ 2012-08-01 18:21 UTC (permalink / raw) To: linux-can Cc: linux-kernel, Fabio Baltieri, Oliver Hartkopp, Wolfgang Grandegger, Marc Kleine-Budde 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 ^ permalink raw reply related [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-08-01 18:21 ` [PATCH can-next v6] " Fabio Baltieri @ 2012-08-01 21:00 ` Marc Kleine-Budde 2012-08-01 22:38 ` Fabio Baltieri 2012-08-01 21:05 ` Oliver Hartkopp 2012-08-24 5:10 ` Kurt Van Dijck 2 siblings, 1 reply; 67+ messages in thread From: Marc Kleine-Budde @ 2012-08-01 21:00 UTC (permalink / raw) To: Fabio Baltieri Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger [-- 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 --] ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-08-01 21:00 ` Marc Kleine-Budde @ 2012-08-01 22:38 ` Fabio Baltieri 0 siblings, 0 replies; 67+ messages in thread From: Fabio Baltieri @ 2012-08-01 22:38 UTC (permalink / raw) To: Marc Kleine-Budde, Oliver Hartkopp, Wolfgang Grandegger Cc: linux-can, linux-kernel 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-08-01 18:21 ` [PATCH can-next v6] " Fabio Baltieri 2012-08-01 21:00 ` Marc Kleine-Budde @ 2012-08-01 21:05 ` Oliver Hartkopp 2012-08-24 5:10 ` Kurt Van Dijck 2 siblings, 0 replies; 67+ messages in thread From: Oliver Hartkopp @ 2012-08-01 21:05 UTC (permalink / raw) To: linux-can, Fabio Baltieri Cc: linux-kernel, Wolfgang Grandegger, Marc Kleine-Budde 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> ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-08-01 18:21 ` [PATCH can-next v6] " Fabio Baltieri 2012-08-01 21:00 ` Marc Kleine-Budde 2012-08-01 21:05 ` Oliver Hartkopp @ 2012-08-24 5:10 ` Kurt Van Dijck 2012-08-24 11:28 ` Marc Kleine-Budde 2 siblings, 1 reply; 67+ messages in thread From: Kurt Van Dijck @ 2012-08-24 5:10 UTC (permalink / raw) To: Fabio Baltieri Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger, Marc Kleine-Budde 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-08-24 5:10 ` Kurt Van Dijck @ 2012-08-24 11:28 ` Marc Kleine-Budde 2012-08-24 12:42 ` Kurt Van Dijck 0 siblings, 1 reply; 67+ messages in thread From: Marc Kleine-Budde @ 2012-08-24 11:28 UTC (permalink / raw) To: Fabio Baltieri, linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger [-- 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 --] ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-08-24 11:28 ` Marc Kleine-Budde @ 2012-08-24 12:42 ` Kurt Van Dijck 2012-08-24 22:01 ` Fabio Baltieri 0 siblings, 1 reply; 67+ messages in thread From: Kurt Van Dijck @ 2012-08-24 12:42 UTC (permalink / raw) To: Marc Kleine-Budde Cc: Fabio Baltieri, linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-08-24 12:42 ` Kurt Van Dijck @ 2012-08-24 22:01 ` Fabio Baltieri 2012-08-25 20:25 ` Kurt Van Dijck 2012-09-03 12:40 ` Marc Kleine-Budde 0 siblings, 2 replies; 67+ messages in thread From: Fabio Baltieri @ 2012-08-24 22:01 UTC (permalink / raw) To: Marc Kleine-Budde, linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-08-24 22:01 ` Fabio Baltieri @ 2012-08-25 20:25 ` Kurt Van Dijck 2012-09-03 12:40 ` Marc Kleine-Budde 1 sibling, 0 replies; 67+ messages in thread From: Kurt Van Dijck @ 2012-08-25 20:25 UTC (permalink / raw) To: Fabio Baltieri Cc: Marc Kleine-Budde, linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-08-24 22:01 ` Fabio Baltieri 2012-08-25 20:25 ` Kurt Van Dijck @ 2012-09-03 12:40 ` Marc Kleine-Budde 2012-09-03 18:13 ` Kurt Van Dijck 1 sibling, 1 reply; 67+ messages in thread From: Marc Kleine-Budde @ 2012-09-03 12:40 UTC (permalink / raw) To: Fabio Baltieri Cc: linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger [-- 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 --] ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-09-03 12:40 ` Marc Kleine-Budde @ 2012-09-03 18:13 ` Kurt Van Dijck 2012-09-03 18:29 ` Fabio Baltieri 0 siblings, 1 reply; 67+ messages in thread From: Kurt Van Dijck @ 2012-09-03 18:13 UTC (permalink / raw) To: Marc Kleine-Budde Cc: Fabio Baltieri, linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-09-03 18:13 ` Kurt Van Dijck @ 2012-09-03 18:29 ` Fabio Baltieri 2012-09-03 20:54 ` Oliver Hartkopp 0 siblings, 1 reply; 67+ messages in thread From: Fabio Baltieri @ 2012-09-03 18:29 UTC (permalink / raw) To: Marc Kleine-Budde, linux-can, linux-kernel, Oliver Hartkopp, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-09-03 18:29 ` Fabio Baltieri @ 2012-09-03 20:54 ` Oliver Hartkopp 2012-09-04 7:11 ` Kurt Van Dijck 0 siblings, 1 reply; 67+ messages in thread From: Oliver Hartkopp @ 2012-09-03 20:54 UTC (permalink / raw) To: Fabio Baltieri Cc: Marc Kleine-Budde, linux-can, linux-kernel, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-09-03 20:54 ` Oliver Hartkopp @ 2012-09-04 7:11 ` Kurt Van Dijck 2012-09-04 9:29 ` [PATCH] can: rename LED trigger name on netdev renames Kurt Van Dijck 2012-09-04 20:15 ` [PATCH can-next v6] can: add tx/rx LED trigger support Fabio Baltieri 0 siblings, 2 replies; 67+ messages in thread From: Kurt Van Dijck @ 2012-09-04 7:11 UTC (permalink / raw) To: Oliver Hartkopp Cc: Fabio Baltieri, Marc Kleine-Budde, linux-can, linux-kernel, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH] can: rename LED trigger name on netdev renames 2012-09-04 7:11 ` Kurt Van Dijck @ 2012-09-04 9:29 ` Kurt Van Dijck 2012-09-06 18:59 ` Fabio Baltieri 2012-09-04 20:15 ` [PATCH can-next v6] can: add tx/rx LED trigger support Fabio Baltieri 1 sibling, 1 reply; 67+ messages in thread From: Kurt Van Dijck @ 2012-09-04 9:29 UTC (permalink / raw) To: Fabio Baltieri, Marc Kleine-Budde, Oliver Hartkopp, Wolfgang Grandegger Cc: linux-kernel, linux-can, 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 ^ permalink raw reply related [flat|nested] 67+ messages in thread
* Re: [PATCH] can: rename LED trigger name on netdev renames 2012-09-04 9:29 ` [PATCH] can: rename LED trigger name on netdev renames Kurt Van Dijck @ 2012-09-06 18:59 ` Fabio Baltieri 2012-09-06 19:31 ` Oliver Hartkopp 0 siblings, 1 reply; 67+ messages in thread From: Fabio Baltieri @ 2012-09-06 18:59 UTC (permalink / raw) To: Kurt Van Dijck Cc: Marc Kleine-Budde, Oliver Hartkopp, Wolfgang Grandegger, linux-kernel, linux-can 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH] can: rename LED trigger name on netdev renames 2012-09-06 18:59 ` Fabio Baltieri @ 2012-09-06 19:31 ` Oliver Hartkopp 2012-09-06 20:46 ` Fabio Baltieri 0 siblings, 1 reply; 67+ messages in thread From: Oliver Hartkopp @ 2012-09-06 19:31 UTC (permalink / raw) To: Fabio Baltieri Cc: Kurt Van Dijck, Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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? > ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH] can: rename LED trigger name on netdev renames 2012-09-06 19:31 ` Oliver Hartkopp @ 2012-09-06 20:46 ` Fabio Baltieri 2012-09-07 7:19 ` Kurt Van Dijck 0 siblings, 1 reply; 67+ messages in thread From: Fabio Baltieri @ 2012-09-06 20:46 UTC (permalink / raw) To: Oliver Hartkopp Cc: Kurt Van Dijck, Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH] can: rename LED trigger name on netdev renames 2012-09-06 20:46 ` Fabio Baltieri @ 2012-09-07 7:19 ` Kurt Van Dijck 2012-09-09 16:17 ` [PATCH v2] " Fabio Baltieri 0 siblings, 1 reply; 67+ messages in thread From: Kurt Van Dijck @ 2012-09-07 7:19 UTC (permalink / raw) To: Fabio Baltieri Cc: Oliver Hartkopp, Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH v2] can: rename LED trigger name on netdev renames 2012-09-07 7:19 ` Kurt Van Dijck @ 2012-09-09 16:17 ` Fabio Baltieri 2012-09-10 14:25 ` [PATCH] can: export a safe netdev_priv wrapper for candev Kurt Van Dijck 2012-09-10 14:28 ` [PATCH v3] can: rename LED trigger name on netdev renames Kurt Van Dijck 0 siblings, 2 replies; 67+ messages in thread From: Fabio Baltieri @ 2012-09-09 16:17 UTC (permalink / raw) To: Kurt Van Dijck Cc: Oliver Hartkopp, Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can, 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 ^ permalink raw reply related [flat|nested] 67+ messages in thread
* [PATCH] can: export a safe netdev_priv wrapper for candev 2012-09-09 16:17 ` [PATCH v2] " Fabio Baltieri @ 2012-09-10 14:25 ` Kurt Van Dijck 2012-09-10 18:22 ` Oliver Hartkopp 2012-09-10 18:29 ` Fabio Baltieri 2012-09-10 14:28 ` [PATCH v3] can: rename LED trigger name on netdev renames Kurt Van Dijck 1 sibling, 2 replies; 67+ messages in thread From: Kurt Van Dijck @ 2012-09-10 14:25 UTC (permalink / raw) To: Fabio Baltieri Cc: Oliver Hartkopp, Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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); ^ permalink raw reply related [flat|nested] 67+ messages in thread
* Re: [PATCH] can: export a safe netdev_priv wrapper for candev 2012-09-10 14:25 ` [PATCH] can: export a safe netdev_priv wrapper for candev Kurt Van Dijck @ 2012-09-10 18:22 ` Oliver Hartkopp 2012-09-10 18:29 ` Fabio Baltieri 1 sibling, 0 replies; 67+ messages in thread From: Oliver Hartkopp @ 2012-09-10 18:22 UTC (permalink / raw) To: Fabio Baltieri, Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can, Kurt Van Dijck 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); > ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH] can: export a safe netdev_priv wrapper for candev 2012-09-10 14:25 ` [PATCH] can: export a safe netdev_priv wrapper for candev Kurt Van Dijck 2012-09-10 18:22 ` Oliver Hartkopp @ 2012-09-10 18:29 ` Fabio Baltieri 2012-09-10 19:55 ` [PATCH v2] " Kurt Van Dijck 1 sibling, 1 reply; 67+ messages in thread From: Fabio Baltieri @ 2012-09-10 18:29 UTC (permalink / raw) To: Oliver Hartkopp, Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH v2] can: export a safe netdev_priv wrapper for candev 2012-09-10 18:29 ` Fabio Baltieri @ 2012-09-10 19:55 ` Kurt Van Dijck 0 siblings, 0 replies; 67+ messages in thread From: Kurt Van Dijck @ 2012-09-10 19:55 UTC (permalink / raw) To: Fabio Baltieri Cc: Oliver Hartkopp, Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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); ^ permalink raw reply related [flat|nested] 67+ messages in thread
* [PATCH v3] can: rename LED trigger name on netdev renames 2012-09-09 16:17 ` [PATCH v2] " Fabio Baltieri 2012-09-10 14:25 ` [PATCH] can: export a safe netdev_priv wrapper for candev Kurt Van Dijck @ 2012-09-10 14:28 ` Kurt Van Dijck 2012-09-10 18:25 ` Oliver Hartkopp 1 sibling, 1 reply; 67+ messages in thread From: Kurt Van Dijck @ 2012-09-10 14:28 UTC (permalink / raw) To: Fabio Baltieri Cc: Oliver Hartkopp, Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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 ^ permalink raw reply related [flat|nested] 67+ messages in thread
* Re: [PATCH v3] can: rename LED trigger name on netdev renames 2012-09-10 14:28 ` [PATCH v3] can: rename LED trigger name on netdev renames Kurt Van Dijck @ 2012-09-10 18:25 ` Oliver Hartkopp 2012-09-10 18:40 ` Fabio Baltieri 0 siblings, 1 reply; 67+ messages in thread From: Oliver Hartkopp @ 2012-09-10 18:25 UTC (permalink / raw) To: Fabio Baltieri, Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH v3] can: rename LED trigger name on netdev renames 2012-09-10 18:25 ` Oliver Hartkopp @ 2012-09-10 18:40 ` Fabio Baltieri 2012-09-10 19:01 ` Oliver Hartkopp 2012-09-10 20:06 ` [PATCH v4] " Kurt Van Dijck 0 siblings, 2 replies; 67+ messages in thread From: Fabio Baltieri @ 2012-09-10 18:40 UTC (permalink / raw) To: Oliver Hartkopp Cc: Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH v3] can: rename LED trigger name on netdev renames 2012-09-10 18:40 ` Fabio Baltieri @ 2012-09-10 19:01 ` Oliver Hartkopp 2012-09-10 20:08 ` Kurt Van Dijck 2012-09-10 20:06 ` [PATCH v4] " Kurt Van Dijck 1 sibling, 1 reply; 67+ messages in thread From: Oliver Hartkopp @ 2012-09-10 19:01 UTC (permalink / raw) To: Fabio Baltieri, Kurt Van Dijck Cc: Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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 > ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH v3] can: rename LED trigger name on netdev renames 2012-09-10 19:01 ` Oliver Hartkopp @ 2012-09-10 20:08 ` Kurt Van Dijck 2012-09-11 5:42 ` Oliver Hartkopp 2012-09-11 8:05 ` Kurt Van Dijck 0 siblings, 2 replies; 67+ messages in thread From: Kurt Van Dijck @ 2012-09-10 20:08 UTC (permalink / raw) To: Oliver Hartkopp Cc: Fabio Baltieri, Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH v3] can: rename LED trigger name on netdev renames 2012-09-10 20:08 ` Kurt Van Dijck @ 2012-09-11 5:42 ` Oliver Hartkopp 2012-09-11 7:13 ` Fabio Baltieri 2012-09-11 8:05 ` Kurt Van Dijck 1 sibling, 1 reply; 67+ messages in thread From: Oliver Hartkopp @ 2012-09-11 5:42 UTC (permalink / raw) To: Fabio Baltieri, Kurt Van Dijck Cc: Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH v3] can: rename LED trigger name on netdev renames 2012-09-11 5:42 ` Oliver Hartkopp @ 2012-09-11 7:13 ` Fabio Baltieri 2012-09-12 7:22 ` Kurt Van Dijck 0 siblings, 1 reply; 67+ messages in thread From: Fabio Baltieri @ 2012-09-11 7:13 UTC (permalink / raw) To: Oliver Hartkopp Cc: Kurt Van Dijck, Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH v3] can: rename LED trigger name on netdev renames 2012-09-11 7:13 ` Fabio Baltieri @ 2012-09-12 7:22 ` Kurt Van Dijck 0 siblings, 0 replies; 67+ messages in thread From: Kurt Van Dijck @ 2012-09-12 7:22 UTC (permalink / raw) To: Fabio Baltieri Cc: Oliver Hartkopp, Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH v3] can: rename LED trigger name on netdev renames 2012-09-10 20:08 ` Kurt Van Dijck 2012-09-11 5:42 ` Oliver Hartkopp @ 2012-09-11 8:05 ` Kurt Van Dijck 1 sibling, 0 replies; 67+ messages in thread From: Kurt Van Dijck @ 2012-09-11 8:05 UTC (permalink / raw) To: Oliver Hartkopp, Fabio Baltieri, Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* [PATCH v4] can: rename LED trigger name on netdev renames 2012-09-10 18:40 ` Fabio Baltieri 2012-09-10 19:01 ` Oliver Hartkopp @ 2012-09-10 20:06 ` Kurt Van Dijck 2012-09-11 21:04 ` Fabio Baltieri 1 sibling, 1 reply; 67+ messages in thread From: Kurt Van Dijck @ 2012-09-10 20:06 UTC (permalink / raw) To: Fabio Baltieri Cc: Oliver Hartkopp, Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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 ^ permalink raw reply related [flat|nested] 67+ messages in thread
* Re: [PATCH v4] can: rename LED trigger name on netdev renames 2012-09-10 20:06 ` [PATCH v4] " Kurt Van Dijck @ 2012-09-11 21:04 ` Fabio Baltieri 0 siblings, 0 replies; 67+ messages in thread From: Fabio Baltieri @ 2012-09-11 21:04 UTC (permalink / raw) To: Oliver Hartkopp, Marc Kleine-Budde, Wolfgang Grandegger, linux-kernel, linux-can 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-09-04 7:11 ` Kurt Van Dijck 2012-09-04 9:29 ` [PATCH] can: rename LED trigger name on netdev renames Kurt Van Dijck @ 2012-09-04 20:15 ` Fabio Baltieri 2012-09-06 10:33 ` Kurt Van Dijck 1 sibling, 1 reply; 67+ messages in thread From: Fabio Baltieri @ 2012-09-04 20:15 UTC (permalink / raw) To: Oliver Hartkopp, Marc Kleine-Budde, linux-can, linux-kernel, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-09-04 20:15 ` [PATCH can-next v6] can: add tx/rx LED trigger support Fabio Baltieri @ 2012-09-06 10:33 ` Kurt Van Dijck 2012-09-06 11:17 ` Fabio Baltieri 0 siblings, 1 reply; 67+ messages in thread From: Kurt Van Dijck @ 2012-09-06 10:33 UTC (permalink / raw) To: Fabio Baltieri Cc: Oliver Hartkopp, Marc Kleine-Budde, linux-can, linux-kernel, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-09-06 10:33 ` Kurt Van Dijck @ 2012-09-06 11:17 ` Fabio Baltieri 2012-09-06 15:11 ` Kurt Van Dijck 0 siblings, 1 reply; 67+ messages in thread From: Fabio Baltieri @ 2012-09-06 11:17 UTC (permalink / raw) To: Fabio Baltieri, Oliver Hartkopp, Marc Kleine-Budde, linux-can, linux-kernel, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-09-06 11:17 ` Fabio Baltieri @ 2012-09-06 15:11 ` Kurt Van Dijck 2012-09-06 20:57 ` Fabio Baltieri 0 siblings, 1 reply; 67+ messages in thread From: Kurt Van Dijck @ 2012-09-06 15:11 UTC (permalink / raw) To: Fabio Baltieri Cc: Oliver Hartkopp, Marc Kleine-Budde, linux-can, linux-kernel, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-09-06 15:11 ` Kurt Van Dijck @ 2012-09-06 20:57 ` Fabio Baltieri 2012-09-07 7:04 ` Kurt Van Dijck 0 siblings, 1 reply; 67+ messages in thread From: Fabio Baltieri @ 2012-09-06 20:57 UTC (permalink / raw) To: Oliver Hartkopp, Marc Kleine-Budde, linux-can, linux-kernel, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-09-06 20:57 ` Fabio Baltieri @ 2012-09-07 7:04 ` Kurt Van Dijck 2012-09-07 18:59 ` Fabio Baltieri 0 siblings, 1 reply; 67+ messages in thread From: Kurt Van Dijck @ 2012-09-07 7:04 UTC (permalink / raw) To: Fabio Baltieri Cc: Oliver Hartkopp, Marc Kleine-Budde, linux-can, linux-kernel, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v6] can: add tx/rx LED trigger support 2012-09-07 7:04 ` Kurt Van Dijck @ 2012-09-07 18:59 ` Fabio Baltieri 0 siblings, 0 replies; 67+ messages in thread From: Fabio Baltieri @ 2012-09-07 18:59 UTC (permalink / raw) To: Oliver Hartkopp, Marc Kleine-Budde, linux-can, linux-kernel, Wolfgang Grandegger 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support 2012-07-30 19:20 [PATCH can-next v3 1/2] can: add tx/rx LED trigger support Fabio Baltieri 2012-07-30 19:20 ` [PATCH can-next v3 2/2] can: flexcan: add " Fabio Baltieri 2012-07-30 21:17 ` [PATCH can-next v3 1/2] can: add tx/rx " Marc Kleine-Budde @ 2012-07-31 8:46 ` Wolfgang Grandegger 2012-07-31 10:12 ` Marc Kleine-Budde 2012-07-31 12:14 ` Fabio Baltieri 2 siblings, 2 replies; 67+ messages in thread From: Wolfgang Grandegger @ 2012-07-31 8:46 UTC (permalink / raw) To: Fabio Baltieri Cc: linux-can, linux-kernel, Oliver Hartkopp, Marc Kleine-Budde 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. ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support 2012-07-31 8:46 ` [PATCH can-next v3 1/2] " Wolfgang Grandegger @ 2012-07-31 10:12 ` Marc Kleine-Budde 2012-07-31 11:55 ` Fabio Baltieri 2012-07-31 12:14 ` Fabio Baltieri 1 sibling, 1 reply; 67+ messages in thread From: Marc Kleine-Budde @ 2012-07-31 10:12 UTC (permalink / raw) To: Wolfgang Grandegger Cc: Fabio Baltieri, linux-can, linux-kernel, Oliver Hartkopp [-- 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 --] ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support 2012-07-31 10:12 ` Marc Kleine-Budde @ 2012-07-31 11:55 ` Fabio Baltieri 0 siblings, 0 replies; 67+ messages in thread From: Fabio Baltieri @ 2012-07-31 11:55 UTC (permalink / raw) To: Marc Kleine-Budde Cc: Wolfgang Grandegger, linux-can, linux-kernel, Oliver Hartkopp 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
* Re: [PATCH can-next v3 1/2] can: add tx/rx LED trigger support 2012-07-31 8:46 ` [PATCH can-next v3 1/2] " Wolfgang Grandegger 2012-07-31 10:12 ` Marc Kleine-Budde @ 2012-07-31 12:14 ` Fabio Baltieri 1 sibling, 0 replies; 67+ messages in thread From: Fabio Baltieri @ 2012-07-31 12:14 UTC (permalink / raw) To: Wolfgang Grandegger Cc: linux-can, linux-kernel, Oliver Hartkopp, Marc Kleine-Budde 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 ^ permalink raw reply [flat|nested] 67+ messages in thread
end of thread, other threads:[~2012-09-12 7:22 UTC | newest] Thread overview: 67+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-30 19:20 [PATCH can-next v3 1/2] can: add tx/rx LED trigger support Fabio Baltieri 2012-07-30 19:20 ` [PATCH can-next v3 2/2] can: flexcan: add " Fabio Baltieri 2012-07-30 21:17 ` [PATCH can-next v3 1/2] can: add tx/rx " Marc Kleine-Budde 2012-07-31 6:57 ` Fabio Baltieri 2012-07-31 7:10 ` Marc Kleine-Budde 2012-07-31 11:57 ` Fabio Baltieri 2012-07-31 12:00 ` Marc Kleine-Budde 2012-07-31 22:05 ` [PATCH can-next v4] " Fabio Baltieri 2012-08-01 9:36 ` Marc Kleine-Budde 2012-08-01 10:07 ` Marc Kleine-Budde 2012-08-01 10:30 ` Fabio Baltieri 2012-08-01 11:37 ` Marc Kleine-Budde 2012-08-01 11:49 ` [PATCH can-next v5 1/2] " Fabio Baltieri 2012-08-01 11:49 ` [PATCH can-next v5 2/2] can: flexcan: add " Fabio Baltieri 2012-08-01 11:53 ` Marc Kleine-Budde 2012-08-01 12:24 ` Fabio Baltieri 2012-08-01 12:30 ` Marc Kleine-Budde 2012-08-01 21:02 ` Marc Kleine-Budde 2012-08-01 11:59 ` [PATCH can-next v5 1/2] can: add tx/rx " Marc Kleine-Budde 2012-08-01 12:06 ` Oliver Hartkopp 2012-08-01 12:18 ` Marc Kleine-Budde 2012-08-01 18:21 ` [PATCH can-next v6] " Fabio Baltieri 2012-08-01 21:00 ` Marc Kleine-Budde 2012-08-01 22:38 ` Fabio Baltieri 2012-08-01 21:05 ` Oliver Hartkopp 2012-08-24 5:10 ` Kurt Van Dijck 2012-08-24 11:28 ` Marc Kleine-Budde 2012-08-24 12:42 ` Kurt Van Dijck 2012-08-24 22:01 ` Fabio Baltieri 2012-08-25 20:25 ` Kurt Van Dijck 2012-09-03 12:40 ` Marc Kleine-Budde 2012-09-03 18:13 ` Kurt Van Dijck 2012-09-03 18:29 ` Fabio Baltieri 2012-09-03 20:54 ` Oliver Hartkopp 2012-09-04 7:11 ` Kurt Van Dijck 2012-09-04 9:29 ` [PATCH] can: rename LED trigger name on netdev renames Kurt Van Dijck 2012-09-06 18:59 ` Fabio Baltieri 2012-09-06 19:31 ` Oliver Hartkopp 2012-09-06 20:46 ` Fabio Baltieri 2012-09-07 7:19 ` Kurt Van Dijck 2012-09-09 16:17 ` [PATCH v2] " Fabio Baltieri 2012-09-10 14:25 ` [PATCH] can: export a safe netdev_priv wrapper for candev Kurt Van Dijck 2012-09-10 18:22 ` Oliver Hartkopp 2012-09-10 18:29 ` Fabio Baltieri 2012-09-10 19:55 ` [PATCH v2] " Kurt Van Dijck 2012-09-10 14:28 ` [PATCH v3] can: rename LED trigger name on netdev renames Kurt Van Dijck 2012-09-10 18:25 ` Oliver Hartkopp 2012-09-10 18:40 ` Fabio Baltieri 2012-09-10 19:01 ` Oliver Hartkopp 2012-09-10 20:08 ` Kurt Van Dijck 2012-09-11 5:42 ` Oliver Hartkopp 2012-09-11 7:13 ` Fabio Baltieri 2012-09-12 7:22 ` Kurt Van Dijck 2012-09-11 8:05 ` Kurt Van Dijck 2012-09-10 20:06 ` [PATCH v4] " Kurt Van Dijck 2012-09-11 21:04 ` Fabio Baltieri 2012-09-04 20:15 ` [PATCH can-next v6] can: add tx/rx LED trigger support Fabio Baltieri 2012-09-06 10:33 ` Kurt Van Dijck 2012-09-06 11:17 ` Fabio Baltieri 2012-09-06 15:11 ` Kurt Van Dijck 2012-09-06 20:57 ` Fabio Baltieri 2012-09-07 7:04 ` Kurt Van Dijck 2012-09-07 18:59 ` Fabio Baltieri 2012-07-31 8:46 ` [PATCH can-next v3 1/2] " Wolfgang Grandegger 2012-07-31 10:12 ` Marc Kleine-Budde 2012-07-31 11:55 ` Fabio Baltieri 2012-07-31 12:14 ` Fabio Baltieri
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).