linux-kernel - Re: [PATCH v2 1/1] gpiolib: Introduce for_each_gpio_desc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 20 May 2021 10:07:03 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Kent Gibson <warthog618@...il.com>, linux-gpio@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH v2 1/1] gpiolib: Introduce for_each_gpio_desc_if() macro

On Tue, May 18, 2021 at 11:33:39AM +0300, Andy Shevchenko wrote:
> In a few places we are using a loop against all GPIO descriptors
> with a given flag for a given device. Replace it with a consolidated
> for_each type of macro.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
> v2: fixed compilation issue (LKP), injected if (test_bit) into the loop
>  drivers/gpio/gpiolib-of.c    | 10 ++++------
>  drivers/gpio/gpiolib-sysfs.c |  7 ++-----
>  drivers/gpio/gpiolib.c       |  7 +++----
>  drivers/gpio/gpiolib.h       |  7 +++++++
>  4 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index bbcc7c073f63..2f8f3f0c8373 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -711,14 +711,12 @@ static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
>  static void of_gpiochip_remove_hog(struct gpio_chip *chip,
>  				   struct device_node *hog)
>  {
> -	struct gpio_desc *descs = chip->gpiodev->descs;
> +	struct gpio_desc *desc;
>  	unsigned int i;
>  
> -	for (i = 0; i < chip->ngpio; i++) {
> -		if (test_bit(FLAG_IS_HOGGED, &descs[i].flags) &&
> -		    descs[i].hog == hog)
> -			gpiochip_free_own_desc(&descs[i]);
> -	}
> +	for_each_gpio_desc_if(i, chip, desc, FLAG_IS_HOGGED)
> +		if (desc->hog == hog)
> +			gpiochip_free_own_desc(desc);

The _if suffix here is too vague.

Please use a more descriptive name so that you don't need to look at the
implementation to understand what the macro does.

Perhaps call it 

	for_each_gpio_desc_with_flag()

or just add the more generic macro 

	for_each_gpio_desc()

and open-code the test so that it's clear what's going on here.

>  struct gpio_desc *gpiochip_get_desc(struct gpio_chip *gc, unsigned int hwnum);
> +
> +#define for_each_gpio_desc_if(i, gc, desc, flag)		\
> +	for (i = 0, desc = gpiochip_get_desc(gc, i);		\
> +	     i < gc->ngpio;					\
> +	     i++, desc = gpiochip_get_desc(gc, i))		\
> +		if (!test_bit(flag, &desc->flags)) {} else
> +
>  int gpiod_get_array_value_complex(bool raw, bool can_sleep,
>  				  unsigned int array_size,
>  				  struct gpio_desc **desc_array,

Johan