gpio: Remove VLA from gpiolib

PATCH Superseded Laura Abbott 3 participants 5 messages May 18, 2018 – May 21, 2018 GPIO

Share & tools

Laura Abbott

author

2018-05-18 17:53 UTC #0 lore raw

PATCH v8 1/1 gpio: Remove VLA from gpiolib

The new challenge is to remove VLAs from the kernel (see https://lkml.org/lkml/2018/3/7/621) to eventually turn on -Wvla.

Using a kmalloc array is the easy way to fix this but kmalloc is still more expensive than stack allocation. Introduce a fast path with a fixed size stack array to cover most chip with gpios below some fixed amount. The slow path dynamically allocates an array to cover those chips with a large number of gpios.

Reviewed-by Geert Uytterhoeven <[email protected]>

Reviewed-by Phil Reid <[email protected]>

Reviewed-and-tested-by Lukas Wunner <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

---
v8: Adjust lower bound

---
 drivers/gpio/Kconfig          | 12 ++++++
 drivers/gpio/gpiolib.c        | 76 +++++++++++++++++++++++++++--------
 drivers/gpio/gpiolib.h        |  2 +-
 include/linux/gpio/consumer.h | 10 +++--
 4 files changed, 78 insertions(+), 22 deletions(-)

drivers/gpio/Kconfig +12 -0

index b960f6f35abd..71c0ab46f216 100644
@@ -22,6 +22,18 @@ menuconfig GPIOLIB
 
 if GPIOLIB
 
+config GPIOLIB_FASTPATH_LIMIT
+	int "Maximum number of GPIOs for fast path"
+	range 32 512
+	default 512
+	help
+	   This adjusts the point at which certain APIs will switch from
+	   using a stack allocated buffer to a dynamically allocated buffer.
+
+	   You shouldn't need to change this unless you really need to
+	   optimize either stack space or performance. Change this carefully
+	   since setting an incorrect value could cause stack corruption.
+
 config OF_GPIO
 	def_bool y
 	depends on OF

drivers/gpio/gpiolib.c +59 ~~-17~~

168 lines not on this page · raw message

drivers/gpio/gpiolib.h +1 -1

index ad456b6f9d8b..1a8e20363861 100644
@@ -188,7 +188,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 				  unsigned int array_size,
 				  struct gpio_desc **desc_array,
 				  int *value_array);
-void gpiod_set_array_value_complex(bool raw, bool can_sleep,
+int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 				   unsigned int array_size,
 				   struct gpio_desc **desc_array,
 				   int *value_array);

include/linux/gpio/consumer.h +6 -4

index dbd065963296..243112c7fa7d 100644
@@ -116,7 +116,7 @@ int gpiod_get_raw_array_value(unsigned int array_size,
 			      struct gpio_desc **desc_array,
 			      int *value_array);
 void gpiod_set_raw_value(struct gpio_desc *desc, int value);
-void gpiod_set_raw_array_value(unsigned int array_size,
+int gpiod_set_raw_array_value(unsigned int array_size,
 			       struct gpio_desc **desc_array,
 			       int *value_array);

@@ -134,7 +134,7 @@ int gpiod_get_raw_array_value_cansleep(unsigned int array_size,
 				       struct gpio_desc **desc_array,
 				       int *value_array);
 void gpiod_set_raw_value_cansleep(struct gpio_desc *desc, int value);
-void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
+int gpiod_set_raw_array_value_cansleep(unsigned int array_size,
 					struct gpio_desc **desc_array,
 					int *value_array);

@@ -369,12 +369,13 @@ static inline void gpiod_set_raw_value(struct gpio_desc *desc, int value)
 	/* GPIO can never have been requested */
 	WARN_ON(1);
 }
-static inline void gpiod_set_raw_array_value(unsigned int array_size,
+static inline int gpiod_set_raw_array_value(unsigned int array_size,
 					     struct gpio_desc **desc_array,
 					     int *value_array)
 {
 	/* GPIO can never have been requested */
 	WARN_ON(1);
+	return 0;
 }
 
 static inline int gpiod_get_value_cansleep(const struct gpio_desc *desc)

@@ -423,12 +424,13 @@ static inline void gpiod_set_raw_value_cansleep(struct gpio_desc *desc,
 	/* GPIO can never have been requested */
 	WARN_ON(1);
 }
-static inline void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
+static inline int gpiod_set_raw_array_value_cansleep(unsigned int array_size,
 						struct gpio_desc **desc_array,
 						int *value_array)
 {
 	/* GPIO can never have been requested */
 	WARN_ON(1);
+	return 0;
 }
 
 static inline int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)

Signature · 1 line

2.17.0

Andy Shevchenko

2018-05-18 20:07 UTC #1 lore raw

Quoted 16 lines · Laura Abbott

The new challenge is to remove VLAs from the kernel (see https://lkml.org/lkml/2018/3/7/621) to eventually turn on -Wvla.
Using a kmalloc array is the easy way to fix this but kmalloc is still more expensive than stack allocation. Introduce a fast path with a fixed size stack array to cover most chip with gpios below some fixed amount. The slow path dynamically allocates an array to cover those chips with a large number of gpios.
drivers/gpio/gpiolib.c +4 -0
+               unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)];
+               unsigned long *mask, *bits;
                int first, j, ret;
 
+               if (likely(chip->ngpio <= FASTPATH_NGPIO)) {
+                       mask = fastpath;

drivers/gpio/gpiolib.c int gpiod_get_array_value_complex(bool raw, bool can_sleep, · line 2745

Quoted 10 lines

drivers/gpio/gpiolib.c +10 -0

+               } else {
+                       mask = kmalloc_array(2 * BITS_TO_LONGS(chip->ngpio),
+                                          sizeof(*mask),
+                                          can_sleep ? GFP_KERNEL : GFP_ATOMIC);
+                       if (!mask)
+                               return -ENOMEM;
+               }
+
+               bits = mask + BITS_TO_LONGS(chip->ngpio);
+               memset(mask, 0, BITS_TO_LONGS(chip->ngpio) * sizeof(*mask));

Wouldn't be better

bitmap_zero(mask, chip->ngpio);

Review

Quoted 2 lines

(quoted diff) +2 -0

+               bits = mask + BITS_TO_LONGS(chip->ngpio);
+               memset(mask, 0, BITS_TO_LONGS(chip->ngpio) * sizeof(*mask));

Ditto.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Geert Uytterhoeven

2018-05-18 21:05 UTC #2 lore raw

Hi Andy,

Quoted 28 lines · Andy Shevchenko

Quoted 16 lines · Laura Abbott
The new challenge is to remove VLAs from the kernel (see https://lkml.org/lkml/2018/3/7/621) to eventually turn on -Wvla.
Using a kmalloc array is the easy way to fix this but kmalloc is still more expensive than stack allocation. Introduce a fast path with a fixed size stack array to cover most chip with gpios below some fixed amount. The slow path dynamically allocates an array to cover those chips with a large number of gpios.
drivers/gpio/gpiolib.c +4 -0
+               unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)];
+               unsigned long *mask, *bits;
                int first, j, ret;
 
+               if (likely(chip->ngpio <= FASTPATH_NGPIO)) {
+                       mask = fastpath;
drivers/gpio/gpiolib.c int gpiod_get_array_value_complex(bool raw, bool can_sleep, · line 2745
Quoted 10 lines
drivers/gpio/gpiolib.c +10 -0
+               } else {
+                       mask = kmalloc_array(2 * BITS_TO_LONGS(chip->ngpio),
+                                          sizeof(*mask),
+                                          can_sleep ? GFP_KERNEL : GFP_ATOMIC);
+                       if (!mask)
+                               return -ENOMEM;
+               }
+
+               bits = mask + BITS_TO_LONGS(chip->ngpio);
+               memset(mask, 0, BITS_TO_LONGS(chip->ngpio) * sizeof(*mask));

Quoted 5 lines

Wouldn't be better
bitmap_zero(mask, chip->ngpio);
?

chip->ngpio is never const, so bitmap_zero() would always fall back to plain memset().

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Andy Shevchenko

2018-05-18 21:12 UTC #3 lore raw

Quoted 7 lines · Geert Uytterhoeven

Quoted 5 lines · Andy Shevchenko
Quoted 2 lines · Laura Abbott
(quoted diff) +2 -0
+               bits = mask + BITS_TO_LONGS(chip->ngpio);
+               memset(mask, 0, BITS_TO_LONGS(chip->ngpio) * sizeof(*mask));

Quoted 8 lines

Quoted 5 lines
Wouldn't be better
bitmap_zero(mask, chip->ngpio);
?
chip->ngpio is never const, so bitmap_zero() would always fall back to plain memset().

Yes. But it also much more readable and understandable here.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Laura Abbott

author

2018-05-21 17:30 UTC #4 lore raw

Quoted 11 lines · Andy Shevchenko

Quoted 9 lines · Geert Uytterhoeven
Quoted 7 lines · Andy Shevchenko
Review
Quoted 2 lines · Laura Abbott
(quoted diff) +2 -0
+               bits = mask + BITS_TO_LONGS(chip->ngpio);
+               memset(mask, 0, BITS_TO_LONGS(chip->ngpio) * sizeof(*mask));
Wouldn't be better

Quoted 9 lines

Quoted 6 lines
Quoted 3 lines
bitmap_zero(mask, chip->ngpio);
?
chip->ngpio is never const, so bitmap_zero() would always fall back to plain memset().
Yes. But it also much more readable and understandable here.

I wasn't aware of that API. I'll switch to that.