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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Thu, Feb 13, 2025 at 8:35 AM Johan Korsnes
<johan.korsnes@xxxxxxxxxxxxx> 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.
>
> To verify the correctness of this fix, a `trylock` patch was applied,
> 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"/hide the issue.
>
> Signed-off-by: Johan Korsnes <johan.korsnes@xxxxxxxxxxxxx>
> Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Reviewed-by: Haibo Chen <haibo.chen@xxxxxxx>
> Cc: Bartosz Golaszewski <brgl@xxxxxxxx>
>

This is a fix and should be backported. Can you add Cc: stable and Fixes: tags?

> ---
>
> v2
>  - Added description on correcctness to commit text
>  - Added Reviewed-by from Walleij and Haibo
> ---
>  drivers/gpio/gpio-vf610.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
> index c4f34a347cb6..3527487d42c8 100644
> --- a/drivers/gpio/gpio-vf610.c
> +++ b/drivers/gpio/gpio-vf610.c
> @@ -36,6 +36,7 @@ struct vf610_gpio_port {
>         struct clk *clk_port;
>         struct clk *clk_gpio;
>         int irq;
> +       spinlock_t lock; /* protect gpio direction registers */
>  };
>
>  #define GPIO_PDOR              0x00
> @@ -121,12 +122,15 @@ static int vf610_gpio_direction_input(struct gpio_chip *chip, unsigned int gpio)
>  {
>         struct vf610_gpio_port *port = gpiochip_get_data(chip);
>         u32 mask = BIT(gpio);
> +       unsigned long flags;
>         u32 val;
>
>         if (port->sdata->have_paddr) {
> +               spin_lock_irqsave(&port->lock, flags);

Please use lock guards from cleanup.h in new code. It doesn't cost you
anything and results in shorter code (you won't need the flags
variable). Just do guard(spinlock_irqsave)(&port->lock) here.

Bart

>                 val = vf610_gpio_readl(port->gpio_base + GPIO_PDDR);
>                 val &= ~mask;
>                 vf610_gpio_writel(val, port->gpio_base + GPIO_PDDR);
> +               spin_unlock_irqrestore(&port->lock, flags);
>         }