Re: [PATCH 5/5] gpio: cdev: don't overwrite the whole flags field when configuring lines
From: Kent Gibson
Date: Wed Nov 04 2026 - 23:45:50 EST
On Wed, Nov 04, 2026 at 04:37:00PM +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.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: b7adfb6076ff ("gpio: cdev: update flags at once when reconfiguring from user-space")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxxxxxxxx>
> ---
> drivers/gpio/gpiolib-cdev.c | 118 ++++++++++++++++++++++++--------------------
> 1 file changed, 65 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 3f1f8c9acfc3c10d5b3e86b70009f1226793cbc7..b7f0aec12c8551e39d042a1999fdfb5683ae97d7 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -140,24 +140,30 @@ static int linehandle_validate_flags(u32 flags)
> return 0;
> }
>
> -static void linehandle_flags_to_desc_flags(u32 lflags, unsigned long *flagsp)
> -{
> - unsigned long flags = READ_ONCE(*flagsp);
> -
> - 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);
> +static void linehandle_flags_to_desc_flags(u32 lflags, struct gpio_desc *desc)
This used to be a conversion function, hence the a_to_b naming.
Now it is specifically updating the desc flags, so how about
linehandle_update_desc_flags()?
> +{
> + 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 val = 0;
> +
Agree with Andy - this should be flags, not val.
> + __assign_bit(GPIOD_FLAG_ACTIVE_LOW, &val,
> + lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW);
> + __assign_bit(GPIOD_FLAG_OPEN_DRAIN, &val,
> + lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN);
> + __assign_bit(GPIOD_FLAG_OPEN_SOURCE, &val,
> + lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE);
> + __assign_bit(GPIOD_FLAG_PULL_UP, &val,
> + lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP);
> + __assign_bit(GPIOD_FLAG_PULL_DOWN, &val,
> + lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN);
> + __assign_bit(GPIOD_FLAG_BIAS_DISABLE, &val,
> + lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE);
> +
> + gpiod_update_flags(desc, mask, val);
> }
>
> static long linehandle_set_config(struct linehandle_state *lh,
> @@ -182,7 +188,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_flags_to_desc_flags(lflags, desc);
>
> if (lflags & GPIOHANDLE_REQUEST_OUTPUT) {
> int val = !!gcnf.default_values[i];
> @@ -346,7 +352,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_flags_to_desc_flags(handlereq.flags, desc);
>
> ret = gpiod_set_transitory(desc, false);
> if (ret < 0)
> @@ -1224,36 +1230,42 @@ static int gpio_v2_line_config_validate(struct gpio_v2_line_config *lc,
> }
>
> static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags,
> - unsigned long *flagsp)
How about linereq_update_desc_flags()?
> -{
> - 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);
> -
> - WRITE_ONCE(*flagsp, flags);
> + struct gpio_desc *desc)
> +{
> + 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 val = 0;
> +
And again.
> + __assign_bit(GPIOD_FLAG_ACTIVE_LOW, &val,
> + lflags & GPIO_V2_LINE_FLAG_ACTIVE_LOW);
> + __assign_bit(GPIOD_FLAG_EDGE_RISING, &val,
> + lflags & GPIO_V2_LINE_FLAG_EDGE_RISING);
> + __assign_bit(GPIOD_FLAG_EDGE_FALLING, &val,
> + lflags & GPIO_V2_LINE_FLAG_EDGE_FALLING);
> + __assign_bit(GPIOD_FLAG_OPEN_DRAIN, &val,
> + lflags & GPIO_V2_LINE_FLAG_OPEN_DRAIN);
> + __assign_bit(GPIOD_FLAG_OPEN_SOURCE, &val,
> + lflags & GPIO_V2_LINE_FLAG_OPEN_SOURCE);
> + __assign_bit(GPIOD_FLAG_PULL_UP, &val,
> + lflags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP);
> + __assign_bit(GPIOD_FLAG_PULL_DOWN, &val,
> + lflags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN);
> + __assign_bit(GPIOD_FLAG_BIAS_DISABLE, &val,
> + lflags & GPIO_V2_LINE_FLAG_BIAS_DISABLED);
> + __assign_bit(GPIOD_FLAG_EVENT_CLOCK_REALTIME, &val,
> + lflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME);
> + __assign_bit(GPIOD_FLAG_EVENT_CLOCK_HTE, &val,
> + lflags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_HTE);
> +
> + gpiod_update_flags(desc, mask, val);
> }
>
> static long linereq_get_values(struct linereq *lr, void __user *ip)
> @@ -1411,7 +1423,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);
> + gpio_v2_line_config_flags_to_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);
> @@ -1667,7 +1679,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);
> + gpio_v2_line_config_flags_to_desc_flags(flags, desc);
>
> ret = gpiod_set_transitory(desc, false);
> if (ret < 0)
> @@ -2062,7 +2074,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_flags_to_desc_flags(lflags, desc);
>
> ret = gpiod_direction_input(desc);
> if (ret)
>
Overall I like the series.
Good to see there is a raw version of try_cmpxchg() that can be combined with
the existing atomic bitops without the questionable casting I was using
with the atomic_long_try_cmpxchg(). That makes a whole load of sense.
A couple of things not covered in these patches, but related to flag
consistency:
GPIOD_FLAG_USED_AS_IRQ is used as a lock over GPIOD_FLAG_IRQ_IS_ENABLED.
e.g. gpiochip_enable_irq() and gpiochip_disable_irq() both warn and drop
thru if it is not set.
Yet gpiochip_lock_as_irq() does not check if it is already set, even just to
raise a warning. Why not?
Does the updating of those two flags by gpiochip_lock_as_irq() and
gpiochip_unlock_as_irq() need to be atomic? Not individually atomic but
as a pair, else they race with gpiochip_enable_irq() and
gpiochip_disable_irq()?
Similarly, don't gpiochip_enable_irq() and gpiochip_disable_irq() need
to perform an atomic test (of GPIOD_FLAG_USED_AS_IRQ) and set
(of GPIOD_FLAG_IRQ_IS_ENABLED) so as not to race with the lock/unlock?
Cheers,
Kent.