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

[PATCH v1 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>
---
 drivers/leds/trigger/ledtrig-gpio.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
index 33cc99a1a16a..31f456dd4417 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. */
@@ -181,7 +181,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 v1 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()

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/leds/trigger/ledtrig-gpio.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
index 31f456dd4417..b01862b94c99 100644
--- a/drivers/leds/trigger/ledtrig-gpio.c
+++ b/drivers/leds/trigger/ledtrig-gpio.c
@@ -57,13 +57,13 @@ static ssize_t gpio_trig_brightness_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t n)
 {
 	struct gpio_trig_data *gpio_data = led_trigger_get_drvdata(dev);
-	unsigned desired_brightness;
+	u8 desired_brightness;
 	int ret;
 
-	ret = sscanf(buf, "%u", &desired_brightness);
-	if (ret < 1 || desired_brightness > 255) {
+	ret = kstrtou8(buf, 10, &desired_brightness);
+	if (ret) {
 		dev_err(dev, "invalid value\n");
-		return -EINVAL;
+		return ret;
 	}
 
 	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 v1 1/2] leds: trigger: gpio: GPIO 0 is valid

[Jacek Anaszewski]

Hi Andy,

Thank you for the patch.

On 8/21/19 7:17 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>
> ---
>  drivers/leds/trigger/ledtrig-gpio.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
> index 33cc99a1a16a..31f456dd4417 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;

It looks odd to me. I'd just assign invalid constant gpio number
e.g. -1.

Note that we should also do that in gpio_trig_activate(), where
gpio_data->gpio is initialized to 0 by kzalloc(). That later can
have nasty side effect in gpio_trig_gpio_store() when gpio to set
is 0. Then the condition "if (gpio_data->gpio == gpio)" will evaluate
to true and gpio_trig_irq() handler will not be registered.

>  		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. */
> @@ -181,7 +181,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);
>  }
> 

-- 
Best regards,
Jacek Anaszewski

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

[Jacek Anaszewski]

Hi Andy,

Thank you for the patch.

On 8/21/19 7:17 PM, 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()
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/leds/trigger/ledtrig-gpio.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/leds/trigger/ledtrig-gpio.c b/drivers/leds/trigger/ledtrig-gpio.c
> index 31f456dd4417..b01862b94c99 100644
> --- a/drivers/leds/trigger/ledtrig-gpio.c
> +++ b/drivers/leds/trigger/ledtrig-gpio.c
> @@ -57,13 +57,13 @@ static ssize_t gpio_trig_brightness_store(struct device *dev,
>  		struct device_attribute *attr, const char *buf, size_t n)
>  {
>  	struct gpio_trig_data *gpio_data = led_trigger_get_drvdata(dev);
> -	unsigned desired_brightness;
> +	u8 desired_brightness;
>  	int ret;
>  
> -	ret = sscanf(buf, "%u", &desired_brightness);
> -	if (ret < 1 || desired_brightness > 255) {

LED_FULL (255) is already not a hard limit for quite a long time.
While we are at it we could change that to led->max_brightness.

> +	ret = kstrtou8(buf, 10, &desired_brightness);
> +	if (ret) {
>  		dev_err(dev, "invalid value\n");
> -		return -EINVAL;
> +		return ret;
>  	}
>  
>  	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)
> 

-- 
Best regards,
Jacek Anaszewski

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

[Andy Shevchenko]

On Sat, Aug 24, 2019 at 06:47:54PM +0200, Jacek Anaszewski wrote:
> On 8/21/19 7:17 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>

> > -	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;
> 
> It looks odd to me. I'd just assign invalid constant gpio number
> e.g. -1.

Current ABI (unsigned) doesn't allow us to do this. Internally we can redefine
invalid GPIO line number to -1 or so, but does it worth it?  And actually I
would prefer -EINVAL or -ENOENT in such cases.

> Note that we should also do that in gpio_trig_activate(), where
> gpio_data->gpio is initialized to 0 by kzalloc(). That later can
> have nasty side effect in gpio_trig_gpio_store() when gpio to set
> is 0. Then the condition "if (gpio_data->gpio == gpio)" will evaluate
> to true and gpio_trig_irq() handler will not be registered.

Thanks for spotting this!

-- 
With Best Regards,
Andy Shevchenko

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

[Jacek Anaszewski]

On 8/26/19 11:57 AM, Andy Shevchenko wrote:
> On Sat, Aug 24, 2019 at 06:47:54PM +0200, Jacek Anaszewski wrote:
>> On 8/21/19 7:17 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>
> 
>>> -	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;
>>
>> It looks odd to me. I'd just assign invalid constant gpio number
>> e.g. -1.
> 
> Current ABI (unsigned) doesn't allow us to do this. Internally we can redefine

Ah, right, missed that.

> invalid GPIO line number to -1 or so, but does it worth it?  And actually I
> would prefer -EINVAL or -ENOENT in such cases.

OK, we can keep your "= gpio" assignment in view of the above, but need
to return error instead of "n".

>> Note that we should also do that in gpio_trig_activate(), where
>> gpio_data->gpio is initialized to 0 by kzalloc(). That later can
>> have nasty side effect in gpio_trig_gpio_store() when gpio to set
>> is 0. Then the condition "if (gpio_data->gpio == gpio)" will evaluate
>> to true and gpio_trig_irq() handler will not be registered.
> 
> Thanks for spotting this!
> 

-- 
Best regards,
Jacek Anaszewski

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

[Andy Shevchenko]

On Mon, Aug 26, 2019 at 08:36:04PM +0200, Jacek Anaszewski wrote:
> On 8/26/19 11:57 AM, Andy Shevchenko wrote:
> > On Sat, Aug 24, 2019 at 06:47:54PM +0200, Jacek Anaszewski wrote:
> >> On 8/21/19 7:17 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>
> > 
> >>> -	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;
> >>
> >> It looks odd to me. I'd just assign invalid constant gpio number
> >> e.g. -1.
> > 
> > Current ABI (unsigned) doesn't allow us to do this. Internally we can redefine
> 
> Ah, right, missed that.
> 
> > invalid GPIO line number to -1 or so, but does it worth it?  And actually I
> > would prefer -EINVAL or -ENOENT in such cases.
> 
> OK, we can keep your "= gpio" assignment in view of the above, but need
> to return error instead of "n".

Then we will break an ABI, where user expects no error it suddenly will be one.

-- 
With Best Regards,
Andy Shevchenko