* [PATCH v2 0/5] gpiolib: improve concurrent access to the flags field of struct gpio_desc
@ 2026-11-09 10:27 Bartosz Golaszewski
2026-11-09 10:27 ` [PATCH v2 1/5] gpiolib: don't overwrite the entire flags field in gpiod_get_direction() Bartosz Golaszewski
` (4 more replies)
0 siblings, 5 replies; 8+ messages in thread
From: Bartosz Golaszewski @ 2026-11-09 10:27 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko, Kent Gibson,
Adrian Ambrozewicz
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, stable
This is a follow-up to our discussion[1] about desc->flags and possible
races with users who don't/can't synchronize with other means.
It's an attempt to use try_cmpxchg() to update only the required flags
atomically where needed + minor fixes using atomic variants of bitops
where it makes sense.
[1] https://lore.kernel.org/all/20261015153714.GA105746@rigel/
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
Changes in v2:
- Drop the custom gpiod_update_flags() function and use the existing
set_mask_bits() macro instead
- Add a patch clearing debounce_period_us before GPIOD_FLAG_REQUESTED as
suggested by Kent
- Rename the functions updating flags in gpiolib-cdev.c
- Don't rename the flags variable
- Link to v1: https://patch.msgid.link/20261104-gpio-desc-flags-atomic-v1-0-e23cedf619ba@oss.qualcomm.com
---
Bartosz Golaszewski (5):
gpiolib: don't overwrite the entire flags field in gpiod_get_direction()
gpio: shared: use atomic set_bit() for descriptor flags
gpiolib: clear debounce period before the "requested" flag
gpiolib: don't overwrite the entire flags field in gpiod_free_commit()
gpio: cdev: don't overwrite the whole flags field when configuring lines
drivers/gpio/gpiolib-cdev.c | 111 +++++++++++++++++++++++-------------------
drivers/gpio/gpiolib-shared.c | 6 +--
drivers/gpio/gpiolib.c | 36 +++++++-------
3 files changed, 83 insertions(+), 70 deletions(-)
---
base-commit: ddfe51b0a0f5dc9e415cc2c8ad8db17532dd426b
change-id: 20261104-gpio-desc-flags-atomic-cc963a1ccfd0
Best regards,
--
Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v2 1/5] gpiolib: don't overwrite the entire flags field in gpiod_get_direction() 2026-11-09 10:27 [PATCH v2 0/5] gpiolib: improve concurrent access to the flags field of struct gpio_desc Bartosz Golaszewski @ 2026-11-09 10:27 ` Bartosz Golaszewski 2026-11-09 10:27 ` [PATCH v2 2/5] gpio: shared: use atomic set_bit() for descriptor flags Bartosz Golaszewski ` (3 subsequent siblings) 4 siblings, 0 replies; 8+ messages in thread From: Bartosz Golaszewski @ 2026-11-09 10:27 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko, Kent Gibson, Adrian Ambrozewicz Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, stable gpiod_get_direction() snapshots desc->flags with READ_ONCE(), updates FLAG_IS_OUT in the local copy and writes the whole word back with WRITE_ONCE(). Any bit that a concurrent user sets or clears atomically in between - for instance: FLAG_USED_AS_IRQ from gpiochip_lock_as_irq() - is silently lost. Update FLAG_IS_OUT with a single atomic assign_bit() directly on desc->flags. The earlier READ_ONCE() snapshot is only used for the open-drain check and stays. Cc: stable@vger.kernel.org Fixes: 2559f2e09211 ("gpio: reinforce desc->flags handling") Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com> --- drivers/gpio/gpiolib.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index fbdeda2f278713a47c2e3d2a6640f387f6d92227..63b65b646fbcfa9ea0f724d1d3e1ac22ba28f638 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -486,8 +486,7 @@ int gpiod_get_direction(struct gpio_desc *desc) if (ret > 0) ret = 1; - assign_bit(GPIOD_FLAG_IS_OUT, &flags, !ret); - WRITE_ONCE(desc->flags, flags); + assign_bit(GPIOD_FLAG_IS_OUT, &desc->flags, !ret); return ret; } -- 2.47.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/5] gpio: shared: use atomic set_bit() for descriptor flags 2026-11-09 10:27 [PATCH v2 0/5] gpiolib: improve concurrent access to the flags field of struct gpio_desc Bartosz Golaszewski 2026-11-09 10:27 ` [PATCH v2 1/5] gpiolib: don't overwrite the entire flags field in gpiod_get_direction() Bartosz Golaszewski @ 2026-11-09 10:27 ` Bartosz Golaszewski 2026-11-09 10:27 ` [PATCH v2 3/5] gpiolib: clear debounce period before the "requested" flag Bartosz Golaszewski ` (2 subsequent siblings) 4 siblings, 0 replies; 8+ messages in thread From: Bartosz Golaszewski @ 2026-11-09 10:27 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko, Kent Gibson, Adrian Ambrozewicz Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, stable Use the atomic set_bit() instead of __set_bit() on desc->flags so that every modification of the field uses atomic bitops. No functional change as these run before any line of the chip can be requested. Cc: stable@vger.kernel.org Fixes: eb374f764a70 ("gpio: provide gpiod_is_shared()") Fixes: 310a4a9cbb17 ("gpio: shared: shorten the critical section in gpiochip_setup_shared()") Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com> --- drivers/gpio/gpiolib-shared.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpio/gpiolib-shared.c b/drivers/gpio/gpiolib-shared.c index 8c9ac9d9330490b17fb3936b8758beb97a308a15..d94fb94f31c2f487d2e45924e11c33570466a0f1 100644 --- a/drivers/gpio/gpiolib-shared.c +++ b/drivers/gpio/gpiolib-shared.c @@ -532,8 +532,8 @@ int gpiochip_setup_shared(struct gpio_chip *gc) * This is a shared GPIO proxy. Mark its * descriptor as such and return here. */ - __set_bit(GPIOD_FLAG_SHARED_PROXY, - &gdev->descs[0].flags); + set_bit(GPIOD_FLAG_SHARED_PROXY, + &gdev->descs[0].flags); return 0; } } @@ -573,7 +573,7 @@ int gpiochip_setup_shared(struct gpio_chip *gc) desc = &gdev->descs[entry->offset]; - __set_bit(GPIOD_FLAG_SHARED, &desc->flags); + set_bit(GPIOD_FLAG_SHARED, &desc->flags); /* * Shared GPIOs are not requested via the normal path. Make * them inaccessible to anyone even before we register the -- 2.47.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 3/5] gpiolib: clear debounce period before the "requested" flag 2026-11-09 10:27 [PATCH v2 0/5] gpiolib: improve concurrent access to the flags field of struct gpio_desc Bartosz Golaszewski 2026-11-09 10:27 ` [PATCH v2 1/5] gpiolib: don't overwrite the entire flags field in gpiod_get_direction() Bartosz Golaszewski 2026-11-09 10:27 ` [PATCH v2 2/5] gpio: shared: use atomic set_bit() for descriptor flags Bartosz Golaszewski @ 2026-11-09 10:27 ` Bartosz Golaszewski 2026-11-09 10:27 ` [PATCH v2 4/5] gpiolib: don't overwrite the entire flags field in gpiod_free_commit() Bartosz Golaszewski 2026-11-09 10:27 ` [PATCH v2 5/5] gpio: cdev: don't overwrite the whole flags field when configuring lines Bartosz Golaszewski 4 siblings, 0 replies; 8+ messages in thread From: Bartosz Golaszewski @ 2026-11-09 10:27 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko, Kent Gibson, Adrian Ambrozewicz Cc: linux-gpio, linux-kernel, Bartosz Golaszewski The debounce_period_us field is protected by the GPIOD_FLAG_REQUESTED flag, so it should be cleared before the flag, else it could potentially race with a new request. Suggested-by: Kent Gibson <warthog618@gmail.com> Closes: https://lore.kernel.org/all/20261105042905.GB8436@rigel/ Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com> --- drivers/gpio/gpiolib.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 63b65b646fbcfa9ea0f724d1d3e1ac22ba28f638..af2d67d977f4925558ab77c34b37c3c29bd12a6f 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2693,6 +2693,10 @@ void gpiod_free_commit(struct gpio_desc *desc) if (guard.gc->free) guard.gc->free(guard.gc, gpiod_hwgpio(desc)); +#ifdef CONFIG_GPIO_CDEV + WRITE_ONCE(desc->debounce_period_us, 0); +#endif + clear_bit(GPIOD_FLAG_ACTIVE_LOW, &flags); clear_bit(GPIOD_FLAG_REQUESTED, &flags); clear_bit(GPIOD_FLAG_OPEN_DRAIN, &flags); @@ -2710,9 +2714,6 @@ void gpiod_free_commit(struct gpio_desc *desc) #endif desc_set_label(desc, NULL); WRITE_ONCE(desc->flags, flags); -#ifdef CONFIG_GPIO_CDEV - WRITE_ONCE(desc->debounce_period_us, 0); -#endif gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_RELEASED); } } -- 2.47.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 4/5] gpiolib: don't overwrite the entire flags field in gpiod_free_commit() 2026-11-09 10:27 [PATCH v2 0/5] gpiolib: improve concurrent access to the flags field of struct gpio_desc Bartosz Golaszewski ` (2 preceding siblings ...) 2026-11-09 10:27 ` [PATCH v2 3/5] gpiolib: clear debounce period before the "requested" flag Bartosz Golaszewski @ 2026-11-09 10:27 ` Bartosz Golaszewski 2026-11-09 21:53 ` Andy Shevchenko 2026-11-09 10:27 ` [PATCH v2 5/5] gpio: cdev: don't overwrite the whole flags field when configuring lines Bartosz Golaszewski 4 siblings, 1 reply; 8+ messages in thread From: Bartosz Golaszewski @ 2026-11-09 10:27 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko, Kent Gibson, Adrian Ambrozewicz Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, stable Use gpiod_update_flags() to clear all request-related flags in one go instead of risking racing with concurrent users and overwriting any changes they could have done between READ_ONCE() and WRITE_ONCE(). Cc: stable@vger.kernel.org Fixes: 35b545332b80 ("gpio: remove gpio_lock") Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com> --- drivers/gpio/gpiolib.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index af2d67d977f4925558ab77c34b37c3c29bd12a6f..4c9c524389ca13e422bca8c217b7f47477cc0dd4 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -3,6 +3,7 @@ #include <linux/acpi.h> #include <linux/array_size.h> #include <linux/bitmap.h> +#include <linux/bitops.h> #include <linux/cleanup.h> #include <linux/compat.h> #include <linux/debugfs.h> @@ -2696,24 +2697,23 @@ void gpiod_free_commit(struct gpio_desc *desc) #ifdef CONFIG_GPIO_CDEV WRITE_ONCE(desc->debounce_period_us, 0); #endif - - clear_bit(GPIOD_FLAG_ACTIVE_LOW, &flags); - clear_bit(GPIOD_FLAG_REQUESTED, &flags); - clear_bit(GPIOD_FLAG_OPEN_DRAIN, &flags); - clear_bit(GPIOD_FLAG_OPEN_SOURCE, &flags); - clear_bit(GPIOD_FLAG_PULL_UP, &flags); - clear_bit(GPIOD_FLAG_PULL_DOWN, &flags); - clear_bit(GPIOD_FLAG_BIAS_DISABLE, &flags); - clear_bit(GPIOD_FLAG_EDGE_RISING, &flags); - clear_bit(GPIOD_FLAG_EDGE_FALLING, &flags); - clear_bit(GPIOD_FLAG_EVENT_CLOCK_REALTIME, &flags); - clear_bit(GPIOD_FLAG_EVENT_CLOCK_HTE, &flags); - clear_bit(GPIOD_FLAG_IS_HOGGED, &flags); #ifdef CONFIG_OF_DYNAMIC WRITE_ONCE(desc->hog, NULL); #endif desc_set_label(desc, NULL); - WRITE_ONCE(desc->flags, flags); + set_mask_bits(&desc->flags, + BIT(GPIOD_FLAG_ACTIVE_LOW) | + BIT(GPIOD_FLAG_REQUESTED) | + BIT(GPIOD_FLAG_OPEN_DRAIN) | + BIT(GPIOD_FLAG_OPEN_SOURCE) | + BIT(GPIOD_FLAG_PULL_UP) | + BIT(GPIOD_FLAG_PULL_DOWN) | + BIT(GPIOD_FLAG_BIAS_DISABLE) | + BIT(GPIOD_FLAG_EDGE_RISING) | + BIT(GPIOD_FLAG_EDGE_FALLING) | + BIT(GPIOD_FLAG_EVENT_CLOCK_REALTIME) | + BIT(GPIOD_FLAG_EVENT_CLOCK_HTE) | + BIT(GPIOD_FLAG_IS_HOGGED), 0); gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_RELEASED); } } -- 2.47.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 4/5] gpiolib: don't overwrite the entire flags field in gpiod_free_commit() 2026-11-09 10:27 ` [PATCH v2 4/5] gpiolib: don't overwrite the entire flags field in gpiod_free_commit() Bartosz Golaszewski @ 2026-11-09 21:53 ` Andy Shevchenko 0 siblings, 0 replies; 8+ messages in thread From: Andy Shevchenko @ 2026-11-09 21:53 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Linus Walleij, Bartosz Golaszewski, Kent Gibson, Adrian Ambrozewicz, linux-gpio, linux-kernel, stable On Mon, Nov 09, 2026 at 11:27:08AM +0100, Bartosz Golaszewski wrote: > Use gpiod_update_flags() to clear all request-related flags in one go > instead of risking racing with concurrent users and overwriting any > changes they could have done between READ_ONCE() and WRITE_ONCE(). ... > #include <linux/acpi.h> > #include <linux/array_size.h> > #include <linux/bitmap.h> > +#include <linux/bitops.h> > #include <linux/cleanup.h> > #include <linux/compat.h> > #include <linux/debugfs.h> No need, bitmap API implies all bitops. > + set_mask_bits(&desc->flags, > + BIT(GPIOD_FLAG_ACTIVE_LOW) | > + BIT(GPIOD_FLAG_REQUESTED) | > + BIT(GPIOD_FLAG_OPEN_DRAIN) | > + BIT(GPIOD_FLAG_OPEN_SOURCE) | > + BIT(GPIOD_FLAG_PULL_UP) | > + BIT(GPIOD_FLAG_PULL_DOWN) | > + BIT(GPIOD_FLAG_BIAS_DISABLE) | > + BIT(GPIOD_FLAG_EDGE_RISING) | > + BIT(GPIOD_FLAG_EDGE_FALLING) | > + BIT(GPIOD_FLAG_EVENT_CLOCK_REALTIME) | > + BIT(GPIOD_FLAG_EVENT_CLOCK_HTE) | > + BIT(GPIOD_FLAG_IS_HOGGED), 0); Indentation is broken (and it has spaces, which makes me totally confused what the point of those spaces in the first place?). -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 5/5] gpio: cdev: don't overwrite the whole flags field when configuring lines 2026-11-09 10:27 [PATCH v2 0/5] gpiolib: improve concurrent access to the flags field of struct gpio_desc Bartosz Golaszewski ` (3 preceding siblings ...) 2026-11-09 10:27 ` [PATCH v2 4/5] gpiolib: don't overwrite the entire flags field in gpiod_free_commit() Bartosz Golaszewski @ 2026-11-09 10:27 ` Bartosz Golaszewski 2026-11-09 21:58 ` Andy Shevchenko 4 siblings, 1 reply; 8+ messages in thread From: Bartosz Golaszewski @ 2026-11-09 10:27 UTC (permalink / raw) To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko, Kent Gibson, Adrian Ambrozewicz Cc: linux-gpio, linux-kernel, Bartosz Golaszewski, stable linehandle_flags_to_desc_flags() and gpio_v2_line_config_flags_to_desc_flags() read the flags word with READ_ONCE(), modify the local copy and write it back with WRITE_ONCE(). The per-request config_mutex does not exclude sysfs, the irq core or drivers touching the same descriptor, so bits they set or clear concurrently can be lost. Build the mask of bits owned by the line configuration and their new values locally and apply them with gpiod_update_flags(). The whole group changes in a single atomic step. While at it: rename the functions whose semantics changed from "conversion" to "flags update" accordingly. Cc: stable@vger.kernel.org Fixes: b7adfb6076ff ("gpio: cdev: update flags at once when reconfiguring from user-space") Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com> --- drivers/gpio/gpiolib-cdev.c | 111 +++++++++++++++++++++++++------------------- 1 file changed, 62 insertions(+), 49 deletions(-) diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index f68e33cbd7d4cd5153e82ad77768cf52a90601d6..50baee455283c3aeb25ed92f3936c5f6a5b215bf 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -3,6 +3,7 @@ #include <linux/anon_inodes.h> #include <linux/atomic.h> #include <linux/bitmap.h> +#include <linux/bitops.h> #include <linux/build_bug.h> #include <linux/cdev.h> #include <linux/cleanup.h> @@ -140,24 +141,30 @@ static int linehandle_validate_flags(u32 flags) return 0; } -static void linehandle_flags_to_desc_flags(u32 lflags, unsigned long *flagsp) +static void linehandle_update_desc_flags(u32 lflags, struct gpio_desc *desc) { - unsigned long flags = READ_ONCE(*flagsp); + const unsigned long mask = BIT(GPIOD_FLAG_ACTIVE_LOW) | + BIT(GPIOD_FLAG_OPEN_DRAIN) | + BIT(GPIOD_FLAG_OPEN_SOURCE) | + BIT(GPIOD_FLAG_PULL_UP) | + BIT(GPIOD_FLAG_PULL_DOWN) | + BIT(GPIOD_FLAG_BIAS_DISABLE); + unsigned long flags = 0; - assign_bit(GPIOD_FLAG_ACTIVE_LOW, &flags, - lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW); - assign_bit(GPIOD_FLAG_OPEN_DRAIN, &flags, - lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN); - assign_bit(GPIOD_FLAG_OPEN_SOURCE, &flags, - lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE); - assign_bit(GPIOD_FLAG_PULL_UP, &flags, - lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP); - assign_bit(GPIOD_FLAG_PULL_DOWN, &flags, - lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN); - assign_bit(GPIOD_FLAG_BIAS_DISABLE, &flags, - lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE); + __assign_bit(GPIOD_FLAG_ACTIVE_LOW, &flags, + lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW); + __assign_bit(GPIOD_FLAG_OPEN_DRAIN, &flags, + lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN); + __assign_bit(GPIOD_FLAG_OPEN_SOURCE, &flags, + lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE); + __assign_bit(GPIOD_FLAG_PULL_UP, &flags, + lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP); + __assign_bit(GPIOD_FLAG_PULL_DOWN, &flags, + lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN); + __assign_bit(GPIOD_FLAG_BIAS_DISABLE, &flags, + lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE); - WRITE_ONCE(*flagsp, flags); + set_mask_bits(&desc->flags, mask, flags); } static long linehandle_set_config(struct linehandle_state *lh, @@ -182,7 +189,7 @@ static long linehandle_set_config(struct linehandle_state *lh, for (i = 0; i < lh->num_descs; i++) { desc = lh->descs[i]; - linehandle_flags_to_desc_flags(lflags, &desc->flags); + linehandle_update_desc_flags(lflags, desc); if (lflags & GPIOHANDLE_REQUEST_OUTPUT) { int val = !!gcnf.default_values[i]; @@ -339,7 +346,7 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) if (ret) return ret; lh->descs[i] = desc; - linehandle_flags_to_desc_flags(handlereq.flags, &desc->flags); + linehandle_update_desc_flags(handlereq.flags, desc); ret = gpiod_set_transitory(desc, false); if (ret < 0) @@ -1216,37 +1223,43 @@ static int gpio_v2_line_config_validate(struct gpio_v2_line_config *lc, return 0; } -static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags, - unsigned long *flagsp) +static void linereq_update_desc_flags(u64 lflags, + struct gpio_desc *desc) { - unsigned long flags = READ_ONCE(*flagsp); - - assign_bit(GPIOD_FLAG_ACTIVE_LOW, &flags, - lflags & GPIO_V2_LINE_FLAG_ACTIVE_LOW); - - assign_bit(GPIOD_FLAG_EDGE_RISING, &flags, - lflags & GPIO_V2_LINE_FLAG_EDGE_RISING); - assign_bit(GPIOD_FLAG_EDGE_FALLING, &flags, - lflags & GPIO_V2_LINE_FLAG_EDGE_FALLING); - - assign_bit(GPIOD_FLAG_OPEN_DRAIN, &flags, - lflags & GPIO_V2_LINE_FLAG_OPEN_DRAIN); - assign_bit(GPIOD_FLAG_OPEN_SOURCE, &flags, - lflags & GPIO_V2_LINE_FLAG_OPEN_SOURCE); - - assign_bit(GPIOD_FLAG_PULL_UP, &flags, - lflags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP); - assign_bit(GPIOD_FLAG_PULL_DOWN, &flags, - lflags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN); - assign_bit(GPIOD_FLAG_BIAS_DISABLE, &flags, - lflags & GPIO_V2_LINE_FLAG_BIAS_DISABLED); - - assign_bit(GPIOD_FLAG_EVENT_CLOCK_REALTIME, &flags, - lflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME); - assign_bit(GPIOD_FLAG_EVENT_CLOCK_HTE, &flags, - lflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE); + const unsigned long mask = BIT(GPIOD_FLAG_ACTIVE_LOW) | + BIT(GPIOD_FLAG_EDGE_RISING) | + BIT(GPIOD_FLAG_EDGE_FALLING) | + BIT(GPIOD_FLAG_OPEN_DRAIN) | + BIT(GPIOD_FLAG_OPEN_SOURCE) | + BIT(GPIOD_FLAG_PULL_UP) | + BIT(GPIOD_FLAG_PULL_DOWN) | + BIT(GPIOD_FLAG_BIAS_DISABLE) | + BIT(GPIOD_FLAG_EVENT_CLOCK_REALTIME) | + BIT(GPIOD_FLAG_EVENT_CLOCK_HTE); + unsigned long flags = 0; - WRITE_ONCE(*flagsp, flags); + __assign_bit(GPIOD_FLAG_ACTIVE_LOW, &flags, + lflags & GPIO_V2_LINE_FLAG_ACTIVE_LOW); + __assign_bit(GPIOD_FLAG_EDGE_RISING, &flags, + lflags & GPIO_V2_LINE_FLAG_EDGE_RISING); + __assign_bit(GPIOD_FLAG_EDGE_FALLING, &flags, + lflags & GPIO_V2_LINE_FLAG_EDGE_FALLING); + __assign_bit(GPIOD_FLAG_OPEN_DRAIN, &flags, + lflags & GPIO_V2_LINE_FLAG_OPEN_DRAIN); + __assign_bit(GPIOD_FLAG_OPEN_SOURCE, &flags, + lflags & GPIO_V2_LINE_FLAG_OPEN_SOURCE); + __assign_bit(GPIOD_FLAG_PULL_UP, &flags, + lflags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP); + __assign_bit(GPIOD_FLAG_PULL_DOWN, &flags, + lflags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN); + __assign_bit(GPIOD_FLAG_BIAS_DISABLE, &flags, + lflags & GPIO_V2_LINE_FLAG_BIAS_DISABLED); + __assign_bit(GPIOD_FLAG_EVENT_CLOCK_REALTIME, &flags, + lflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME); + __assign_bit(GPIOD_FLAG_EVENT_CLOCK_HTE, &flags, + lflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE); + + set_mask_bits(&desc->flags, mask, flags); } static long linereq_get_values(struct linereq *lr, void __user *ip) @@ -1404,7 +1417,7 @@ static long linereq_set_config(struct linereq *lr, void __user *ip) */ if (!(flags & GPIO_V2_LINE_DIRECTION_FLAGS)) continue; - gpio_v2_line_config_flags_to_desc_flags(flags, &desc->flags); + linereq_update_desc_flags(flags, desc); edflags = flags & GPIO_V2_LINE_EDGE_DETECTOR_FLAGS; if (flags & GPIO_V2_LINE_FLAG_OUTPUT) { int val = gpio_v2_line_config_output_value(&lc, i); @@ -1660,7 +1673,7 @@ static int linereq_create(struct gpio_device *gdev, void __user *ip) lr->lines[i].desc = desc; flags = gpio_v2_line_config_flags(lc, i); - gpio_v2_line_config_flags_to_desc_flags(flags, &desc->flags); + linereq_update_desc_flags(flags, desc); ret = gpiod_set_transitory(desc, false); if (ret < 0) @@ -2055,7 +2068,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip) le->desc = desc; le->eflags = eflags; - linehandle_flags_to_desc_flags(lflags, &desc->flags); + linehandle_update_desc_flags(lflags, desc); ret = gpiod_direction_input(desc); if (ret) -- 2.47.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 5/5] gpio: cdev: don't overwrite the whole flags field when configuring lines 2026-11-09 10:27 ` [PATCH v2 5/5] gpio: cdev: don't overwrite the whole flags field when configuring lines Bartosz Golaszewski @ 2026-11-09 21:58 ` Andy Shevchenko 0 siblings, 0 replies; 8+ messages in thread From: Andy Shevchenko @ 2026-11-09 21:58 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Linus Walleij, Bartosz Golaszewski, Kent Gibson, Adrian Ambrozewicz, linux-gpio, linux-kernel, stable On Mon, Nov 09, 2026 at 11:27:09AM +0100, Bartosz Golaszewski wrote: > linehandle_flags_to_desc_flags() and > gpio_v2_line_config_flags_to_desc_flags() read the flags word with > READ_ONCE(), modify the local copy and write it back with WRITE_ONCE(). > > The per-request config_mutex does not exclude sysfs, the irq core or > drivers touching the same descriptor, so bits they set or clear > concurrently can be lost. > > Build the mask of bits owned by the line configuration and their new > values locally and apply them with gpiod_update_flags(). The whole > group changes in a single atomic step. > > While at it: rename the functions whose semantics changed from > "conversion" to "flags update" accordingly. ... > @@ -3,6 +3,7 @@ > #include <linux/anon_inodes.h> > #include <linux/atomic.h> > #include <linux/bitmap.h> > +#include <linux/bitops.h> > #include <linux/build_bug.h> > #include <linux/cdev.h> > #include <linux/cleanup.h> Unneeded hunk as explained in the previous reply. ... > -static void linehandle_flags_to_desc_flags(u32 lflags, unsigned long *flagsp) > +static void linehandle_update_desc_flags(u32 lflags, struct gpio_desc *desc) > { > - unsigned long flags = READ_ONCE(*flagsp); > + const unsigned long mask = BIT(GPIOD_FLAG_ACTIVE_LOW) | > + BIT(GPIOD_FLAG_OPEN_DRAIN) | > + BIT(GPIOD_FLAG_OPEN_SOURCE) | > + BIT(GPIOD_FLAG_PULL_UP) | > + BIT(GPIOD_FLAG_PULL_DOWN) | > + BIT(GPIOD_FLAG_BIAS_DISABLE); Usually we don't do such an assignments in the definition blocks. I believe the split will look better. > + unsigned long flags = 0; > mask = ...; (and with longer line you may combine close bits, like OD/OS or PD,PU, and DISABLE). > - assign_bit(GPIOD_FLAG_ACTIVE_LOW, &flags, > - lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW); > - assign_bit(GPIOD_FLAG_OPEN_DRAIN, &flags, > - lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN); > - assign_bit(GPIOD_FLAG_OPEN_SOURCE, &flags, > - lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE); > - assign_bit(GPIOD_FLAG_PULL_UP, &flags, > - lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP); > - assign_bit(GPIOD_FLAG_PULL_DOWN, &flags, > - lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN); > - assign_bit(GPIOD_FLAG_BIAS_DISABLE, &flags, > - lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE); > + __assign_bit(GPIOD_FLAG_ACTIVE_LOW, &flags, > + lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW); > + __assign_bit(GPIOD_FLAG_OPEN_DRAIN, &flags, > + lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN); > + __assign_bit(GPIOD_FLAG_OPEN_SOURCE, &flags, > + lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE); > + __assign_bit(GPIOD_FLAG_PULL_UP, &flags, > + lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP); > + __assign_bit(GPIOD_FLAG_PULL_DOWN, &flags, > + lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN); > + __assign_bit(GPIOD_FLAG_BIAS_DISABLE, &flags, > + lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE); ... > +static void linereq_update_desc_flags(u64 lflags, > + struct gpio_desc *desc) This is ugliest part of this patch in my opinion. Just make it a single line. `clang-format` is in use I assume? ... > + const unsigned long mask = BIT(GPIOD_FLAG_ACTIVE_LOW) | > + BIT(GPIOD_FLAG_EDGE_RISING) | > + BIT(GPIOD_FLAG_EDGE_FALLING) | > + BIT(GPIOD_FLAG_OPEN_DRAIN) | > + BIT(GPIOD_FLAG_OPEN_SOURCE) | > + BIT(GPIOD_FLAG_PULL_UP) | > + BIT(GPIOD_FLAG_PULL_DOWN) | > + BIT(GPIOD_FLAG_BIAS_DISABLE) | > + BIT(GPIOD_FLAG_EVENT_CLOCK_REALTIME) | > + BIT(GPIOD_FLAG_EVENT_CLOCK_HTE); Same comment as before. > + unsigned long flags = 0; -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-11-09 21:58 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-11-09 10:27 [PATCH v2 0/5] gpiolib: improve concurrent access to the flags field of struct gpio_desc Bartosz Golaszewski 2026-11-09 10:27 ` [PATCH v2 1/5] gpiolib: don't overwrite the entire flags field in gpiod_get_direction() Bartosz Golaszewski 2026-11-09 10:27 ` [PATCH v2 2/5] gpio: shared: use atomic set_bit() for descriptor flags Bartosz Golaszewski 2026-11-09 10:27 ` [PATCH v2 3/5] gpiolib: clear debounce period before the "requested" flag Bartosz Golaszewski 2026-11-09 10:27 ` [PATCH v2 4/5] gpiolib: don't overwrite the entire flags field in gpiod_free_commit() Bartosz Golaszewski 2026-11-09 21:53 ` Andy Shevchenko 2026-11-09 10:27 ` [PATCH v2 5/5] gpio: cdev: don't overwrite the whole flags field when configuring lines Bartosz Golaszewski 2026-11-09 21:58 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®