[PATCHv3] 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.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: Split out from the series since patches have been picked up
independently. Fold in patch from Lukas Wunner to introduce slow/fast
paths. I took his suggestions to go with 384 as the maximum number of
gpios. Also fixed one 0-day bot issue where I forgot to change the
return type.
---
 drivers/gpio/gpiolib.c        | 74 +++++++++++++++++++++++++++++++++----------
 drivers/gpio/gpiolib.h        |  2 +-
 include/linux/gpio/consumer.h | 10 +++---
 3 files changed, 65 insertions(+), 21 deletions(-)

On Wed, Mar 28, 2018 at 11:18:09AM -0700, Laura Abbott wrote:
> 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.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v3: Split out from the series since patches have been picked up
> independently. Fold in patch from Lukas Wunner to introduce slow/fast
> paths. I took his suggestions to go with 384 as the maximum number of
> gpios. Also fixed one 0-day bot issue where I forgot to change the
> return type.

I've just given this a whirl with gpio-hammer and it works nicely, so FWIW:

Reviewed-and-tested-by: Lukas Wunner <lukas@wunner.de>

Thanks a lot for doing this Laura, and most of all thanks for the
proverbial "dogged persistence and patience"!
(https://lwn.net/Articles/697029/)

Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Mar 28, 2018 at 9:18 PM, Laura Abbott <labbott@redhat.com> wrote:
> 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.

> +               ret = gpiod_set_array_value_complex(false,
>                                               true,
>                                               lh->numdescs,
>                                               lh->descs,
>                                               vals);
> +               if (ret)
> +                       return ret;
> +
>                 return 0;

Can't we

return gpiod_set_array_value_complex(); ?


> +                       slowpath = kcalloc(2 * BITS_TO_LONGS(chip->ngpio),
> +                                          sizeof(*slowpath),
> +                                          can_sleep ? GFP_KERNEL : GFP_ATOMIC);


> +                       if (slowpath)
> +                               kfree(slowpath);

> +               if (slowpath)
> +                       kfree(slowpath);

Since slowpath is a pointer, conditionals above are redundant.

> +                       slowpath = kcalloc(2 * BITS_TO_LONGS(chip->ngpio),
> +                                          sizeof(*slowpath),
> +                                          can_sleep ? GFP_KERNEL : GFP_ATOMIC);

> +               if (slowpath)
> +                       kfree(slowpath);

Ditto.

On 03/30/2018 07:33 AM, Andy Shevchenko wrote:
> On Wed, Mar 28, 2018 at 9:18 PM, Laura Abbott <labbott@redhat.com> wrote:
>> 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.
> 
>> +               ret = gpiod_set_array_value_complex(false,
>>                                                true,
>>                                                lh->numdescs,
>>                                                lh->descs,
>>                                                vals);
>> +               if (ret)
>> +                       return ret;
>> +
>>                  return 0;
> 
> Can't we
> 
> return gpiod_set_array_value_complex(); ?
> 
> 

Yeah I'll clean that up for v4.

>> +                       slowpath = kcalloc(2 * BITS_TO_LONGS(chip->ngpio),
>> +                                          sizeof(*slowpath),
>> +                                          can_sleep ? GFP_KERNEL : GFP_ATOMIC);
> 
> 
>> +                       if (slowpath)
>> +                               kfree(slowpath);
> 
>> +               if (slowpath)
>> +                       kfree(slowpath);
> 
> Since slowpath is a pointer, conditionals above are redundant.
> 
>> +                       slowpath = kcalloc(2 * BITS_TO_LONGS(chip->ngpio),
>> +                                          sizeof(*slowpath),
>> +                                          can_sleep ? GFP_KERNEL : GFP_ATOMIC);
> 
>> +               if (slowpath)
>> +                       kfree(slowpath);
> 
> Ditto.
> 

This was caught by a coccinelle script via 0-day but I think the request
was to not do it. I'll add a comment explaining why we are going against
style.

Thanks,
Laura
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 2018-03-28 20:18, Laura Abbott wrote:
> 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.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v3: Split out from the series since patches have been picked up
> independently. Fold in patch from Lukas Wunner to introduce slow/fast
> paths. I took his suggestions to go with 384 as the maximum number of
> gpios. Also fixed one 0-day bot issue where I forgot to change the
> return type.
>  
>  	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 fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)];
> +		unsigned long *slowpath = NULL, *mask, *bits;
>  		int first, j, ret;
>  
> +		if (likely(chip->ngpio <= FASTPATH_NGPIO)) {
> +			memset(fastpath, 0, sizeof(fastpath));
> +			mask = fastpath;
> +			bits = fastpath + BITS_TO_LONGS(FASTPATH_NGPIO);
> +		} else {
> +			slowpath = kcalloc(2 * BITS_TO_LONGS(chip->ngpio),
> +					   sizeof(*slowpath),
> +					   can_sleep ? GFP_KERNEL : GFP_ATOMIC);
> +			if (!slowpath)
> +				return -ENOMEM;
> +			mask = slowpath;
> +			bits = slowpath + BITS_TO_LONGS(chip->ngpio);
> +		}
> +

To avoid the static analysis complaints about the "if (slowpath)
kfree(slowpath)" pattern, I'd suggest getting rid of the slowpath
variable and simply assign the kcalloc directly to mask. Then the
condition becomes "if (mask != fastpath) kfree(mask)". A comment won't
silence subsequenct coccicheck runs, and unfortunately probably won't
prevent certain individuals from sending auto-generated patches.

Maybe also pull out the "bits = mask + BITS_TO_LONGS(chip->ngpio)" to
the common path.

Another thing: Maybe a pr_warn or at least a pr_info would be in order
when a gpio chip with > FASTPATH_NGPIO lines is registered? And if that
triggers for a lot of different SoCs, one could consider changing it to
a CONFIG_ thing, or just bumping it to 512 if that would seem to cover
all the reports.

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html