| Message ID | d71c523a8a0af5628da59a44223a357102c2a193.1540627754.git.lukas@wunner.de |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: bcm2835: Use raw spinlock for RT compatibility | expand |
On Sat, Oct 27, 2018 at 10:15 AM Lukas Wunner <lukas@wunner.de> wrote: > The BCM2835 pinctrl driver acquires a spinlock in its ->irq_enable, > ->irq_disable and ->irq_set_type callbacks. Spinlocks become sleeping > locks with CONFIG_PREEMPT_RT_FULL=y, therefore invocation of one of the > callbacks in atomic context may cause a hard lockup if at least two GPIO > pins in the same bank are used as interrupts. The issue doesn't occur > with just a single interrupt pin per bank because the lock is never > contended. I'm experiencing such lockups with GPIO 8 and 28 used as > level-triggered interrupts, i.e. with ->irq_disable being invoked on > reception of every IRQ. > > The critical section protected by the spinlock is very small (one bitop > and one RMW of an MMIO register), hence converting to a raw spinlock > seems a better trade-off than converting the driver to threaded IRQ > handling (which would increase latency to handle an interrupt). > > Cc: Mathias Duckeck <m.duckeck@kunbus.de> > Signed-off-by: Lukas Wunner <lukas@wunner.de> Looks good to me but adding Julia Cartwright and Grygorii Strashko to this thread because they usually know this stuff way better than me. Yours, Linus Walleij
On Sun, Oct 28, 2018 at 08:27:35PM +0100, Linus Walleij wrote: > On Sat, Oct 27, 2018 at 10:15 AM Lukas Wunner <lukas@wunner.de> wrote: > > > The BCM2835 pinctrl driver acquires a spinlock in its ->irq_enable, > > ->irq_disable and ->irq_set_type callbacks. Spinlocks become sleeping > > locks with CONFIG_PREEMPT_RT_FULL=y, therefore invocation of one of the > > callbacks in atomic context may cause a hard lockup if at least two GPIO > > pins in the same bank are used as interrupts. The issue doesn't occur > > with just a single interrupt pin per bank because the lock is never > > contended. I'm experiencing such lockups with GPIO 8 and 28 used as > > level-triggered interrupts, i.e. with ->irq_disable being invoked on > > reception of every IRQ. > > > > The critical section protected by the spinlock is very small (one bitop > > and one RMW of an MMIO register), hence converting to a raw spinlock > > seems a better trade-off than converting the driver to threaded IRQ > > handling (which would increase latency to handle an interrupt). > > > > Cc: Mathias Duckeck <m.duckeck@kunbus.de> > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > Looks good to me but adding Julia Cartwright and Grygorii > Strashko to this thread because they usually know this > stuff way better than me. The change seems reasonable to me. As a side note: the bitops used to manage the enabled_irq_map are the atomic variants (set_bit, etc), is this actually necessary? In other words, are they ever accessed outside of the spinlock? Julia
On Sat, Oct 27, 2018 at 10:15 AM Lukas Wunner <lukas@wunner.de> wrote: > The BCM2835 pinctrl driver acquires a spinlock in its ->irq_enable, > ->irq_disable and ->irq_set_type callbacks. Spinlocks become sleeping > locks with CONFIG_PREEMPT_RT_FULL=y, therefore invocation of one of the > callbacks in atomic context may cause a hard lockup if at least two GPIO > pins in the same bank are used as interrupts. The issue doesn't occur > with just a single interrupt pin per bank because the lock is never > contended. I'm experiencing such lockups with GPIO 8 and 28 used as > level-triggered interrupts, i.e. with ->irq_disable being invoked on > reception of every IRQ. > > The critical section protected by the spinlock is very small (one bitop > and one RMW of an MMIO register), hence converting to a raw spinlock > seems a better trade-off than converting the driver to threaded IRQ > handling (which would increase latency to handle an interrupt). > > Cc: Mathias Duckeck <m.duckeck@kunbus.de> > Signed-off-by: Lukas Wunner <lukas@wunner.de> Patch applied. Yours, Linus Walleij
On Mon, Oct 29, 2018 at 3:45 PM Julia Cartwright <julia@ni.com> wrote: > The change seems reasonable to me. Thanks. (I record that as an Acked-by.) > As a side note: the bitops used to manage the enabled_irq_map are the > atomic variants (set_bit, etc), is this actually necessary? In other > words, are they ever accessed outside of the spinlock? Lukas can you look into this? On a further side note I have developed an aversion toward the implicit semantics in __set_bit() for non-atomic set_bit(), I was thinking to send Torvalds something like a sed script to simply rename all the __set_bit() and friends to set_bit_nonatomic() etc or set_bit_na() so it becomes human readable that he could run before finalizing the next kernel. If people think it is a good idea. Yours, Linus Walleij
[cc += Simon Arlott, Chris Boot, Stephen Warren] On Mon, Oct 29, 2018 at 02:45:08PM +0000, Julia Cartwright wrote: > On Sat, Oct 27, 2018 at 10:15 AM Lukas Wunner <lukas@wunner.de> wrote: > > The BCM2835 pinctrl driver acquires a spinlock in its ->irq_enable, > > ->irq_disable and ->irq_set_type callbacks. Spinlocks become sleeping > > locks with CONFIG_PREEMPT_RT_FULL=y, therefore invocation of one of the > > callbacks in atomic context may cause a hard lockup if at least two GPIO > > pins in the same bank are used as interrupts. The issue doesn't occur > > with just a single interrupt pin per bank because the lock is never > > contended. I'm experiencing such lockups with GPIO 8 and 28 used as > > level-triggered interrupts, i.e. with ->irq_disable being invoked on > > reception of every IRQ. > > > > The critical section protected by the spinlock is very small (one bitop > > and one RMW of an MMIO register), hence converting to a raw spinlock > > seems a better trade-off than converting the driver to threaded IRQ > > handling (which would increase latency to handle an interrupt). > > The change seems reasonable to me. > > As a side note: the bitops used to manage the enabled_irq_map are the > atomic variants (set_bit, etc), is this actually necessary? In other > words, are they ever accessed outside of the spinlock? There's a single occurrence of enabled_irq_map being used outside the spinlock, it's in the IRQ handler itself, bcm2835_gpio_irq_handle_bank(): events = bcm2835_gpio_rd(pc, GPEDS0 + bank * 4); events &= mask; events &= pc->enabled_irq_map[bank]; However this use of enabled_irq_map seems gratuitous to me: Once the IRQ has been disabled by bcm2835_gpio_irq_disable(), its bit should no longer be set in the GPEDS0 / GPEDS1 register. Vice-versa on enablement. So why does the IRQ handler take enabled_irq_map into consideration at all? Maybe this is a remnant of an older version of the driver, so adding the authors of e1b2dc70cd5b (which introduced the driver) to cc. So what I have in mind is removal of enabled_irq_map from the IRQ handler plus use of non-atomic bitops. I'll give the patch below a spin when I'm back in the office on Monday to test if it breaks anything. Thanks, Lukas -- >8 -- diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c index b77195a..72d7aec 100644 --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c @@ -377,7 +377,6 @@ static void bcm2835_gpio_irq_handle_bank(struct bcm2835_pinctrl *pc, events = bcm2835_gpio_rd(pc, GPEDS0 + bank * 4); events &= mask; - events &= pc->enabled_irq_map[bank]; for_each_set_bit(offset, &events, 32) { gpio = (32 * bank) + offset; generic_handle_irq(irq_linear_revmap(pc->gpio_chip.irqdomain, @@ -473,7 +472,7 @@ static void bcm2835_gpio_irq_enable(struct irq_data *data) unsigned long flags; raw_spin_lock_irqsave(&pc->irq_lock[bank], flags); - set_bit(offset, &pc->enabled_irq_map[bank]); + __set_bit(offset, &pc->enabled_irq_map[bank]); bcm2835_gpio_irq_config(pc, gpio, true); raw_spin_unlock_irqrestore(&pc->irq_lock[bank], flags); } @@ -491,7 +490,7 @@ static void bcm2835_gpio_irq_disable(struct irq_data *data) bcm2835_gpio_irq_config(pc, gpio, false); /* Clear events that were latched prior to clearing event sources */ bcm2835_gpio_set_bit(pc, GPEDS0, gpio); - clear_bit(offset, &pc->enabled_irq_map[bank]); + __clear_bit(offset, &pc->enabled_irq_map[bank]); raw_spin_unlock_irqrestore(&pc->irq_lock[bank], flags); }
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c index fa530913a2c8..08925d24180b 100644 --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c @@ -90,7 +90,7 @@ struct bcm2835_pinctrl { struct gpio_chip gpio_chip; struct pinctrl_gpio_range gpio_range; - spinlock_t irq_lock[BCM2835_NUM_BANKS]; + raw_spinlock_t irq_lock[BCM2835_NUM_BANKS]; }; /* pins are just named GPIO0..GPIO53 */ @@ -461,10 +461,10 @@ static void bcm2835_gpio_irq_enable(struct irq_data *data) unsigned bank = GPIO_REG_OFFSET(gpio); unsigned long flags; - spin_lock_irqsave(&pc->irq_lock[bank], flags); + raw_spin_lock_irqsave(&pc->irq_lock[bank], flags); set_bit(offset, &pc->enabled_irq_map[bank]); bcm2835_gpio_irq_config(pc, gpio, true); - spin_unlock_irqrestore(&pc->irq_lock[bank], flags); + raw_spin_unlock_irqrestore(&pc->irq_lock[bank], flags); } static void bcm2835_gpio_irq_disable(struct irq_data *data) @@ -476,12 +476,12 @@ static void bcm2835_gpio_irq_disable(struct irq_data *data) unsigned bank = GPIO_REG_OFFSET(gpio); unsigned long flags; - spin_lock_irqsave(&pc->irq_lock[bank], flags); + raw_spin_lock_irqsave(&pc->irq_lock[bank], flags); bcm2835_gpio_irq_config(pc, gpio, false); /* Clear events that were latched prior to clearing event sources */ bcm2835_gpio_set_bit(pc, GPEDS0, gpio); clear_bit(offset, &pc->enabled_irq_map[bank]); - spin_unlock_irqrestore(&pc->irq_lock[bank], flags); + raw_spin_unlock_irqrestore(&pc->irq_lock[bank], flags); } static int __bcm2835_gpio_irq_set_type_disabled(struct bcm2835_pinctrl *pc, @@ -584,7 +584,7 @@ static int bcm2835_gpio_irq_set_type(struct irq_data *data, unsigned int type) unsigned long flags; int ret; - spin_lock_irqsave(&pc->irq_lock[bank], flags); + raw_spin_lock_irqsave(&pc->irq_lock[bank], flags); if (test_bit(offset, &pc->enabled_irq_map[bank])) ret = __bcm2835_gpio_irq_set_type_enabled(pc, gpio, type); @@ -596,7 +596,7 @@ static int bcm2835_gpio_irq_set_type(struct irq_data *data, unsigned int type) else irq_set_handler_locked(data, handle_level_irq); - spin_unlock_irqrestore(&pc->irq_lock[bank], flags); + raw_spin_unlock_irqrestore(&pc->irq_lock[bank], flags); return ret; } @@ -1047,7 +1047,7 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev) for_each_set_bit(offset, &events, 32) bcm2835_gpio_wr(pc, GPEDS0 + i * 4, BIT(offset)); - spin_lock_init(&pc->irq_lock[i]); + raw_spin_lock_init(&pc->irq_lock[i]); } err = gpiochip_add_data(&pc->gpio_chip, pc);
The BCM2835 pinctrl driver acquires a spinlock in its ->irq_enable, ->irq_disable and ->irq_set_type callbacks. Spinlocks become sleeping locks with CONFIG_PREEMPT_RT_FULL=y, therefore invocation of one of the callbacks in atomic context may cause a hard lockup if at least two GPIO pins in the same bank are used as interrupts. The issue doesn't occur with just a single interrupt pin per bank because the lock is never contended. I'm experiencing such lockups with GPIO 8 and 28 used as level-triggered interrupts, i.e. with ->irq_disable being invoked on reception of every IRQ. The critical section protected by the spinlock is very small (one bitop and one RMW of an MMIO register), hence converting to a raw spinlock seems a better trade-off than converting the driver to threaded IRQ handling (which would increase latency to handle an interrupt). Cc: Mathias Duckeck <m.duckeck@kunbus.de> Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/pinctrl/bcm/pinctrl-bcm2835.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)