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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox