Messages in this thread | | | Date | Fri, 20 Apr 2012 14:19:14 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH ] leds: add new transient trigger for one shot timer support |
| |
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); > +} > + > > ... >
| |