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

[Fabio Baltieri <fabio.baltieri@gmail.com>]

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