| Message ID | 20220427144620.9105-2-pmalgujar@marvell.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: thunderx: Marvell GPIO changes. | expand |
On Wed, Apr 27, 2022 at 4:46 PM Piyush Malgujar <pmalgujar@marvell.com> wrote: > > Using irqsave/irqrestore locking variants to avoid any deadlock. > I see you'll be resending this anyway so would you mind providing an example of a deadlock that is possible with no-irqsave variants? Thanks. Bart > Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com> > ---
On Mon, May 02, 2022 at 01:18:49PM +0200, Bartosz Golaszewski wrote: > On Wed, Apr 27, 2022 at 4:46 PM Piyush Malgujar <pmalgujar@marvell.com> wrote: > > > > Using irqsave/irqrestore locking variants to avoid any deadlock. > > > > I see you'll be resending this anyway so would you mind providing an > example of a deadlock that is possible with no-irqsave variants? > Thanks. > > Bart > Hi Bartosz, Thanks for the review. Please find below the issue scenario: In the case when HARDIRQ-safe -> HARDIRQ-unsafe lock order is detected and interrupt occurs, deadlock could occur. ======================================================== WARNING: possible irq lock inversion dependency detected 5.18.0-rc6 #4 Not tainted -------------------------------------------------------- swapper/3/0 just changed the state of lock: ffff000110904cd8 (lock_class){-...}-{2:2}, at: handle_fasteoi_ack_irq+0x2c/0x1b0 but this lock took another, HARDIRQ-unsafe lock in the past: (&txgpio->lock){+.+.}-{2:2} and interrupts could create inverse lock ordering between them. other info that might help us debug this: Possible interrupt unsafe locking scenario: CPU0 CPU1 ---- ---- lock(&txgpio->lock); local_irq_disable(); lock(lock_class); lock(&txgpio->lock); <Interrupt> lock(lock_class); *** DEADLOCK *** ========================================================== Thanks, Piyush > > Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com> > > ---
On Wed, May 25, 2022 at 3:17 PM Piyush Malgujar <pmalgujar@marvell.com> wrote: > > On Mon, May 02, 2022 at 01:18:49PM +0200, Bartosz Golaszewski wrote: > > On Wed, Apr 27, 2022 at 4:46 PM Piyush Malgujar <pmalgujar@marvell.com> wrote: > > > > > > Using irqsave/irqrestore locking variants to avoid any deadlock. > > > > > > > I see you'll be resending this anyway so would you mind providing an > > example of a deadlock that is possible with no-irqsave variants? > > Thanks. > > > > Bart > > > Hi Bartosz, > > Thanks for the review. > > Please find below the issue scenario: > In the case when HARDIRQ-safe -> HARDIRQ-unsafe lock order is detected > and interrupt occurs, deadlock could occur. > > ======================================================== > WARNING: possible irq lock inversion dependency detected > 5.18.0-rc6 #4 Not tainted > -------------------------------------------------------- > swapper/3/0 just changed the state of lock: > ffff000110904cd8 (lock_class){-...}-{2:2}, at: handle_fasteoi_ack_irq+0x2c/0x1b0 > but this lock took another, HARDIRQ-unsafe lock in the past: > (&txgpio->lock){+.+.}-{2:2} > > > and interrupts could create inverse lock ordering between them. > > > other info that might help us debug this: > Possible interrupt unsafe locking scenario: > > CPU0 CPU1 > ---- ---- > lock(&txgpio->lock); > local_irq_disable(); > lock(lock_class); > lock(&txgpio->lock); > <Interrupt> > lock(lock_class); > > *** DEADLOCK *** > > ========================================================== > > Thanks, > Piyush > > > Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com> > > > --- Thanks. What I meant exactly was: resend it with that info in the commit message. Bart
diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c index 9f66deab46eaa99d05413a996b585284c433574d..bb2b40e4033b00134af35592b6b7c7f83cf6c737 100644 --- a/drivers/gpio/gpio-thunderx.c +++ b/drivers/gpio/gpio-thunderx.c @@ -104,16 +104,17 @@ static int thunderx_gpio_request(struct gpio_chip *chip, unsigned int line) static int thunderx_gpio_dir_in(struct gpio_chip *chip, unsigned int line) { struct thunderx_gpio *txgpio = gpiochip_get_data(chip); + unsigned long flags; if (!thunderx_gpio_is_gpio(txgpio, line)) return -EIO; - raw_spin_lock(&txgpio->lock); + raw_spin_lock_irqsave(&txgpio->lock, flags); clear_bit(line, txgpio->invert_mask); clear_bit(line, txgpio->od_mask); writeq(txgpio->line_entries[line].fil_bits, txgpio->register_base + bit_cfg_reg(line)); - raw_spin_unlock(&txgpio->lock); + raw_spin_unlock_irqrestore(&txgpio->lock, flags); return 0; } @@ -135,11 +136,12 @@ static int thunderx_gpio_dir_out(struct gpio_chip *chip, unsigned int line, { struct thunderx_gpio *txgpio = gpiochip_get_data(chip); u64 bit_cfg = txgpio->line_entries[line].fil_bits | GPIO_BIT_CFG_TX_OE; + unsigned long flags; if (!thunderx_gpio_is_gpio(txgpio, line)) return -EIO; - raw_spin_lock(&txgpio->lock); + raw_spin_lock_irqsave(&txgpio->lock, flags); thunderx_gpio_set(chip, line, value); @@ -151,7 +153,7 @@ static int thunderx_gpio_dir_out(struct gpio_chip *chip, unsigned int line, writeq(bit_cfg, txgpio->register_base + bit_cfg_reg(line)); - raw_spin_unlock(&txgpio->lock); + raw_spin_unlock_irqrestore(&txgpio->lock, flags); return 0; } @@ -188,11 +190,12 @@ static int thunderx_gpio_set_config(struct gpio_chip *chip, int ret = -ENOTSUPP; struct thunderx_gpio *txgpio = gpiochip_get_data(chip); void __iomem *reg = txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_SET; + unsigned long flags; if (!thunderx_gpio_is_gpio(txgpio, line)) return -EIO; - raw_spin_lock(&txgpio->lock); + raw_spin_lock_irqsave(&txgpio->lock, flags); orig_invert = test_bit(line, txgpio->invert_mask); new_invert = orig_invert; orig_od = test_bit(line, txgpio->od_mask); @@ -243,7 +246,7 @@ static int thunderx_gpio_set_config(struct gpio_chip *chip, default: break; } - raw_spin_unlock(&txgpio->lock); + raw_spin_unlock_irqrestore(&txgpio->lock, flags); /* * If currently output and OPEN_DRAIN changed, install the new @@ -329,6 +332,7 @@ static int thunderx_gpio_irq_set_type(struct irq_data *d, struct thunderx_gpio *txgpio = gpiochip_get_data(gc); struct thunderx_line *txline = &txgpio->line_entries[irqd_to_hwirq(d)]; + unsigned long flags; u64 bit_cfg; irqd_set_trigger_type(d, flow_type); @@ -342,7 +346,7 @@ static int thunderx_gpio_irq_set_type(struct irq_data *d, irq_set_handler_locked(d, handle_fasteoi_mask_irq); } - raw_spin_lock(&txgpio->lock); + raw_spin_lock_irqsave(&txgpio->lock, flags); if (flow_type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_LEVEL_LOW)) { bit_cfg |= GPIO_BIT_CFG_PIN_XOR; set_bit(txline->line, txgpio->invert_mask); @@ -351,7 +355,7 @@ static int thunderx_gpio_irq_set_type(struct irq_data *d, } clear_bit(txline->line, txgpio->od_mask); writeq(bit_cfg, txgpio->register_base + bit_cfg_reg(txline->line)); - raw_spin_unlock(&txgpio->lock); + raw_spin_unlock_irqrestore(&txgpio->lock, flags); return IRQ_SET_MASK_OK; }
Using irqsave/irqrestore locking variants to avoid any deadlock. Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com> --- drivers/gpio/gpio-thunderx.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)