linux-kernel - Re: [PATCH RFC v2 3/6] gpio: aggregator: export symbols of the gpio-fwd library

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]

Message-ID: <Z9hXcr_GPhPt_gel@smile.fi.intel.com>
Date: Mon, 17 Mar 2025 19:10:10 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Thomas Richard <thomas.richard@...tlin.com>
Cc: Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
	thomas.petazzoni@...tlin.com, DanieleCleri@...on.eu,
	GaryWang@...on.com.tw
Subject: Re: [PATCH RFC v2 3/6] gpio: aggregator: export symbols of the
 gpio-fwd library

On Mon, Mar 17, 2025 at 04:38:01PM +0100, Thomas Richard wrote:
> Export all symbols and create header file for the gpio-fwd library.

...

>  #include <linux/gpio/consumer.h>
>  #include <linux/gpio/driver.h>
> +#include <linux/gpio/gpio-fwd.h>

Please, name it forwarder.h.

>  #include <linux/gpio/machine.h>

...

> +int gpio_fwd_get_direction(struct gpio_chip *chip, unsigned int offset)
>  {
>  	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>  
>  	return gpiod_get_direction(fwd->descs[offset]);
>  }
> +EXPORT_SYMBOL_GPL(gpio_fwd_get_direction);

No namespace? Ditto for all exports.

> -static int gpio_fwd_get_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
> -				 unsigned long *bits)
> +static int gpio_fwd_get_multiple_unlocked(struct gpiochip_fwd *fwd,
> +					  unsigned long *mask, unsigned long *bits)
>  {
>  	struct gpio_desc **descs = fwd_tmp_descs(fwd);
>  	unsigned long *values = fwd_tmp_values(fwd);
> @@ -332,8 +320,8 @@ static int gpio_fwd_get_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
>  	return 0;
>  }
>  
> -static int gpio_fwd_get_multiple_locked(struct gpio_chip *chip,
> -					unsigned long *mask, unsigned long *bits)
> +int gpio_fwd_get_multiple(struct gpio_chip *chip, unsigned long *mask,
> +			  unsigned long *bits)
>  {
>  	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>  	unsigned long flags;
> @@ -341,16 +329,17 @@ static int gpio_fwd_get_multiple_locked(struct gpio_chip *chip,
>  
>  	if (chip->can_sleep) {
>  		mutex_lock(&fwd->mlock);
> -		error = gpio_fwd_get_multiple(fwd, mask, bits);
> +		error = gpio_fwd_get_multiple_unlocked(fwd, mask, bits);
>  		mutex_unlock(&fwd->mlock);
>  	} else {
>  		spin_lock_irqsave(&fwd->slock, flags);
> -		error = gpio_fwd_get_multiple(fwd, mask, bits);
> +		error = gpio_fwd_get_multiple_unlocked(fwd, mask, bits);
>  		spin_unlock_irqrestore(&fwd->slock, flags);
>  	}
>  
>  	return error;
>  }
> +EXPORT_SYMBOL_GPL(gpio_fwd_get_multiple);

These two are nicely named. Instead of touching them, just simply add an
exported wrapper. We can optimize it latter if needed, but it reduces a lot
the churn in this patch.

...

> -static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
> -				  unsigned long *bits)
> +static void gpio_fwd_set_multiple_unlocked(struct gpiochip_fwd *fwd,
> +					   unsigned long *mask,
> +					   unsigned long *bits)
>  {
>  	struct gpio_desc **descs = fwd_tmp_descs(fwd);
>  	unsigned long *values = fwd_tmp_values(fwd);
> @@ -404,37 +395,40 @@ static void gpio_fwd_set_multiple(struct gpiochip_fwd *fwd, unsigned long *mask,
>  		gpiod_set_array_value(j, descs, NULL, values);
>  }
>  
> -static void gpio_fwd_set_multiple_locked(struct gpio_chip *chip,
> -					 unsigned long *mask, unsigned long *bits)
> +void gpio_fwd_set_multiple(struct gpio_chip *chip, unsigned long *mask,
> +			   unsigned long *bits)
>  {
>  	struct gpiochip_fwd *fwd = gpiochip_get_data(chip);
>  	unsigned long flags;
>  
>  	if (chip->can_sleep) {
>  		mutex_lock(&fwd->mlock);
> -		gpio_fwd_set_multiple(fwd, mask, bits);
> +		gpio_fwd_set_multiple_unlocked(fwd, mask, bits);
>  		mutex_unlock(&fwd->mlock);
>  	} else {
>  		spin_lock_irqsave(&fwd->slock, flags);
> -		gpio_fwd_set_multiple(fwd, mask, bits);
> +		gpio_fwd_set_multiple_unlocked(fwd, mask, bits);
>  		spin_unlock_irqrestore(&fwd->slock, flags);
>  	}
>  }
> +EXPORT_SYMBOL_GPL(gpio_fwd_set_multiple);

Ditto.

...

> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_GPIO_FWD_H
> +#define __LINUX_GPIO_FWD_H

This header uses something that is defined in other headers. Please follow IWYU
principle.

...

> +struct gpiochip_fwd_timing {
> +	u32 ramp_up_us;
> +	u32 ramp_down_us;

types.h

> +};

...

> +struct gpiochip_fwd {
> +	struct device *dev;

struct device;

// forward declaration is enough.

> +	struct gpio_chip chip;

Where is this being defined?

> +	struct gpio_desc **descs;

> +	union {
> +		struct mutex mlock;	/* protects tmp[] if can_sleep */
> +		spinlock_t slock;	/* protects tmp[] if !can_sleep */

And these?

> +	};
> +	struct gpiochip_fwd_timing *delay_timings;
> +	unsigned long tmp[];		/* values and descs for multiple ops */
> +};

-- 
With Best Regards,
Andy Shevchenko