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

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 19 Apr 2012 22:04:34 -0600
Shuah Khan <shuahkhan@gmail.com> wrote:

> This patch adds a new transient trigger for one shot timer support and is
> to be used for the following example use cases:
> 
> - Control of vibrate (phones, tablets etc.) hardware by userspace app.
> - Use of LED by userspace app as activity indicator.
> - Use of LED by userspace app as a kind of watchdog indicator -- as
>        long as the app is alive, it can keep the LED illuminated, if it dies
>        the LED will be extinguished automatically.
> - Use by any userspace app that needs a transient GPIO output
> 
> Transient trigger exports two attributes:
>        transient_enabled -     one shot timer enable mechanism.
>                                1 when enabled, 0 when disabled.
>                                enabled state indicates a timer
>                                with a value of transient_time running.
>                                disabled state indicates no active timer
>                                running.
>        transient_time -        one shot timer value. When transient_enabled
>                                is set, transient_time value is used to start
>                                a timer that runs once.
>        When timer expires transient_enabled goes back to disabled state,
>        transient_time is left at the set value to be used when transient
>        is enabled at a future time. This will allow user app to set the
>        time once and enable it to run it once for the specified value as
>        needed.

Are there no comments from anyone on this?

>
> ...
>
> 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."

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

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


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

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


> +		return size;
> +	}
> +
> +	transient_data->transient_enabled = (int) state;

The typecast is unneeded.

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

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

> +	}
> +	printk(KERN_DEBUG "Deativated led transient trigger %s\n",
> +		led_cdev->name);
> +}
> +
>
> ...
>