| Message ID | 20230926102914.6145-1-dg573847474@gmail.com |
|---|---|
| State | Accepted |
| Commit | 9e8bc2dda5a7a8e2babc9975f4b11c9a6196e490 |
| Headers | show |
| Series | gpio: timberdale: Fix potential deadlock on &tgpio->lock | expand |
On Tue, Sep 26, 2023 at 10:29:14AM +0000, Chengfeng Ye wrote: > As timbgpio_irq_enable()/timbgpio_irq_disable() callback could be > executed under irq context, it could introduce double locks on > &tgpio->lock if it preempts other execution units requiring > the same locks. > > timbgpio_gpio_set() > --> timbgpio_update_bit() > --> spin_lock(&tgpio->lock) > <interrupt> > --> timbgpio_irq_disable() > --> spin_lock_irqsave(&tgpio->lock) > > This flaw was found by an experimental static analysis tool I am > developing for irq-related deadlock. > > To prevent the potential deadlock, the patch uses spin_lock_irqsave() > on &tgpio->lock inside timbgpio_gpio_set() to prevent the possible > deadlock scenario. Okay, makes sense. Reviewed-by: Andy Shevchenko <andy@kernel.org> Question to the users of this hardware if they ever want to have this IRQ chip in the RT environment. In that case the locking type needs to be raw.
On Tue, Sep 26, 2023 at 12:29 PM Chengfeng Ye <dg573847474@gmail.com> wrote: > > As timbgpio_irq_enable()/timbgpio_irq_disable() callback could be > executed under irq context, it could introduce double locks on > &tgpio->lock if it preempts other execution units requiring > the same locks. > > timbgpio_gpio_set() > --> timbgpio_update_bit() > --> spin_lock(&tgpio->lock) > <interrupt> > --> timbgpio_irq_disable() > --> spin_lock_irqsave(&tgpio->lock) > > This flaw was found by an experimental static analysis tool I am > developing for irq-related deadlock. > > To prevent the potential deadlock, the patch uses spin_lock_irqsave() > on &tgpio->lock inside timbgpio_gpio_set() to prevent the possible > deadlock scenario. > > Signed-off-by: Chengfeng Ye <dg573847474@gmail.com> > --- > drivers/gpio/gpio-timberdale.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpio-timberdale.c b/drivers/gpio/gpio-timberdale.c > index bbd9e9191199..fad979797486 100644 > --- a/drivers/gpio/gpio-timberdale.c > +++ b/drivers/gpio/gpio-timberdale.c > @@ -43,9 +43,10 @@ static int timbgpio_update_bit(struct gpio_chip *gpio, unsigned index, > unsigned offset, bool enabled) > { > struct timbgpio *tgpio = gpiochip_get_data(gpio); > + unsigned long flags; > u32 reg; > > - spin_lock(&tgpio->lock); > + spin_lock_irqsave(&tgpio->lock, flags); > reg = ioread32(tgpio->membase + offset); > > if (enabled) > @@ -54,7 +55,7 @@ static int timbgpio_update_bit(struct gpio_chip *gpio, unsigned index, > reg &= ~(1 << index); > > iowrite32(reg, tgpio->membase + offset); > - spin_unlock(&tgpio->lock); > + spin_unlock_irqrestore(&tgpio->lock, flags); > > return 0; > } > -- > 2.17.1 > Applied, thanks! Bart
diff --git a/drivers/gpio/gpio-timberdale.c b/drivers/gpio/gpio-timberdale.c index bbd9e9191199..fad979797486 100644 --- a/drivers/gpio/gpio-timberdale.c +++ b/drivers/gpio/gpio-timberdale.c @@ -43,9 +43,10 @@ static int timbgpio_update_bit(struct gpio_chip *gpio, unsigned index, unsigned offset, bool enabled) { struct timbgpio *tgpio = gpiochip_get_data(gpio); + unsigned long flags; u32 reg; - spin_lock(&tgpio->lock); + spin_lock_irqsave(&tgpio->lock, flags); reg = ioread32(tgpio->membase + offset); if (enabled) @@ -54,7 +55,7 @@ static int timbgpio_update_bit(struct gpio_chip *gpio, unsigned index, reg &= ~(1 << index); iowrite32(reg, tgpio->membase + offset); - spin_unlock(&tgpio->lock); + spin_unlock_irqrestore(&tgpio->lock, flags); return 0; }
As timbgpio_irq_enable()/timbgpio_irq_disable() callback could be executed under irq context, it could introduce double locks on &tgpio->lock if it preempts other execution units requiring the same locks. timbgpio_gpio_set() --> timbgpio_update_bit() --> spin_lock(&tgpio->lock) <interrupt> --> timbgpio_irq_disable() --> spin_lock_irqsave(&tgpio->lock) This flaw was found by an experimental static analysis tool I am developing for irq-related deadlock. To prevent the potential deadlock, the patch uses spin_lock_irqsave() on &tgpio->lock inside timbgpio_gpio_set() to prevent the possible deadlock scenario. Signed-off-by: Chengfeng Ye <dg573847474@gmail.com> --- drivers/gpio/gpio-timberdale.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)