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

[Petko Manolov <petko.manolov@xxxxxxxxxxxx>]

On 21-12-05 01:18:23, 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 :)

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