Re: [PATCH] Add LED pattern trigger

[One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>]

> + * 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 ?