On Tue, Jul 31, 2012 at 12:12:59PM +0200, Marc Kleine-Budde wrote:

quoted

quoted

+/*
+ * 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.

quoted

quoted

+	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).

quoted

quoted

+}
+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