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

[Bough Chen <haibo.chen@xxxxxxx>]

> -----Original Message-----
> From: Johan Korsnes <johan.korsnes@xxxxxxxxxxxxx>
> Sent: 2025年2月10日 16:53
> To: Bough Chen <haibo.chen@xxxxxxx>; Linus Walleij <linus.walleij@xxxxxxxxxx>
> Cc: linux-gpio@xxxxxxxxxxxxxxx; Bartosz Golaszewski
> <bartosz.golaszewski@xxxxxxxxxx>; imx@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] gpio: vf610: add locking to gpio direction functions
> 
> [You don't often get email from johan.korsnes@xxxxxxxxxxxxx. Learn why this is
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On 2/7/25 7:21 AM, Bough Chen wrote:
> >> -----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.
> >>
> 
> Hi Linus and Haibo,
> 
> Thanks for the review! I'll include this in v2.
> 
> >> 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().
> >
> 
> Could you please explain the race condition we fix by adding locking to these
> other functions? F.ex. the vf610_gpio_set(), in which scenario would the lack of
> locking cause an issue? It's a single write to either the set or clear register. Is this
> related to how the writel_relaxed() works on different architectures?
> 

Sorry, I think twice of this condition, seems no need to add lock for these functions.

Regards
Haibo Chen

> Kind regards,
> Johan
> 
> >>
> >> 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