Re: [PATCH v3 6/6] gpiolib: notify user-space about in-kernel line state changes

[Kent Gibson <warthog618@xxxxxxxxx>]

On Tue, Oct 15, 2024 at 12:56:18PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> We currently only notify user-space about line config changes that are
> made from user-space. Any kernel config changes are not signalled.
>
> Let's improve the situation by emitting the events closer to the source.
> To that end let's call the relevant notifier chain from the functions
> setting direction, gpiod_set_config(), gpiod_set_consumer_name() and
> gpiod_toggle_active_low(). This covers all the options that we can
> inform the user-space about. We ignore events which don't have
> corresponding flags exported to user-space on purpose - otherwise the
> user would see a config-changed event but the associated line-info would
> remain unchanged.
>
> gpiod_direction_output/input() can be called from any context.
> Fortunately, we now emit line state events using an atomic notifier
> chain, so it's no longer an issue.
>
> Let's also add non-notifying wrappers around the direction setters in
> order to not emit superfluous reconfigure events when requesting the
> lines as the initial config should be part of the request notification.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> ---
>  drivers/gpio/gpiolib-cdev.c | 12 +++-----
>  drivers/gpio/gpiolib.c      | 71 ++++++++++++++++++++++++++++++++++++++++-----
>  drivers/gpio/gpiolib.h      |  2 ++
>  3 files changed, 70 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index f8c69ef33888..1296e6cbcef7 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -196,8 +196,6 @@ static long linehandle_set_config(struct linehandle_state *lh,
>  			if (ret)
>  				return ret;
>  		}
> -
> -		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
>  	}
>  	return 0;
>  }
> @@ -363,11 +361,11 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>  		if (lflags & GPIOHANDLE_REQUEST_OUTPUT) {
>  			int val = !!handlereq.default_values[i];
>
> -			ret = gpiod_direction_output(desc, val);
> +			ret = gpiod_direction_output_nonotify(desc, val);
>  			if (ret)
>  				goto out_free_lh;
>  		} else if (lflags & GPIOHANDLE_REQUEST_INPUT) {
> -			ret = gpiod_direction_input(desc);
> +			ret = gpiod_direction_input_nonotify(desc);
>  			if (ret)
>  				goto out_free_lh;
>  		}
> @@ -1568,8 +1566,6 @@ static long linereq_set_config(struct linereq *lr, void __user *ip)
>  		}
>
>  		WRITE_ONCE(line->edflags, edflags);
> -
> -		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
>  	}
>  	return 0;
>  }
> @@ -1826,11 +1822,11 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip)
>  		if (flags & GPIO_V2_LINE_FLAG_OUTPUT) {
>  			int val = gpio_v2_line_config_output_value(lc, i);
>
> -			ret = gpiod_direction_output(desc, val);
> +			ret = gpiod_direction_output_nonotify(desc, val);
>  			if (ret)
>  				goto out_free_linereq;
>  		} else if (flags & GPIO_V2_LINE_FLAG_INPUT) {
> -			ret = gpiod_direction_input(desc);
> +			ret = gpiod_direction_input_nonotify(desc);
>  			if (ret)
>  				goto out_free_linereq;
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index fafa759ce743..4303b6a689af 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2673,6 +2673,18 @@ int gpio_set_debounce_timeout(struct gpio_desc *desc, unsigned int debounce)
>   * 0 on success, or negative errno on failure.
>   */
>  int gpiod_direction_input(struct gpio_desc *desc)
> +{
> +	int ret;
> +
> +	ret = gpiod_direction_input_nonotify(desc);
> +	if (ret == 0)
> +		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(gpiod_direction_input);
> +
> +int gpiod_direction_input_nonotify(struct gpio_desc *desc)
>  {
>  	int ret = 0;
>
> @@ -2720,7 +2732,6 @@ int gpiod_direction_input(struct gpio_desc *desc)
>
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(gpiod_direction_input);
>
>  static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
>  {
> @@ -2782,8 +2793,15 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
>   */
>  int gpiod_direction_output_raw(struct gpio_desc *desc, int value)
>  {
> +	int ret;
> +
>  	VALIDATE_DESC(desc);
> -	return gpiod_direction_output_raw_commit(desc, value);
> +
> +	ret = gpiod_direction_output_raw_commit(desc, value);
> +	if (ret == 0)
> +		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(gpiod_direction_output_raw);
>
> @@ -2801,6 +2819,18 @@ EXPORT_SYMBOL_GPL(gpiod_direction_output_raw);
>   * 0 on success, or negative errno on failure.
>   */
>  int gpiod_direction_output(struct gpio_desc *desc, int value)
> +{
> +	int ret;
> +
> +	ret = gpiod_direction_output_nonotify(desc, value);
> +	if (ret == 0)
> +		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(gpiod_direction_output);
> +
> +int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value)
>  {
>  	unsigned long flags;
>  	int ret;
> @@ -2863,7 +2893,6 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
>  		set_bit(FLAG_IS_OUT, &desc->flags);
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(gpiod_direction_output);
>
>  /**
>   * gpiod_enable_hw_timestamp_ns - Enable hardware timestamp in nanoseconds.
> @@ -2942,13 +2971,34 @@ EXPORT_SYMBOL_GPL(gpiod_disable_hw_timestamp_ns);
>   */
>  int gpiod_set_config(struct gpio_desc *desc, unsigned long config)
>  {
> +	int ret;
> +
>  	VALIDATE_DESC(desc);
>
>  	CLASS(gpio_chip_guard, guard)(desc);
>  	if (!guard.gc)
>  		return -ENODEV;
>
> -	return gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> +	ret = gpio_do_set_config(guard.gc, gpio_chip_hwgpio(desc), config);
> +	if (ret == 0) {
> +		/* These are the only options we notify the userspace about. */
> +		switch (pinconf_to_config_param(config)) {
> +		case PIN_CONFIG_BIAS_DISABLE:
> +		case PIN_CONFIG_BIAS_PULL_DOWN:
> +		case PIN_CONFIG_BIAS_PULL_UP:
> +		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> +		case PIN_CONFIG_DRIVE_OPEN_SOURCE:
> +		case PIN_CONFIG_DRIVE_PUSH_PULL:
> +		case PIN_CONFIG_INPUT_DEBOUNCE:
> +			gpiod_line_state_notify(desc,
> +						GPIO_V2_LINE_CHANGED_CONFIG);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return ret;
>  }

Ah, the debounce - I forgot about that, and other features that cdev
might emulate.

What happens if userspace requests a line with debounce that is
supported by hardware?  Seems to me we'll see both a LINE_REQUESTED and a
LINE_CONFIG_CHANGED when the line is requested.

Conversely, what if a config change impacts features that don't result in a
notification from gpiod_set_config(), like active low, or emulated
drive or debounce?

Other than those little wrinkles, it all looks good to me.
Though I haven't done any testing on it yet - will do as soon as I can.

Cheers,
Kent.

>  EXPORT_SYMBOL_GPL(gpiod_set_config);
>
> @@ -3015,6 +3065,7 @@ void gpiod_toggle_active_low(struct gpio_desc *desc)
>  {
>  	VALIDATE_DESC_VOID(desc);
>  	change_bit(FLAG_ACTIVE_LOW, &desc->flags);
> +	gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
>  }
>  EXPORT_SYMBOL_GPL(gpiod_toggle_active_low);
>
> @@ -3659,9 +3710,15 @@ EXPORT_SYMBOL_GPL(gpiod_cansleep);
>   */
>  int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name)
>  {
> +	int ret;
> +
>  	VALIDATE_DESC(desc);
>
> -	return desc_set_label(desc, name);
> +	ret = desc_set_label(desc, name);
> +	if (ret == 0)
> +		gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(gpiod_set_consumer_name);
>
> @@ -4539,10 +4596,10 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
>
>  	/* Process flags */
>  	if (dflags & GPIOD_FLAGS_BIT_DIR_OUT)
> -		ret = gpiod_direction_output(desc,
> +		ret = gpiod_direction_output_nonotify(desc,
>  				!!(dflags & GPIOD_FLAGS_BIT_DIR_VAL));
>  	else
> -		ret = gpiod_direction_input(desc);
> +		ret = gpiod_direction_input_nonotify(desc);
>
>  	return ret;
>  }
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index 2799157a1f6b..fc321d281346 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -155,6 +155,8 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
>  int gpiod_set_transitory(struct gpio_desc *desc, bool transitory);
>
>  void gpiod_line_state_notify(struct gpio_desc *desc, unsigned long action);
> +int gpiod_direction_output_nonotify(struct gpio_desc *desc, int value);
> +int gpiod_direction_input_nonotify(struct gpio_desc *desc);
>
>  struct gpio_desc_label {
>  	struct rcu_head rh;
>
> --
> 2.43.0
>