On Fri 2019-08-30 18:08:20, Andy Shevchenko wrote:
> sscanf() is a heavy one and moreover requires additional boundary checks.
> Convert driver to use kstrtox() and replace kstrtoul() by kstrtobool()
> in gpio_trig_inverted_store().
> 
> While here, check the desired brightness against maximum defined for
> a certain LED.

One change per patch, please.

Because this one will not end well.

> @@ -60,10 +60,10 @@ static ssize_t gpio_trig_brightness_store(struct device *dev,
>  	unsigned desired_brightness;
>  	int ret;
>  
> -	ret = sscanf(buf, "%u", &desired_brightness);
> -	if (ret < 1 || desired_brightness > 255) {
> +	ret = kstrtouint(buf, 10, &desired_brightness);
> +	if (ret || desired_brightness > gpio_data->led->max_brightness) {
>  		dev_err(dev, "invalid value\n");
> -		return -EINVAL;
> +		return ret ? ret : -EINVAL;
>  	}

We have people writing 255 into brightness, because that's what we
used to do even for on/off LEDS. It is expected to work even for leds
with max_brightness of 1.

So... we want to saturate here, not return -EINVAL. (And we will
eventually want to switch on/off leds to max_brightness = 1...)

> @@ -86,16 +86,13 @@ static ssize_t gpio_trig_inverted_store(struct device *dev,
>  {
>  	struct led_classdev *led = led_trigger_get_led(dev);
>  	struct gpio_trig_data *gpio_data = led_trigger_get_drvdata(dev);
> -	unsigned long inverted;
> +	bool inverted;
>  	int ret;
>  
> -	ret = kstrtoul(buf, 10, &inverted);
> -	if (ret < 0)
> +	ret = kstrtobool(buf, &inverted);
> +	if (ret)
>  		return ret;
>  
> -	if (inverted > 1)
> -		return -EINVAL;
> -
>  	gpio_data->inverted = inverted;
>  
>  	/* After inverting, we need to update the LED. */

So, this accepted 0/1. Now it also accepts true false and many other pairs.

Which... might be ok. But probably should be separated.

Best regards,
									Pavel
									
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html