> + * Based on Richard Purdie's ledtrig-timer.c and Atsushi Nemoto's
> + * ledtrig-heartbeat.c and Shuah Khan's ledtrig-transient.c
I stil think this belongs in user space except for platforms with hardware
acceleration for it.
> +#define MAX_PATTEN_LEN 255
Arbitary limits that are not needed if it was in userspace, and not it
seems a sensible one - why not use 256 ?
> +static ssize_t pattern_delay_unit_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t size)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
> + unsigned long state;
> + ssize_t ret = -EINVAL;
> +
> + ret = kstrtoul(buf, 10, &state);
> + if (ret)
> + return ret;
> +
> + pattern_data->delay_unit = state;
What happens if this is zero ?
> +static ssize_t pattern_pattern_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t size)
> +{
> + struct led_classdev *led_cdev = dev_get_drvdata(dev);
> + struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
> + int i;
> + ssize_t ret = -EINVAL;
> +
> + int len = (size > MAX_PATTEN_LEN) ? MAX_PATTEN_LEN : (size - 1);
> +
> + /* legality check */
> + for (i = 0; i < len; i++) {
> + if (buf[i] != ' ' && buf[i] != '#' && buf[i] != '/')
> + return ret;
> + }
> +
> + del_timer_sync(&pattern_data->timer);
> +
> + memcpy(pattern_data->pattern, buf, len);
> + pattern_data->pattern[len] = '\0';
> + pattern_data->pattern_len = len;
> + pattern_data->count = 0;
> +
> + mod_timer(&pattern_data->timer, jiffies + 1);
What if the pattern isn't currently active ?
> + return size;
You only consumed len bytes so you should return len here.
> +}
> +
> +static DEVICE_ATTR(pattern, 0644, pattern_pattern_show, pattern_pattern_store);
> +static DEVICE_ATTR(delay_unit, 0644,
> + pattern_delay_unit_show, pattern_delay_unit_store);
Why are these world readable. If patterns tell you an action is due they
provide information that other processes shouldn't have access to.
> + memset(tdata->pattern, 0, MAX_PATTEN_LEN + 1);
Why +1, you don't need a zero terminator you know the length
Why allocate a fixed 256 byte blob when you can make the data the end of
the struct (ie pattern[0] in the declaration) and not waste memory.
> +static void pattern_trig_deactivate(struct led_classdev *led_cdev)
> +{
> + struct pattern_trig_data *pattern_data = led_cdev->trigger_data;
> +
> + if (led_cdev->activated) {
> + del_timer_sync(&pattern_data->timer);
> + device_remove_file(led_cdev->dev, &dev_attr_pattern);
> + device_remove_file(led_cdev->dev, &dev_attr_delay_unit);
This doesn't as far as I can see do what you think. If I have the file
currently open then device_remove_file will not remove my existing access
to it, but you just released the pattern data so I now write to free
memory.
> + led_cdev->trigger_data = NULL;
> + led_cdev->activated = false;
> + kfree(pattern_data);
> + }
> + __led_set_brightness(led_cdev, LED_OFF);
> +}
> +
> +static struct led_trigger pattern_trigger = {
const ?
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
