Re: [PATCH ] leds: add new transient trigger for one shot timer support

[Shuah Khan <shuahkhan@gmail.com>]

> 
> Are there no comments from anyone on this?
Thanks for the comments.
> 
> >
> > ...
> >
> > config LEDS_TRIGGER_TRANSIENT
> > +	tristate "LED Transient Trigger"
> > +	depends on LEDS_TRIGGERS
> > +	help
> > +	  This allows one time enable of a transient state on GPIO/PWM based
> > +	  hadrware.
> 
> Make it "This allows one time enabling of a transient state on GPIO/PWM
> based hardware."
Will do.
> 
> > +	  If unsure, say Y.
> > +
> >
> > ...
> >
> > +static void transient_timer_function(unsigned long data)
> > +{
> > +	struct led_classdev *led_cdev = (struct led_classdev *) data;
> > +	struct transient_trig_data *transient_data = led_cdev->trigger_data;
> > +
> > +	if (transient_data->transient_enabled) {
> > +		transient_data->transient_enabled = 0;
> > +		led_cdev->brightness_set(led_cdev, LED_OFF);
> > +		del_timer(&transient_data->timer);
> 
> Deleting the timer from within its handler is ...  odd.  Also it is a
> bit racy against a concurrent add_timer() on a different CPU.
Good point. Will fix.
> 
> > +	}
> > +}
> > +
> >
> > ...
> >
> > +static ssize_t led_transient_enabled_store(struct device *dev,
> > +		struct device_attribute *attr, const char *buf, size_t size)
> > +{
> > +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> > +	struct transient_trig_data *transient_data = led_cdev->trigger_data;
> > +	unsigned long state;
> > +	ssize_t ret = -EINVAL;
> > +
> > +	ret = kstrtoul(buf, 10, &state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (state != 1 && state != 0)
> > +		return ret;
> 
> Bug - we'll return 0 here.  Use "return -EINVAL" and remove the above
> initialisation of `ret'.

Good point. Will fix.
> 
> 
> > +	/* cancel the running timer */
> > +	if (state == 0) {
> > +		transient_timer_function((unsigned long) led_cdev);
> 
> And this is perhaps why transient_timer_function() does del_timer().
> 
> I suggest it would be cleaner and simpler to do
> 
> 	transient_data->transient_enabled = 0;
> 	del_timer(...);
> 
> right here.

Will fix it.
> 
> This is all rather racy in its handling of ->transient_enabled (at
> least), but afacit the races are harmless.

I am a bit concerned about it as well. Does adding a mutex to
trigger_data a good way to go to protect transient_enabled? I will give
that a try.

> The typecast is unneeded.
ok
> 
> > +	/* start timer with transient_time value */
> > +	if (state == 1 && transient_data->transient_time != 0) {
> > +		led_cdev->brightness_set(led_cdev, LED_FULL);
> > +		mod_timer(&transient_data->timer,
> > +			  jiffies + transient_data->transient_time);
> > +	}
> > +
> > +	return size;
> > +}
> > +
> >
> > ...
> >
> > +static ssize_t led_transient_time_store(struct device *dev,
> > +		struct device_attribute *attr, const char *buf, size_t size)
> > +{
> > +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> > +	struct transient_trig_data *transient_data = led_cdev->trigger_data;
> > +	unsigned long state;
> > +	ssize_t ret = -EINVAL;
> 
> Unneeded initialisation.
Yup
> 
> > +	ret = kstrtoul(buf, 10, &state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	transient_data->transient_time = state;
> > +
> > +	return size;
> > +}
> > +
> >
> > ...
> >
> > +static void transient_trig_deactivate(struct led_classdev *led_cdev)
> > +{
> > +	struct transient_trig_data *transient_data = led_cdev->trigger_data;
> > +
> > +	if (led_cdev->activated) {
> > +		device_remove_file(led_cdev->dev, &dev_attr_transient_enabled);
> > +		device_remove_file(led_cdev->dev, &dev_attr_transient_time);
> > +		del_timer_sync(&transient_data->timer);
> > +		led_cdev->trigger_data = NULL;
> > +		led_cdev->activated = false;
> > +		kfree(transient_data);
> 
> OK.  But it might be nicer to kill off the timer before doing anything else.
Yes that is correct - will fix it.
> 
> > +	}
> > +	printk(KERN_DEBUG "Deativated led transient trigger %s\n",
> > +		led_cdev->name);
> > +}
> > +
> >
> > ...
> >
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>