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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
