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

[Johan Korsnes <johan.korsnes@xxxxxxxxxxxxx>]

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?

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
>