[PATCH v2 1/2] leds: trigger: gpio: GPIO 0 is valid

[PATCH v2 1/2] leds: trigger: gpio: GPIO 0 is valid

[Andy Shevchenko]

Allow all valid GPIOs to be used in the driver.

Fixes: 17354bfe8527 ("leds: Add gpio-led trigger")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
- set initial GPIO value to -ENOENT (Jacek)
 drivers/leds/trigger/ledtrig-gpio.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
index 33cc99a1a16a..dc64679b1a92 100644
--- a/drivers/leds/trigger/ledtrig-gpio.c
+++ b/drivers/leds/trigger/ledtrig-gpio.c
@@ -131,10 +131,10 @@ static ssize_t gpio_trig_gpio_store(struct device *dev,
 	if (gpio_data->gpio == gpio)
 		return n;
 
-	if (!gpio) {
-		if (gpio_data->gpio != 0)
+	if (!gpio_is_valid(gpio)) {
+		if (gpio_is_valid(gpio_data->gpio))
 			free_irq(gpio_to_irq(gpio_data->gpio), led);
-		gpio_data->gpio = 0;
+		gpio_data->gpio = gpio;
 		return n;
 	}
 
@@ -144,7 +144,7 @@ static ssize_t gpio_trig_gpio_store(struct device *dev,
 	if (ret) {
 		dev_err(dev, "request_irq failed with error %d\n", ret);
 	} else {
-		if (gpio_data->gpio != 0)
+		if (gpio_is_valid(gpio_data->gpio))
 			free_irq(gpio_to_irq(gpio_data->gpio), led);
 		gpio_data->gpio = gpio;
 		/* After changing the GPIO, we need to update the LED. */
@@ -172,6 +172,8 @@ static int gpio_trig_activate(struct led_classdev *led)
 		return -ENOMEM;
 
 	gpio_data->led = led;
+	gpio_data->gpio = -ENOENT;
+
 	led_set_trigger_data(led, gpio_data);
 
 	return 0;
@@ -181,7 +183,7 @@ static void gpio_trig_deactivate(struct led_classdev *led)
 {
 	struct gpio_trig_data *gpio_data = led_get_trigger_data(led);
 
-	if (gpio_data->gpio != 0)
+	if (gpio_is_valid(gpio_data->gpio))
 		free_irq(gpio_to_irq(gpio_data->gpio), led);
 	kfree(gpio_data);
 }
-- 
2.23.0.rc1

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

[Andy Shevchenko]

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.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
- check against max_brightness (Jacek)
 drivers/leds/trigger/ledtrig-gpio.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
index dc64679b1a92..2d70c35588cb 100644
--- a/drivers/leds/trigger/ledtrig-gpio.c
+++ b/drivers/leds/trigger/ledtrig-gpio.c
@@ -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;
 	}
 
 	gpio_data->desired_brightness = desired_brightness;
@@ -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. */
@@ -122,10 +119,10 @@ static ssize_t gpio_trig_gpio_store(struct device *dev,
 	unsigned gpio;
 	int ret;
 
-	ret = sscanf(buf, "%u", &gpio);
-	if (ret < 1) {
+	ret = kstrtouint(buf, 10, &gpio);
+	if (ret) {
 		dev_err(dev, "couldn't read gpio number\n");
-		return -EINVAL;
+		return ret;
 	}
 
 	if (gpio_data->gpio == gpio)
-- 
2.23.0.rc1

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2059 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

[Jacek Anaszewski]

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);

So the condition should be limited to checking error code.

>> @@ -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
> 									
> 

-- 
Best regards,
Jacek Anaszewski

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

[Jacek Anaszewski]

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

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

[Andy Shevchenko]

On Sun, Sep 01, 2019 at 12:23:17PM +0200, 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...)

Agree. Thank you for your review!

> 
> > @@ -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.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] leds: trigger: gpio: GPIO 0 is valid

[Jacek Anaszewski]

Hi Andy,

Thank you for the v2.

On 8/30/19 5:08 PM, Andy Shevchenko wrote:
> Allow all valid GPIOs to be used in the driver.
> 
> Fixes: 17354bfe8527 ("leds: Add gpio-led trigger")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> - set initial GPIO value to -ENOENT (Jacek)
>  drivers/leds/trigger/ledtrig-gpio.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
> index 33cc99a1a16a..dc64679b1a92 100644
> --- a/drivers/leds/trigger/ledtrig-gpio.c
> +++ b/drivers/leds/trigger/ledtrig-gpio.c
> @@ -131,10 +131,10 @@ static ssize_t gpio_trig_gpio_store(struct device *dev,
>  	if (gpio_data->gpio == gpio)
>  		return n;
>  
> -	if (!gpio) {
> -		if (gpio_data->gpio != 0)
> +	if (!gpio_is_valid(gpio)) {
> +		if (gpio_is_valid(gpio_data->gpio))
>  			free_irq(gpio_to_irq(gpio_data->gpio), led);
> -		gpio_data->gpio = 0;
> +		gpio_data->gpio = gpio;
>  		return n;
>  	}
>  
> @@ -144,7 +144,7 @@ static ssize_t gpio_trig_gpio_store(struct device *dev,
>  	if (ret) {
>  		dev_err(dev, "request_irq failed with error %d\n", ret);
>  	} else {
> -		if (gpio_data->gpio != 0)
> +		if (gpio_is_valid(gpio_data->gpio))
>  			free_irq(gpio_to_irq(gpio_data->gpio), led);
>  		gpio_data->gpio = gpio;
>  		/* After changing the GPIO, we need to update the LED. */
> @@ -172,6 +172,8 @@ static int gpio_trig_activate(struct led_classdev *led)
>  		return -ENOMEM;
>  
>  	gpio_data->led = led;
> +	gpio_data->gpio = -ENOENT;
> +
>  	led_set_trigger_data(led, gpio_data);
>  
>  	return 0;
> @@ -181,7 +183,7 @@ static void gpio_trig_deactivate(struct led_classdev *led)
>  {
>  	struct gpio_trig_data *gpio_data = led_get_trigger_data(led);
>  
> -	if (gpio_data->gpio != 0)
> +	if (gpio_is_valid(gpio_data->gpio))
>  		free_irq(gpio_to_irq(gpio_data->gpio), led);
>  	kfree(gpio_data);
>  }
> 

Both 1/2 and 2/2 applied.

-- 
Best regards,
Jacek Anaszewski