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
