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

[Johan Korsnes <johan.korsnes@remarkable.no>]

On 2/7/25 7:21 AM, Bough Chen wrote:
>> -----Original Message-----
>> From: Linus Walleij <linus.walleij@linaro.org>
>> Sent: 2025年2月7日 2:29
>> To: Johan Korsnes <johan.korsnes@remarkable.no>
>> Cc: linux-gpio@vger.kernel.org; Bartosz Golaszewski
>> <bartosz.golaszewski@linaro.org>; Bough Chen <haibo.chen@nxp.com>
>> 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@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.
>>

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?

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