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

[Hans Verkuil <hverkuil@xxxxxxxxx>]

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@xxxxxxxxx> 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
>