| Message ID | 20241010-gpio-notify-in-kernel-events-v2-6-b560411f7c59@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpio: notify user-space about config changes in the kernel | expand |
On Thu, Oct 10, 2024 at 11:10:27AM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.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. > So lines requested from kernel space will result in a LINE_REQUESTED and then a series of LINE_CHANGED_CONFIG? Whereas for lines requested from userspace those will be collapsed into the one LINE_REQUESTED event? That's not ideal, but I realise making the rest of the kernel behave as per cdev would be non-trivial, so ok - it all comes out in the wash. And it is clearly better than nothing. Cheers, Kent.
On Mon, Oct 14, 2024 at 4:24 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Thu, Oct 10, 2024 at 11:10:27AM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.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. > > > > So lines requested from kernel space will result in a LINE_REQUESTED and > then a series of LINE_CHANGED_CONFIG? Whereas for lines requested from > userspace those will be collapsed into the one LINE_REQUESTED event? No, why? I added the notification about the request to gpiod_find_and_request() which is called by all the kernel getters and it already configures all the flags without emitting events and calls the non-notify variant of the direction setter. When a kernel driver requests a GPIO, I only see a single event UNLESS after the gpiod_get() call returns, it sets direction or changes config - just like user-space. Bart > That's not ideal, but I realise making the rest of the kernel behave as > per cdev would be non-trivial, so ok - it all comes out in the wash. > And it is clearly better than nothing. > > Cheers, > Kent. >
On Mon, Oct 14, 2024 at 10:13:59AM +0200, Bartosz Golaszewski wrote: > On Mon, Oct 14, 2024 at 4:24 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Thu, Oct 10, 2024 at 11:10:27AM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.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. > > > > > > > So lines requested from kernel space will result in a LINE_REQUESTED and > > then a series of LINE_CHANGED_CONFIG? Whereas for lines requested from > > userspace those will be collapsed into the one LINE_REQUESTED event? > > No, why? I added the notification about the request to > gpiod_find_and_request() which is called by all the kernel getters and > it already configures all the flags without emitting events and calls > the non-notify variant of the direction setter. When a kernel driver > requests a GPIO, I only see a single event UNLESS after the > gpiod_get() call returns, it sets direction or changes config - just > like user-space. > Oh, ok, I was assuming there could be others using gpiolib the same way cdev does. So cdev is the only one that takes the gpiod_request(), gpiod_direction_output() etc path? All good then. Cheers, Kent.
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index 7eae0b17a1d6..ff702fe59ecd 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 9b10f47832d5..0986854ab1ed 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; } 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;