Re: Question: pinctrl-backed GPIO set_config and gpio_chip::can_sleep

[Bartosz Golaszewski]

On Thu, 18 Jun 2026 15:15:30 +0200, Linus Walleij <linusw@xxxxxxxxxx> said:
> Hi Runyu,
>
> thanks for your analysis!
>
> On Thu, Jun 18, 2026 at 7:42 AM Runyu Xiao <runyu.xiao@xxxxxxxxxx> wrote:
>
>> The path we are concerned about is:
>>
>>   gpiod_set_config()
>>     -> gpio_do_set_config()
>>        -> gpiochip_generic_config()
>>        -> pinctrl_gpio_set_config()
>>        -> pinctrl_get_device_gpio_range()
>>        -> mutex_lock(&pctldev->mutex)
>
> That would be mutex_lock(&pinctrldev_list_mutex); would it not?
>
>> If gpiod_cansleep() returns false, a GPIO forwarder or another consumer
>> can choose an atomic carrier and call gpiod_set_config() while holding a
>> spinlock.
>
> I see the problem.
>
>> The local draft I am considering marks only these controllers as
>> sleeping:
>>
>>   pinctrl: at91-pio4: mark the GPIO controller as sleeping
>>   pinctrl: stm32: mark the GPIO controller as sleeping
>>   pinctrl: sunxi: mark the GPIO controller as sleeping
>>
>> The reason is that all three expose gpiochip_generic_config() and can
>> therefore reach the pinctrl mutex from the GPIO set_config path, while
>> their current gpio_chip registration does not set can_sleep.
>
> But that's not the right solution is it? These controllers can probably
> just write a register and be done with it, they all look like they are
> memory-mapped? That means we introduce sleep where not needed.
>
> Can we simply replace pinctrldev_list_mutex with a spinlock?

Oh I've tried, I've give it a few attempts in the past. It's not a "simply"
case this one! :)

> The list isn't gonna be huge and all in-memory anyway.
> If it takes too much time we need to think about putting the
> ranges in a better data structure such as the maple tree.
>

FWIW radix tree provides some RCU synchronization IIRC.

> mutex_lock(&pinctrldev_list_mutex); could then be turned
> into spinlock_irqsave() or even better
> guard(spinlock_irqsave)(&pinctrldev_list_lock) in
> pinctrl_get_device_gpio_range().
>

I recall running into places where a mutex would be taken in atomic context
in that case.

Bart

> This would mean we just take two different spinlocks in seqence
> and save state in each so it should work just fine.
>
> Yours,
> Linus Walleij
>