gpiod_set_value(): BUG: sleeping function called from invalid context

gpiod_set_value(): BUG: sleeping function called from invalid context

[Hans Verkuil]

Hi all,

While testing the media cec-gpio driver on a Raspberry Pi 4B with CONFIG_DEBUG_ATOMIC_SLEEP
set I got this stack trace:

[ 1674.731319] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:281
[ 1674.739891] in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 0, name: swapper/3
[ 1674.748011] Preemption disabled at:
[ 1674.748015] [<ffff80001019887c>] find_vm_area+0x28/0x9c
[ 1674.756851] CPU: 3 PID: 0 Comm: swapper/3 Tainted: G         C        5.14.0-hdmi-dbg #58
[ 1674.765149] Hardware name: Raspberry Pi 4 Model B Rev 1.4 (DT)
[ 1674.771061] Call trace:
[ 1674.773534]  dump_backtrace+0x0/0x1b0
[ 1674.777251]  show_stack+0x18/0x24
[ 1674.780612]  dump_stack_lvl+0x68/0x84
[ 1674.784326]  dump_stack+0x18/0x34
[ 1674.787685]  ___might_sleep+0x148/0x180
[ 1674.791572]  __might_sleep+0x54/0x90
[ 1674.795195]  mutex_lock+0x28/0x80
[ 1674.798556]  pinctrl_get_device_gpio_range+0x3c/0x110
[ 1674.803677]  pinctrl_gpio_direction+0x3c/0xd0
[ 1674.808093]  pinctrl_gpio_direction_output+0x14/0x20
[ 1674.813124]  bcm2835_gpio_direction_output+0x64/0x80
[ 1674.818160]  gpio_set_open_drain_value_commit+0x6c/0xf0
[ 1674.823458]  gpiod_set_value_nocheck+0x8c/0xb0
[ 1674.827961]  gpiod_set_value+0x4c/0xd0
[ 1674.831760]  cec_gpio_low+0x34/0x40
[ 1674.835295]  cec_pin_timer+0x58c/0x11c4
[ 1674.839182]  __hrtimer_run_queues+0x140/0x1e0
[ 1674.843602]  hrtimer_interrupt+0xf4/0x2cc
[ 1674.847668]  arch_timer_handler_phys+0x38/0x50
[ 1674.852174]  handle_percpu_devid_irq+0x9c/0x160
[ 1674.856768]  handle_domain_irq+0x98/0xe4
[ 1674.860742]  gic_handle_irq+0x4c/0xd0
[ 1674.864452]  call_on_irq_stack+0x2c/0x5c
[ 1674.868428]  do_interrupt_handler+0x54/0x60
[ 1674.872669]  el1_interrupt+0x30/0x80
[ 1674.876291]  el1h_64_irq_handler+0x18/0x24
[ 1674.880443]  el1h_64_irq+0x78/0x7c
[ 1674.883889]  arch_cpu_idle+0x18/0x2c
[ 1674.887512]  default_idle_call+0x28/0x74
[ 1674.891486]  do_idle+0x23c/0x27c
[ 1674.894754]  cpu_startup_entry+0x24/0x70
[ 1674.898725]  secondary_start_kernel+0x140/0x164
[ 1674.903315]  __secondary_switched+0x94/0x98

The drivers/media/cec/platform/cec-gpio/cec-gpio.c driver uses an open drain
gpio connected to the HDMI CEC pin. From the dts:

        cec-gpio-sink {
                compatible = "cec-gpio";
                cec-gpios = <&gpio 6 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
                hpd-gpios = <&gpio 23 GPIO_ACTIVE_HIGH>;
        };

gpiod_set_value() is supposed to be usable from an atomic context, so this
appears to be a bug. It's probably been there for quite a long time. I suspect
OPEN_DRAIN isn't very common, and I think this might be the first time I tested
this driver with this kernel config option set.

Any suggestions?

Regards,

	Hans

Re: gpiod_set_value(): BUG: sleeping function called from invalid context

[Linus Walleij]

On Wed, Dec 1, 2021 at 10:40 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:

> [ 1674.787685]  ___might_sleep+0x148/0x180
> [ 1674.791572]  __might_sleep+0x54/0x90
> [ 1674.795195]  mutex_lock+0x28/0x80
> [ 1674.798556]  pinctrl_get_device_gpio_range+0x3c/0x110

There is the error ^

> gpiod_set_value() is supposed to be usable from an atomic context, so this
> appears to be a bug. It's probably been there for quite a long time. I suspect
> OPEN_DRAIN isn't very common, and I think this might be the first time I tested
> this driver with this kernel config option set.

Nah has nothing to do with open drain :)

> Any suggestions?

pinctrl_get_device_gpio_range() needs to be modified to not take a mutex
because mutex:es can sleep.

static int pinctrl_get_device_gpio_range(unsigned gpio,
                                         struct pinctrl_dev **outdev,
                                         struct pinctrl_gpio_range **outrange)
{
        struct pinctrl_dev *pctldev;

        mutex_lock(&pinctrldev_list_mutex);

BLAM!

And this definitely needs to be called on this path so no way out
of that.

This mutex pinctrldev_list_mutex is there to protect from devices coming
and going as we look for devices with ranges on.

One way to solve it would be to simply replace it with a spinlock, maybe
that works? (Check the code carefully so there are no things like calls
into drivers that may fire interrupts etc...)

If it still need to be sleepable (mutex-ish) you need to convert it to
use RCU I think? (Which would be pretty cool anyway.)

Yours,
Linus Walleij

Re: gpiod_set_value(): BUG: sleeping function called from invalid context

[Petko Manolov]

On 21-12-05 01:18:23, Linus Walleij wrote:
> On Wed, Dec 1, 2021 at 10:40 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
> > [ 1674.787685]  ___might_sleep+0x148/0x180
> > [ 1674.791572]  __might_sleep+0x54/0x90
> > [ 1674.795195]  mutex_lock+0x28/0x80
> > [ 1674.798556]  pinctrl_get_device_gpio_range+0x3c/0x110
> 
> There is the error ^
> 
> > gpiod_set_value() is supposed to be usable from an atomic context, so this 
> > appears to be a bug. It's probably been there for quite a long time. I 
> > suspect OPEN_DRAIN isn't very common, and I think this might be the first 
> > time I tested this driver with this kernel config option set.
> 
> Nah has nothing to do with open drain :)

OPEN_DRAIN is regularly used in single wire (1W) bit-bang drivers, so no, it 
isn't related to this flag.

> > Any suggestions?
> 
> pinctrl_get_device_gpio_range() needs to be modified to not take a mutex 
> because mutex:es can sleep.
> 
> static int pinctrl_get_device_gpio_range(unsigned gpio,
>                                          struct pinctrl_dev **outdev,
>                                          struct pinctrl_gpio_range **outrange)
> {
>         struct pinctrl_dev *pctldev;
> 
>         mutex_lock(&pinctrldev_list_mutex);
> 
> BLAM!
> 
> And this definitely needs to be called on this path so no way out of that.
> 
> This mutex pinctrldev_list_mutex is there to protect from devices coming and 
> going as we look for devices with ranges on.
> 
> One way to solve it would be to simply replace it with a spinlock, maybe that 
> works? (Check the code carefully so there are no things like calls into 
> drivers that may fire interrupts etc...)

Have not seen the code in discussion, but unless the spinlock is taken for a 
really short period of time, an alternative should be used.  I am not sure 
refcount would do the trick here, though.

> If it still need to be sleepable (mutex-ish) you need to convert it to use RCU 
> I think? (Which would be pretty cool anyway.)

Yeah, this might be RCU-able and, if done, will also make Paul happier. ;)


		Petko

Re: gpiod_set_value(): BUG: sleeping function called from invalid context

[Hans Verkuil]

On 05/12/2021 01:18, Linus Walleij wrote:
> On Wed, Dec 1, 2021 at 10:40 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
>> [ 1674.787685]  ___might_sleep+0x148/0x180
>> [ 1674.791572]  __might_sleep+0x54/0x90
>> [ 1674.795195]  mutex_lock+0x28/0x80
>> [ 1674.798556]  pinctrl_get_device_gpio_range+0x3c/0x110
> 
> There is the error ^
> 
>> gpiod_set_value() is supposed to be usable from an atomic context, so this
>> appears to be a bug. It's probably been there for quite a long time. I suspect
>> OPEN_DRAIN isn't very common, and I think this might be the first time I tested
>> this driver with this kernel config option set.
> 
> Nah has nothing to do with open drain :)
> 
>> Any suggestions?
> 
> pinctrl_get_device_gpio_range() needs to be modified to not take a mutex
> because mutex:es can sleep.
> 
> static int pinctrl_get_device_gpio_range(unsigned gpio,
>                                          struct pinctrl_dev **outdev,
>                                          struct pinctrl_gpio_range **outrange)
> {
>         struct pinctrl_dev *pctldev;
> 
>         mutex_lock(&pinctrldev_list_mutex);
> 
> BLAM!
> 
> And this definitely needs to be called on this path so no way out
> of that.

Any idea why this hasn't been seen/reported before? According to git blame
that mutex_lock has been there since 2013. Does nobody test with
CONFIG_DEBUG_ATOMIC_SLEEP? Am I really the first to encounter this?

Before spending time on this I'd like to be sure that, yes, I'm really the
first in 8 years to see this, and this wasn't introduced by something else.

> 
> This mutex pinctrldev_list_mutex is there to protect from devices coming
> and going as we look for devices with ranges on.
> 
> One way to solve it would be to simply replace it with a spinlock, maybe
> that works? (Check the code carefully so there are no things like calls
> into drivers that may fire interrupts etc...)
> 
> If it still need to be sleepable (mutex-ish) you need to convert it to
> use RCU I think? (Which would be pretty cool anyway.)

RCU seems like a reasonable alternative, but I will have to read up on it
since it's the first time I'll be using this. All those quizzes in the RCU
docs (1) scare the hell out of me :-)

Regards,

	Hans

(1) https://www.kernel.org/doc/html/latest/RCU/listRCU.html

> 
> Yours,
> Linus Walleij
> 

Re: gpiod_set_value(): BUG: sleeping function called from invalid context

[Hans Verkuil]

On 05/12/2021 11:45, Hans Verkuil wrote:
> On 05/12/2021 01:18, Linus Walleij wrote:
>> On Wed, Dec 1, 2021 at 10:40 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>>> [ 1674.787685]  ___might_sleep+0x148/0x180
>>> [ 1674.791572]  __might_sleep+0x54/0x90
>>> [ 1674.795195]  mutex_lock+0x28/0x80
>>> [ 1674.798556]  pinctrl_get_device_gpio_range+0x3c/0x110
>>
>> There is the error ^
>>
>>> gpiod_set_value() is supposed to be usable from an atomic context, so this
>>> appears to be a bug. It's probably been there for quite a long time. I suspect
>>> OPEN_DRAIN isn't very common, and I think this might be the first time I tested
>>> this driver with this kernel config option set.
>>
>> Nah has nothing to do with open drain :)
>>
>>> Any suggestions?
>>
>> pinctrl_get_device_gpio_range() needs to be modified to not take a mutex
>> because mutex:es can sleep.
>>
>> static int pinctrl_get_device_gpio_range(unsigned gpio,
>>                                          struct pinctrl_dev **outdev,
>>                                          struct pinctrl_gpio_range **outrange)
>> {
>>         struct pinctrl_dev *pctldev;
>>
>>         mutex_lock(&pinctrldev_list_mutex);
>>
>> BLAM!
>>
>> And this definitely needs to be called on this path so no way out
>> of that.
> 
> Any idea why this hasn't been seen/reported before? According to git blame
> that mutex_lock has been there since 2013. Does nobody test with
> CONFIG_DEBUG_ATOMIC_SLEEP? Am I really the first to encounter this?
> 
> Before spending time on this I'd like to be sure that, yes, I'm really the
> first in 8 years to see this, and this wasn't introduced by something else.
> 
>>
>> This mutex pinctrldev_list_mutex is there to protect from devices coming
>> and going as we look for devices with ranges on.
>>
>> One way to solve it would be to simply replace it with a spinlock, maybe
>> that works? (Check the code carefully so there are no things like calls
>> into drivers that may fire interrupts etc...)
>>
>> If it still need to be sleepable (mutex-ish) you need to convert it to
>> use RCU I think? (Which would be pretty cool anyway.)
> 
> RCU seems like a reasonable alternative, but I will have to read up on it
> since it's the first time I'll be using this. All those quizzes in the RCU
> docs (1) scare the hell out of me :-)

It's worse: pinctrl_get_device_gpio_range() takes pinctrldev_list_mutex, but
in the loop it calls pinctrl_match_gpio_range() which calls
mutex_lock(&pctldev->mutex). So it's two mutexes that this function takes from
what is supposed to be atomic context.

In addition, pinctrl_get_device_gpio_range() is called from pinctrl_gpio_direction(),
which also takes pctldev->mutex afterwards.

I'm not sure if the pctldev->mutex can be replaced with rcu, I suspect not.

For something that's supposed to be atomic, there seem to be an awful lot of mutexes...

In the case of the bcm2711 the bcm2835_gpio_direction_output() function calls
pinctrl_gpio_direction_output, which in turn (via pinctrl_gpio_direction()
and pinmux_gpio_direction()) calls bcm2835_pmx_gpio_set_direction().

It appears to me that bcm2835_gpio_direction_output() can call
bcm2835_pmx_gpio_set_direction() directly, thus avoiding all the mutexes.
That's exactly what samsung/pinctrl-samsung.c and qcom/pinctrl-msm.c do,
from what I can tell.

Are pinctrl drivers supposed to call pinctrl_gpio_direction_output() from
the direction_output() op? Isn't that perhaps the bug?

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
> (1) https://www.kernel.org/doc/html/latest/RCU/listRCU.html
> 
>>
>> Yours,
>> Linus Walleij
>>
> 

Re: gpiod_set_value(): BUG: sleeping function called from invalid context

[Petko Manolov]

On 21-12-05 12:59:46, Hans Verkuil wrote:
>

<snip>

> I'm not sure if the pctldev->mutex can be replaced with rcu, I suspect not.

Not in this case. :)

> For something that's supposed to be atomic, there seem to be an awful lot of 
> mutexes...

By looking at the generic gpiolib code, it seems that none of these are supposed 
to be called from atomic context.

> In the case of the bcm2711 the bcm2835_gpio_direction_output() function calls
> pinctrl_gpio_direction_output, which in turn (via pinctrl_gpio_direction()
> and pinmux_gpio_direction()) calls bcm2835_pmx_gpio_set_direction().

Is the aforementioned code mainlined or is it in some other git tree?  I'd like 
to take a quick look.

> It appears to me that bcm2835_gpio_direction_output() can call
> bcm2835_pmx_gpio_set_direction() directly, thus avoiding all the mutexes.
> That's exactly what samsung/pinctrl-samsung.c and qcom/pinctrl-msm.c do,
> from what I can tell.
> 
> Are pinctrl drivers supposed to call pinctrl_gpio_direction_output() from the 
> direction_output() op? Isn't that perhaps the bug?

If direction_output() is supposed to be called from atomic context, then that's 
where the bug is.  Again, i am not familiar with this particular subsystem, but 
these are pretty basic kernel concepts people must pay attention to.  You should 
perhaps focus on Broadcom's code rather than pinctrl's.


		Petko

Re: gpiod_set_value(): BUG: sleeping function called from invalid context

[Hans Verkuil]

On 05/12/2021 15:49, Petko Manolov wrote:
> On 21-12-05 12:59:46, Hans Verkuil wrote:
>>
> 
> <snip>
> 
>> I'm not sure if the pctldev->mutex can be replaced with rcu, I suspect not.
> 
> Not in this case. :)
> 
>> For something that's supposed to be atomic, there seem to be an awful lot of 
>> mutexes...
> 
> By looking at the generic gpiolib code, it seems that none of these are supposed 
> to be called from atomic context.
> 
>> In the case of the bcm2711 the bcm2835_gpio_direction_output() function calls
>> pinctrl_gpio_direction_output, which in turn (via pinctrl_gpio_direction()
>> and pinmux_gpio_direction()) calls bcm2835_pmx_gpio_set_direction().
> 
> Is the aforementioned code mainlined or is it in some other git tree?  I'd like 
> to take a quick look.

Mainline.

> 
>> It appears to me that bcm2835_gpio_direction_output() can call
>> bcm2835_pmx_gpio_set_direction() directly, thus avoiding all the mutexes.
>> That's exactly what samsung/pinctrl-samsung.c and qcom/pinctrl-msm.c do,
>> from what I can tell.
>>
>> Are pinctrl drivers supposed to call pinctrl_gpio_direction_output() from the 
>> direction_output() op? Isn't that perhaps the bug?
> 
> If direction_output() is supposed to be called from atomic context, then that's 
> where the bug is.  Again, i am not familiar with this particular subsystem, but 
> these are pretty basic kernel concepts people must pay attention to.  You should 
> perhaps focus on Broadcom's code rather than pinctrl's.

That's my suspicion as well, except that the same thing can be seen in several
other non-Broadcom pinctrl drivers as well. Probably copy-and-paste.

Linus, can you confirm that it is indeed the Broadcom pinctrl driver that's wrong?
And thus several others as well, 'git grep pinctrl_gpio_direction_output drivers'
reports a bunch of them.

Regards,

	Hans

Re: gpiod_set_value(): BUG: sleeping function called from invalid context

[Linus Walleij]

On Sun, Dec 5, 2021 at 11:45 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:

> Any idea why this hasn't been seen/reported before? According to git blame
> that mutex_lock has been there since 2013. Does nobody test with
> CONFIG_DEBUG_ATOMIC_SLEEP? Am I really the first to encounter this?
>
> Before spending time on this I'd like to be sure that, yes, I'm really the
> first in 8 years to see this, and this wasn't introduced by something else.

Probably because people don't CONFIG_DEBUG_ATOMIC_SLEEP
so much and then it will also work fine unless they have a GPIO
that is backed by a pin control driver, because it's only then that
you have to go and dereference ranges.

There are lots of GPIO drivers not backed by pin control drivers.

Already at two variables the test matrix is big enough that people
don't really get there to test it...

> > If it still need to be sleepable (mutex-ish) you need to convert it to
> > use RCU I think? (Which would be pretty cool anyway.)
>
> RCU seems like a reasonable alternative, but I will have to read up on it
> since it's the first time I'll be using this. All those quizzes in the RCU
> docs (1) scare the hell out of me :-)

Don't say this to Paul McKenney he is very invested in the quizzes,
haha. I think this comes from authors such as Donald Knuth who use
them as educational tools.

Yours,
Linus Walleij

Re: gpiod_set_value(): BUG: sleeping function called from invalid context

[Linus Walleij]

On Sun, Dec 5, 2021 at 12:59 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:

> Are pinctrl drivers supposed to call pinctrl_gpio_direction_output() from
> the direction_output() op? Isn't that perhaps the bug?

Yes they are. But they might not have to.

Here is how pinctrl_gpio_direction_input() and pinctrl_gpio_direction_output()
work:

pinctrl_gpio_direction_output()
   pinctrl_gpio_direction(OUTPUT)
       pinctrl_get_device_gpio_range() <- find the range
       gpio_to_pin() <- Look up corresponding pin on the pin controller
       pinmux_gpio_direction() <- shortcut to set the pin into right direction
            ops->gpio_set_direction() <- Call the pin mux driver

So GPIO drivers that are backed by pin controllers cross call to set
up the pin controller properties of the pin into the right mode.
So it becomes a GPIO line with the expected direction from the pin
control side of things.

This is especially necessary if the GPIO driver and pin control (mux)
driver are two different drivers that communicate with each other like
this.

This happens for example with drivers/gpio/gpio-pl061.c that is
used with a few different pin controllers, so we don't know which
pin controller is providing the pin control back-end.

For combined GPIO and pin controllers (all of it in the same file)
it is of course possible to shortcut this cumbersome path and just
call whatever mux function we need instead of going in and
out of the subsystems like this. That should be done dropping
some comment but drivers are free to do what they like
to optimize.

You can maybe fix the BCM2835 like this, but that will not fix
the problem for everyone else.

I'd prefer if everyone called pinctrl_gpio_direction_output() and
we got this fixed... because many are affected.

Maybe we should just make these mutexes into spinlocks
if no other solution is possible? It's not like this is
called constantly other than by the CEC driver ;)

Yours,
Linus Walleij

Re: gpiod_set_value(): BUG: sleeping function called from invalid context

[Hans Verkuil]

Hi Linus,

Thank you for your reply!

On 06/12/2021 01:32, Linus Walleij wrote:
> On Sun, Dec 5, 2021 at 12:59 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
>> Are pinctrl drivers supposed to call pinctrl_gpio_direction_output() from
>> the direction_output() op? Isn't that perhaps the bug?
> 
> Yes they are. But they might not have to.
> 
> Here is how pinctrl_gpio_direction_input() and pinctrl_gpio_direction_output()
> work:
> 
> pinctrl_gpio_direction_output()
>    pinctrl_gpio_direction(OUTPUT)
>        pinctrl_get_device_gpio_range() <- find the range
>        gpio_to_pin() <- Look up corresponding pin on the pin controller
>        pinmux_gpio_direction() <- shortcut to set the pin into right direction
>             ops->gpio_set_direction() <- Call the pin mux driver
> 
> So GPIO drivers that are backed by pin controllers cross call to set
> up the pin controller properties of the pin into the right mode.
> So it becomes a GPIO line with the expected direction from the pin
> control side of things.
> 
> This is especially necessary if the GPIO driver and pin control (mux)
> driver are two different drivers that communicate with each other like
> this.
> 
> This happens for example with drivers/gpio/gpio-pl061.c that is
> used with a few different pin controllers, so we don't know which
> pin controller is providing the pin control back-end.

That's fair enough, but in that case I think the GPIO driver should set
can_sleep to true. Perhaps the pinctrl core can even check that can_sleep
is set to true if it attempts to call pinctrl_gpio_direction().

> 
> For combined GPIO and pin controllers (all of it in the same file)
> it is of course possible to shortcut this cumbersome path and just
> call whatever mux function we need instead of going in and
> out of the subsystems like this. That should be done dropping
> some comment but drivers are free to do what they like
> to optimize.

I believe that's the correct approach for such combined GPIO and pin
controllers. That whole loop through pinctrl_gpio_direction_output/input
just adds unnecessary complexity. Which is fine if you have no choice,
but wasteful otherwise.

> 
> You can maybe fix the BCM2835 like this, but that will not fix
> the problem for everyone else.
> 
> I'd prefer if everyone called pinctrl_gpio_direction_output() and
> we got this fixed... because many are affected.
> 
> Maybe we should just make these mutexes into spinlocks
> if no other solution is possible? It's not like this is
> called constantly other than by the CEC driver ;)

It's likely also useful for i2c-gpio.c. I see that that one uses
gpiod_set_value_cansleep(), so using mutexes is fine, but avoiding that loop
through pinctrl will help improve timings there as well, even without
lock contention.

I'll post some patches, that should help discussing this.

Regards,

	Hans

> 
> Yours,
> Linus Walleij
>