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月13日 14:13
> 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
> 
> On 2/10/25 10:35 AM, Bough Chen wrote:
> >> -----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.
> >
> 
> Hi Haibo,
> 
> May I add your Reviewed-by or Acked-by in v2?

Yes, sure.

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