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.
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.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 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;
--
2.43.0On 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.
© 2016 - 2024 Red Hat, Inc.