gpiolib: cdev: don't record a line direction the driver refused

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: memory leak in gpiod_set_array_value_complex() on error paths
- [High] gpio: non-atomic desc->flags update clobbers concurrent IRQ flags --

commit 910242af9150f094f4676cb26fb8b58cc8edfa96
Author: Adrian Ambrozewicz <[email protected]>

gpiolib: cdev: don't record a line direction the driver refused

The commit modifies gpio_v2_line_config_flags_to_desc_flags() to avoid setting the GPIOD_FLAG_IS_OUT flag before the hardware direction is confirmed by the driver. This fixes an issue where refused configurations would leave the line erroneously reported as an output or input.

drivers/gpio/gpiolib-cdev.c static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags, · line 1227

Quoted 7 lines

drivers/gpio/gpiolib-cdev.c +0 -0

index 5d53bfcdf7268..9bbee3a1f9c9c 100644
@@ -1226,14 +1226,13 @@ static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags,
 {
 	unsigned long flags = READ_ONCE(*flagsp);

[Severity: High]
Does this non-atomic read-modify-write clobber concurrent atomic updates to desc->flags made by the IRQ subsystem?

If a driver or IRQ domain calls gpiochip_lock_as_irq() or gpiochip_enable_irq() concurrently, they atomically update bits like GPIOD_FLAG_USED_AS_IRQ:

	set_bit(GPIOD_FLAG_USED_AS_IRQ, &desc->flags);
	set_bit(GPIOD_FLAG_IRQ_IS_ENABLED, &desc->flags);

Because the IRQ subsystem does not check GPIOD_FLAG_REQUESTED before using the line and does not share locks with the cdev ioctl, these atomic updates can happen between this READ_ONCE() and the subsequent WRITE_ONCE(*flagsp, flags) at the end of this function.

Wouldn't the atomic updates be silently overwritten when this function writes back the stale flags value?

Losing these flags disables the safety check in gpiod_direction_output_nonotify(), which could allow user-space to force an active interrupt line into output mode.

drivers/gpio/gpiolib-cdev.c static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags, · line 1236

Quoted 13 lines

drivers/gpio/gpiolib-cdev.c +4 -5

+	/*
+	 * GPIOD_FLAG_IS_OUT is left to gpiod_direction_input/output(), which
+	 * only update it once the chip has accepted the direction.
+	 */
 	assign_bit(GPIOD_FLAG_ACTIVE_LOW, &flags,
 		   lflags & GPIO_V2_LINE_FLAG_ACTIVE_LOW);
 
-	if (lflags & GPIO_V2_LINE_FLAG_OUTPUT)
-		set_bit(GPIOD_FLAG_IS_OUT, &flags);
-	else if (lflags & GPIO_V2_LINE_FLAG_INPUT)
-		clear_bit(GPIOD_FLAG_IS_OUT, &flags);
-

[Severity: High]
By leaving GPIOD_FLAG_IS_OUT unset when a driver refuses the output direction, does this create a memory leak in gpiod_set_array_value_complex() when user-space subsequently calls GPIO_V2_LINE_SET_VALUES_IOCTL?

Looking at gpiod_set_array_value_complex() in drivers/gpio/gpiolib.c, mask and bits can be dynamically allocated with bitmap_alloc() for large GPIO chips:

			if (unlikely(!test_bit(GPIOD_FLAG_IS_OUT, &desc->flags)))
				return -EPERM;

If a descriptor is not configured as an output, the function immediately returns -EPERM.

Doesn't this bypass the bitmap_free() calls at the end of the function and leak mask and bits?

The same direct return leak appears to occur if the driver's set_multiple callback fails:

		if (count != 0) {
			ret = gpiochip_set_multiple(guard.gc, mask, bits);
			if (ret)
				return ret;

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Quoted 8 lines · On Mon, Oct 12, 2026 at 02:04:58PM +0000, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: memory leak in gpiod_set_array_value_complex() on error paths
- [High] gpio: non-atomic desc->flags update clobbers concurrent IRQ flags --

commit 910242af9150f094f4676cb26fb8b58cc8edfa96
Author: Adrian Ambrozewicz <[email protected]>

Hi Bart,

following this up with you directly since Sashiko doesn't currently let original authors or maintainers know about problems it discovers in existing code, just the patch author and the list.

Quoted 31 lines

gpiolib: cdev: don't record a line direction the driver refused

The commit modifies gpio_v2_line_config_flags_to_desc_flags() to avoid setting the GPIOD_FLAG_IS_OUT flag before the hardware direction is confirmed by the driver. This fixes an issue where refused configurations would leave the line erroneously reported as an output or input.

drivers/gpio/gpiolib-cdev.c static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags, · line 1227

Quoted 7 lines

drivers/gpio/gpiolib-cdev.c +0 -0

index 5d53bfcdf7268..9bbee3a1f9c9c 100644
@@ -1226,14 +1226,13 @@ static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags,
 {
 	unsigned long flags = READ_ONCE(*flagsp);

[Severity: High]
Does this non-atomic read-modify-write clobber concurrent atomic updates to desc->flags made by the IRQ subsystem?

If a driver or IRQ domain calls gpiochip_lock_as_irq() or gpiochip_enable_irq() concurrently, they atomically update bits like GPIOD_FLAG_USED_AS_IRQ:

	set_bit(GPIOD_FLAG_USED_AS_IRQ, &desc->flags);
	set_bit(GPIOD_FLAG_IRQ_IS_ENABLED, &desc->flags);

Because the IRQ subsystem does not check GPIOD_FLAG_REQUESTED before using the line and does not share locks with the cdev ioctl, these atomic updates can happen between this READ_ONCE() and the subsequent WRITE_ONCE(*flagsp, flags) at the end of this function.

Quoted 7 lines

Wouldn't the atomic updates be silently overwritten when this function writes back the stale flags value?

Losing these flags disables the safety check in gpiod_direction_output_nonotify(), which could allow user-space to force an active interrupt line into output mode.

This isn't a scenario that I'd considered - I was probably assuming GPIOD_FLAG_REQUESTED being set would provide protection by making the line unavailable.

How would you like to handle this?

Quoted 32 lines

drivers/gpio/gpiolib-cdev.c static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags, · line 1236

Quoted 13 lines

drivers/gpio/gpiolib-cdev.c +4 -5

+	/*
+	 * GPIOD_FLAG_IS_OUT is left to gpiod_direction_input/output(), which
+	 * only update it once the chip has accepted the direction.
+	 */
 	assign_bit(GPIOD_FLAG_ACTIVE_LOW, &flags,
 		   lflags & GPIO_V2_LINE_FLAG_ACTIVE_LOW);
 
-	if (lflags & GPIO_V2_LINE_FLAG_OUTPUT)
-		set_bit(GPIOD_FLAG_IS_OUT, &flags);
-	else if (lflags & GPIO_V2_LINE_FLAG_INPUT)
-		clear_bit(GPIOD_FLAG_IS_OUT, &flags);
-

[Severity: High]
By leaving GPIOD_FLAG_IS_OUT unset when a driver refuses the output direction, does this create a memory leak in gpiod_set_array_value_complex() when user-space subsequently calls GPIO_V2_LINE_SET_VALUES_IOCTL?

Looking at gpiod_set_array_value_complex() in drivers/gpio/gpiolib.c, mask and bits can be dynamically allocated with bitmap_alloc() for large GPIO chips:

			if (unlikely(!test_bit(GPIOD_FLAG_IS_OUT, &desc->flags)))
				return -EPERM;

If a descriptor is not configured as an output, the function immediately returns -EPERM.

Doesn't this bypass the bitmap_free() calls at the end of the function and leak mask and bits?

Quoted 8 lines

The same direct return leak appears to occur if the driver's set_multiple callback fails:

		if (count != 0) {
			ret = gpiochip_set_multiple(guard.gc, mask, bits);
			if (ret)
				return ret;

This also looks like a valid concern in existing code. Unlikely, but valid.

It is also not something we can readily test.
Would you be open to extending gpio-sim so we can simulate constrained drivers to better test alternate paths in gpiolib and cdev?
e.g. input only, output only, not supporting bias, supporting open drain, open source, debounce, ...?
I haven't put any thought into what that might look like on the userspace side, much less done any actual work on it, - just wondering if you had or would at least be ok with it.

Cheers,
Kent.

Quoted 2 lines

--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Quoted 55 lines · Kent Gibson

Quoted 8 lines · On Mon, Oct 12, 2026 at 02:04:58PM +0000, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: memory leak in gpiod_set_array_value_complex() on error paths
- [High] gpio: non-atomic desc->flags update clobbers concurrent IRQ flags --

commit 910242af9150f094f4676cb26fb8b58cc8edfa96
Author: Adrian Ambrozewicz <[email protected]>

Hi Bart,

following this up with you directly since Sashiko doesn't currently let original authors or maintainers know about problems it discovers in existing code, just the patch author and the list.

Quoted 38 lines

gpiolib: cdev: don't record a line direction the driver refused

The commit modifies gpio_v2_line_config_flags_to_desc_flags() to avoid setting the GPIOD_FLAG_IS_OUT flag before the hardware direction is confirmed by the driver. This fixes an issue where refused configurations would leave the line erroneously reported as an output or input.

drivers/gpio/gpiolib-cdev.c static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags, · line 1227

Quoted 7 lines

drivers/gpio/gpiolib-cdev.c +0 -0

index 5d53bfcdf7268..9bbee3a1f9c9c 100644
@@ -1226,14 +1226,13 @@ static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags,
 {
 	unsigned long flags = READ_ONCE(*flagsp);

[Severity: High]
Does this non-atomic read-modify-write clobber concurrent atomic updates to desc->flags made by the IRQ subsystem?

If a driver or IRQ domain calls gpiochip_lock_as_irq() or gpiochip_enable_irq() concurrently, they atomically update bits like GPIOD_FLAG_USED_AS_IRQ:

	set_bit(GPIOD_FLAG_USED_AS_IRQ, &desc->flags);
	set_bit(GPIOD_FLAG_IRQ_IS_ENABLED, &desc->flags);

Because the IRQ subsystem does not check GPIOD_FLAG_REQUESTED before using the line and does not share locks with the cdev ioctl, these atomic updates can happen between this READ_ONCE() and the subsequent WRITE_ONCE(*flagsp, flags) at the end of this function.

Wouldn't the atomic updates be silently overwritten when this function writes back the stale flags value?

Losing these flags disables the safety check in gpiod_direction_output_nonotify(), which could allow user-space to force an active interrupt line into output mode.

Quoted 6 lines

This isn't a scenario that I'd considered - I was probably assuming GPIOD_FLAG_REQUESTED being set would provide protection by making the line unavailable.

How would you like to handle this?

Sign... By adding it to the heap of GPIO issues found by sashiko. :(

We can't protect the flags with mutex or spinlock due to the multitude of issues that forced us to use SRCU. We could maybe consider atomic_t for the flags field and that would allow us to use various atomic ops and possibly it would be enough to cover all the read-modify-write cases we have. I need to think about it and will also ask the clanker for help.

Quoted 43 lines

Quoted 32 lines

drivers/gpio/gpiolib-cdev.c static void gpio_v2_line_config_flags_to_desc_flags(u64 lflags, · line 1236

Quoted 13 lines

drivers/gpio/gpiolib-cdev.c +4 -5

+	/*
+	 * GPIOD_FLAG_IS_OUT is left to gpiod_direction_input/output(), which
+	 * only update it once the chip has accepted the direction.
+	 */
 	assign_bit(GPIOD_FLAG_ACTIVE_LOW, &flags,
 		   lflags & GPIO_V2_LINE_FLAG_ACTIVE_LOW);
 
-	if (lflags & GPIO_V2_LINE_FLAG_OUTPUT)
-		set_bit(GPIOD_FLAG_IS_OUT, &flags);
-	else if (lflags & GPIO_V2_LINE_FLAG_INPUT)
-		clear_bit(GPIOD_FLAG_IS_OUT, &flags);
-

[Severity: High]
By leaving GPIOD_FLAG_IS_OUT unset when a driver refuses the output direction, does this create a memory leak in gpiod_set_array_value_complex() when user-space subsequently calls GPIO_V2_LINE_SET_VALUES_IOCTL?

Looking at gpiod_set_array_value_complex() in drivers/gpio/gpiolib.c, mask and bits can be dynamically allocated with bitmap_alloc() for large GPIO chips:

			if (unlikely(!test_bit(GPIOD_FLAG_IS_OUT, &desc->flags)))
				return -EPERM;

If a descriptor is not configured as an output, the function immediately returns -EPERM.

Doesn't this bypass the bitmap_free() calls at the end of the function and leak mask and bits?

Quoted 8 lines

The same direct return leak appears to occur if the driver's set_multiple callback fails:

		if (count != 0) {
			ret = gpiochip_set_multiple(guard.gc, mask, bits);
			if (ret)
				return ret;

This also looks like a valid concern in existing code. Unlikely, but valid.

Quoted 9 lines

It is also not something we can readily test.
Would you be open to extending gpio-sim so we can simulate constrained drivers to better test alternate paths in gpiolib and cdev?
e.g. input only, output only, not supporting bias, supporting open drain, open source, debounce, ...?
I haven't put any thought into what that might look like on the userspace side, much less done any actual work on it, - just wondering if you had or would at least be ok with it.

Definitely! I don't see why I would ever be against extending the test coverage. :)

Quoted 5 lines · Bartosz Golaszewski

We can't protect the flags with mutex or spinlock due to the multitude of issues that forced us to use SRCU. We could maybe consider atomic_t for the flags field and that would allow us to use various atomic ops and possibly it would be enough to cover all the read-modify-write cases we have. I need to think about it and will also ask the clanker for help.

I was thinking along the lines of reverting to atomic bitops directly on the flags, e.g. reverting b7adfb6076ff, without changing the underlying type to atomic_t.

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

Doesn't this bypass the bitmap_free() calls at the end of the function and leak mask and bits?

This also looks like a valid concern in existing code. Unlikely, but valid.

It is also not something we can readily test.

gpiod_free_commit() does the same read-modify-write: it reads the flags, calls ->free(), which may sleep, and writes the word back. Patch 8 of chip-removal v2 (for the debounce race Sashiko found on the event clock fix) relies on that write: it resets debounce_period_us first and makes the write a release store, pairing with the test_and_set_bit() in gpiod_request_commit(), so that a new owner sees the reset. With atomic bitops there, clearing GPIOD_FLAG_REQUESTED last with clear_bit_unlock() would give the same ordering. v2 keeps the release store, as that matches the code today.

v2 of this patch only drops the GPIOD_FLAG_IS_OUT lines from gpio_v2_line_config_flags_to_desc_flags(), which b7adfb6076ff also touches, so a revert and this patch only need rebasing over each other.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Doesn't this bypass the bitmap_free() calls at the end of the function and leak mask and bits?

This also looks like a valid concern in existing code. Unlikely, but valid.

Quoted 1 line

Quoted 1 line

Quoted 1 line

It is also not something we can readily test.

It is: both the -EPERM return and the one after a failed gpiochip_set_multiple() skip the bitmap_free() calls when the bitmaps were allocated. The -EPERM path shows up with gpio-sim as it is: on a chip with more lines than CONFIG_GPIOLIB_FASTPATH_LIMIT (512 by default; I used 600), GPIO_V2_LINE_SET_VALUES_IOCTL on a line requested as input leaves the two bitmaps to kmemleak.

Unless Bart would rather take it
himself, I'll send the fix as patch 7 of chip-removal v2:

Quoted 20 lines

gpiod_free_commit() does the same read-modify-write: it reads the flags, calls ->free(), which may sleep, and writes the word back. Patch 8 of chip-removal v2 (for the debounce race Sashiko found on the event clock fix) relies on that write: it resets debounce_period_us first and makes the write a release store, pairing with the test_and_set_bit() in gpiod_request_commit(), so that a new owner sees the reset. With atomic bitops there, clearing GPIOD_FLAG_REQUESTED last with clear_bit_unlock() would give the same ordering. v2 keeps the release store, as that matches the code today.

v2 of this patch only drops the GPIOD_FLAG_IS_OUT lines from gpio_v2_line_config_flags_to_desc_flags(), which b7adfb6076ff also touches, so a revert and this patch only need rebasing over each other.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Doesn't this bypass the bitmap_free() calls at the end of the function and leak mask and bits?

This also looks like a valid concern in existing code. Unlikely, but valid.

Quoted 8 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

It is also not something we can readily test.

It is: both the -EPERM return and the one after a failed gpiochip_set_multiple() skip the bitmap_free() calls when the bitmaps were allocated. The -EPERM path shows up with gpio-sim as it is: on a chip with more lines than CONFIG_GPIOLIB_FASTPATH_LIMIT (512 by default; I used 600), GPIO_V2_LINE_SET_VALUES_IOCTL on a line requested as input leaves the two bitmaps to kmemleak.

I was thinking specifically of the gpiochip_set_multiple() error path. How do you elicit a gpiochip_set_multiple() failure from gpio-sim - it always returns 0?

Quoted 3 lines

Unless Bart would rather take it
himself, I'll send the fix as patch 7 of chip-removal v2:

Oh nuts, I overlooked the EPERM case.
Bart has already merged a patch for the gpiod_set_multiple() failure leak[1]
- you were Cced.
I would keep patches in your removal series focused on that and keep patches for existing problems separate so we can get that series merged as soon as possible.

I'll send out a patch for the EPERM path shortly.

I was thinking specifically of the gpiochip_set_multiple() error path. How do you elicit a gpiochip_set_multiple() failure from gpio-sim - it always returns 0?

Oh nuts, I overlooked the EPERM case.
Bart has already merged a patch for the gpiod_set_multiple() failure leak[1]
- you were Cced.

I would keep patches in your removal series focused on that and keep patches for existing problems separate so we can get that series merged as soon as possible.

I'll send out a patch for the EPERM path shortly.

Quoted 7 lines

We can't protect the flags with mutex or spinlock due to the multitude of issues that forced us to use SRCU. We could maybe consider atomic_t for the flags field and that would allow us to use various atomic ops and possibly it would be enough to cover all the read-modify-write cases we have. I need to think about it and will also ask the clanker for help.

I was thinking along the lines of reverting to atomic bitops directly on the flags, e.g. reverting b7adfb6076ff, without changing the underlying type to atomic_t.
Given that most of the time we are operating on individual flags.

With atomic_t you could also use xchg variants for bulk ops?

Quoted 4 lines

Definitely! I don't see why I would ever be against extending the test coverage. :)

Ok, I'll try to give it some thought.

Quoted 8 lines · Kent Gibson

Quoted 7 lines

We can't protect the flags with mutex or spinlock due to the multitude of issues that forced us to use SRCU. We could maybe consider atomic_t for the flags field and that would allow us to use various atomic ops and possibly it would be enough to cover all the read-modify-write cases we have. I need to think about it and will also ask the clanker for help.

Quoted 5 lines

I was thinking along the lines of reverting to atomic bitops directly on the flags, e.g. reverting b7adfb6076ff, without changing the underlying type to atomic_t.
Given that most of the time we are operating on individual flags.

That's still two threads fighting over the flags effectively, right? Ideally we should modify the entire set of flags at once. If that proves to be impossible, going back to atomic bitops is probably the next best thing.

Quoted 2 lines

With atomic_t you could also use xchg variants for bulk ops?

I'm not following, could you rephrase or give an example?

Quoted 7 lines

Quoted 4 lines

Definitely! I don't see why I would ever be against extending the test coverage. :)

Ok, I'll try to give it some thought.

Please also take a look at gpiolib-kunit.c. It may be a better fit for this kind of testing.

Quoted 15 lines · Bartosz Golaszewski

Quoted 8 lines · Kent Gibson

Quoted 7 lines

We can't protect the flags with mutex or spinlock due to the multitude of issues that forced us to use SRCU. We could maybe consider atomic_t for the flags field and that would allow us to use various atomic ops and possibly it would be enough to cover all the read-modify-write cases we have. I need to think about it and will also ask the clanker for help.

Quoted 5 lines

I was thinking along the lines of reverting to atomic bitops directly on the flags, e.g. reverting b7adfb6076ff, without changing the underlying type to atomic_t.
Given that most of the time we are operating on individual flags.

Quoted 4 lines

That's still two threads fighting over the flags effectively, right? Ideally we should modify the entire set of flags at once. If that proves to be impossible, going back to atomic bitops is probably the next best thing.

Yeah, that change wouldn't resolve that conflict - it is just about deciding which framework you plan to use for the flags.
The IRQ flag problem, and any other flag contention lurking, needs to be addressed separately either way.

Quoted 5 lines

Quoted 2 lines

With atomic_t you could also use xchg variants for bulk ops?

I'm not following, could you rephrase or give an example?

I mean atomic_xchg(), atomic_cmpxchg() etc.
I thought atomic_t was intended for counters, not bitmaps of flags, and I don't have any example, but I expect the code to be pretty hideous - You would need to READ_ONCE(), check the flags are in a suitable state, update them, then atomic_try_cmpxchg() with your updated value. Rinse and repeat if the try_cmpxchg fails.

Quoted 32 lines · Kent Gibson

Quoted 15 lines · Bartosz Golaszewski

Quoted 8 lines · Kent Gibson

Quoted 7 lines

We can't protect the flags with mutex or spinlock due to the multitude of issues that forced us to use SRCU. We could maybe consider atomic_t for the flags field and that would allow us to use various atomic ops and possibly it would be enough to cover all the read-modify-write cases we have. I need to think about it and will also ask the clanker for help.

Quoted 5 lines

I was thinking along the lines of reverting to atomic bitops directly on the flags, e.g. reverting b7adfb6076ff, without changing the underlying type to atomic_t.
Given that most of the time we are operating on individual flags.

Quoted 4 lines

That's still two threads fighting over the flags effectively, right? Ideally we should modify the entire set of flags at once. If that proves to be impossible, going back to atomic bitops is probably the next best thing.

Yeah, that change wouldn't resolve that conflict - it is just about deciding which framework you plan to use for the flags.
The IRQ flag problem, and any other flag contention lurking, needs to be addressed separately either way.

Quoted 5 lines

Quoted 2 lines

With atomic_t you could also use xchg variants for bulk ops?

I'm not following, could you rephrase or give an example?

Quoted 7 lines

I mean atomic_xchg(), atomic_cmpxchg() etc.
I thought atomic_t was intended for counters, not bitmaps of flags, and I don't have any example, but I expect the code to be pretty hideous - You would need to READ_ONCE(), check the flags are in a suitable state, update them, then atomic_try_cmpxchg() with your updated value. Rinse and repeat if the try_cmpxchg fails.

Here is an example of an MT-safe gpiochip_lock_as_irq() and gpiochip_unlock_as_irq() that demonstrate what I had in mind:

drivers/gpio/gpiolib.c +24 -11

index 0586fc1f9564..9446cfad17d3 100644
@@ -4223,6 +4223,7 @@ EXPORT_SYMBOL_GPL(gpiod_to_irq);
 int gpiochip_lock_as_irq(struct gpio_chip *gc, unsigned int offset)
 {
        struct gpio_desc *desc;
+       unsigned long flags, new_flags;
 
        desc = gpiochip_get_desc(gc, offset);
        if (IS_ERR(desc))

@@ -4242,17 +4243,25 @@ int gpiochip_lock_as_irq(struct gpio_chip *gc, unsigned int offset)
                }
        }
 
-       /* To be valid for IRQ the line needs to be input or open drain */
-       if (test_bit(GPIOD_FLAG_IS_OUT, &desc->flags) &&
-           !test_bit(GPIOD_FLAG_OPEN_DRAIN, &desc->flags)) {
-               gpiochip_err(gc,
-                            "%s: tried to flag a GPIO set as output for IRQ\n",
-                            __func__);
-               return -EIO;
-       }
+       do {
+               flags = READ_ONCE(desc->flags);
+               /* no WARN_ON here as this is resolving a known potential race */
+               if (test_bit(GPIOD_FLAG_USED_AS_IRQ, &flags))
+                       return -EIO;  /* not sure about that error code */
+
+               /* To be valid for IRQ the line needs to be input or open drain */
+               if (test_bit(GPIOD_FLAG_IS_OUT, &flags) &&
+                   !test_bit(GPIOD_FLAG_OPEN_DRAIN, &flags)) {
+                       gpiochip_err(gc,
+                                    "%s: tried to flag a GPIO set as output for IRQ\n",
+                                    __func__);
+                       return -EIO;
+               }
 
-       set_bit(GPIOD_FLAG_USED_AS_IRQ, &desc->flags);
-       set_bit(GPIOD_FLAG_IRQ_IS_ENABLED, &desc->flags);
+               new_flags = flags;
+               __set_bit(GPIOD_FLAG_USED_AS_IRQ, &new_flags);
+               __set_bit(GPIOD_FLAG_IRQ_IS_ENABLED, &new_flags);
+       } while (!atomic_long_try_cmpxchg((atomic_long_t *)&desc->flags, &flags, new_flags));
 
        return 0;
 }

@@ -4274,8 +4283,12 @@ void gpiochip_unlock_as_irq(struct gpio_chip *gc, unsigned int offset)
        if (IS_ERR(desc))
                return;
 
-       clear_bit(GPIOD_FLAG_USED_AS_IRQ, &desc->flags);
+       /* here you should hold the lock if you are unlocking, so if not WARN_ON and bail */
+       if (WARN_ON(!test_bit(GPIOD_FLAG_USED_AS_IRQ, &desc->flags)))
+                       return;
+       /* USED_AS_IRQ effectively protects IRQ_IS_ENABLED and so must be cleared last */
        clear_bit(GPIOD_FLAG_IRQ_IS_ENABLED, &desc->flags);
+       clear_bit(GPIOD_FLAG_USED_AS_IRQ, &desc->flags);
 }
 EXPORT_SYMBOL_GPL(gpiochip_unlock_as_irq);
 

The xchg approach is used where you need to test multiple flags simultaneously, as per gpiochip_lock_as_irq(), and possibly also where you want to set a bunch at once - e.g. gpiod_free_commit().
But you can still use atomic bitops for individual flag cases, as per gpiochip_unlock_as_irq(), though you need to be mindful of ordering - particularly when releasing flags that act as locks, such as GPIOD_FLAG_USED_AS_IRQ and GPIOD_FLAG_REQUESTED.

These cases are simple as they are only modifying flags. It becomes more complex where you also have to apply config to the driver and rollback the flags if that fails, such as in gpiod_direction_XXX.

Not sure how kosher this is from a kernel perspective, but that is what I was thinking.

Quoted 4 lines · Kent Gibson

Quoted 2 lines · Kent Gibson

Quoted 1 line · Bartosz Golaszewski

On Thu, 15 Oct 2026 17:37:14 +0200, Kent Gibson <[email protected]> said:

Quoted 3 lines

Not sure how kosher this is from a kernel perspective, but that is what I was thinking.

There are some instances of casting an int to atomic_t but given the "aligned" attribute in its type definition, I'm not sure if this is fine either. There are roughly 100 places where desc->flags is dereferenced under drivers/gpio/. All of them would require inspection and rework.

Quoted 5 lines · Bartosz Golaszewski

Quoted 4 lines · Kent Gibson

Quoted 2 lines · Kent Gibson

Quoted 1 line · Bartosz Golaszewski

On Thu, 15 Oct 2026 17:37:14 +0200, Kent Gibson <[email protected]> said:

Quoted 9 lines

Quoted 3 lines

Not sure how kosher this is from a kernel perspective, but that is what I was thinking.

There are some instances of casting an int to atomic_t but given the "aligned" attribute in its type definition, I'm not sure if this is fine either. There are roughly 100 places where desc->flags is dereferenced under drivers/gpio/. All of them would require inspection and rework.

I did try changing the flags to atomic_long_t, but that made all the existing atomic bitops complain about the type. :(.
Could we add an alignment attribute to the desc flags to match the atcomic_t so the cast will be robust?

Alternatively if bitops could be made to work seemlessly with atomic_t...

When a line is requested or reconfigured through the v2 uAPI, gpio_v2_line_config_flags_to_desc_flags() copies the requested configuration into the descriptor flags, GPIOD_FLAG_IS_OUT included, before gpiod_direction_input_nonotify() or gpiod_direction_output_nonotify() asks the driver for the direction. If the driver refuses, for instance with -EOPNOTSUPP from .direction_input() for a line that can only be an output, the ioctl fails but the flag keeps the refused direction. gpiod_free_commit() does not touch GPIOD_FLAG_IS_OUT, so this outlives the failed request, and GPIO_V2_GET_LINEINFO_IOCTL reports the line as an input while the hardware still drives it, until a request without direction flags makes gpiod_request_commit() read the direction back from the driver:

  GPIO_V2_GET_LINE_IOCTL, line 0, input:         -EOPNOTSUPP
  GPIO_V2_GET_LINEINFO_IOCTL, line 0:            input
  GPIO_V2_GET_LINE_IOCTL, line 0, no direction:  ok
  GPIO_V2_GET_LINEINFO_IOCTL, line 0:            output

GPIO_V2_LINE_SET_CONFIG_IOCTL updates the flags in the same order. There the line stays requested, and since commit 92ac7de3175e ("gpiolib: don't allow setting values on input lines") GPIO_V2_LINE_SET_VALUES_IOCTL on it then fails with -EPERM, though the line is still an output. A line whose driver refuses to make it an output is likewise reported as one.

gpiod_direction_input_nonotify() and gpiod_direction_output_nonotify() set or clear GPIOD_FLAG_IS_OUT themselves once the driver has accepted the direction, emulated open drain and open source included, and nothing between the copy and those calls reads the flag. Leave it to them, as the v1 uAPI already does.

The other flags that the helper copies stay as they are. Active low, drive and bias are what gpiod_direction_*() applies, drive and bias possibly before the driver refuses the direction; edges and event clock are the character device's own. None of them is read back from the driver, so there is no earlier state that is more accurate to go back to. A failed request releases the line, which clears them, and a failed reconfiguration leaves the line, like the lines before it, with what was applied up to the failure.

Reproduced under QEMU with a test GPIO chip whose line 0 refuses input and line 1 refuses output: both refused requests and both refused reconfigurations leave the line info with the refused direction, and setting the value of the reconfigured output fails with -EPERM. With this change the line info shows the direction of the hardware in all four cases and setting the value works. First seen on real hardware with v6.12.

Fixes 3c0d9c635ae2 ("gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL")

Cc [email protected]

Signed-off-by Adrian Ambrozewicz <[email protected]>

---
 drivers/gpio/gpiolib-cdev.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

drivers/gpio/gpiolib-cdev.c +0 -0

index 5d53bfcdf726..9bbee3a1f9c9 100644

I would change the subject to "don't set GPIOD_FLAG_IS_OUT" as cdev doesn't interact directly with the driver - gpiolib does, and the change here is unconditional.

Not a fan of comments describing why code that should not be here is not here.

Having gpiolib manage GPIOD_FLAG_IS_OUT to keep it consistent with the driver state makes sense, so no problem with the code change itself.