Re: [PATCH] gpio: vf610: add locking to gpio direction functions

[Linus Walleij <linus.walleij@linaro.org>]

Hi Johan,

thanks for your patch!

On Thu, Feb 6, 2025 at 7:17 PM Johan Korsnes
<johan.korsnes@remarkable.no> wrote:

> Add locking to `vf610_gpio_direction_input|output()` functions. Without
> this locking, a race condition exists between concurrent calls to these
> functions, potentially leading to incorrect GPIO direction settings.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Cc: Haibo Chen <haibo.chen@nxp.com>
> Signed-off-by: Johan Korsnes <johan.korsnes@remarkable.no>

Looks correct to me, verified by looking at the most tested
driver gpio-mmio.c and seeing there is a lock there indeed.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

> where after a couple of reboots the race was confirmed. I.e., one user
> had to wait before acquiring the lock. With this patch the race has not
> been encountered. It's worth mentioning that any type of debugging
> (printing, tracing, etc.) would "resolve" the issue.

Typical. I would include this in the commit message, people care.

Looking at the driver it seems vf610_gpio_irq_mask()/vf610_gpio_irq_unmask()
could have a similar issue, both write the same register.

Both issues could be fixed by converting the driver to use
gpio-mmio() with bgpio_init() which would also implement
get/set_multiple support for free.

I have no idea why this driver isn't using gpio-mmio.
Not your fault though, just pointing out obvious improvement
opportunities.

Yours,
Linus Walleij