gpio: Remove VLA from gpiolib

PATCH Superseded Laura Abbott 2 participants 2 messages May 17, 2018 – May 18, 2018 GPIO

Share & tools

Laura Abbott

author

2018-05-17 22:32 UTC #0 lore raw

PATCH v7 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 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]>

---
v7: Tweaked the Kconfig text to clarify the wording. Also fixed a few other comments from Geert, including the earlier suggestion to reduce the zeroing since I was wrong about that.

I've kept most of the existing Tested/Reviewed-by (thank you!). If you'd like to take another look/do another test feel free.

---
 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..e3f826257871 100644
@@ -22,6 +22,18 @@ menuconfig GPIOLIB
 
 if GPIOLIB
 
+config GPIOLIB_FASTPATH_LIMIT
+	int "Maximum number of GPIOs for fast path"
+	range 16 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

Geert Uytterhoeven

Reviewed-by on v7 1/1

2018-05-18 06:51 UTC #1 lore raw

Hi Laura,

Quoted 10 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.

Quoted 8 lines

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]>
---
v7: Tweaked the Kconfig text to clarify the wording. Also fixed a few other comments from Geert, including the earlier suggestion to reduce the zeroing since I was wrong about that.

Thanks for the update!

With the minor nit below resolved:

Reviewed-by Geert Uytterhoeven <[email protected]>

drivers/gpio/Kconfig menuconfig GPIOLIB · line 27

Quoted 9 lines

drivers/gpio/Kconfig +3 -0

@@ -22,6 +22,18 @@ menuconfig GPIOLIB
 
 if GPIOLIB
 
+config GPIOLIB_FASTPATH_LIMIT
+       int "Maximum number of GPIOs for fast path"
+       range 16 512

While you can indeed fit 2 * 16 bits in a long, a lower limit of 16 doesn't make much sense, as you will use 2 longs (mask + bits) anyway. So please increase it to 32.

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