GPIOLIB locking is broken and how to fix it

GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski]

Hi!

I've been scratching my head over it for a couple days and I wanted to
pick your brains a bit.

The existing locking in GPIOLIB is utterly broken. We have a global
spinlock that "protects" the list of GPIO devices but also the
descriptor objects (and who knows what else). I put "protects" in
quotation marks because the spinlock is released and re-acquired in
several places where the code needs to call functions that can
possibly sleep. I don't have to tell you it makes the spinlock useless
and doesn't protect anything.

An example of that is gpiod_request_commit() where in the time between
releasing the lock in order to call gc->request() and acquiring it
again, gpiod_free_commit() can be called, thus undoing a part of the
changes we just introduced in the first part of this function. We'd
then return from gc->request() and continue acting like we've just
requested the GPIO leading to undefined behavior.

There are more instances of this pattern. This seems to be a way to
work around the fact that we have GPIO API functions that can be
called from atomic context (gpiod_set/get_value(),
gpiod_direction_input/output(), etc.) that in their implementation
call driver callbacks that may as well sleep (gc->set(),
gc->direction_output(), etc.).

Protecting the list of GPIO devices is simple. It should be a mutex as
the list should never be modified from atomic context. This can be
easily factored out right now. Protecting GPIO descriptors is
trickier. If we use a spinlock for that, we'll run into problems with
GPIO drivers that can sleep. If we use a mutex, we'll have a problem
with users calling GPIO functions from atomic context.

One idea I have is introducing a strict limit on which functions can
be used from atomic context (we don't enforce anything ATM in
functions that don't have the _cansleep suffix in their names) and
check which parts of the descriptor struct they modify. Then protect
these parts with a spinlock in very limited critical sections. Have a
mutex for everything else that can only be accessed from process
context.

Another one is introducing strict APIs like gpiod_set_value_atomic()
that'll be designed to be called from atomic context exclusively and
be able to handle it. Everything else must only be called from process
context. This of course would be a treewide change as we'd need to
modify all GPIO calls in interrupt handlers.

I'd like to hear your ideas as this change is vital before we start
protecting gdev->chip with SRCU in all API calls.

Bart

Re: GPIOLIB locking is broken and how to fix it

[Andy Shevchenko]

On Fri, Nov 24, 2023 at 05:00:36PM +0100, Bartosz Golaszewski wrote:
> Hi!
> 
> I've been scratching my head over it for a couple days and I wanted to
> pick your brains a bit.
> 
> The existing locking in GPIOLIB is utterly broken. We have a global
> spinlock that "protects" the list of GPIO devices but also the
> descriptor objects (and who knows what else). I put "protects" in
> quotation marks because the spinlock is released and re-acquired in
> several places where the code needs to call functions that can
> possibly sleep. I don't have to tell you it makes the spinlock useless
> and doesn't protect anything.
> 
> An example of that is gpiod_request_commit() where in the time between
> releasing the lock in order to call gc->request() and acquiring it
> again, gpiod_free_commit() can be called, thus undoing a part of the
> changes we just introduced in the first part of this function. We'd
> then return from gc->request() and continue acting like we've just
> requested the GPIO leading to undefined behavior.
> 
> There are more instances of this pattern. This seems to be a way to
> work around the fact that we have GPIO API functions that can be
> called from atomic context (gpiod_set/get_value(),
> gpiod_direction_input/output(), etc.) that in their implementation
> call driver callbacks that may as well sleep (gc->set(),
> gc->direction_output(), etc.).
> 
> Protecting the list of GPIO devices is simple. It should be a mutex as
> the list should never be modified from atomic context. This can be
> easily factored out right now. Protecting GPIO descriptors is
> trickier. If we use a spinlock for that, we'll run into problems with
> GPIO drivers that can sleep. If we use a mutex, we'll have a problem
> with users calling GPIO functions from atomic context.
> 
> One idea I have is introducing a strict limit on which functions can
> be used from atomic context (we don't enforce anything ATM in
> functions that don't have the _cansleep suffix in their names) and
> check which parts of the descriptor struct they modify. Then protect
> these parts with a spinlock in very limited critical sections. Have a
> mutex for everything else that can only be accessed from process
> context.
> 
> Another one is introducing strict APIs like gpiod_set_value_atomic()
> that'll be designed to be called from atomic context exclusively and
> be able to handle it. Everything else must only be called from process
> context. This of course would be a treewide change as we'd need to
> modify all GPIO calls in interrupt handlers.
> 
> I'd like to hear your ideas as this change is vital before we start
> protecting gdev->chip with SRCU in all API calls.

Brief side note: If we can really fix something (partially) right now,
do it, otherwise technical debt kills us.

(Most likely I refer to the list of the GPIO devices.)

-- 
With Best Regards,
Andy Shevchenko

Re: GPIOLIB locking is broken and how to fix it

[Andy Shevchenko]

On Fri, Nov 24, 2023 at 07:27:54PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 24, 2023 at 05:00:36PM +0100, Bartosz Golaszewski wrote:
> > Hi!
> > 
> > I've been scratching my head over it for a couple days and I wanted to
> > pick your brains a bit.
> > 
> > The existing locking in GPIOLIB is utterly broken. We have a global
> > spinlock that "protects" the list of GPIO devices but also the
> > descriptor objects (and who knows what else). I put "protects" in
> > quotation marks because the spinlock is released and re-acquired in
> > several places where the code needs to call functions that can
> > possibly sleep. I don't have to tell you it makes the spinlock useless
> > and doesn't protect anything.
> > 
> > An example of that is gpiod_request_commit() where in the time between
> > releasing the lock in order to call gc->request() and acquiring it
> > again, gpiod_free_commit() can be called, thus undoing a part of the
> > changes we just introduced in the first part of this function. We'd
> > then return from gc->request() and continue acting like we've just
> > requested the GPIO leading to undefined behavior.
> > 
> > There are more instances of this pattern. This seems to be a way to
> > work around the fact that we have GPIO API functions that can be
> > called from atomic context (gpiod_set/get_value(),
> > gpiod_direction_input/output(), etc.) that in their implementation
> > call driver callbacks that may as well sleep (gc->set(),
> > gc->direction_output(), etc.).
> > 
> > Protecting the list of GPIO devices is simple. It should be a mutex as
> > the list should never be modified from atomic context. This can be
> > easily factored out right now. Protecting GPIO descriptors is
> > trickier. If we use a spinlock for that, we'll run into problems with
> > GPIO drivers that can sleep. If we use a mutex, we'll have a problem
> > with users calling GPIO functions from atomic context.
> > 
> > One idea I have is introducing a strict limit on which functions can
> > be used from atomic context (we don't enforce anything ATM in
> > functions that don't have the _cansleep suffix in their names) and
> > check which parts of the descriptor struct they modify. Then protect
> > these parts with a spinlock in very limited critical sections. Have a
> > mutex for everything else that can only be accessed from process
> > context.
> > 
> > Another one is introducing strict APIs like gpiod_set_value_atomic()
> > that'll be designed to be called from atomic context exclusively and
> > be able to handle it. Everything else must only be called from process
> > context. This of course would be a treewide change as we'd need to
> > modify all GPIO calls in interrupt handlers.
> > 
> > I'd like to hear your ideas as this change is vital before we start
> > protecting gdev->chip with SRCU in all API calls.
> 
> Brief side note: If we can really fix something (partially) right now,
> do it, otherwise technical debt kills us.
> 
> (Most likely I refer to the list of the GPIO devices.)

Another brief note is to look at irq_get_desc_lock() / irq_put_desc_unlock()
and related APIs. Yet you might probably not need all the complications and
esp. raw spinlock, but perhaps you can get the idea.

-- 
With Best Regards,
Andy Shevchenko

Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski]

On Fri, Nov 24, 2023 at 6:28 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Nov 24, 2023 at 05:00:36PM +0100, Bartosz Golaszewski wrote:
> > Hi!
> >

[snip]

> >
> > I'd like to hear your ideas as this change is vital before we start
> > protecting gdev->chip with SRCU in all API calls.
>
> Brief side note: If we can really fix something (partially) right now,
> do it, otherwise technical debt kills us.
>
> (Most likely I refer to the list of the GPIO devices.)

I will do it but I wanted to already start the discussion on the wider picture.

Bart

>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>

Re: GPIOLIB locking is broken and how to fix it

[Linus Walleij]

On Fri, Nov 24, 2023 at 5:00 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> There are more instances of this pattern. This seems to be a way to
> work around the fact that we have GPIO API functions that can be
> called from atomic context (gpiod_set/get_value(),
> gpiod_direction_input/output(), etc.) that in their implementation
> call driver callbacks that may as well sleep (gc->set(),
> gc->direction_output(), etc.).

Correct, AFAIK this is why this looks like it does.

> Protecting the list of GPIO devices is simple. It should be a mutex as
> the list should never be modified from atomic context.

If you are referring to gpio_lock then go back and look how that
got where it is today:

git checkout d2876d08d86f2 (initial gpiolib commit 2008)

The ultimately confusing thing is that:

1. Yes it is protecting the list of gpio chips
2. The struct holding items in the list of the gpio chips is
  called gpio_desc...

Then this comment:

/* gpio_lock prevents conflicts during gpio_desc[] table updates.
 * While any GPIO is requested, its gpio_chip is not removable;
 * each GPIO's "requested" flag serves as a lock and refcount.
 */

Is cargo-culted to present day and is talking about gpio_desc, but
nowadays gpio_desc means something completely different...

OK I'll send a patch just deleting this comment, it looks insane.

> This can be easily factored out right now.

JustDoIt :)

> Protecting GPIO descriptors is
> trickier. If we use a spinlock for that, we'll run into problems with
> GPIO drivers that can sleep. If we use a mutex, we'll have a problem
> with users calling GPIO functions from atomic context.
>
> One idea I have is introducing a strict limit on which functions can
> be used from atomic context (we don't enforce anything ATM in
> functions that don't have the _cansleep suffix in their names) and
> check which parts of the descriptor struct they modify. Then protect
> these parts with a spinlock in very limited critical sections. Have a
> mutex for everything else that can only be accessed from process
> context.
>
> Another one is introducing strict APIs like gpiod_set_value_atomic()
> that'll be designed to be called from atomic context exclusively and
> be able to handle it. Everything else must only be called from process
> context. This of course would be a treewide change as we'd need to
> modify all GPIO calls in interrupt handlers.

This is a much harder problem.

Many of the current API functions can be called from atomic and
nonatomic contexts alike :/ this has historical reasons of course.
Back in 2008 most GPIO chips were just on-SoC and resulted in
a register write: no problem. Now we have quite a bunch of GPIOs
on I2C, SPI ... and the API looks the same.

The _cansleep functions were supposed to be used explicitly in
places where it is OK that the GPIO can sleep (as in: I don't care
if you sleep or not), and every other site using the non-_cansleep
versions should be where it has to be atomic.

Every call where non-_cansleep is called but it doesn't matter is
essentially a bug, they should be using _cansleep versions.
(Oh boy ... such much bug.)

In 2008 when it was introduced, *_cansleep had one single user:
drivers/leds/led-gpio.c because it was assumed that users of
that driver would not care if LEDs are on GPIO expanders or
on SoC-resident GPIOs. Ironically that driver now keeps track
of whether the GPIO is sleepable or not...

So if you propose turning this on it's head by creating *_atomic
and opt-in to atomic behaviour instead of opting out of it, I'd say
yes, but only if we delete all uses of _cansleep at the same time
and that means the default behaviour becomes _cansleep.

Keeping both around at the same time is going to be
a complete mess.

Yours,
Linus Walleij

Re: GPIOLIB locking is broken and how to fix it

[Kent Gibson]

On Fri, Nov 24, 2023 at 05:00:36PM +0100, Bartosz Golaszewski wrote:
> Hi!
>
> I've been scratching my head over it for a couple days and I wanted to
> pick your brains a bit.
>
> The existing locking in GPIOLIB is utterly broken. We have a global
> spinlock that "protects" the list of GPIO devices but also the
> descriptor objects (and who knows what else). I put "protects" in
> quotation marks because the spinlock is released and re-acquired in
> several places where the code needs to call functions that can
> possibly sleep. I don't have to tell you it makes the spinlock useless
> and doesn't protect anything.
>

You have to tell me, cos I don't think it makes it useless, it just
constrains the contexts in which it is providing protection. No lock
provides protection when it isn't locked - the problem here is the
assumption that it does.

Depending on the scope of the locking, and what else it may block,
releasing and re-aquiring may be the correct strategy - just don't
assume state across the unlock.

If there is no lock held during driver callbacks, then they, and
anything they call, need to be aware that things can change, and GPIOLIB
needs to ensure that nothing the driver is accessing is freed until the
callbacks return.

> An example of that is gpiod_request_commit() where in the time between
> releasing the lock in order to call gc->request() and acquiring it
> again, gpiod_free_commit() can be called, thus undoing a part of the
> changes we just introduced in the first part of this function. We'd
> then return from gc->request() and continue acting like we've just
> requested the GPIO leading to undefined behavior.
>

So GPIOLIB needs to check for that after re-acquiring the lock.

> There are more instances of this pattern. This seems to be a way to
> work around the fact that we have GPIO API functions that can be
> called from atomic context (gpiod_set/get_value(),
> gpiod_direction_input/output(), etc.) that in their implementation
> call driver callbacks that may as well sleep (gc->set(),
> gc->direction_output(), etc.).
>

Which is fine - as long as GPIOLIB is aware that things can change while
it doesn't hold the lock.  And as long as it doesn't go freeing objects
still in use.

> Protecting the list of GPIO devices is simple. It should be a mutex as
> the list should never be modified from atomic context. This can be
> easily factored out right now.


> Protecting GPIO descriptors is
> trickier. If we use a spinlock for that, we'll run into problems with
> GPIO drivers that can sleep. If we use a mutex, we'll have a problem
> with users calling GPIO functions from atomic context.
>

Generally I would advocate for not holding locks over callbacks.
If the callback requires locks then it (or the code it calls) should
request them itself.
GPIOLIB just needs to ensure the desc isn't freed during the callback.

The contract for the driver callbacks is not clear to me. e.g. can they
be called concurrently (e.g. different cdev requests trying to set
different lines and so both calling gc->set())
If so, are the drivers aware of that?
If not, a mutex over the callbacks would seem very appropriate to
enforce that contract, independent of protecting the desc.
(though ideally the drivers are be aware that they need to provide
their own locking as appropriate)

> One idea I have is introducing a strict limit on which functions can
> be used from atomic context (we don't enforce anything ATM in
> functions that don't have the _cansleep suffix in their names) and
> check which parts of the descriptor struct they modify. Then protect
> these parts with a spinlock in very limited critical sections. Have a
> mutex for everything else that can only be accessed from process
> context.
>

... and needs to call cansleep within the critical section itself.

> Another one is introducing strict APIs like gpiod_set_value_atomic()
> that'll be designed to be called from atomic context exclusively and
> be able to handle it. Everything else must only be called from process
> context. This of course would be a treewide change as we'd need to
> modify all GPIO calls in interrupt handlers.
>
> I'd like to hear your ideas as this change is vital before we start
> protecting gdev->chip with SRCU in all API calls.
>

As above, I'm not clear on the driver callback contract, so there is a
good chance anything more specific I have to add would be incoherent.

I do have concerns that adding or changing locking could inadvertantly
constrain otherwise valid concurrency, but safe is better than fast.
But safe AND fast is ideal.
Sorry if that is obvious and not much help.

Cheers,
Kent.

Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski]

On Sat, Nov 25, 2023 at 2:29 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Nov 24, 2023 at 05:00:36PM +0100, Bartosz Golaszewski wrote:
> > Hi!
> >
> > I've been scratching my head over it for a couple days and I wanted to
> > pick your brains a bit.
> >
> > The existing locking in GPIOLIB is utterly broken. We have a global
> > spinlock that "protects" the list of GPIO devices but also the
> > descriptor objects (and who knows what else). I put "protects" in
> > quotation marks because the spinlock is released and re-acquired in
> > several places where the code needs to call functions that can
> > possibly sleep. I don't have to tell you it makes the spinlock useless
> > and doesn't protect anything.
> >
>
> You have to tell me, cos I don't think it makes it useless, it just
> constrains the contexts in which it is providing protection. No lock
> provides protection when it isn't locked - the problem here is the
> assumption that it does.
>
> Depending on the scope of the locking, and what else it may block,
> releasing and re-aquiring may be the correct strategy - just don't
> assume state across the unlock.
>
> If there is no lock held during driver callbacks, then they, and
> anything they call, need to be aware that things can change, and GPIOLIB
> needs to ensure that nothing the driver is accessing is freed until the
> callbacks return.
>
> > An example of that is gpiod_request_commit() where in the time between
> > releasing the lock in order to call gc->request() and acquiring it
> > again, gpiod_free_commit() can be called, thus undoing a part of the
> > changes we just introduced in the first part of this function. We'd
> > then return from gc->request() and continue acting like we've just
> > requested the GPIO leading to undefined behavior.
> >
>
> So GPIOLIB needs to check for that after re-acquiring the lock.
>
> > There are more instances of this pattern. This seems to be a way to
> > work around the fact that we have GPIO API functions that can be
> > called from atomic context (gpiod_set/get_value(),
> > gpiod_direction_input/output(), etc.) that in their implementation
> > call driver callbacks that may as well sleep (gc->set(),
> > gc->direction_output(), etc.).
> >
>
> Which is fine - as long as GPIOLIB is aware that things can change while
> it doesn't hold the lock.  And as long as it doesn't go freeing objects
> still in use.
>
> > Protecting the list of GPIO devices is simple. It should be a mutex as
> > the list should never be modified from atomic context. This can be
> > easily factored out right now.
>
>
> > Protecting GPIO descriptors is
> > trickier. If we use a spinlock for that, we'll run into problems with
> > GPIO drivers that can sleep. If we use a mutex, we'll have a problem
> > with users calling GPIO functions from atomic context.
> >
>
> Generally I would advocate for not holding locks over callbacks.
> If the callback requires locks then it (or the code it calls) should
> request them itself.
> GPIOLIB just needs to ensure the desc isn't freed during the callback.
>

That is quite tricky though, isn't it? Let's consider our request/free
example. Even if we start gpiod_request_commit() by calling
gc->request() and then acquire the gpio_desc lock and modify the
descriptor, we may have a concurrent gpiod_free_commit() that will do
the same in reverse order. Even if the driver in question uses some
locking, we'll have time between when it releases the lock and when we
acquire the desc lock (or vice-versa for free). We can now end up in a
situation when we call gc->request(), return from it,
gpiod_free_commit() is called, acquires desc lock before
gpiod_request_commit() marking the descriptor as freed, we then
acquire the lock in gpiod_request_commit() and start marking the
descriptor as requested while gc->free() is called. We cannot know the
driver-specific state even if we wanted to re-check it within the
critical section.

This particular example could be a non-issue if we only dealt out
descriptors for exclusive usage to consumers but unfortunately we
don't. We currently have lots of places in drivers/gpio/ and - oh
horror! - outside where users just shamelessly access the descriptor
array in a GPIO device and fiddle with its internals so this may
happen. That's another problem. But as we read and/or modify the
descriptors in almost all GPIO callbacks, this isn't the only use-case
that can lead to undefined behavior.

I'd say that driver-specific locking should be used whenever the
driver resources are shared between different subsystems. Some drivers
expose a GPIO chip but also an I2C expander or a PWM chip and they all
share resources. Then we also have power-management callbacks and
whatnot.

For GPIOLIB callbacks exclusively, I think we should have a hard
contract for concurrency. Which brings me neatly to...

> The contract for the driver callbacks is not clear to me. e.g. can they
> be called concurrently (e.g. different cdev requests trying to set
> different lines and so both calling gc->set())

AFAIK there is no contract. None is documented and - with GPIOLIB's
current state - no implicit contract is enforced either.

> If so, are the drivers aware of that?
> If not, a mutex over the callbacks would seem very appropriate to
> enforce that contract, independent of protecting the desc.

Not a mutex. Not exclusively anyway. I suppose we could go with an
abstraction layer like what regmap does. If the chip is marked as
can_sleep then we use mutex, if not, we use spinlock. Possibly have a
flag allowing users to disable locking if they are certain concurrency
is not an issue.

> (though ideally the drivers are be aware that they need to provide
> their own locking as appropriate)
>

We should put a contract in place saying: if your driver is certain to
not share any resources with any other subsystem: don't use your own
locking. Otherwise, protect only the shared structures.

> > One idea I have is introducing a strict limit on which functions can
> > be used from atomic context (we don't enforce anything ATM in
> > functions that don't have the _cansleep suffix in their names) and
> > check which parts of the descriptor struct they modify. Then protect
> > these parts with a spinlock in very limited critical sections. Have a
> > mutex for everything else that can only be accessed from process
> > context.
> >
>
> ... and needs to call cansleep within the critical section itself.
>
> > Another one is introducing strict APIs like gpiod_set_value_atomic()
> > that'll be designed to be called from atomic context exclusively and
> > be able to handle it. Everything else must only be called from process
> > context. This of course would be a treewide change as we'd need to
> > modify all GPIO calls in interrupt handlers.
> >
> > I'd like to hear your ideas as this change is vital before we start
> > protecting gdev->chip with SRCU in all API calls.
> >
>
> As above, I'm not clear on the driver callback contract, so there is a
> good chance anything more specific I have to add would be incoherent.
>
> I do have concerns that adding or changing locking could inadvertantly
> constrain otherwise valid concurrency, but safe is better than fast.
> But safe AND fast is ideal.

This is why my ultimate goal is to protect the gpio_chip pointer in
gpio_device against sudden disappearance of the provider with SRCU -
possibly the fastest sleepable locking mechanism we have in the
kernel.

SRCU could also be used to check if a desc is requested before
entering the proper API call in the ideal world where we don't allow
using non-requested descriptors making it possible to avoid real locks
in struct gpio_desc.

Bart

> Sorry if that is obvious and not much help.
>

Re: GPIOLIB locking is broken and how to fix it

[Kent Gibson]

On Sat, Nov 25, 2023 at 09:13:22PM +0100, Bartosz Golaszewski wrote:
> On Sat, Nov 25, 2023 at 2:29 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Fri, Nov 24, 2023 at 05:00:36PM +0100, Bartosz Golaszewski wrote:
> > > Hi!
> > >

> >
> > Generally I would advocate for not holding locks over callbacks.
> > If the callback requires locks then it (or the code it calls) should
> > request them itself.
> > GPIOLIB just needs to ensure the desc isn't freed during the callback.
> >
>
> That is quite tricky though, isn't it?

Well it is kernel code, so yeah.
You were expecting it to be easy ;-)?

> Let's consider our request/free
> example. Even if we start gpiod_request_commit() by calling
> gc->request() and then acquire the gpio_desc lock and modify the
> descriptor, we may have a concurrent gpiod_free_commit() that will do
> the same in reverse order. Even if the driver in question uses some
> locking, we'll have time between when it releases the lock and when we
> acquire the desc lock (or vice-versa for free). We can now end up in a
> situation when we call gc->request(), return from it,
> gpiod_free_commit() is called, acquires desc lock before
> gpiod_request_commit() marking the descriptor as freed, we then
> acquire the lock in gpiod_request_commit() and start marking the
> descriptor as requested while gc->free() is called. We cannot know the
> driver-specific state even if we wanted to re-check it within the
> critical section.
>

So you use transitional states - not just requested/freed.
Before the gc->request() call gpiod_request_commit() marks the desc as
being requested (while it holds the appropriate lock of course).
If the gpiod_free_commit() comes along and sees that mark, again while
holdiing the lock, then it marks the desc as to be freed and leaves it
for gpiod_request_commit() to cleanup.
When gpiod_request_commit() re-acquires the lock after the gc->request()
call it sees the to be freed mark and performs the cleanup instead of the
commit finalisation.

I'm generalising here and not looking at code, so the specifics may be
wrong, but this is a solvable problem.

> This particular example could be a non-issue if we only dealt out
> descriptors for exclusive usage to consumers but unfortunately we
> don't. We currently have lots of places in drivers/gpio/ and - oh
> horror! - outside where users just shamelessly access the descriptor
> array in a GPIO device and fiddle with its internals so this may
> happen. That's another problem. But as we read and/or modify the
> descriptors in almost all GPIO callbacks, this isn't the only use-case
> that can lead to undefined behavior.
>

That is a very different problem - another breach of contract, or lack
of contract. And a problem that can't be solved by changing the locks.

> I'd say that driver-specific locking should be used whenever the
> driver resources are shared between different subsystems. Some drivers
> expose a GPIO chip but also an I2C expander or a PWM chip and they all
> share resources. Then we also have power-management callbacks and
> whatnot.
>
> For GPIOLIB callbacks exclusively, I think we should have a hard
> contract for concurrency. Which brings me neatly to...
>
> > The contract for the driver callbacks is not clear to me. e.g. can they
> > be called concurrently (e.g. different cdev requests trying to set
> > different lines and so both calling gc->set())
>
> AFAIK there is no contract. None is documented and - with GPIOLIB's
> current state - no implicit contract is enforced either.
>

And that is the actual problem here - the lack of contracts.

> > If so, are the drivers aware of that?
> > If not, a mutex over the callbacks would seem very appropriate to
> > enforce that contract, independent of protecting the desc.
>
> Not a mutex. Not exclusively anyway. I suppose we could go with an
> abstraction layer like what regmap does. If the chip is marked as
> can_sleep then we use mutex, if not, we use spinlock. Possibly have a
> flag allowing users to disable locking if they are certain concurrency
> is not an issue.
>
> > (though ideally the drivers are be aware that they need to provide
> > their own locking as appropriate)
> >
>
> We should put a contract in place saying: if your driver is certain to
> not share any resources with any other subsystem: don't use your own
> locking. Otherwise, protect only the shared structures.
>

This is why I advocate for not holding any locks during the callbacks -
then there are no restrictions imposed on the driver and no way it can
inadvertantly deadlock.

> > > One idea I have is introducing a strict limit on which functions can
> > > be used from atomic context (we don't enforce anything ATM in
> > > functions that don't have the _cansleep suffix in their names) and
> > > check which parts of the descriptor struct they modify. Then protect
> > > these parts with a spinlock in very limited critical sections. Have a
> > > mutex for everything else that can only be accessed from process
> > > context.
> > >
> >
> > ... and needs to call cansleep within the critical section itself.
> >
> > > Another one is introducing strict APIs like gpiod_set_value_atomic()
> > > that'll be designed to be called from atomic context exclusively and
> > > be able to handle it. Everything else must only be called from process
> > > context. This of course would be a treewide change as we'd need to
> > > modify all GPIO calls in interrupt handlers.
> > >
> > > I'd like to hear your ideas as this change is vital before we start
> > > protecting gdev->chip with SRCU in all API calls.
> > >
> >
> > As above, I'm not clear on the driver callback contract, so there is a
> > good chance anything more specific I have to add would be incoherent.
> >
> > I do have concerns that adding or changing locking could inadvertantly
> > constrain otherwise valid concurrency, but safe is better than fast.
> > But safe AND fast is ideal.
>
> This is why my ultimate goal is to protect the gpio_chip pointer in
> gpio_device against sudden disappearance of the provider with SRCU -
> possibly the fastest sleepable locking mechanism we have in the
> kernel.
>
> SRCU could also be used to check if a desc is requested before
> entering the proper API call in the ideal world where we don't allow
> using non-requested descriptors making it possible to avoid real locks
> in struct gpio_desc.
>

So why allow the usage of non-requested descriptors?

Cheers,
Kent.

Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski]

On Sun, Nov 26, 2023 at 1:06 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Sat, Nov 25, 2023 at 09:13:22PM +0100, Bartosz Golaszewski wrote:
> > On Sat, Nov 25, 2023 at 2:29 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Fri, Nov 24, 2023 at 05:00:36PM +0100, Bartosz Golaszewski wrote:
> > > > Hi!
> > > >
>
> > >
> > > Generally I would advocate for not holding locks over callbacks.
> > > If the callback requires locks then it (or the code it calls) should
> > > request them itself.
> > > GPIOLIB just needs to ensure the desc isn't freed during the callback.
> > >
> >
> > That is quite tricky though, isn't it?
>
> Well it is kernel code, so yeah.
> You were expecting it to be easy ;-)?
>
> > Let's consider our request/free
> > example. Even if we start gpiod_request_commit() by calling
> > gc->request() and then acquire the gpio_desc lock and modify the
> > descriptor, we may have a concurrent gpiod_free_commit() that will do
> > the same in reverse order. Even if the driver in question uses some
> > locking, we'll have time between when it releases the lock and when we
> > acquire the desc lock (or vice-versa for free). We can now end up in a
> > situation when we call gc->request(), return from it,
> > gpiod_free_commit() is called, acquires desc lock before
> > gpiod_request_commit() marking the descriptor as freed, we then
> > acquire the lock in gpiod_request_commit() and start marking the
> > descriptor as requested while gc->free() is called. We cannot know the
> > driver-specific state even if we wanted to re-check it within the
> > critical section.
> >
>
> So you use transitional states - not just requested/freed.
> Before the gc->request() call gpiod_request_commit() marks the desc as
> being requested (while it holds the appropriate lock of course).
> If the gpiod_free_commit() comes along and sees that mark, again while
> holdiing the lock, then it marks the desc as to be freed and leaves it
> for gpiod_request_commit() to cleanup.
> When gpiod_request_commit() re-acquires the lock after the gc->request()
> call it sees the to be freed mark and performs the cleanup instead of the
> commit finalisation.
>
> I'm generalising here and not looking at code, so the specifics may be
> wrong, but this is a solvable problem.
>
> > This particular example could be a non-issue if we only dealt out
> > descriptors for exclusive usage to consumers but unfortunately we
> > don't. We currently have lots of places in drivers/gpio/ and - oh
> > horror! - outside where users just shamelessly access the descriptor
> > array in a GPIO device and fiddle with its internals so this may
> > happen. That's another problem. But as we read and/or modify the
> > descriptors in almost all GPIO callbacks, this isn't the only use-case
> > that can lead to undefined behavior.
> >
>
> That is a very different problem - another breach of contract, or lack
> of contract. And a problem that can't be solved by changing the locks.
>
> > I'd say that driver-specific locking should be used whenever the
> > driver resources are shared between different subsystems. Some drivers
> > expose a GPIO chip but also an I2C expander or a PWM chip and they all
> > share resources. Then we also have power-management callbacks and
> > whatnot.
> >
> > For GPIOLIB callbacks exclusively, I think we should have a hard
> > contract for concurrency. Which brings me neatly to...
> >
> > > The contract for the driver callbacks is not clear to me. e.g. can they
> > > be called concurrently (e.g. different cdev requests trying to set
> > > different lines and so both calling gc->set())
> >
> > AFAIK there is no contract. None is documented and - with GPIOLIB's
> > current state - no implicit contract is enforced either.
> >
>
> And that is the actual problem here - the lack of contracts.
>
> > > If so, are the drivers aware of that?
> > > If not, a mutex over the callbacks would seem very appropriate to
> > > enforce that contract, independent of protecting the desc.
> >
> > Not a mutex. Not exclusively anyway. I suppose we could go with an
> > abstraction layer like what regmap does. If the chip is marked as
> > can_sleep then we use mutex, if not, we use spinlock. Possibly have a
> > flag allowing users to disable locking if they are certain concurrency
> > is not an issue.
> >
> > > (though ideally the drivers are be aware that they need to provide
> > > their own locking as appropriate)
> > >
> >
> > We should put a contract in place saying: if your driver is certain to
> > not share any resources with any other subsystem: don't use your own
> > locking. Otherwise, protect only the shared structures.
> >
>
> This is why I advocate for not holding any locks during the callbacks -
> then there are no restrictions imposed on the driver and no way it can
> inadvertantly deadlock.
>

I don't think it's even possible if we want correctness. For one: if
we want full hot-unplugability, we'll need to keep some lock around
gdev->chip. This is where I think SRCU will work fine. But the GPIOLIB
API functions will often read and/or modify the gpio_desc structure in
addition to calling the relevant GPIOLIB callback and often based on
its return value. I'm not sure it's possible to implement anything
lockless here. We could possibly allow providers who are *absolutely*
sure they don't need locking to disable it when registering the GPIO
chip but by default we should provide some and put it in the future
contract. This way GPIO chips that don't share their GPIO chip
structures, will be able to drop locking.

I'd like to implement a set of gpiod_lock() and gpiod_unlock()
functions which will abstract the locking. We'd then have a union of
struct mutex and spinlock_t and use the right one depending on the
value of can_sleep of the parent chip.

> > > > One idea I have is introducing a strict limit on which functions can
> > > > be used from atomic context (we don't enforce anything ATM in
> > > > functions that don't have the _cansleep suffix in their names) and
> > > > check which parts of the descriptor struct they modify. Then protect
> > > > these parts with a spinlock in very limited critical sections. Have a
> > > > mutex for everything else that can only be accessed from process
> > > > context.
> > > >
> > >
> > > ... and needs to call cansleep within the critical section itself.
> > >
> > > > Another one is introducing strict APIs like gpiod_set_value_atomic()
> > > > that'll be designed to be called from atomic context exclusively and
> > > > be able to handle it. Everything else must only be called from process
> > > > context. This of course would be a treewide change as we'd need to
> > > > modify all GPIO calls in interrupt handlers.
> > > >
> > > > I'd like to hear your ideas as this change is vital before we start
> > > > protecting gdev->chip with SRCU in all API calls.
> > > >
> > >
> > > As above, I'm not clear on the driver callback contract, so there is a
> > > good chance anything more specific I have to add would be incoherent.
> > >
> > > I do have concerns that adding or changing locking could inadvertantly
> > > constrain otherwise valid concurrency, but safe is better than fast.
> > > But safe AND fast is ideal.
> >
> > This is why my ultimate goal is to protect the gpio_chip pointer in
> > gpio_device against sudden disappearance of the provider with SRCU -
> > possibly the fastest sleepable locking mechanism we have in the
> > kernel.
> >
> > SRCU could also be used to check if a desc is requested before
> > entering the proper API call in the ideal world where we don't allow
> > using non-requested descriptors making it possible to avoid real locks
> > in struct gpio_desc.
> >
>
> So why allow the usage of non-requested descriptors?
>

Because years of technical debt, that's why. :)

Bart

> Cheers,
> Kent.
>

Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski]

On Tue, Nov 28, 2023 at 11:47 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>

[snip]

>
> Because years of technical debt, that's why. :)
>

Speaking of technical debt: you may have noticed that despite stating
I'm almost done last week, I still haven't submitted my locking
rework.

The reason for that is that I'm stuck on some corner-cases related to
the GPIO <-> pinctrl interaction. Specifically the fact that we have
GPIOLIB API functions that may be called from atomic context which may
end up calling into pinctrl where a mutex will be acquired.

An example of that is any of the GPIO chips that don't set the
can_sleep field in struct gpio_chip but still use
gpiochip_generic_config() (e.g. tegra186). We can then encounter the
following situation:

irq_handler() // in atomic context
  gpiod_direction_output() // line is open-drain
    gpio_set_config()
      gpiochip_generic_config()
        pinctrl_gpio_set_config()
          mutex_lock()

Currently we don't take any locks nor synchronize in any other way
(which is wrong as concurrent gpiod_direction_output() and
gpiod_direction_input() will get in each other's way). Using a mutex
will be fine but for non-sleeping chips if we use a spinlock here (no
other choice really) we'll set off fireworks.

One of the ideas I have is using the fact that we already use atomic
bitops in most places. Let's not take locks but add a new flag:
FLAG_SETTING_DIRECTION. Now when we go into
gpiod_direction_output/input(), we test and set it. A subsequent call
will fail with EBUSY or EAGAIN as long as it's set. It will have no
effect on set/get() - any synchronization will be left to the driver.
When we're done, we clear it after setting the relevant direction
flag.

Does this make any sense? There's still the label pointer and debounce
period stored in the descriptor but these are not accessed in atomic
context AFAICT.

Bart

Re: GPIOLIB locking is broken and how to fix it

[Kent Gibson]

On Thu, Dec 07, 2023 at 07:37:54PM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 28, 2023 at 11:47 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
>
> [snip]
>
> >
> > Because years of technical debt, that's why. :)
> >
>
> Speaking of technical debt: you may have noticed that despite stating
> I'm almost done last week, I still haven't submitted my locking
> rework.
>
> The reason for that is that I'm stuck on some corner-cases related to
> the GPIO <-> pinctrl interaction. Specifically the fact that we have
> GPIOLIB API functions that may be called from atomic context which may
> end up calling into pinctrl where a mutex will be acquired.
>

To be clear, that is an existing pinctrl mutex?

> An example of that is any of the GPIO chips that don't set the
> can_sleep field in struct gpio_chip but still use
> gpiochip_generic_config() (e.g. tegra186). We can then encounter the
> following situation:
>
> irq_handler() // in atomic context
>   gpiod_direction_output() // line is open-drain
>     gpio_set_config()
>       gpiochip_generic_config()
>         pinctrl_gpio_set_config()
>           mutex_lock()
>

Isn't using a mutex (the pinctrl one here) from atomic always a
problem?  Shouldn't this flow be handed off to irq_thread()?

> Currently we don't take any locks nor synchronize in any other way
> (which is wrong as concurrent gpiod_direction_output() and
> gpiod_direction_input() will get in each other's way). Using a mutex
> will be fine but for non-sleeping chips if we use a spinlock here (no
> other choice really) we'll set off fireworks.
>
> One of the ideas I have is using the fact that we already use atomic
> bitops in most places. Let's not take locks but add a new flag:
> FLAG_SETTING_DIRECTION. Now when we go into
> gpiod_direction_output/input(), we test and set it. A subsequent call
> will fail with EBUSY or EAGAIN as long as it's set. It will have no
> effect on set/get() - any synchronization will be left to the driver.
> When we're done, we clear it after setting the relevant direction
> flag.
>
> Does this make any sense? There's still the label pointer and debounce
> period stored in the descriptor but these are not accessed in atomic
> context AFAICT.
>

Makes sense to me, as it is basically the sub-state solution I suggested
earlier for request/release, but applied to direction.  Not sure about
the contention behaviour though, as that is something userspace will
see and might not be expecting.

OTOH I'm starting to think that serialising callbacks might be a good idea
- unless it is crystal clear to the driver authors that the callbacks may
be called concurrently.

The debounce is really a cdev field.  Putting it in the desc made sense
to me at the time as it is per-line, not per-request, but perhaps it
should moved into the cdev linereq, even if that means having to alloc
space for it, just to get it out of your hair.

Cheers,
Kent.

Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski]

On Fri, Dec 8, 2023 at 2:01 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Dec 07, 2023 at 07:37:54PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Nov 28, 2023 at 11:47 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> >
> > [snip]
> >
> > >
> > > Because years of technical debt, that's why. :)
> > >
> >
> > Speaking of technical debt: you may have noticed that despite stating
> > I'm almost done last week, I still haven't submitted my locking
> > rework.
> >
> > The reason for that is that I'm stuck on some corner-cases related to
> > the GPIO <-> pinctrl interaction. Specifically the fact that we have
> > GPIOLIB API functions that may be called from atomic context which may
> > end up calling into pinctrl where a mutex will be acquired.
> >
>
> To be clear, that is an existing pinctrl mutex?

Yes, I'm talking about pctldev->mutex. TBH set_config IMO should never
be called from atomic context but that's already there and will be
hard to change it now. :(

>
> > An example of that is any of the GPIO chips that don't set the
> > can_sleep field in struct gpio_chip but still use
> > gpiochip_generic_config() (e.g. tegra186). We can then encounter the
> > following situation:
> >
> > irq_handler() // in atomic context
> >   gpiod_direction_output() // line is open-drain
> >     gpio_set_config()
> >       gpiochip_generic_config()
> >         pinctrl_gpio_set_config()
> >           mutex_lock()
> >
>
> Isn't using a mutex (the pinctrl one here) from atomic always a
> problem?  Shouldn't this flow be handed off to irq_thread()?
>

This is a corner-case. Typically we won't be calling gpio_set_config()
from gpiod_direction_output(). This only happens if the line has
special config like open-source/open-drain. Any other places where we
may end up calling into pinctrl is request() and free() and those
already might sleep.

> > Currently we don't take any locks nor synchronize in any other way
> > (which is wrong as concurrent gpiod_direction_output() and
> > gpiod_direction_input() will get in each other's way). Using a mutex
> > will be fine but for non-sleeping chips if we use a spinlock here (no
> > other choice really) we'll set off fireworks.
> >
> > One of the ideas I have is using the fact that we already use atomic
> > bitops in most places. Let's not take locks but add a new flag:
> > FLAG_SETTING_DIRECTION. Now when we go into
> > gpiod_direction_output/input(), we test and set it. A subsequent call
> > will fail with EBUSY or EAGAIN as long as it's set. It will have no
> > effect on set/get() - any synchronization will be left to the driver.
> > When we're done, we clear it after setting the relevant direction
> > flag.
> >
> > Does this make any sense? There's still the label pointer and debounce
> > period stored in the descriptor but these are not accessed in atomic
> > context AFAICT.
> >
>
> Makes sense to me, as it is basically the sub-state solution I suggested
> earlier for request/release, but applied to direction.  Not sure about
> the contention behaviour though, as that is something userspace will
> see and might not be expecting.
>

User-space will never call from atomic context. We could potentially
use a work-queue here even for sleeping chips and serialize the
operations

> OTOH I'm starting to think that serialising callbacks might be a good idea
> - unless it is crystal clear to the driver authors that the callbacks may
> be called concurrently.

This was my initial idea: use mutex for sleeping chips, spinlock for
non-sleeping ones and make it possible for drivers to disable locking
entirely. Except that we cannot use spinlock for chips interacting
with pinctrl at all even if they can never sleep. :/

>
> The debounce is really a cdev field.  Putting it in the desc made sense
> to me at the time as it is per-line, not per-request, but perhaps it
> should moved into the cdev linereq, even if that means having to alloc
> space for it, just to get it out of your hair.
>

This sounds good actually.

Bart

> Cheers,
> Kent.

Re: GPIOLIB locking is broken and how to fix it

[Kent Gibson]

On Fri, Dec 08, 2023 at 09:13:17AM +0100, Bartosz Golaszewski wrote:
> On Fri, Dec 8, 2023 at 2:01 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Dec 07, 2023 at 07:37:54PM +0100, Bartosz Golaszewski wrote:
> > > On Tue, Nov 28, 2023 at 11:47 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >
> > >
> > > [snip]
> > >
> > > >
> > > > Because years of technical debt, that's why. :)
> > > >
> > >
> > > Speaking of technical debt: you may have noticed that despite stating
> > > I'm almost done last week, I still haven't submitted my locking
> > > rework.
> > >
> > > The reason for that is that I'm stuck on some corner-cases related to
> > > the GPIO <-> pinctrl interaction. Specifically the fact that we have
> > > GPIOLIB API functions that may be called from atomic context which may
> > > end up calling into pinctrl where a mutex will be acquired.
> > >
> >
> > To be clear, that is an existing pinctrl mutex?
>
> Yes, I'm talking about pctldev->mutex. TBH set_config IMO should never
> be called from atomic context but that's already there and will be
> hard to change it now. :(
>
> >
> > > An example of that is any of the GPIO chips that don't set the
> > > can_sleep field in struct gpio_chip but still use
> > > gpiochip_generic_config() (e.g. tegra186). We can then encounter the
> > > following situation:
> > >
> > > irq_handler() // in atomic context
> > >   gpiod_direction_output() // line is open-drain
> > >     gpio_set_config()
> > >       gpiochip_generic_config()
> > >         pinctrl_gpio_set_config()
> > >           mutex_lock()
> > >
> >
> > Isn't using a mutex (the pinctrl one here) from atomic always a
> > problem?  Shouldn't this flow be handed off to irq_thread()?
> >
>
> This is a corner-case. Typically we won't be calling gpio_set_config()
> from gpiod_direction_output(). This only happens if the line has
> special config like open-source/open-drain. Any other places where we
> may end up calling into pinctrl is request() and free() and those
> already might sleep.
>

Why does it matter that it is a corner case?
So it is currently possible for that corner case to hit a mutex within
atomic context??

> > > Currently we don't take any locks nor synchronize in any other way
> > > (which is wrong as concurrent gpiod_direction_output() and
> > > gpiod_direction_input() will get in each other's way). Using a mutex
> > > will be fine but for non-sleeping chips if we use a spinlock here (no
> > > other choice really) we'll set off fireworks.
> > >
> > > One of the ideas I have is using the fact that we already use atomic
> > > bitops in most places. Let's not take locks but add a new flag:
> > > FLAG_SETTING_DIRECTION. Now when we go into
> > > gpiod_direction_output/input(), we test and set it. A subsequent call
> > > will fail with EBUSY or EAGAIN as long as it's set. It will have no
> > > effect on set/get() - any synchronization will be left to the driver.
> > > When we're done, we clear it after setting the relevant direction
> > > flag.
> > >
> > > Does this make any sense? There's still the label pointer and debounce
> > > period stored in the descriptor but these are not accessed in atomic
> > > context AFAICT.
> > >
> >
> > Makes sense to me, as it is basically the sub-state solution I suggested
> > earlier for request/release, but applied to direction.  Not sure about
> > the contention behaviour though, as that is something userspace will
> > see and might not be expecting.
> >
>
> User-space will never call from atomic context.

Don't you need to do the test and set in either case?

> We could potentially
> use a work-queue here even for sleeping chips and serialize the
> operations
>
> > OTOH I'm starting to think that serialising callbacks might be a good idea
> > - unless it is crystal clear to the driver authors that the callbacks may
> > be called concurrently.
>
> This was my initial idea: use mutex for sleeping chips, spinlock for
> non-sleeping ones and make it possible for drivers to disable locking
> entirely. Except that we cannot use spinlock for chips interacting
> with pinctrl at all even if they can never sleep. :/
>
> >
> > The debounce is really a cdev field.  Putting it in the desc made sense
> > to me at the time as it is per-line, not per-request, but perhaps it
> > should moved into the cdev linereq, even if that means having to alloc
> > space for it, just to get it out of your hair.
> >
>
> This sounds good actually.
>

Yeah, no need to risk other GPIO users messing with it if it is only there
for cdev.
Want me to take a look at it or are you happy to take care of it?

Cheers,
Kent.

Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski]

On Fri, Dec 8, 2023 at 9:38 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Dec 08, 2023 at 09:13:17AM +0100, Bartosz Golaszewski wrote:
> > On Fri, Dec 8, 2023 at 2:01 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Thu, Dec 07, 2023 at 07:37:54PM +0100, Bartosz Golaszewski wrote:
> > > > On Tue, Nov 28, 2023 at 11:47 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > >
> > > >
> > > > [snip]
> > > >
> > > > >
> > > > > Because years of technical debt, that's why. :)
> > > > >
> > > >
> > > > Speaking of technical debt: you may have noticed that despite stating
> > > > I'm almost done last week, I still haven't submitted my locking
> > > > rework.
> > > >
> > > > The reason for that is that I'm stuck on some corner-cases related to
> > > > the GPIO <-> pinctrl interaction. Specifically the fact that we have
> > > > GPIOLIB API functions that may be called from atomic context which may
> > > > end up calling into pinctrl where a mutex will be acquired.
> > > >
> > >
> > > To be clear, that is an existing pinctrl mutex?
> >
> > Yes, I'm talking about pctldev->mutex. TBH set_config IMO should never
> > be called from atomic context but that's already there and will be
> > hard to change it now. :(
> >
> > >
> > > > An example of that is any of the GPIO chips that don't set the
> > > > can_sleep field in struct gpio_chip but still use
> > > > gpiochip_generic_config() (e.g. tegra186). We can then encounter the
> > > > following situation:
> > > >
> > > > irq_handler() // in atomic context
> > > >   gpiod_direction_output() // line is open-drain
> > > >     gpio_set_config()
> > > >       gpiochip_generic_config()
> > > >         pinctrl_gpio_set_config()
> > > >           mutex_lock()
> > > >
> > >
> > > Isn't using a mutex (the pinctrl one here) from atomic always a
> > > problem?  Shouldn't this flow be handed off to irq_thread()?
> > >
> >
> > This is a corner-case. Typically we won't be calling gpio_set_config()
> > from gpiod_direction_output(). This only happens if the line has
> > special config like open-source/open-drain. Any other places where we
> > may end up calling into pinctrl is request() and free() and those
> > already might sleep.
> >
>
> Why does it matter that it is a corner case?
> So it is currently possible for that corner case to hit a mutex within
> atomic context??
>
> > > > Currently we don't take any locks nor synchronize in any other way
> > > > (which is wrong as concurrent gpiod_direction_output() and
> > > > gpiod_direction_input() will get in each other's way). Using a mutex
> > > > will be fine but for non-sleeping chips if we use a spinlock here (no
> > > > other choice really) we'll set off fireworks.
> > > >
> > > > One of the ideas I have is using the fact that we already use atomic
> > > > bitops in most places. Let's not take locks but add a new flag:
> > > > FLAG_SETTING_DIRECTION. Now when we go into
> > > > gpiod_direction_output/input(), we test and set it. A subsequent call
> > > > will fail with EBUSY or EAGAIN as long as it's set. It will have no
> > > > effect on set/get() - any synchronization will be left to the driver.
> > > > When we're done, we clear it after setting the relevant direction
> > > > flag.
> > > >
> > > > Does this make any sense? There's still the label pointer and debounce
> > > > period stored in the descriptor but these are not accessed in atomic
> > > > context AFAICT.
> > > >
> > >
> > > Makes sense to me, as it is basically the sub-state solution I suggested
> > > earlier for request/release, but applied to direction.  Not sure about
> > > the contention behaviour though, as that is something userspace will
> > > see and might not be expecting.
> > >
> >
> > User-space will never call from atomic context.
>
> Don't you need to do the test and set in either case?
>
> > We could potentially
> > use a work-queue here even for sleeping chips and serialize the
> > operations
> >
> > > OTOH I'm starting to think that serialising callbacks might be a good idea
> > > - unless it is crystal clear to the driver authors that the callbacks may
> > > be called concurrently.
> >
> > This was my initial idea: use mutex for sleeping chips, spinlock for
> > non-sleeping ones and make it possible for drivers to disable locking
> > entirely. Except that we cannot use spinlock for chips interacting
> > with pinctrl at all even if they can never sleep. :/
> >
> > >
> > > The debounce is really a cdev field.  Putting it in the desc made sense
> > > to me at the time as it is per-line, not per-request, but perhaps it
> > > should moved into the cdev linereq, even if that means having to alloc
> > > space for it, just to get it out of your hair.
> > >
> >
> > This sounds good actually.
> >
>
> Yeah, no need to risk other GPIO users messing with it if it is only there
> for cdev.
> Want me to take a look at it or are you happy to take care of it?
>

If you'll find the time to do it in the following days then sure, go
ahead, otherwise, I'll have some pare cycles today and next week to
spend on it.

Bart

> Cheers,
> Kent.

Re: GPIOLIB locking is broken and how to fix it

[Kent Gibson]

On Fri, Dec 08, 2023 at 10:52:09AM +0100, Bartosz Golaszewski wrote:
> On Fri, Dec 8, 2023 at 9:38 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Fri, Dec 08, 2023 at 09:13:17AM +0100, Bartosz Golaszewski wrote:
> > > On Fri, Dec 8, 2023 at 2:01 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > On Thu, Dec 07, 2023 at 07:37:54PM +0100, Bartosz Golaszewski wrote:
> > > > > On Tue, Nov 28, 2023 at 11:47 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > >
> > > > >
> > > > > [snip]
> > >
> >
> > Yeah, no need to risk other GPIO users messing with it if it is only there
> > for cdev.
> > Want me to take a look at it or are you happy to take care of it?
> >
>
> If you'll find the time to do it in the following days then sure, go
> ahead, otherwise, I'll have some pare cycles today and next week to
> spend on it.
>

It would probably take me longer than that to context switch, so go for
it.

Cheers,
Kent.

Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski]

On Fri, Dec 8, 2023 at 11:27 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Dec 08, 2023 at 10:52:09AM +0100, Bartosz Golaszewski wrote:
> > On Fri, Dec 8, 2023 at 9:38 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Fri, Dec 08, 2023 at 09:13:17AM +0100, Bartosz Golaszewski wrote:
> > > > On Fri, Dec 8, 2023 at 2:01 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > >
> > > > > On Thu, Dec 07, 2023 at 07:37:54PM +0100, Bartosz Golaszewski wrote:
> > > > > > On Tue, Nov 28, 2023 at 11:47 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > > >
> > > > > >
> > > > > > [snip]
> > > >
> > >
> > > Yeah, no need to risk other GPIO users messing with it if it is only there
> > > for cdev.
> > > Want me to take a look at it or are you happy to take care of it?
> > >
> >
> > If you'll find the time to do it in the following days then sure, go
> > ahead, otherwise, I'll have some pare cycles today and next week to
> > spend on it.
> >
>
> It would probably take me longer than that to context switch, so go for
> it.
>

Well I went for it and it turns out to be quite tricky. We have
linereq and gpio_chardev_data that have independent lifetimes and the
only resource they share is the gpio_device and - by extension - gpio
descriptors . If we want to store some additional data locally within
the context of gpiolib-cdev.c then I see the following options:

1. Global radix_tree containing additional configuration
(debounce_period_us for now) looked up by the descriptor's address.
Lookup can be done locklessly using RCU and from atomic context
(interrupt handlers in cdev code).

2. Reference counted wrapper around descriptors. It would look something like:

struct gpio_cdev_desc {
    struct kref ref;
    struct gpio_desc *desc;
    unsigned int debounce_period_us;
};

And then struct gpio_chardev_data would store an array of pointers to
this wrapper while struct line would store a pointer to it instead of
directly referencing struct gpio_desc.

Any other ideas?

Bart

> Cheers,
> Kent.
>

Re: GPIOLIB locking is broken and how to fix it

[Kent Gibson]

On Fri, Dec 08, 2023 at 07:54:40PM +0100, Bartosz Golaszewski wrote:
> On Fri, Dec 8, 2023 at 11:27 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Fri, Dec 08, 2023 at 10:52:09AM +0100, Bartosz Golaszewski wrote:
> > > On Fri, Dec 8, 2023 at 9:38 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > On Fri, Dec 08, 2023 at 09:13:17AM +0100, Bartosz Golaszewski wrote:
> > > > > On Fri, Dec 8, 2023 at 2:01 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > > >
> > > > > > On Thu, Dec 07, 2023 at 07:37:54PM +0100, Bartosz Golaszewski wrote:
> > > > > > > On Tue, Nov 28, 2023 at 11:47 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > > > >
> > > > > > >
> > > > > > > [snip]
> > > > >
> > > >
> > > > Yeah, no need to risk other GPIO users messing with it if it is only there
> > > > for cdev.
> > > > Want me to take a look at it or are you happy to take care of it?
> > > >
> > >
> > > If you'll find the time to do it in the following days then sure, go
> > > ahead, otherwise, I'll have some pare cycles today and next week to
> > > spend on it.
> > >
> >
> > It would probably take me longer than that to context switch, so go for
> > it.
> >
>
> Well I went for it and it turns out to be quite tricky. We have
> linereq and gpio_chardev_data that have independent lifetimes and the
> only resource they share is the gpio_device and - by extension - gpio
> descriptors . If we want to store some additional data locally within
> the context of gpiolib-cdev.c then I see the following options:
>

Well that probably explains why putting it in the desc made so much
sense at the time.

Lets take a look at the code...

I had thought it could be moved to struct line (contained within
struct linereq), so basically replacing line->desc->debounce_period_us
with line->debounce_period_us.  That almost works, but the problem there
is that gpio_desc_to_lineinfo() returns the debounce period in the line
info - and there is no way to access the linereq/line from the desc...

Ah, so the lineinfo_get/_v1() functions that call
gpio_desc_to_lineinfo() also have the gpio_chardev_data to work with -
now I see where you are coming from.
(Debounce is not relevant for v1, so that reduces the problem to
lineinfo_get().)

> 1. Global radix_tree containing additional configuration
> (debounce_period_us for now) looked up by the descriptor's address.
> Lookup can be done locklessly using RCU and from atomic context
> (interrupt handlers in cdev code).
>

The irq handlers are already protected from changes to debounce period.
It is either set before the irqs are enabled (in the request), or the
irq is disabled, debounce updated, and irq re-enabled (in set_config).

> 2. Reference counted wrapper around descriptors. It would look something like:
>
> struct gpio_cdev_desc {
>     struct kref ref;
>     struct gpio_desc *desc;
>     unsigned int debounce_period_us;
> };
>
> And then struct gpio_chardev_data would store an array of pointers to
> this wrapper while struct line would store a pointer to it instead of
> directly referencing struct gpio_desc.
>
> Any other ideas?
>

I still think the primary location for any additional line config is in
struct line - that makes it clear and simple for the majority of cdev and
matches the lifetimes of the accessors (other than lineinfo_get()).

The only issue being how to access that info from lineinfo_get().
I guess we can't stop reporting the debounce in the line info :-(.

Rather than worry about an linkage between gpio_chardev_data and the
linereq/line, I would consider a separate copy of this supplemental line
info for the chardev, possibly using a radix_tree as you suggest.
That would be updated by the linereq to match its value in struct line.
So basically Option 1 but for a shadow copy of the additional info.

I'm not a fan of globals, so would go per chip and indexed by line
offset rather than desc. But then, given the lifetime issues at play, that
would have to be in a reference counted object that both gpio_chardev_data
and linereq would reference.  So swings and roundabouts.

Does that make sense? Anyway, that's my $/50.

Cheers,
Kent.

Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski]

On Sat, Dec 9, 2023 at 2:56 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Fri, Dec 08, 2023 at 07:54:40PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Dec 8, 2023 at 11:27 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Fri, Dec 08, 2023 at 10:52:09AM +0100, Bartosz Golaszewski wrote:
> > > > On Fri, Dec 8, 2023 at 9:38 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > >
> > > > > On Fri, Dec 08, 2023 at 09:13:17AM +0100, Bartosz Golaszewski wrote:
> > > > > > On Fri, Dec 8, 2023 at 2:01 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > > > >
> > > > > > > On Thu, Dec 07, 2023 at 07:37:54PM +0100, Bartosz Golaszewski wrote:
> > > > > > > > On Tue, Nov 28, 2023 at 11:47 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > > > > >
> > > > > > > >
> > > > > > > > [snip]
> > > > > >
> > > > >
> > > > > Yeah, no need to risk other GPIO users messing with it if it is only there
> > > > > for cdev.
> > > > > Want me to take a look at it or are you happy to take care of it?
> > > > >
> > > >
> > > > If you'll find the time to do it in the following days then sure, go
> > > > ahead, otherwise, I'll have some pare cycles today and next week to
> > > > spend on it.
> > > >
> > >
> > > It would probably take me longer than that to context switch, so go for
> > > it.
> > >
> >
> > Well I went for it and it turns out to be quite tricky. We have
> > linereq and gpio_chardev_data that have independent lifetimes and the
> > only resource they share is the gpio_device and - by extension - gpio
> > descriptors . If we want to store some additional data locally within
> > the context of gpiolib-cdev.c then I see the following options:
> >
>
> Well that probably explains why putting it in the desc made so much
> sense at the time.
>
> Lets take a look at the code...
>
> I had thought it could be moved to struct line (contained within
> struct linereq), so basically replacing line->desc->debounce_period_us
> with line->debounce_period_us.  That almost works, but the problem there
> is that gpio_desc_to_lineinfo() returns the debounce period in the line
> info - and there is no way to access the linereq/line from the desc...
>
> Ah, so the lineinfo_get/_v1() functions that call
> gpio_desc_to_lineinfo() also have the gpio_chardev_data to work with -
> now I see where you are coming from.
> (Debounce is not relevant for v1, so that reduces the problem to
> lineinfo_get().)
>
> > 1. Global radix_tree containing additional configuration
> > (debounce_period_us for now) looked up by the descriptor's address.
> > Lookup can be done locklessly using RCU and from atomic context
> > (interrupt handlers in cdev code).
> >
>
> The irq handlers are already protected from changes to debounce period.
> It is either set before the irqs are enabled (in the request), or the
> irq is disabled, debounce updated, and irq re-enabled (in set_config).
>
> > 2. Reference counted wrapper around descriptors. It would look something like:
> >
> > struct gpio_cdev_desc {
> >     struct kref ref;
> >     struct gpio_desc *desc;
> >     unsigned int debounce_period_us;
> > };
> >
> > And then struct gpio_chardev_data would store an array of pointers to
> > this wrapper while struct line would store a pointer to it instead of
> > directly referencing struct gpio_desc.
> >
> > Any other ideas?
> >
>
> I still think the primary location for any additional line config is in
> struct line - that makes it clear and simple for the majority of cdev and
> matches the lifetimes of the accessors (other than lineinfo_get()).
>
> The only issue being how to access that info from lineinfo_get().
> I guess we can't stop reporting the debounce in the line info :-(.
>
> Rather than worry about an linkage between gpio_chardev_data and the
> linereq/line, I would consider a separate copy of this supplemental line
> info for the chardev, possibly using a radix_tree as you suggest.
> That would be updated by the linereq to match its value in struct line.
> So basically Option 1 but for a shadow copy of the additional info.
>

We still need to connect linereq with its "parent" gpio_chardev_data
somehow and make this link weak so that it can survive one or the
other being destroyed. Maybe a notifier in linereq to which
gpio_chardev_data would subscribe? It would send out notifications on
changes to debounce_period which gpio_chardev_data could store. When
linereq goes out of scope it sends a corresponding notification
allowing gpio_chardev_data to unsubscribe before linereq is freed,
while when gpio_chardev_data goes out of scope first, it unsubscribes
when being released.

Bart

> I'm not a fan of globals, so would go per chip and indexed by line
> offset rather than desc. But then, given the lifetime issues at play, that
> would have to be in a reference counted object that both gpio_chardev_data
> and linereq would reference.  So swings and roundabouts.
>
> Does that make sense? Anyway, that's my $/50.
>
> Cheers,
> Kent.
>

Re: GPIOLIB locking is broken and how to fix it

[Kent Gibson]

On Sat, Dec 09, 2023 at 08:24:59PM +0100, Bartosz Golaszewski wrote:
> On Sat, Dec 9, 2023 at 2:56 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Fri, Dec 08, 2023 at 07:54:40PM +0100, Bartosz Golaszewski wrote:
> > > On Fri, Dec 8, 2023 at 11:27 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > On Fri, Dec 08, 2023 at 10:52:09AM +0100, Bartosz Golaszewski wrote:
> > > > > On Fri, Dec 8, 2023 at 9:38 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > > >
> > > > > > On Fri, Dec 08, 2023 at 09:13:17AM +0100, Bartosz Golaszewski wrote:
> > > > > > > On Fri, Dec 8, 2023 at 2:01 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Thu, Dec 07, 2023 at 07:37:54PM +0100, Bartosz Golaszewski wrote:
> > > > > > > > > On Tue, Nov 28, 2023 at 11:47 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > [snip]
> > > > > > >
> > > > > >
> > > > > > Yeah, no need to risk other GPIO users messing with it if it is only there
> > > > > > for cdev.
> > > > > > Want me to take a look at it or are you happy to take care of it?
> > > > > >
> > > > >
> > > > > If you'll find the time to do it in the following days then sure, go
> > > > > ahead, otherwise, I'll have some pare cycles today and next week to
> > > > > spend on it.
> > > > >
> > > >
> > > > It would probably take me longer than that to context switch, so go for
> > > > it.
> > > >
> > >
> > > Well I went for it and it turns out to be quite tricky. We have
> > > linereq and gpio_chardev_data that have independent lifetimes and the
> > > only resource they share is the gpio_device and - by extension - gpio
> > > descriptors . If we want to store some additional data locally within
> > > the context of gpiolib-cdev.c then I see the following options:
> > >
> >
> > Well that probably explains why putting it in the desc made so much
> > sense at the time.
> >
> > Lets take a look at the code...
> >
> > I had thought it could be moved to struct line (contained within
> > struct linereq), so basically replacing line->desc->debounce_period_us
> > with line->debounce_period_us.  That almost works, but the problem there
> > is that gpio_desc_to_lineinfo() returns the debounce period in the line
> > info - and there is no way to access the linereq/line from the desc...
> >
> > Ah, so the lineinfo_get/_v1() functions that call
> > gpio_desc_to_lineinfo() also have the gpio_chardev_data to work with -
> > now I see where you are coming from.
> > (Debounce is not relevant for v1, so that reduces the problem to
> > lineinfo_get().)
> >
> > > 1. Global radix_tree containing additional configuration
> > > (debounce_period_us for now) looked up by the descriptor's address.
> > > Lookup can be done locklessly using RCU and from atomic context
> > > (interrupt handlers in cdev code).
> > >
> >
> > The irq handlers are already protected from changes to debounce period.
> > It is either set before the irqs are enabled (in the request), or the
> > irq is disabled, debounce updated, and irq re-enabled (in set_config).
> >

Having had a closer look at the code and strictly speaking, one of the
corner cases (updating the period for sw debouncing) doesn't do the
disable/enable but lives with the possibility of the debouncer using a
stale value for one debounce period (as that is much simpler).

But the end result is the same - the debounce period doesn't require
additional locking.

> > > 2. Reference counted wrapper around descriptors. It would look something like:
> > >
> > > struct gpio_cdev_desc {
> > >     struct kref ref;
> > >     struct gpio_desc *desc;
> > >     unsigned int debounce_period_us;
> > > };
> > >
> > > And then struct gpio_chardev_data would store an array of pointers to
> > > this wrapper while struct line would store a pointer to it instead of
> > > directly referencing struct gpio_desc.
> > >
> > > Any other ideas?
> > >
> >
> > I still think the primary location for any additional line config is in
> > struct line - that makes it clear and simple for the majority of cdev and
> > matches the lifetimes of the accessors (other than lineinfo_get()).
> >
> > The only issue being how to access that info from lineinfo_get().
> > I guess we can't stop reporting the debounce in the line info :-(.
> >
> > Rather than worry about an linkage between gpio_chardev_data and the
> > linereq/line, I would consider a separate copy of this supplemental line
> > info for the chardev, possibly using a radix_tree as you suggest.
> > That would be updated by the linereq to match its value in struct line.
> > So basically Option 1 but for a shadow copy of the additional info.
> >
>
> We still need to connect linereq with its "parent" gpio_chardev_data
> somehow and make this link weak so that it can survive one or the
> other being destroyed. Maybe a notifier in linereq to which
> gpio_chardev_data would subscribe? It would send out notifications on
> changes to debounce_period which gpio_chardev_data could store. When
> linereq goes out of scope it sends a corresponding notification
> allowing gpio_chardev_data to unsubscribe before linereq is freed,
> while when gpio_chardev_data goes out of scope first, it unsubscribes
> when being released.
>

No, there has to be a link between both and the supplemental info.
For gpio_chardev_data that is to create lineinfo, and for the linereq it
is to keep the value reported in lineinfo mirroring the current value.
Below I suggested making the supplemental info a reference counted
object, with chip scope, referenced by both gpio_chardev_data and the
linereq. So last one out turns off the lights.

Having the shadow copy allows most usage to avoid the tree lookup and any
associated locking (assuming the tree isn't inherently thread safe and
requires a spinlock to prevent modification during a lookup).
It is only populating the lineinfo or updating the value that would
require the lookup, and neither are a hot path (if there is such a thing
in cdev).

Hmmm, the radix_tree allocates a page of entries at a time, which might
be a bit of overkill per-chip, so maybe a global is the way to go?
Or something other than a radix_tree, say a rbtree?

All this is getting rather complicated just to relocate one field, so I'm
starting to reconsider whether the desc was the right place for it after
all. ¯\_(ツ)_/¯

OTOH, I've partially coded my suggestion, to the point of skeletons for
the supplemental info, so if you like I'm happy to finish that off and
provide patches.  Though what remains is probably 90% of the work...

Cheers,
Kent.

> Bart
>
> > I'm not a fan of globals, so would go per chip and indexed by line
> > offset rather than desc. But then, given the lifetime issues at play, that
> > would have to be in a reference counted object that both gpio_chardev_data
> > and linereq would reference.  So swings and roundabouts.
> >
> > Does that make sense? Anyway, that's my $/50.
> >
> > Cheers,
> > Kent.
> >

Re: GPIOLIB locking is broken and how to fix it

[Kent Gibson]

On Sun, Dec 10, 2023 at 10:34:47AM +0800, Kent Gibson wrote:
> > >
> >
> > We still need to connect linereq with its "parent" gpio_chardev_data
> > somehow and make this link weak so that it can survive one or the
> > other being destroyed. Maybe a notifier in linereq to which
> > gpio_chardev_data would subscribe? It would send out notifications on
> > changes to debounce_period which gpio_chardev_data could store. When
> > linereq goes out of scope it sends a corresponding notification
> > allowing gpio_chardev_data to unsubscribe before linereq is freed,
> > while when gpio_chardev_data goes out of scope first, it unsubscribes
> > when being released.
> >
>
> No, there has to be a link between both and the supplemental info.
> For gpio_chardev_data that is to create lineinfo, and for the linereq it
> is to keep the value reported in lineinfo mirroring the current value.
> Below I suggested making the supplemental info a reference counted
> object, with chip scope, referenced by both gpio_chardev_data and the
> linereq. So last one out turns off the lights.
>
> Having the shadow copy allows most usage to avoid the tree lookup and any
> associated locking (assuming the tree isn't inherently thread safe and
> requires a spinlock to prevent modification during a lookup).
> It is only populating the lineinfo or updating the value that would
> require the lookup, and neither are a hot path (if there is such a thing
> in cdev).
>
> Hmmm, the radix_tree allocates a page of entries at a time, which might
> be a bit of overkill per-chip, so maybe a global is the way to go?
> Or something other than a radix_tree, say a rbtree?
>
> All this is getting rather complicated just to relocate one field, so I'm
> starting to reconsider whether the desc was the right place for it after
> all. ¯\_(ツ)_/¯
>
> OTOH, I've partially coded my suggestion, to the point of skeletons for
> the supplemental info, so if you like I'm happy to finish that off and
> provide patches.  Though what remains is probably 90% of the work...
>

Bah, just ignore me wrt the supplemental info per chip.
That solution only works for the chip fd used to request the lines.
If you close the chip and re-open it there will be no connection between
the new gpio_chardev_data and the existing line requests or the
supplemental info.

So it would have to be a global indexed by desc as you suggested.
Well that sucks.

Cheers,
Kent.

Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski]

On Sun, Dec 10, 2023 at 2:28 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Sun, Dec 10, 2023 at 10:34:47AM +0800, Kent Gibson wrote:
> > > >
> > >
> > > We still need to connect linereq with its "parent" gpio_chardev_data
> > > somehow and make this link weak so that it can survive one or the
> > > other being destroyed. Maybe a notifier in linereq to which
> > > gpio_chardev_data would subscribe? It would send out notifications on
> > > changes to debounce_period which gpio_chardev_data could store. When
> > > linereq goes out of scope it sends a corresponding notification
> > > allowing gpio_chardev_data to unsubscribe before linereq is freed,
> > > while when gpio_chardev_data goes out of scope first, it unsubscribes
> > > when being released.
> > >
> >
> > No, there has to be a link between both and the supplemental info.
> > For gpio_chardev_data that is to create lineinfo, and for the linereq it
> > is to keep the value reported in lineinfo mirroring the current value.
> > Below I suggested making the supplemental info a reference counted
> > object, with chip scope, referenced by both gpio_chardev_data and the
> > linereq. So last one out turns off the lights.
> >
> > Having the shadow copy allows most usage to avoid the tree lookup and any
> > associated locking (assuming the tree isn't inherently thread safe and
> > requires a spinlock to prevent modification during a lookup).
> > It is only populating the lineinfo or updating the value that would
> > require the lookup, and neither are a hot path (if there is such a thing
> > in cdev).
> >
> > Hmmm, the radix_tree allocates a page of entries at a time, which might
> > be a bit of overkill per-chip, so maybe a global is the way to go?
> > Or something other than a radix_tree, say a rbtree?
> >
> > All this is getting rather complicated just to relocate one field, so I'm
> > starting to reconsider whether the desc was the right place for it after
> > all. ¯\_(ツ)_/¯
> >
> > OTOH, I've partially coded my suggestion, to the point of skeletons for
> > the supplemental info, so if you like I'm happy to finish that off and
> > provide patches.  Though what remains is probably 90% of the work...
> >
>
> Bah, just ignore me wrt the supplemental info per chip.
> That solution only works for the chip fd used to request the lines.
> If you close the chip and re-open it there will be no connection between
> the new gpio_chardev_data and the existing line requests or the
> supplemental info.
>
> So it would have to be a global indexed by desc as you suggested.
> Well that sucks.
>

Yeah, I don't see any other way if we want to contain this within
gpiolib-cdev.c. I tried fiddling with notifiers but it went nowhere.
:(

Bart

> Cheers,
> Kent.
>

Re: GPIOLIB locking is broken and how to fix it

[Kent Gibson]

On Mon, Dec 11, 2023 at 04:10:27PM +0100, Bartosz Golaszewski wrote:
> On Sun, Dec 10, 2023 at 2:28 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Sun, Dec 10, 2023 at 10:34:47AM +0800, Kent Gibson wrote:
> > > > >
> > > >
> >
> > Bah, just ignore me wrt the supplemental info per chip.
> > That solution only works for the chip fd used to request the lines.
> > If you close the chip and re-open it there will be no connection between
> > the new gpio_chardev_data and the existing line requests or the
> > supplemental info.
> >
> > So it would have to be a global indexed by desc as you suggested.
> > Well that sucks.
> >
>
> Yeah, I don't see any other way if we want to contain this within
> gpiolib-cdev.c. I tried fiddling with notifiers but it went nowhere.
> :(
>

I've got a working patch that uses a global rbtree to contain any
struct line that contains supplemental information required by
the chip to fill out the info, i.e. a non-zero debounce period.
The rbtree keeps the memory overhead minimal, as compared to a radix_tree
or xarray, and only the lines containing supplemental info go in the tree,
so the tree size, and the lookup overhead, is kept to a minimum.
There is no performance impact on general use, though there is a minor
overhead in adding the line to the tree when necessary or doing the lookup
to construct lineinfo.

As well as reducing the visibility of that cdev specific field, it reduces
the size of the gpio_desc at the expense of larger struct line, which is
a net win (as there are fewer struct lines than descs), albeit a small one.

I'll tidy it up and submit it so you can see if that works for you.

Cheers,
Kent.

Re: GPIOLIB locking is broken and how to fix it

[Linus Walleij]

On Thu, Dec 7, 2023 at 7:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> The reason for that is that I'm stuck on some corner-cases related to
> the GPIO <-> pinctrl interaction. Specifically the fact that we have
> GPIOLIB API functions that may be called from atomic context which may
> end up calling into pinctrl where a mutex will be acquired.

OK I see the problem.

> An example of that is any of the GPIO chips that don't set the
> can_sleep field in struct gpio_chip but still use
> gpiochip_generic_config() (e.g. tegra186). We can then encounter the
> following situation:
>
> irq_handler() // in atomic context
>   gpiod_direction_output() // line is open-drain
>     gpio_set_config()
>       gpiochip_generic_config()
>         pinctrl_gpio_set_config()
>           mutex_lock()
>
> Currently we don't take any locks nor synchronize in any other way
> (which is wrong as concurrent gpiod_direction_output() and
> gpiod_direction_input() will get in each other's way).

The only thing that really make sense to protect from here is
concurrent access to the same register (such as if a single
register contains multiple bits to set a number of GPIOs at
output or input).

The real usecases for gpiod_direction_* I know of are limited to:

1. Once when the GPIO is obtained.

2. In strict sequence switching back and forth as in
    drivers/i2c/busses/i2c-cbus-gpio.c
    cbus_transfer()

But *two* execution contexts contesting over *the same* GPIO?
I've never heard of that one. But I'm not sure that is what you mean
to address? Sounds like a theoretical problem to me.

> One of the ideas I have is using the fact that we already use atomic
> bitops in most places. Let's not take locks but add a new flag:
> FLAG_SETTING_DIRECTION. Now when we go into
> gpiod_direction_output/input(), we test and set it. A subsequent call
> will fail with EBUSY or EAGAIN as long as it's set. It will have no
> effect on set/get() - any synchronization will be left to the driver.
> When we're done, we clear it after setting the relevant direction
> flag.

Given that I think the situation is entirely theoretical I'm certainly
happy with that solution.

Whoever want to do this crazy thing can very well teach their code
to recover from errors as well...

> Does this make any sense? There's still the label pointer and debounce
> period stored in the descriptor but these are not accessed in atomic
> context AFAICT.

Sounds fair to me, if it's even a problem. But I trust you, so if you
think this is needed, I'm game!

Yours,
Linus Walleij

Re: GPIOLIB locking is broken and how to fix it

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1926 bytes --]

On Fri, Dec 08, 2023 at 02:12:45PM +0100, Linus Walleij wrote:
> On Thu, Dec 7, 2023 at 7:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 
> > The reason for that is that I'm stuck on some corner-cases related to
> > the GPIO <-> pinctrl interaction. Specifically the fact that we have
> > GPIOLIB API functions that may be called from atomic context which may
> > end up calling into pinctrl where a mutex will be acquired.
> 
> OK I see the problem.
> 
> > An example of that is any of the GPIO chips that don't set the
> > can_sleep field in struct gpio_chip but still use
> > gpiochip_generic_config() (e.g. tegra186). We can then encounter the
> > following situation:
> >
> > irq_handler() // in atomic context
> >   gpiod_direction_output() // line is open-drain
> >     gpio_set_config()
> >       gpiochip_generic_config()
> >         pinctrl_gpio_set_config()
> >           mutex_lock()
> >
> > Currently we don't take any locks nor synchronize in any other way
> > (which is wrong as concurrent gpiod_direction_output() and
> > gpiod_direction_input() will get in each other's way).
> 
> The only thing that really make sense to protect from here is
> concurrent access to the same register (such as if a single
> register contains multiple bits to set a number of GPIOs at
> output or input).
> 
> The real usecases for gpiod_direction_* I know of are limited to:
> 
> 1. Once when the GPIO is obtained.
> 
> 2. In strict sequence switching back and forth as in
>     drivers/i2c/busses/i2c-cbus-gpio.c
>     cbus_transfer()

Isn't this a very special case already? cbus_transfer() holds the spin
lock across the entire function, so it will only work for a very small
set of GPIO providers anyway, right? Anything that's sleepable just is
not going to work. I suspect that direction configuration is then also
not going to sleep, so this should be fine.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski]

On Fri, Dec 8, 2023 at 2:56 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Fri, Dec 08, 2023 at 02:12:45PM +0100, Linus Walleij wrote:
> > On Thu, Dec 7, 2023 at 7:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > > The reason for that is that I'm stuck on some corner-cases related to
> > > the GPIO <-> pinctrl interaction. Specifically the fact that we have
> > > GPIOLIB API functions that may be called from atomic context which may
> > > end up calling into pinctrl where a mutex will be acquired.
> >
> > OK I see the problem.
> >
> > > An example of that is any of the GPIO chips that don't set the
> > > can_sleep field in struct gpio_chip but still use
> > > gpiochip_generic_config() (e.g. tegra186). We can then encounter the
> > > following situation:
> > >
> > > irq_handler() // in atomic context
> > >   gpiod_direction_output() // line is open-drain
> > >     gpio_set_config()
> > >       gpiochip_generic_config()
> > >         pinctrl_gpio_set_config()
> > >           mutex_lock()
> > >
> > > Currently we don't take any locks nor synchronize in any other way
> > > (which is wrong as concurrent gpiod_direction_output() and
> > > gpiod_direction_input() will get in each other's way).
> >
> > The only thing that really make sense to protect from here is
> > concurrent access to the same register (such as if a single
> > register contains multiple bits to set a number of GPIOs at
> > output or input).
> >
> > The real usecases for gpiod_direction_* I know of are limited to:
> >
> > 1. Once when the GPIO is obtained.
> >
> > 2. In strict sequence switching back and forth as in
> >     drivers/i2c/busses/i2c-cbus-gpio.c
> >     cbus_transfer()
>
> Isn't this a very special case already? cbus_transfer() holds the spin
> lock across the entire function, so it will only work for a very small
> set of GPIO providers anyway, right? Anything that's sleepable just is
> not going to work. I suspect that direction configuration is then also
> not going to sleep, so this should be fine.
>

Maybe we could switch to using gpiod_direction_*_raw() here and then
mark regular gpiod_direction_input/output() as might_sleep() and be
done with it? Treat this one as a special-case and then not accept
anyone new calling these from atomic context?

Bart

> Thierry

Re: GPIOLIB locking is broken and how to fix it

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2820 bytes --]

On Fri, Dec 08, 2023 at 03:47:00PM +0100, Bartosz Golaszewski wrote:
> On Fri, Dec 8, 2023 at 2:56 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Fri, Dec 08, 2023 at 02:12:45PM +0100, Linus Walleij wrote:
> > > On Thu, Dec 7, 2023 at 7:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > > The reason for that is that I'm stuck on some corner-cases related to
> > > > the GPIO <-> pinctrl interaction. Specifically the fact that we have
> > > > GPIOLIB API functions that may be called from atomic context which may
> > > > end up calling into pinctrl where a mutex will be acquired.
> > >
> > > OK I see the problem.
> > >
> > > > An example of that is any of the GPIO chips that don't set the
> > > > can_sleep field in struct gpio_chip but still use
> > > > gpiochip_generic_config() (e.g. tegra186). We can then encounter the
> > > > following situation:
> > > >
> > > > irq_handler() // in atomic context
> > > >   gpiod_direction_output() // line is open-drain
> > > >     gpio_set_config()
> > > >       gpiochip_generic_config()
> > > >         pinctrl_gpio_set_config()
> > > >           mutex_lock()
> > > >
> > > > Currently we don't take any locks nor synchronize in any other way
> > > > (which is wrong as concurrent gpiod_direction_output() and
> > > > gpiod_direction_input() will get in each other's way).
> > >
> > > The only thing that really make sense to protect from here is
> > > concurrent access to the same register (such as if a single
> > > register contains multiple bits to set a number of GPIOs at
> > > output or input).
> > >
> > > The real usecases for gpiod_direction_* I know of are limited to:
> > >
> > > 1. Once when the GPIO is obtained.
> > >
> > > 2. In strict sequence switching back and forth as in
> > >     drivers/i2c/busses/i2c-cbus-gpio.c
> > >     cbus_transfer()
> >
> > Isn't this a very special case already? cbus_transfer() holds the spin
> > lock across the entire function, so it will only work for a very small
> > set of GPIO providers anyway, right? Anything that's sleepable just is
> > not going to work. I suspect that direction configuration is then also
> > not going to sleep, so this should be fine.
> >
> 
> Maybe we could switch to using gpiod_direction_*_raw() here and then
> mark regular gpiod_direction_input/output() as might_sleep() and be
> done with it? Treat this one as a special-case and then not accept
> anyone new calling these from atomic context?

Yeah, I2C CBUS already uses gpiod_set_value() in the same context as
gpiod_direction_output()/gpiod_direction_input(), so it would've already
warned about a mismatch anyway. Doing a test-run with the regular
direction accessors marked as might_sleep() should flush out any other
abusers.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski]

On Fri, Dec 8, 2023 at 5:41 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Fri, Dec 08, 2023 at 03:47:00PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Dec 8, 2023 at 2:56 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > On Fri, Dec 08, 2023 at 02:12:45PM +0100, Linus Walleij wrote:
> > > > On Thu, Dec 7, 2023 at 7:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >
> > > > > The reason for that is that I'm stuck on some corner-cases related to
> > > > > the GPIO <-> pinctrl interaction. Specifically the fact that we have
> > > > > GPIOLIB API functions that may be called from atomic context which may
> > > > > end up calling into pinctrl where a mutex will be acquired.
> > > >
> > > > OK I see the problem.
> > > >
> > > > > An example of that is any of the GPIO chips that don't set the
> > > > > can_sleep field in struct gpio_chip but still use
> > > > > gpiochip_generic_config() (e.g. tegra186). We can then encounter the
> > > > > following situation:
> > > > >
> > > > > irq_handler() // in atomic context
> > > > >   gpiod_direction_output() // line is open-drain
> > > > >     gpio_set_config()
> > > > >       gpiochip_generic_config()
> > > > >         pinctrl_gpio_set_config()
> > > > >           mutex_lock()
> > > > >
> > > > > Currently we don't take any locks nor synchronize in any other way
> > > > > (which is wrong as concurrent gpiod_direction_output() and
> > > > > gpiod_direction_input() will get in each other's way).
> > > >
> > > > The only thing that really make sense to protect from here is
> > > > concurrent access to the same register (such as if a single
> > > > register contains multiple bits to set a number of GPIOs at
> > > > output or input).
> > > >
> > > > The real usecases for gpiod_direction_* I know of are limited to:
> > > >
> > > > 1. Once when the GPIO is obtained.
> > > >
> > > > 2. In strict sequence switching back and forth as in
> > > >     drivers/i2c/busses/i2c-cbus-gpio.c
> > > >     cbus_transfer()
> > >
> > > Isn't this a very special case already? cbus_transfer() holds the spin
> > > lock across the entire function, so it will only work for a very small
> > > set of GPIO providers anyway, right? Anything that's sleepable just is
> > > not going to work. I suspect that direction configuration is then also
> > > not going to sleep, so this should be fine.
> > >
> >
> > Maybe we could switch to using gpiod_direction_*_raw() here and then
> > mark regular gpiod_direction_input/output() as might_sleep() and be
> > done with it? Treat this one as a special-case and then not accept
> > anyone new calling these from atomic context?
>
> Yeah, I2C CBUS already uses gpiod_set_value() in the same context as
> gpiod_direction_output()/gpiod_direction_input(), so it would've already
> warned about a mismatch anyway. Doing a test-run with the regular
> direction accessors marked as might_sleep() should flush out any other
> abusers.
>
> Thierry

We cannot possibly test all drivers out there but we can start out by
adding: `WARN_ON(in_atomic())` to the direction setters and getters
for the next release and then possibly switch to a full might_sleep()
if nobody complains?

Bart

Re: GPIOLIB locking is broken and how to fix it

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3693 bytes --]

On Fri, Dec 08, 2023 at 07:30:36PM +0100, Bartosz Golaszewski wrote:
> On Fri, Dec 8, 2023 at 5:41 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Fri, Dec 08, 2023 at 03:47:00PM +0100, Bartosz Golaszewski wrote:
> > > On Fri, Dec 8, 2023 at 2:56 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > >
> > > > On Fri, Dec 08, 2023 at 02:12:45PM +0100, Linus Walleij wrote:
> > > > > On Thu, Dec 7, 2023 at 7:38 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > >
> > > > > > The reason for that is that I'm stuck on some corner-cases related to
> > > > > > the GPIO <-> pinctrl interaction. Specifically the fact that we have
> > > > > > GPIOLIB API functions that may be called from atomic context which may
> > > > > > end up calling into pinctrl where a mutex will be acquired.
> > > > >
> > > > > OK I see the problem.
> > > > >
> > > > > > An example of that is any of the GPIO chips that don't set the
> > > > > > can_sleep field in struct gpio_chip but still use
> > > > > > gpiochip_generic_config() (e.g. tegra186). We can then encounter the
> > > > > > following situation:
> > > > > >
> > > > > > irq_handler() // in atomic context
> > > > > >   gpiod_direction_output() // line is open-drain
> > > > > >     gpio_set_config()
> > > > > >       gpiochip_generic_config()
> > > > > >         pinctrl_gpio_set_config()
> > > > > >           mutex_lock()
> > > > > >
> > > > > > Currently we don't take any locks nor synchronize in any other way
> > > > > > (which is wrong as concurrent gpiod_direction_output() and
> > > > > > gpiod_direction_input() will get in each other's way).
> > > > >
> > > > > The only thing that really make sense to protect from here is
> > > > > concurrent access to the same register (such as if a single
> > > > > register contains multiple bits to set a number of GPIOs at
> > > > > output or input).
> > > > >
> > > > > The real usecases for gpiod_direction_* I know of are limited to:
> > > > >
> > > > > 1. Once when the GPIO is obtained.
> > > > >
> > > > > 2. In strict sequence switching back and forth as in
> > > > >     drivers/i2c/busses/i2c-cbus-gpio.c
> > > > >     cbus_transfer()
> > > >
> > > > Isn't this a very special case already? cbus_transfer() holds the spin
> > > > lock across the entire function, so it will only work for a very small
> > > > set of GPIO providers anyway, right? Anything that's sleepable just is
> > > > not going to work. I suspect that direction configuration is then also
> > > > not going to sleep, so this should be fine.
> > > >
> > >
> > > Maybe we could switch to using gpiod_direction_*_raw() here and then
> > > mark regular gpiod_direction_input/output() as might_sleep() and be
> > > done with it? Treat this one as a special-case and then not accept
> > > anyone new calling these from atomic context?
> >
> > Yeah, I2C CBUS already uses gpiod_set_value() in the same context as
> > gpiod_direction_output()/gpiod_direction_input(), so it would've already
> > warned about a mismatch anyway. Doing a test-run with the regular
> > direction accessors marked as might_sleep() should flush out any other
> > abusers.
> >
> > Thierry
> 
> We cannot possibly test all drivers out there but we can start out by
> adding: `WARN_ON(in_atomic())` to the direction setters and getters
> for the next release and then possibly switch to a full might_sleep()
> if nobody complains?

What's the harm in using might_sleep() right away? If there's a lot of
problems we need to back out the change anyway, so whether we back out
the WARN_ON() or the might_sleep() doesn't really make a difference.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski]

On Mon, Dec 11, 2023 at 11:55 AM Thierry Reding
<thierry.reding@gmail.com> wrote:
>
> On Fri, Dec 08, 2023 at 07:30:36PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Dec 8, 2023 at 5:41 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > >

[snip]

> > >
> > > Yeah, I2C CBUS already uses gpiod_set_value() in the same context as
> > > gpiod_direction_output()/gpiod_direction_input(), so it would've already
> > > warned about a mismatch anyway. Doing a test-run with the regular
> > > direction accessors marked as might_sleep() should flush out any other
> > > abusers.
> > >
> > > Thierry
> >
> > We cannot possibly test all drivers out there but we can start out by
> > adding: `WARN_ON(in_atomic())` to the direction setters and getters
> > for the next release and then possibly switch to a full might_sleep()
> > if nobody complains?
>
> What's the harm in using might_sleep() right away? If there's a lot of
> problems we need to back out the change anyway, so whether we back out
> the WARN_ON() or the might_sleep() doesn't really make a difference.
>

No harm but it turns out that gpiod_direction_input() may also end up
calling into .set_config() (via gpio_set_bias()) so we have the same
problem without the _raw variant to bail us out. I don't want to
introduce it for a single legacy driver that is causing trouble.

Adding Aaro to Cc.

Aaro: do you still have the HW to test this driver? I understand the
need to disable interrupts during writing or reading data - when we
are driving the clock line. But do we also absolutely need to hold the
spinlock when setting the direction of the data line to input? Because
if we don't then we could modify the last remaining offender to not
set GPIO direction with a spinlock held and finally make
gpiod_direction_*() functions sleepable.

Bart

Re: GPIOLIB locking is broken and how to fix it

[Aaro Koskinen]

Hi,

On Mon, Dec 11, 2023 at 04:49:43PM +0100, Bartosz Golaszewski wrote:
> Aaro: do you still have the HW to test this driver?

Yes, and I still use it.

> I understand the need to disable interrupts during writing or reading
> data - when we are driving the clock line. But do we also absolutely
> need to hold the spinlock when setting the direction of the data line
> to input? Because if we don't then we could modify the last remaining
> offender to not set GPIO direction with a spinlock held and finally make
> gpiod_direction_*() functions sleepable.

Hmm, I think it's required to be able to provide atomic xfer function.
That is needed for e.g. for power off.

A.

Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski]

On Tue, Dec 12, 2023 at 11:12 AM Aaro Koskinen <aaro.koskinen@iki.fi> wrote:
>
> Hi,
>
> On Mon, Dec 11, 2023 at 04:49:43PM +0100, Bartosz Golaszewski wrote:
> > Aaro: do you still have the HW to test this driver?
>
> Yes, and I still use it.
>
> > I understand the need to disable interrupts during writing or reading
> > data - when we are driving the clock line. But do we also absolutely
> > need to hold the spinlock when setting the direction of the data line
> > to input? Because if we don't then we could modify the last remaining
> > offender to not set GPIO direction with a spinlock held and finally make
> > gpiod_direction_*() functions sleepable.
>
> Hmm, I think it's required to be able to provide atomic xfer function.
> That is needed for e.g. for power off.
>

By xfer: do you mean a request-response pair? Or just a single atomic,
one-way transfer of data?

Bart

> A.

Re: GPIOLIB locking is broken and how to fix it

[Aaro Koskinen]

Hi,

On Tue, Dec 12, 2023 at 12:00:11PM +0100, Bartosz Golaszewski wrote:
> On Tue, Dec 12, 2023 at 11:12 AM Aaro Koskinen <aaro.koskinen@iki.fi> wrote:
> > On Mon, Dec 11, 2023 at 04:49:43PM +0100, Bartosz Golaszewski wrote:
> > > Aaro: do you still have the HW to test this driver?
> >
> > Yes, and I still use it.
> >
> > > I understand the need to disable interrupts during writing or reading
> > > data - when we are driving the clock line. But do we also absolutely
> > > need to hold the spinlock when setting the direction of the data line
> > > to input? Because if we don't then we could modify the last remaining
> > > offender to not set GPIO direction with a spinlock held and finally make
> > > gpiod_direction_*() functions sleepable.
> >
> > Hmm, I think it's required to be able to provide atomic xfer function.
> > That is needed for e.g. for power off.
> 
> By xfer: do you mean a request-response pair? Or just a single atomic,
> one-way transfer of data?

It's reading a register, then writing it back.

A.

Re: GPIOLIB locking is broken and how to fix it

[Bartosz Golaszewski]

On Tue, Dec 12, 2023 at 3:32 PM Aaro Koskinen <aaro.koskinen@iki.fi> wrote:
>
> Hi,
>
> On Tue, Dec 12, 2023 at 12:00:11PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Dec 12, 2023 at 11:12 AM Aaro Koskinen <aaro.koskinen@iki.fi> wrote:
> > > On Mon, Dec 11, 2023 at 04:49:43PM +0100, Bartosz Golaszewski wrote:
> > > > Aaro: do you still have the HW to test this driver?
> > >
> > > Yes, and I still use it.
> > >
> > > > I understand the need to disable interrupts during writing or reading
> > > > data - when we are driving the clock line. But do we also absolutely
> > > > need to hold the spinlock when setting the direction of the data line
> > > > to input? Because if we don't then we could modify the last remaining
> > > > offender to not set GPIO direction with a spinlock held and finally make
> > > > gpiod_direction_*() functions sleepable.
> > >
> > > Hmm, I think it's required to be able to provide atomic xfer function.
> > > That is needed for e.g. for power off.
> >
> > By xfer: do you mean a request-response pair? Or just a single atomic,
> > one-way transfer of data?
>
> It's reading a register, then writing it back.
>
> A.

Well, I would consider it a valid use-case of changing line direction
where it can't sleep.

Maybe we need to have gpio_direction_input/output_raw_atomic() which
would have very simple semantics? Like no locking provided by gpiolib,
no additional flags interpreted (like open-drain, open-source, bias,
etc.) and no mutex held? It would be up to users who *really* need to
set direction in non-sleeping context to handle this correctly.

We could use it in this driver and possibly in any other which would
fail once we mark the original functions as "might_sleep()".

Bart

Re: GPIOLIB locking is broken and how to fix it

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1317 bytes --]

On Thu, Dec 07, 2023 at 07:37:54PM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 28, 2023 at 11:47 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> 
> [snip]
> 
> >
> > Because years of technical debt, that's why. :)
> >
> 
> Speaking of technical debt: you may have noticed that despite stating
> I'm almost done last week, I still haven't submitted my locking
> rework.
> 
> The reason for that is that I'm stuck on some corner-cases related to
> the GPIO <-> pinctrl interaction. Specifically the fact that we have
> GPIOLIB API functions that may be called from atomic context which may
> end up calling into pinctrl where a mutex will be acquired.
> 
> An example of that is any of the GPIO chips that don't set the
> can_sleep field in struct gpio_chip but still use
> gpiochip_generic_config() (e.g. tegra186). We can then encounter the
> following situation:
> 
> irq_handler() // in atomic context
>   gpiod_direction_output() // line is open-drain
>     gpio_set_config()
>       gpiochip_generic_config()
>         pinctrl_gpio_set_config()
>           mutex_lock()

I don't know of any case (at least on Tegra) where we would change
direction in atomic context. In fact, I would argue that configuration
like this should never happen in atomic context.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: GPIOLIB locking is broken and how to fix it

[Linus Walleij]

On Sun, Nov 26, 2023 at 1:06 AM Kent Gibson <warthog618@gmail.com> wrote:

> So why allow the usage of non-requested descriptors?

Mostly boardfiles in arch/arm and arch/mips do this thing, by way of
the old API using global numbers (that get translated into descriptors).

You can't even blame them: when some of the code was written,
there was no gpio_request() even. David Brownell added that in a
generic way 2008.

We are doing away with it, one driver at a time, one board at a time.

I think I finally purged it from all OMAP boards in v6.7-rc1!

Yours,
Linus Walleij