| Message ID | 20221207013012.395585-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] serial: stm32: Move stm32_usart_transmit_chars() to interrupt thread | expand |
On 2022-12-07 02:30:12 [+0100], Marek Vasut wrote: > Avoid locking in hard interrupt context, move the stm32_usart_transmit_chars() > into the threaded IRQ handler. This fixes the following splat with preempt-rt: > > BUG: scheduling while atomic: (mount)/1289/0x00010001 > Modules linked in: > Preemption disabled at: > [<c0119127>] irq_enter_rcu+0xb/0x42 > CPU: 0 PID: 1289 Comm: (mount) Not tainted 6.1.0-rc7-rt5-stable-standard-00006-gd70aeccb9f0f #17 > Hardware name: STM32 (Device Tree Support) > unwind_backtrace from show_stack+0xb/0xc > show_stack from dump_stack_lvl+0x2b/0x34 > dump_stack_lvl from __schedule_bug+0x53/0x80 > __schedule_bug from __schedule+0x47/0x404 > __schedule from schedule_rtlock+0x15/0x34 > schedule_rtlock from rtlock_slowlock_locked+0x1d7/0x57e > rtlock_slowlock_locked from rt_spin_lock+0x29/0x3c > rt_spin_lock from stm32_usart_interrupt+0xa9/0x110 > stm32_usart_interrupt from __handle_irq_event_percpu+0x73/0x14e > __handle_irq_event_percpu from handle_irq_event_percpu+0x9/0x22 > handle_irq_event_percpu from handle_irq_event+0x53/0x76 > handle_irq_event from handle_fasteoi_irq+0x65/0xa8 > handle_fasteoi_irq from handle_irq_desc+0xf/0x18 > handle_irq_desc from gic_handle_irq+0x45/0x54 > gic_handle_irq from generic_handle_arch_irq+0x19/0x2c > generic_handle_arch_irq from call_with_stack+0xd/0x10 > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> > Cc: Erwan Le Ray <erwan.leray@foss.st.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Jean Philippe Romain <jean-philippe.romain@foss.st.com> > Cc: Jiri Slaby <jirislaby@kernel.org> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Valentin Caron <valentin.caron@foss.st.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-stm32@st-md-mailman.stormreply.com > To: linux-serial@vger.kernel.org > --- > drivers/tty/serial/stm32-usart.c | 19 +++++++++++++------ > 1 file changed, 13 insertions(+), 6 deletions(-) > > diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c > index a1490033aa164..56357a7962edc 100644 > --- a/drivers/tty/serial/stm32-usart.c > +++ b/drivers/tty/serial/stm32-usart.c > @@ -791,11 +791,8 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr) > } > } Why is this handler running as the primary handler to begin with? There is stm32_usart_rs485_rts_disable() -> mctrl_gpio_set() -> gpiod_set_array_value() -> gpiod_set_array_value_complex() -> gpio_chip_set_multiple() -> gc->set_multiple() || gc->set() ? -> bitmap_alloc() boom I don't know if the underlying gpiod is always using a raw_spinlock_t but that bitmap_alloc() (depending on FASTPATH_NGPIO()) is not going to work. pm_wakeup_dev_event() is also using a spinlock_t. > - if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) { > - spin_lock(&port->lock); > - stm32_usart_transmit_chars(port); > - spin_unlock(&port->lock); > - } > + if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) > + return IRQ_WAKE_THREAD; Before that, there is a "stm32_port->throttled" check using the very same lock. > if (stm32_usart_rx_dma_enabled(port)) > return IRQ_WAKE_THREAD; Sebastian
On 12/7/22 09:15, Sebastian Andrzej Siewior wrote: > On 2022-12-07 02:30:12 [+0100], Marek Vasut wrote: >> Avoid locking in hard interrupt context, move the stm32_usart_transmit_chars() >> into the threaded IRQ handler. This fixes the following splat with preempt-rt: >> >> BUG: scheduling while atomic: (mount)/1289/0x00010001 >> Modules linked in: >> Preemption disabled at: >> [<c0119127>] irq_enter_rcu+0xb/0x42 >> CPU: 0 PID: 1289 Comm: (mount) Not tainted 6.1.0-rc7-rt5-stable-standard-00006-gd70aeccb9f0f #17 >> Hardware name: STM32 (Device Tree Support) >> unwind_backtrace from show_stack+0xb/0xc >> show_stack from dump_stack_lvl+0x2b/0x34 >> dump_stack_lvl from __schedule_bug+0x53/0x80 >> __schedule_bug from __schedule+0x47/0x404 >> __schedule from schedule_rtlock+0x15/0x34 >> schedule_rtlock from rtlock_slowlock_locked+0x1d7/0x57e >> rtlock_slowlock_locked from rt_spin_lock+0x29/0x3c >> rt_spin_lock from stm32_usart_interrupt+0xa9/0x110 >> stm32_usart_interrupt from __handle_irq_event_percpu+0x73/0x14e >> __handle_irq_event_percpu from handle_irq_event_percpu+0x9/0x22 >> handle_irq_event_percpu from handle_irq_event+0x53/0x76 >> handle_irq_event from handle_fasteoi_irq+0x65/0xa8 >> handle_fasteoi_irq from handle_irq_desc+0xf/0x18 >> handle_irq_desc from gic_handle_irq+0x45/0x54 >> gic_handle_irq from generic_handle_arch_irq+0x19/0x2c >> generic_handle_arch_irq from call_with_stack+0xd/0x10 >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> --- >> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> >> Cc: Erwan Le Ray <erwan.leray@foss.st.com> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: Jean Philippe Romain <jean-philippe.romain@foss.st.com> >> Cc: Jiri Slaby <jirislaby@kernel.org> >> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> >> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Cc: Valentin Caron <valentin.caron@foss.st.com> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-stm32@st-md-mailman.stormreply.com >> To: linux-serial@vger.kernel.org >> --- >> drivers/tty/serial/stm32-usart.c | 19 +++++++++++++------ >> 1 file changed, 13 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c >> index a1490033aa164..56357a7962edc 100644 >> --- a/drivers/tty/serial/stm32-usart.c >> +++ b/drivers/tty/serial/stm32-usart.c >> @@ -791,11 +791,8 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr) >> } >> } > > Why is this handler running as the primary handler to begin with? > There is > stm32_usart_rs485_rts_disable() > -> mctrl_gpio_set() > -> gpiod_set_array_value() > -> gpiod_set_array_value_complex() > -> gpio_chip_set_multiple() > -> gc->set_multiple() || gc->set() ? > -> bitmap_alloc() boom > > I don't know if the underlying gpiod is always using a raw_spinlock_t > but that bitmap_alloc() (depending on FASTPATH_NGPIO()) is not going to > work. > pm_wakeup_dev_event() is also using a spinlock_t. > > >> - if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) { >> - spin_lock(&port->lock); >> - stm32_usart_transmit_chars(port); >> - spin_unlock(&port->lock); >> - } >> + if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) >> + return IRQ_WAKE_THREAD; > > Before that, there is a "stm32_port->throttled" check using the very > same lock. > >> if (stm32_usart_rx_dma_enabled(port)) >> return IRQ_WAKE_THREAD; Is the suggestion therefore to completely remove the hard IRQ handler and move everything into the threaded IRQ handler ? Are there any drawbacks of doing that to a serial port driver ?
On 2022-12-07 17:55:42 [+0100], Marek Vasut wrote: > > Is the suggestion therefore to completely remove the hard IRQ handler and > move everything into the threaded IRQ handler ? Commit 3489187204eb7 ("serial: stm32: adding dma support") added the threaded mode with DMA support. It didn't say _why_ this is needed. So I would suggest to use request_irq() and merge the two handler. The part where it ignores the IRQ handler in the "stm32_port->throttled" case isn't nice since it returns IRQ_HANDLED while doing nothing. > Are there any drawbacks of doing that to a serial port driver ? The threaded handler runs with disabled interrupts (due to irq-save lock) so there shouldn't be any visible difference other than the thread is gone. I'm not sure what the benefit here actually is. With `threadirqs' and so on PREEMPT_RT the whole routine will be moved in the thread (as it should). Looking at drivers/tty/serial/, the stm32-usart is the only one requesting threaded-interrupts with a primary handler. So I guess it was not needed to begin with. Sebastian
diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c index a1490033aa164..56357a7962edc 100644 --- a/drivers/tty/serial/stm32-usart.c +++ b/drivers/tty/serial/stm32-usart.c @@ -791,11 +791,8 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr) } } - if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) { - spin_lock(&port->lock); - stm32_usart_transmit_chars(port); - spin_unlock(&port->lock); - } + if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) + return IRQ_WAKE_THREAD; if (stm32_usart_rx_dma_enabled(port)) return IRQ_WAKE_THREAD; @@ -808,8 +805,18 @@ static irqreturn_t stm32_usart_threaded_interrupt(int irq, void *ptr) struct uart_port *port = ptr; struct tty_port *tport = &port->state->port; struct stm32_port *stm32_port = to_stm32_port(port); - unsigned int size; + const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs; unsigned long flags; + unsigned int size; + u32 sr; + + sr = readl_relaxed(port->membase + ofs->isr); + + if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) { + spin_lock_irqsave(&port->lock, flags); + stm32_usart_transmit_chars(port); + spin_unlock_irqrestore(&port->lock, flags); + } /* Receiver timeout irq for DMA RX */ if (!stm32_port->throttled) {
Avoid locking in hard interrupt context, move the stm32_usart_transmit_chars() into the threaded IRQ handler. This fixes the following splat with preempt-rt: BUG: scheduling while atomic: (mount)/1289/0x00010001 Modules linked in: Preemption disabled at: [<c0119127>] irq_enter_rcu+0xb/0x42 CPU: 0 PID: 1289 Comm: (mount) Not tainted 6.1.0-rc7-rt5-stable-standard-00006-gd70aeccb9f0f #17 Hardware name: STM32 (Device Tree Support) unwind_backtrace from show_stack+0xb/0xc show_stack from dump_stack_lvl+0x2b/0x34 dump_stack_lvl from __schedule_bug+0x53/0x80 __schedule_bug from __schedule+0x47/0x404 __schedule from schedule_rtlock+0x15/0x34 schedule_rtlock from rtlock_slowlock_locked+0x1d7/0x57e rtlock_slowlock_locked from rt_spin_lock+0x29/0x3c rt_spin_lock from stm32_usart_interrupt+0xa9/0x110 stm32_usart_interrupt from __handle_irq_event_percpu+0x73/0x14e __handle_irq_event_percpu from handle_irq_event_percpu+0x9/0x22 handle_irq_event_percpu from handle_irq_event+0x53/0x76 handle_irq_event from handle_fasteoi_irq+0x65/0xa8 handle_fasteoi_irq from handle_irq_desc+0xf/0x18 handle_irq_desc from gic_handle_irq+0x45/0x54 gic_handle_irq from generic_handle_arch_irq+0x19/0x2c generic_handle_arch_irq from call_with_stack+0xd/0x10 Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> Cc: Erwan Le Ray <erwan.leray@foss.st.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Jean Philippe Romain <jean-philippe.romain@foss.st.com> Cc: Jiri Slaby <jirislaby@kernel.org> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Valentin Caron <valentin.caron@foss.st.com> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-stm32@st-md-mailman.stormreply.com To: linux-serial@vger.kernel.org --- drivers/tty/serial/stm32-usart.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)