Re: [PATCH] gpiolib: cdev: don't record a line direction the driver refused

[Kent Gibson <warthog618@gmail.com>]

On Fri, Oct 16, 2026 at 05:14:20PM +0800, Kent Gibson wrote:
> On Fri, Oct 16, 2026 at 03:23:24AM -0500, Bartosz Golaszewski wrote:
> > On Thu, 15 Oct 2026 17:37:14 +0200, Kent Gibson <warthog618@gmail.com> said:
> > >
> > >> 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.
> > >
> > 
> > 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.
> 
> > > 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.
> 

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 0586fc1f9564..9446cfad17d3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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.

Cheers,
Kent.