Re: [PATCH v2 2/2] leds: trigger: gpio: Convert to use kstrtox()

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

On 9/1/19 1:36 PM, Jacek Anaszewski wrote:
> Hi Pavel,
> 
> On 9/1/19 12:23 PM, Pavel Machek wrote:
>> 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...)
> 
> Good point. We shouldn't fail here but proceed similarly as in case
> of setting brightness for a LED in led_set_brightness_nosleep(), i.e.
> here it should be:
> 
> desired_brightness = min(desired_brightness,
>                          gpio_data->led->->max_brightness);

PS. Dropped the patch.

-- 
Best regards,
Jacek Anaszewski