| Message ID | 20250206181714.417433-1-johan.korsnes@remarkable.no |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: vf610: add locking to gpio direction functions | expand |
On Thu, 6 Feb 2025 at 19:17, 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> Please use the email address that pops up when you call scripts/get_maintainer.pl for patch submission next time. Bart
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
> -----Original Message----- > From: Johan Korsnes <johan.korsnes@remarkable.no> > Sent: 2025年2月10日 16:53 > To: Bough Chen <haibo.chen@nxp.com>; Linus Walleij <linus.walleij@linaro.org> > Cc: linux-gpio@vger.kernel.org; Bartosz Golaszewski > <bartosz.golaszewski@linaro.org>; imx@lists.linux.dev > Subject: Re: [PATCH] gpio: vf610: add locking to gpio direction functions > > [You don't often get email from johan.korsnes@remarkable.no. 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@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? > 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
diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c index c4f34a347cb6..3527487d42c8 100644 --- a/drivers/gpio/gpio-vf610.c +++ b/drivers/gpio/gpio-vf610.c @@ -36,6 +36,7 @@ struct vf610_gpio_port { struct clk *clk_port; struct clk *clk_gpio; int irq; + spinlock_t lock; /* protect gpio direction registers */ }; #define GPIO_PDOR 0x00 @@ -121,12 +122,15 @@ static int vf610_gpio_direction_input(struct gpio_chip *chip, unsigned int gpio) { struct vf610_gpio_port *port = gpiochip_get_data(chip); u32 mask = BIT(gpio); + unsigned long flags; u32 val; if (port->sdata->have_paddr) { + spin_lock_irqsave(&port->lock, flags); val = vf610_gpio_readl(port->gpio_base + GPIO_PDDR); val &= ~mask; vf610_gpio_writel(val, port->gpio_base + GPIO_PDDR); + spin_unlock_irqrestore(&port->lock, flags); } return pinctrl_gpio_direction_input(chip, gpio); @@ -137,14 +141,17 @@ static int vf610_gpio_direction_output(struct gpio_chip *chip, unsigned int gpio { struct vf610_gpio_port *port = gpiochip_get_data(chip); u32 mask = BIT(gpio); + unsigned long flags; u32 val; vf610_gpio_set(chip, gpio, value); if (port->sdata->have_paddr) { + spin_lock_irqsave(&port->lock, flags); val = vf610_gpio_readl(port->gpio_base + GPIO_PDDR); val |= mask; vf610_gpio_writel(val, port->gpio_base + GPIO_PDDR); + spin_unlock_irqrestore(&port->lock, flags); } return pinctrl_gpio_direction_output(chip, gpio); @@ -297,6 +304,7 @@ static int vf610_gpio_probe(struct platform_device *pdev) return -ENOMEM; port->sdata = device_get_match_data(dev); + spin_lock_init(&port->lock); dual_base = port->sdata->have_dual_base;
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> --- To verify the correctness of this fix, a `trylock` patch was applied, 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. --- drivers/gpio/gpio-vf610.c | 8 ++++++++ 1 file changed, 8 insertions(+)