From: Kent Gibson <warthog618@gmail.com>
To: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
Cc: Linus Walleij <linusw@kernel.org>,
Bartosz Golaszewski <brgl@kernel.org>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Adrian Ambrozewicz <aambrozewicz@nvidia.com>,
linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
stable@vger.kernel.org
Subject: Re: [PATCH 5/5] gpio: cdev: don't overwrite the whole flags field when configuring lines
Date: Thu, 5 Nov 2026 12:23:45 +0800 [thread overview]
Message-ID: <20261105042345.GA8436@rigel> (raw)
In-Reply-To: <20261104-gpio-desc-flags-atomic-v1-5-e23cedf619ba@oss.qualcomm.com>
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@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 | 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.
prev parent reply other threads:[~2026-11-05 4:23 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-11-04 15:36 [PATCH 0/5] gpiolib: improve concurrent access to the flags field of struct gpio_desc Bartosz Golaszewski
2026-11-04 15:36 ` [PATCH 1/5] gpiolib: don't overwrite the entire flags field in gpiod_get_direction() Bartosz Golaszewski
2026-11-04 15:36 ` [PATCH 2/5] gpio: shared: use atomic set_bit() for descriptor flags Bartosz Golaszewski
2026-11-04 15:36 ` [PATCH 3/5] gpiolib: provide gpiod_update_flags() Bartosz Golaszewski
2026-11-04 15:52 ` sashiko-bot
2026-11-05 12:59 ` Linus Walleij
2026-11-04 15:36 ` [PATCH 4/5] gpiolib: don't overwrite the entire flags field in gpiod_free_commit() Bartosz Golaszewski
2026-11-05 4:29 ` Kent Gibson
2026-11-04 15:37 ` [PATCH 5/5] gpio: cdev: don't overwrite the whole flags field when configuring lines Bartosz Golaszewski
2026-11-04 21:55 ` Andy Shevchenko
2026-11-05 4:23 ` Kent Gibson [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20261105042345.GA8436@rigel \
--to=warthog618@gmail.com \
--cc=aambrozewicz@nvidia.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=bartosz.golaszewski@oss.qualcomm.com \
--cc=brgl@kernel.org \
--cc=linusw@kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.