| 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
| ||
|
Message-ID: <20241016051944.GA42100@rigel>
Date: Wed, 16 Oct 2024 13:19:44 +0800
From: Kent Gibson <warthog618@...il.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Linus Walleij <linus.walleij@...aro.org>, linux-gpio@...r.kernel.org,
linux-kernel@...r.kernel.org,
Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [PATCH v3 6/6] gpiolib: notify user-space about in-kernel line
state changes
On Tue, Oct 15, 2024 at 12:56:18PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
>
> 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@...aro.org>
> ---
> 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
>
Powered by blists - more mailing lists