Hi,
This is v2 of the series to remove VLAs from the kernel. This is mostly
to collect some of the feedback I've gotten so far, more detailed
descriptions are in the individual patches.
Thanks,
Laura
Laura Abbott (4):
gpio: Remove VLA from gpiolib
gpio: Remove VLA from MAX3191X driver
gpio: Remove VLA from xra1403 driver
gpio: Remove VLA from stmpe driver
drivers/gpio/gpio-max3191x.c | 7 +++-
drivers/gpio/gpio-stmpe.c | 7 +++-
drivers/gpio/gpio-xra1403.c | 8 ++++-
drivers/gpio/gpiolib.c | 78 ++++++++++++++++++++++++++++++++++---------
drivers/gpio/gpiolib.h | 2 +-
include/linux/gpio/consumer.h | 8 +++--
6 files changed, 87 insertions(+), 23 deletions(-)
--
2.14.3
The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)
This patch replaces several a VLA with an appropriate call to
kmalloc_array.
Signed-off-by: Laura Abbott <[email protected]>
---
v2: No changes
---
drivers/gpio/gpio-max3191x.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-max3191x.c b/drivers/gpio/gpio-max3191x.c
index f74b1072e84b..b5b9cb1fda50 100644
--- a/drivers/gpio/gpio-max3191x.c
+++ b/drivers/gpio/gpio-max3191x.c
@@ -315,12 +315,17 @@ static void gpiod_set_array_single_value_cansleep(unsigned int ndescs,
struct gpio_desc **desc,
int value)
{
- int i, values[ndescs];
+ int i, *values;
+
+ values = kmalloc_array(ndescs, sizeof(*values), GFP_KERNEL);
+ if (!values)
+ return;
for (i = 0; i < ndescs; i++)
values[i] = value;
gpiod_set_array_value_cansleep(ndescs, desc, values);
+ kfree(values);
}
static struct gpio_descs *devm_gpiod_get_array_optional_count(
--
2.14.3
The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)
This patch replaces a VLA with an appropriate call to kmalloc_array.
Reviewed-by: Nandor Han <[email protected]>
Signed-off-by: Laura Abbott <[email protected]>
---
v3: Add reviwed-by
---
drivers/gpio/gpio-xra1403.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-xra1403.c b/drivers/gpio/gpio-xra1403.c
index 0230e4b7a2fb..8d4c8e99b251 100644
--- a/drivers/gpio/gpio-xra1403.c
+++ b/drivers/gpio/gpio-xra1403.c
@@ -126,11 +126,16 @@ static void xra1403_dbg_show(struct seq_file *s, struct gpio_chip *chip)
{
int reg;
struct xra1403 *xra = gpiochip_get_data(chip);
- int value[xra1403_regmap_cfg.max_register];
+ int *value;
int i;
unsigned int gcr;
unsigned int gsr;
+ value = kmalloc_array(xra1403_regmap_cfg.max_register, sizeof(*value),
+ GFP_KERNEL);
+ if (!value)
+ return;
+
seq_puts(s, "xra reg:");
for (reg = 0; reg <= xra1403_regmap_cfg.max_register; reg++)
seq_printf(s, " %2.2x", reg);
@@ -154,6 +159,7 @@ static void xra1403_dbg_show(struct seq_file *s, struct gpio_chip *chip)
(gcr & BIT(i)) ? "in" : "out",
(gsr & BIT(i)) ? "hi" : "lo");
}
+ kfree(value);
}
#else
#define xra1403_dbg_show NULL
--
2.14.3
The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621) to eventually
turn on -Wvla.
This patch replaces several VLAs with an appropriate call to
kmalloc_array.
Signed-off-by: Laura Abbott <[email protected]>
---
v2: Switched to kcalloc. Adjusted return types to propegate error code.
---
drivers/gpio/gpiolib.c | 76 ++++++++++++++++++++++++++++++++++---------
drivers/gpio/gpiolib.h | 2 +-
include/linux/gpio/consumer.h | 8 +++--
3 files changed, 66 insertions(+), 20 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d66de67ef307..c81e5d0aea42 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -390,6 +390,7 @@ static long linehandle_ioctl(struct file *filep, unsigned int cmd,
return 0;
} else if (cmd == GPIOHANDLE_SET_LINE_VALUES_IOCTL) {
+ int ret;
/* TODO: check if descriptors are really output */
if (copy_from_user(&ghd, ip, sizeof(ghd)))
return -EFAULT;
@@ -399,11 +400,14 @@ static long linehandle_ioctl(struct file *filep, unsigned int cmd,
vals[i] = !!ghd.values[i];
/* Reuse the array setting function */
- gpiod_set_array_value_complex(false,
+ ret = gpiod_set_array_value_complex(false,
true,
lh->numdescs,
lh->descs,
vals);
+ if (ret)
+ return ret;
+
return 0;
}
return -EINVAL;
@@ -2662,16 +2666,32 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
while (i < array_size) {
struct gpio_chip *chip = desc_array[i]->gdev->chip;
- unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
- unsigned long bits[BITS_TO_LONGS(chip->ngpio)];
+ unsigned long *mask;
+ unsigned long *bits;
int first, j, ret;
+ mask = kcalloc(BITS_TO_LONGS(chip->ngpio),
+ sizeof(*mask),
+ can_sleep ? GFP_KERNEL : GFP_ATOMIC);
+
+ if (!mask)
+ return -ENOMEM;
+
+ bits = kcalloc(BITS_TO_LONGS(chip->ngpio),
+ sizeof(*bits),
+ can_sleep ? GFP_KERNEL : GFP_ATOMIC);
+
+ if (!bits) {
+ kfree(mask);
+ return -ENOMEM;
+ }
+
+
if (!can_sleep)
WARN_ON(chip->can_sleep);
/* collect all inputs belonging to the same chip */
first = i;
- memset(mask, 0, sizeof(mask));
do {
const struct gpio_desc *desc = desc_array[i];
int hwgpio = gpio_chip_hwgpio(desc);
@@ -2682,8 +2702,11 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
(desc_array[i]->gdev->chip == chip));
ret = gpio_chip_get_multiple(chip, mask, bits);
- if (ret)
+ if (ret) {
+ kfree(bits);
+ kfree(mask);
return ret;
+ }
for (j = first; j < i; j++) {
const struct gpio_desc *desc = desc_array[j];
@@ -2695,6 +2718,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
value_array[j] = value;
trace_gpio_value(desc_to_gpio(desc), 1, value);
}
+ kfree(bits);
+ kfree(mask);
}
return 0;
}
@@ -2878,7 +2903,7 @@ static void gpio_chip_set_multiple(struct gpio_chip *chip,
}
}
-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)
@@ -2887,14 +2912,29 @@ void gpiod_set_array_value_complex(bool raw, bool can_sleep,
while (i < array_size) {
struct gpio_chip *chip = desc_array[i]->gdev->chip;
- unsigned long mask[BITS_TO_LONGS(chip->ngpio)];
- unsigned long bits[BITS_TO_LONGS(chip->ngpio)];
+ unsigned long *mask;
+ unsigned long *bits;
int count = 0;
+ mask = kcalloc(BITS_TO_LONGS(chip->ngpio),
+ sizeof(*mask),
+ can_sleep ? GFP_KERNEL : GFP_ATOMIC);
+
+ if (!mask)
+ return -ENOMEM;
+
+ bits = kcalloc(BITS_TO_LONGS(chip->ngpio),
+ sizeof(*bits),
+ can_sleep ? GFP_KERNEL : GFP_ATOMIC);
+
+ if (!bits) {
+ kfree(mask);
+ return -ENOMEM;
+ }
+
if (!can_sleep)
WARN_ON(chip->can_sleep);
- memset(mask, 0, sizeof(mask));
do {
struct gpio_desc *desc = desc_array[i];
int hwgpio = gpio_chip_hwgpio(desc);
@@ -2925,7 +2965,11 @@ void gpiod_set_array_value_complex(bool raw, bool can_sleep,
/* push collected bits to outputs */
if (count != 0)
gpio_chip_set_multiple(chip, mask, bits);
+
+ kfree(mask);
+ kfree(bits);
}
+ return 0;
}
/**
@@ -3000,13 +3044,13 @@ EXPORT_SYMBOL_GPL(gpiod_set_value);
* This function should be called from contexts where we cannot sleep, and will
* complain if the GPIO chip functions potentially sleep.
*/
-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)
{
if (!desc_array)
- return;
- gpiod_set_array_value_complex(true, false, array_size, desc_array,
- value_array);
+ return -EINVAL;
+ return gpiod_set_array_value_complex(true, false, array_size,
+ desc_array, value_array);
}
EXPORT_SYMBOL_GPL(gpiod_set_raw_array_value);
@@ -3326,14 +3370,14 @@ EXPORT_SYMBOL_GPL(gpiod_set_value_cansleep);
*
* This function is to be called from contexts that can sleep.
*/
-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)
{
might_sleep_if(extra_checks);
if (!desc_array)
- return;
- gpiod_set_array_value_complex(true, true, array_size, desc_array,
+ return -EINVAL;
+ return gpiod_set_array_value_complex(true, true, array_size, desc_array,
value_array);
}
EXPORT_SYMBOL_GPL(gpiod_set_raw_array_value_cansleep);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index b17ec6795c81..b64813e3876e 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -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);
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index dbd065963296..da52610cc3bd 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -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)
@@ -429,6 +430,7 @@ static inline void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
{
/* GPIO can never have been requested */
WARN_ON(1);
+ return 0;
}
static inline int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
--
2.14.3
The new challenge is to remove VLAs from the kernel
(see https://lkml.org/lkml/2018/3/7/621)
This patch replaces a VLA with an appropriate call to kmalloc_array.
Signed-off-by: Laura Abbott <[email protected]>
---
v2: Switch to GFP_KERNEL. There was some discussion about if we should
be doing the allocation at all but given a) the allocation is pretty
small and b) we can possibly take a mutex in a called function I think
this is fine.
---
drivers/gpio/gpio-stmpe.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
index f8d7d1cd8488..c2bb20ace6f5 100644
--- a/drivers/gpio/gpio-stmpe.c
+++ b/drivers/gpio/gpio-stmpe.c
@@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
struct stmpe *stmpe = stmpe_gpio->stmpe;
u8 statmsbreg;
int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
- u8 status[num_banks];
+ u8 *status;
int ret;
int i;
+ status = kmalloc_array(num_banks, sizeof(*status), GFP_KERNEL);
+ if (!status)
+ return IRQ_NONE;
+
/*
* the stmpe_block_read() call below, imposes to set statmsbreg
* with the register located at the lowest address. As STMPE1600
@@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
}
}
+ kfree(status);
return IRQ_HANDLED;
}
--
2.14.3
Hi Laura,
I love your patch! Perhaps something to improve:
[auto build test WARNING on v4.16-rc4]
[also build test WARNING on next-20180316]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Laura-Abbott/VLA-removal-from-the-gpio-subsystem/20180317-210828
config: i386-defconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from drivers/clk/clk-gpio.c:18:0:
include/linux/gpio/consumer.h: In function 'gpiod_set_raw_array_value_cansleep':
>> include/linux/gpio/consumer.h:433:9: warning: 'return' with a value, in function returning void
return 0;
^
include/linux/gpio/consumer.h:427:20: note: declared here
static inline void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/return +433 include/linux/gpio/consumer.h
380
381 static inline int gpiod_get_value_cansleep(const struct gpio_desc *desc)
382 {
383 /* GPIO can never have been requested */
384 WARN_ON(1);
385 return 0;
386 }
387 static inline int gpiod_get_array_value_cansleep(unsigned int array_size,
388 struct gpio_desc **desc_array,
389 int *value_array)
390 {
391 /* GPIO can never have been requested */
392 WARN_ON(1);
393 return 0;
394 }
395 static inline void gpiod_set_value_cansleep(struct gpio_desc *desc, int value)
396 {
397 /* GPIO can never have been requested */
398 WARN_ON(1);
399 }
400 static inline void gpiod_set_array_value_cansleep(unsigned int array_size,
401 struct gpio_desc **desc_array,
402 int *value_array)
403 {
404 /* GPIO can never have been requested */
405 WARN_ON(1);
406 }
407 static inline int gpiod_get_raw_value_cansleep(const struct gpio_desc *desc)
408 {
409 /* GPIO can never have been requested */
410 WARN_ON(1);
411 return 0;
412 }
413 static inline int gpiod_get_raw_array_value_cansleep(unsigned int array_size,
414 struct gpio_desc **desc_array,
415 int *value_array)
416 {
417 /* GPIO can never have been requested */
418 WARN_ON(1);
419 return 0;
420 }
421 static inline void gpiod_set_raw_value_cansleep(struct gpio_desc *desc,
422 int value)
423 {
424 /* GPIO can never have been requested */
425 WARN_ON(1);
426 }
427 static inline void gpiod_set_raw_array_value_cansleep(unsigned int array_size,
428 struct gpio_desc **desc_array,
429 int *value_array)
430 {
431 /* GPIO can never have been requested */
432 WARN_ON(1);
> 433 return 0;
434 }
435
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 16/03/2018 02:00, Laura Abbott wrote:
> The new challenge is to remove VLAs from the kernel
> (see https://lkml.org/lkml/2018/3/7/621)
>
> This patch replaces a VLA with an appropriate call to kmalloc_array.
>
> Signed-off-by: Laura Abbott <[email protected]>
> ---
> v2: Switch to GFP_KERNEL. There was some discussion about if we should
> be doing the allocation at all but given a) the allocation is pretty
> small and b) we can possibly take a mutex in a called function I think
> this is fine.
I still think it's a bad idea. It's simple to preallocate the buffer.
But it's up to the maintainer.
> ---
> drivers/gpio/gpio-stmpe.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
> index f8d7d1cd8488..c2bb20ace6f5 100644
> --- a/drivers/gpio/gpio-stmpe.c
> +++ b/drivers/gpio/gpio-stmpe.c
> @@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
> struct stmpe *stmpe = stmpe_gpio->stmpe;
> u8 statmsbreg;
> int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
> - u8 status[num_banks];
> + u8 *status;
> int ret;
> int i;
>
> + status = kmalloc_array(num_banks, sizeof(*status), GFP_KERNEL);
> + if (!status)
> + return IRQ_NONE;
> +
> /*
> * the stmpe_block_read() call below, imposes to set statmsbreg
> * with the register located at the lowest address. As STMPE1600
> @@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
> }
> }
>
> + kfree(status);
> return IRQ_HANDLED;
> }
>
>
--
Regards
Phil Reid
On Thu, Mar 15, 2018 at 11:00:28AM -0700, Laura Abbott wrote:
>
> The new challenge is to remove VLAs from the kernel
> (see https://lkml.org/lkml/2018/3/7/621)
>
> This patch replaces several a VLA with an appropriate call to
> kmalloc_array.
>
> Signed-off-by: Laura Abbott <[email protected]>
Reviewed-and-tested-by: Lukas Wunner <[email protected]>
This one isn't a hotpath, so the kmalloc overhead is negligible.
Did a quick test on a single-chip MAX31913 with no apparent issues.
> ---
> v2: No changes
> ---
> drivers/gpio/gpio-max3191x.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-max3191x.c b/drivers/gpio/gpio-max3191x.c
> index f74b1072e84b..b5b9cb1fda50 100644
> --- a/drivers/gpio/gpio-max3191x.c
> +++ b/drivers/gpio/gpio-max3191x.c
> @@ -315,12 +315,17 @@ static void gpiod_set_array_single_value_cansleep(unsigned int ndescs,
> struct gpio_desc **desc,
> int value)
> {
> - int i, values[ndescs];
> + int i, *values;
> +
> + values = kmalloc_array(ndescs, sizeof(*values), GFP_KERNEL);
> + if (!values)
> + return;
>
> for (i = 0; i < ndescs; i++)
> values[i] = value;
>
> gpiod_set_array_value_cansleep(ndescs, desc, values);
> + kfree(values);
> }
>
> static struct gpio_descs *devm_gpiod_get_array_optional_count(
> --
> 2.14.3
>
On 03/18/2018 06:29 PM, Phil Reid wrote:
> On 16/03/2018 02:00, Laura Abbott wrote:
>> The new challenge is to remove VLAs from the kernel
>> (see https://lkml.org/lkml/2018/3/7/621)
>>
>> This patch replaces a VLA with an appropriate call to kmalloc_array.
>>
>> Signed-off-by: Laura Abbott <[email protected]>
>> ---
>> v2: Switch to GFP_KERNEL. There was some discussion about if we should
>> be doing the allocation at all but given a) the allocation is pretty
>> small and b) we can possibly take a mutex in a called function I think
>> this is fine.
>
> I still think it's a bad idea. It's simple to preallocate the buffer.
> But it's up to the maintainer.
>
I'd feel a lot more confident about doing the global buffer with
guidance from the maintainer. But looking at the platform data, the
maximum number of GPIOs is 24, or 3 banks. Maybe we should just always
stack allocate the maximum since it's fairly small.
Thanks,
Laura
>
>> ---
>> drivers/gpio/gpio-stmpe.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
>> index f8d7d1cd8488..c2bb20ace6f5 100644
>> --- a/drivers/gpio/gpio-stmpe.c
>> +++ b/drivers/gpio/gpio-stmpe.c
>> @@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>> struct stmpe *stmpe = stmpe_gpio->stmpe;
>> u8 statmsbreg;
>> int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
>> - u8 status[num_banks];
>> + u8 *status;
>> int ret;
>> int i;
>> + status = kmalloc_array(num_banks, sizeof(*status), GFP_KERNEL);
>> + if (!status)
>> + return IRQ_NONE;
>> +
>> /*
>> * the stmpe_block_read() call below, imposes to set statmsbreg
>> * with the register located at the lowest address. As STMPE1600
>> @@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>> }
>> }
>> + kfree(status);
>> return IRQ_HANDLED;
>> }
>>
>
>
On 23/03/2018 05:43, Laura Abbott wrote:
> On 03/18/2018 06:29 PM, Phil Reid wrote:
>> On 16/03/2018 02:00, Laura Abbott wrote:
>>> The new challenge is to remove VLAs from the kernel
>>> (see https://lkml.org/lkml/2018/3/7/621)
>>>
>>> This patch replaces a VLA with an appropriate call to kmalloc_array.
>>>
>>> Signed-off-by: Laura Abbott <[email protected]>
>>> ---
>>> v2: Switch to GFP_KERNEL. There was some discussion about if we should
>>> be doing the allocation at all but given a) the allocation is pretty
>>> small and b) we can possibly take a mutex in a called function I think
>>> this is fine.
>>
>> I still think it's a bad idea. It's simple to preallocate the buffer.
>> But it's up to the maintainer.
>>
>
> I'd feel a lot more confident about doing the global buffer with
> guidance from the maintainer. But looking at the platform data, the
> maximum number of GPIOs is 24, or 3 banks. Maybe we should just always
> stack allocate the maximum since it's fairly small.
That's the other way to go.
>>> ---
>>> drivers/gpio/gpio-stmpe.c | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpio/gpio-stmpe.c b/drivers/gpio/gpio-stmpe.c
>>> index f8d7d1cd8488..c2bb20ace6f5 100644
>>> --- a/drivers/gpio/gpio-stmpe.c
>>> +++ b/drivers/gpio/gpio-stmpe.c
>>> @@ -369,10 +369,14 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>>> struct stmpe *stmpe = stmpe_gpio->stmpe;
>>> u8 statmsbreg;
>>> int num_banks = DIV_ROUND_UP(stmpe->num_gpios, 8);
>>> - u8 status[num_banks];
>>> + u8 *status;
>>> int ret;
>>> int i;
>>> + status = kmalloc_array(num_banks, sizeof(*status), GFP_KERNEL);
>>> + if (!status)
>>> + return IRQ_NONE;
>>> +
>>> /*
>>> * the stmpe_block_read() call below, imposes to set statmsbreg
>>> * with the register located at the lowest address. As STMPE1600
>>> @@ -424,6 +428,7 @@ static irqreturn_t stmpe_gpio_irq(int irq, void *dev)
>>> }
>>> }
>>> + kfree(status);
>>> return IRQ_HANDLED;
>>> }
>>>
>>
>>
>
>
>
--
Regards
Phil Reid
Hi Laura,
sorry for slow response :/
On Thu, Mar 22, 2018 at 10:43 PM, Laura Abbott <[email protected]> wrote:
> On 03/18/2018 06:29 PM, Phil Reid wrote:
>>
>> On 16/03/2018 02:00, Laura Abbott wrote:
>>>
>>> The new challenge is to remove VLAs from the kernel
>>> (see https://lkml.org/lkml/2018/3/7/621)
>>>
>>> This patch replaces a VLA with an appropriate call to kmalloc_array.
>>>
>>> Signed-off-by: Laura Abbott <[email protected]>
>>> ---
>>> v2: Switch to GFP_KERNEL. There was some discussion about if we should
>>> be doing the allocation at all but given a) the allocation is pretty
>>> small and b) we can possibly take a mutex in a called function I think
>>> this is fine.
>>
>>
>> I still think it's a bad idea. It's simple to preallocate the buffer.
>> But it's up to the maintainer.
>>
>
> I'd feel a lot more confident about doing the global buffer with
> guidance from the maintainer. But looking at the platform data, the
> maximum number of GPIOs is 24, or 3 banks. Maybe we should just always
> stack allocate the maximum since it's fairly small.
Either way works fine, global (in the state container struct stmpe_gpio)
or stack allocation for 24 bits.
I guess I am maintainer for this, I can test it at least.
Yours,
Linus Walleij