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

[Bough Chen <haibo.chen@xxxxxxx>]

> -----Original Message-----
> From: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Sent: 2025年2月7日 2:29
> To: Johan Korsnes <johan.korsnes@xxxxxxxxxxxxx>
> Cc: linux-gpio@xxxxxxxxxxxxxxx; Bartosz Golaszewski
> <bartosz.golaszewski@xxxxxxxxxx>; Bough Chen <haibo.chen@xxxxxxx>
> Subject: Re: [PATCH] gpio: vf610: add locking to gpio direction functions
> 
> Hi Johan,
> 
> thanks for your patch!
> 
> On Thu, Feb 6, 2025 at 7:17 PM 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.
> >
> > Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > Cc: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > Cc: Haibo Chen <haibo.chen@xxxxxxx>
> > Signed-off-by: Johan Korsnes <johan.korsnes@xxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> 
> > 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.

Indeed, and also the vf610_gpio_set() / vf610_gpio_irq_ack().

> 
> 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.

I check the code, for vf610_gpio_direction_input()/vf610_gpio_direction_output(), to let the input/output really works, need to call pinctrl_gpio_direction_input() for vf610/imx7ulp/imx8ulp SoC.
Refer to drivers/pinctrl/freescale/pinctrl-vf610.c, it implement gpio_set_direction callback. Also for imx7ulp/imx8ulp pinctrl drivers.
This should be the reason why not using gpio-mmio.

Regards
Haibo Chen
> 
> Yours,
> Linus Walleij