[PATCH v3 0/8] gpio: mockup: updates for 4.13

[PATCH v3 0/8] gpio: mockup: updates for 4.13

[Bartosz Golaszewski]

v1 -> v2:
- omit applied patches
- use kstrtoint_from_user() for debugfs input sanitization
- add a patch improving the code in gpio_mockup_event_write()
- bail out from gpio_mockup_event_write() on invalid input even when
  nobody is listening for events

v2 -> v3:
- added the patch replacing devm_kzalloc() with devm_kcalloc() where
  applicable

Bartosz Golaszewski (8):
  gpio: mockup: improve the debugfs input sanitization
  gpio: mockup: tweak gpio_mockup_event_write()
  gpio: mockup: refuse to accept an odd number of GPIO ranges
  gpio: mockup: improve readability
  gpio: mockup: don't return magic numbers from probe()
  gpio: mockup: improve the error message
  gpio: mockup: add myself as author
  gpio: mockup: use devm_kcalloc() where applicable

 drivers/gpio/gpio-mockup.c | 67 ++++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 32 deletions(-)

-- 
2.9.3

[PATCH v3 1/8] gpio: mockup: improve the debugfs input sanitization

[Bartosz Golaszewski]

We're currently only checking the first character of the input to the
debugfs event files, so a string like '0sdfdsf' is valid and indicates
a falling edge event.

Be more strict and only allow '0', '1', '0\n' & '1\n'.

While we're at it: move the sanitization code before the irq_enabled
check so that we indicate an error on invalid input even if nobody is
waiting for events.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index ba8d62a..da76267 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -208,8 +208,7 @@ static ssize_t gpio_mockup_event_write(struct file *file,
 	struct seq_file *sfile;
 	struct gpio_desc *desc;
 	struct gpio_chip *gc;
-	int val;
-	char buf;
+	int rv, val;
 
 	sfile = file->private_data;
 	priv = sfile->private;
@@ -217,19 +216,15 @@ static ssize_t gpio_mockup_event_write(struct file *file,
 	chip = priv->chip;
 	gc = &chip->gc;
 
+	rv = kstrtoint_from_user(usr_buf, size, 0, &val);
+	if (rv)
+		return rv;
+	if (val != 0 && val != 1)
+		return -EINVAL;
+
 	if (!chip->lines[priv->offset].irq_enabled)
 		return size;
 
-	if (copy_from_user(&buf, usr_buf, 1))
-		return -EFAULT;
-
-	if (buf == '0')
-		val = 0;
-	else if (buf == '1')
-		val = 1;
-	else
-		return -EINVAL;
-
 	gpiod_set_value_cansleep(desc, val);
 	priv->chip->irq_ctx.irq = gc->irq_base + priv->offset;
 	irq_work_queue(&priv->chip->irq_ctx.work);
-- 
2.9.3

Re: [PATCH v3 1/8] gpio: mockup: improve the debugfs input sanitization

[Linus Walleij]

On Fri, Jun 9, 2017 at 1:41 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> We're currently only checking the first character of the input to the
> debugfs event files, so a string like '0sdfdsf' is valid and indicates
> a falling edge event.
>
> Be more strict and only allow '0', '1', '0\n' & '1\n'.
>
> While we're at it: move the sanitization code before the irq_enabled
> check so that we indicate an error on invalid input even if nobody is
> waiting for events.
>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>

Patch applied.

Yours,
Linus Walleij

[PATCH v3 2/8] gpio: mockup: tweak gpio_mockup_event_write()

[Bartosz Golaszewski]

Invert the logic of the irq_enabled check and only access the private
data after the input is sanitized.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index da76267..d78e8e0 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -210,24 +210,23 @@ static ssize_t gpio_mockup_event_write(struct file *file,
 	struct gpio_chip *gc;
 	int rv, val;
 
-	sfile = file->private_data;
-	priv = sfile->private;
-	desc = priv->desc;
-	chip = priv->chip;
-	gc = &chip->gc;
-
 	rv = kstrtoint_from_user(usr_buf, size, 0, &val);
 	if (rv)
 		return rv;
 	if (val != 0 && val != 1)
 		return -EINVAL;
 
-	if (!chip->lines[priv->offset].irq_enabled)
-		return size;
+	sfile = file->private_data;
+	priv = sfile->private;
+	desc = priv->desc;
+	chip = priv->chip;
+	gc = &chip->gc;
 
-	gpiod_set_value_cansleep(desc, val);
-	priv->chip->irq_ctx.irq = gc->irq_base + priv->offset;
-	irq_work_queue(&priv->chip->irq_ctx.work);
+	if (chip->lines[priv->offset].irq_enabled) {
+		gpiod_set_value_cansleep(desc, val);
+		priv->chip->irq_ctx.irq = gc->irq_base + priv->offset;
+		irq_work_queue(&priv->chip->irq_ctx.work);
+	}
 
 	return size;
 }
-- 
2.9.3

Re: [PATCH v3 2/8] gpio: mockup: tweak gpio_mockup_event_write()

[Linus Walleij]

On Fri, Jun 9, 2017 at 1:41 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> Invert the logic of the irq_enabled check and only access the private
> data after the input is sanitized.
>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>

Patch applied.

Yours,
Linus Walleij

[PATCH v3 3/8] gpio: mockup: refuse to accept an odd number of GPIO ranges

[Bartosz Golaszewski]

Currently we ignore the last odd range value, since each chip is
described by two values. Be more strict and require the user to
pass an even number of ranges.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index d78e8e0..d95d37a 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -334,7 +334,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 	int ret, i, base, ngpio;
 	char *chip_name;
 
-	if (gpio_mockup_params_nr < 2)
+	if (gpio_mockup_params_nr < 2 || (gpio_mockup_params_nr % 2))
 		return -EINVAL;
 
 	chips = devm_kzalloc(dev,
-- 
2.9.3

Re: [PATCH v3 3/8] gpio: mockup: refuse to accept an odd number of GPIO ranges

[Linus Walleij]

On Fri, Jun 9, 2017 at 1:41 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> Currently we ignore the last odd range value, since each chip is
> described by two values. Be more strict and require the user to
> pass an even number of ranges.
>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>

Patch applied.

Yours,
Linus Walleij

[PATCH v3 4/8] gpio: mockup: improve readability

[Bartosz Golaszewski]

We currently shift bits here and there without actually explaining
what we're doing. Add some helper variables with names indicating
their purpose to improve the code readability.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index d95d37a..0cb6cba 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -27,6 +27,11 @@
 
 #define GPIO_MOCKUP_NAME	"gpio-mockup"
 #define	GPIO_MOCKUP_MAX_GC	10
+/*
+ * We're storing two values per chip: the GPIO base and the number
+ * of GPIO lines.
+ */
+#define GPIO_MOCKUP_MAX_RANGES	(GPIO_MOCKUP_MAX_GC * 2)
 
 enum {
 	GPIO_MOCKUP_DIR_OUT = 0,
@@ -62,7 +67,7 @@ struct gpio_mockup_dbgfs_private {
 	int offset;
 };
 
-static int gpio_mockup_ranges[GPIO_MOCKUP_MAX_GC << 1];
+static int gpio_mockup_ranges[GPIO_MOCKUP_MAX_RANGES];
 static int gpio_mockup_params_nr;
 module_param_array(gpio_mockup_ranges, int, &gpio_mockup_params_nr, 0400);
 
@@ -329,23 +334,24 @@ static int gpio_mockup_add(struct device *dev,
 
 static int gpio_mockup_probe(struct platform_device *pdev)
 {
-	struct gpio_mockup_chip *chips;
+	int ret, i, base, ngpio, num_chips;
 	struct device *dev = &pdev->dev;
-	int ret, i, base, ngpio;
+	struct gpio_mockup_chip *chips;
 	char *chip_name;
 
 	if (gpio_mockup_params_nr < 2 || (gpio_mockup_params_nr % 2))
 		return -EINVAL;
 
-	chips = devm_kzalloc(dev,
-			     sizeof(*chips) * (gpio_mockup_params_nr >> 1),
-			     GFP_KERNEL);
+	/* Each chip is described by two values. */
+	num_chips = gpio_mockup_params_nr / 2;
+
+	chips = devm_kzalloc(dev, sizeof(*chips) * num_chips, GFP_KERNEL);
 	if (!chips)
 		return -ENOMEM;
 
 	platform_set_drvdata(pdev, chips);
 
-	for (i = 0; i < gpio_mockup_params_nr >> 1; i++) {
+	for (i = 0; i < num_chips; i++) {
 		base = gpio_mockup_ranges[i * 2];
 
 		if (base == -1)
-- 
2.9.3

Re: [PATCH v3 4/8] gpio: mockup: improve readability

[Linus Walleij]

On Fri, Jun 9, 2017 at 1:41 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> We currently shift bits here and there without actually explaining
> what we're doing. Add some helper variables with names indicating
> their purpose to improve the code readability.
>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>

Patch applied.

Yours,
Linus Walleij

[PATCH v3 5/8] gpio: mockup: don't return magic numbers from probe()

[Bartosz Golaszewski]

When the requested number of GPIO lines is 0, return -EINVAL, not
-1 which is -EPERM.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 0cb6cba..ab2d38e 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -369,7 +369,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 			ret = gpio_mockup_add(dev, &chips[i],
 					      chip_name, base, ngpio);
 		} else {
-			ret = -1;
+			ret = -EINVAL;
 		}
 
 		if (ret) {
-- 
2.9.3

Re: [PATCH v3 5/8] gpio: mockup: don't return magic numbers from probe()

[Linus Walleij]

On Fri, Jun 9, 2017 at 1:41 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> When the requested number of GPIO lines is 0, return -EINVAL, not
> -1 which is -EPERM.
>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>

Patch applied.

Yours,
Linus Walleij

[PATCH v3 6/8] gpio: mockup: improve the error message

[Bartosz Golaszewski]

Indicate the error number and make the message a bit more elaborate.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index ab2d38e..2f4fe41 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -373,8 +373,9 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 		}
 
 		if (ret) {
-			dev_err(dev, "gpio<%d..%d> add failed\n",
-				base, base < 0 ? ngpio : base + ngpio);
+			dev_err(dev,
+				"adding gpiochip failed: %d (base: %d, ngpio: %d)\n",
+				ret, base, base < 0 ? ngpio : base + ngpio);
 
 			return ret;
 		}
-- 
2.9.3

Re: [PATCH v3 6/8] gpio: mockup: improve the error message

[Linus Walleij]

On Fri, Jun 9, 2017 at 1:41 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> Indicate the error number and make the message a bit more elaborate.
>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>

Patch applied.

Yours,
Linus Walleij

[PATCH v3 7/8] gpio: mockup: add myself as author

[Bartosz Golaszewski]

Just taking credit for the recent changes and new features. :)

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 2f4fe41..536a229 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 2014  Kamlakant Patel <kamlakant.patel@broadcom.com>
  * Copyright (C) 2015-2016  Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>
+ * Copyright (C) 2017 Bartosz Golaszewski <brgl@bgdev.pl>
  *
  * This program is free software; you can redistribute  it and/or modify it
  * under  the terms of  the GNU General  Public License as published by the
@@ -432,5 +433,6 @@ module_exit(mock_device_exit);
 
 MODULE_AUTHOR("Kamlakant Patel <kamlakant.patel@broadcom.com>");
 MODULE_AUTHOR("Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>");
+MODULE_AUTHOR("Bartosz Golaszewski <brgl@bgdev.pl>");
 MODULE_DESCRIPTION("GPIO Testing driver");
 MODULE_LICENSE("GPL v2");
-- 
2.9.3

Re: [PATCH v3 7/8] gpio: mockup: add myself as author

[Linus Walleij]

On Fri, Jun 9, 2017 at 1:41 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> Just taking credit for the recent changes and new features. :)
>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>

Patch applied.

Yours,
Linus Walleij

[PATCH v3 8/8] gpio: mockup: use devm_kcalloc() where applicable

[Bartosz Golaszewski]

When allocating a zeroed array of objects use devm_kcalloc() instead
of manually calculating the required size and using devm_kzalloc().

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 536a229..a6565e1 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -128,7 +128,7 @@ static int gpio_mockup_name_lines(struct device *dev,
 	char **names;
 	int i;
 
-	names = devm_kzalloc(dev, sizeof(char *) * gc->ngpio, GFP_KERNEL);
+	names = devm_kcalloc(dev, gc->ngpio, sizeof(char *), GFP_KERNEL);
 	if (!names)
 		return -ENOMEM;
 
@@ -308,8 +308,8 @@ static int gpio_mockup_add(struct device *dev,
 	gc->get_direction = gpio_mockup_get_direction;
 	gc->to_irq = gpio_mockup_to_irq;
 
-	chip->lines = devm_kzalloc(dev, sizeof(*chip->lines) * gc->ngpio,
-				   GFP_KERNEL);
+	chip->lines = devm_kcalloc(dev, gc->ngpio,
+				   sizeof(*chip->lines), GFP_KERNEL);
 	if (!chip->lines)
 		return -ENOMEM;
 
@@ -346,7 +346,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 	/* Each chip is described by two values. */
 	num_chips = gpio_mockup_params_nr / 2;
 
-	chips = devm_kzalloc(dev, sizeof(*chips) * num_chips, GFP_KERNEL);
+	chips = devm_kcalloc(dev, num_chips, sizeof(*chips), GFP_KERNEL);
 	if (!chips)
 		return -ENOMEM;
 
-- 
2.9.3

Re: [PATCH v3 8/8] gpio: mockup: use devm_kcalloc() where applicable

[Linus Walleij]

On Fri, Jun 9, 2017 at 1:41 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> When allocating a zeroed array of objects use devm_kcalloc() instead
> of manually calculating the required size and using devm_kzalloc().
>
> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>

Patch applied.

Yours,
Linus Walleij

Re: [PATCH v3 0/8] gpio: mockup: updates for 4.13

[Andy Shevchenko]

On Fri, Jun 9, 2017 at 2:41 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> v1 -> v2:
> - omit applied patches
> - use kstrtoint_from_user() for debugfs input sanitization
> - add a patch improving the code in gpio_mockup_event_write()
> - bail out from gpio_mockup_event_write() on invalid input even when
>   nobody is listening for events
>
> v2 -> v3:
> - added the patch replacing devm_kzalloc() with devm_kcalloc() where
>   applicable
>

FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Bartosz Golaszewski (8):
>   gpio: mockup: improve the debugfs input sanitization
>   gpio: mockup: tweak gpio_mockup_event_write()
>   gpio: mockup: refuse to accept an odd number of GPIO ranges
>   gpio: mockup: improve readability
>   gpio: mockup: don't return magic numbers from probe()
>   gpio: mockup: improve the error message
>   gpio: mockup: add myself as author
>   gpio: mockup: use devm_kcalloc() where applicable
>
>  drivers/gpio/gpio-mockup.c | 67 ++++++++++++++++++++++++----------------------
>  1 file changed, 35 insertions(+), 32 deletions(-)
>
> --
> 2.9.3
>



-- 
With Best Regards,
Andy Shevchenko