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

[Hans Verkuil <hverkuil@xxxxxxxxx>]

On 05/12/2021 01:18, Linus Walleij wrote:
> On Wed, Dec 1, 2021 at 10:40 AM Hans Verkuil <hverkuil@xxxxxxxxx> 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
>