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
