[PATCH v3] serial: stm32: optimize spin lock usage
Johan Hovold
johan at kernel.org
Fri Apr 16 15:10:52 BST 2021
On Fri, Apr 16, 2021 at 06:10:41PM +0800, dillon.minfei at gmail.com wrote:
> From: dillon min <dillon.minfei at gmail.com>
>
> This patch aims to fix two potential bug:
> - no lock to protect uart register in this case
>
> stm32_usart_threaded_interrupt()
> spin_lock(&port->lock);
> ...
> stm32_usart_receive_chars()
> uart_handle_sysrq_char();
> sysrq_function();
> printk();
> stm32_usart_console_write();
> locked = 0; //since port->sysrq is not zero,
> no lock to protect forward register
> access.
>
> - if add spin_trylock_irqsave() to protect uart register for sysrq = 1 case,
> that might got recursive locking under UP.
> So, use uart_prepare_sysrq_char(), uart_unlock_and_check_sysrq()
> move sysrq handler position to irq/thread_d handler, just record
> sysrq_ch in stm32_usart_receive_chars() by uart_prepare_sysrq_char()
> delay the sysrq process to next interrupt handler.
>
> new flow:
>
> stm32_usart_threaded_interrupt()/stm32_usart_interrupt()
> spin_lock_irqsave(&port->lock);
> ...
> uart_unlock_and_check_sysrq();
> spin_unlock_irqrestore();
> handle_sysrq(sysrq_ch);
> stm32_usart_threaded_interrupt()//stm32_usart_interrupt() return
>
> Cc: Johan Hovold <johan at kernel.org>
> Cc: Alexandre Torgue <alexandre.torgue at foss.st.com>
> Cc: Maxime Coquelin <mcoquelin.stm32 at gmail.com>
> Cc: Gerald Baeza <gerald.baeza at foss.st.com>
> Cc: Erwan Le Ray <erwan.leray at foss.st.com>
> Reported-by: kernel test robot <lkp at intel.com>
> Signed-off-by: dillon min <dillon.minfei at gmail.com>
> ---
> v3: add uart_prepare_sysrq_char(), uart_unlock_and_check_sysrq() to move
> sysrq handler inside interrupt routinei to avoid recursive locking,
> according to Johan Hovold suggestion, thanks.
>
> drivers/tty/serial/stm32-usart.c | 24 +++++++++++-------------
> 1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index b3675cf25a69..981f50ec784e 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -271,7 +271,7 @@ static void stm32_usart_receive_chars(struct uart_port *port, bool threaded)
> }
> }
>
> - if (uart_handle_sysrq_char(port, c))
> + if (uart_prepare_sysrq_char(port, c))
> continue;
> uart_insert_char(port, sr, USART_SR_ORE, c, flag);
> }
> @@ -457,9 +457,10 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
> struct uart_port *port = ptr;
> struct stm32_port *stm32_port = to_stm32_port(port);
> const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
> + unsigned long flags;
> u32 sr;
>
> - spin_lock(&port->lock);
> + spin_lock_irqsave(&port->lock, flags);
>
> sr = readl_relaxed(port->membase + ofs->isr);
>
> @@ -477,7 +478,7 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
> if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch))
> stm32_usart_transmit_chars(port);
>
> - spin_unlock(&port->lock);
> + uart_unlock_and_check_sysrq(port, flags);
>
> if (stm32_port->rx_ch)
> return IRQ_WAKE_THREAD;
> @@ -489,13 +490,14 @@ static irqreturn_t stm32_usart_threaded_interrupt(int irq, void *ptr)
> {
> struct uart_port *port = ptr;
> struct stm32_port *stm32_port = to_stm32_port(port);
> + unsigned long flags;
>
> - spin_lock(&port->lock);
> + spin_lock_irqsave(&port->lock, flags);
This essentially turns the threaded handler into a non-threaded one,
which is a bad idea.
> if (stm32_port->rx_ch)
> stm32_usart_receive_chars(port, true);
>
> - spin_unlock(&port->lock);
> + uart_unlock_and_check_sysrq(port, flags);
>
> return IRQ_HANDLED;
> }
You also didn't base this patch on tty-next, which has a number of
updates to this driver. Before noting that myself, I had fixed a couple
of deadlocks in this driver which turned out to have been incidentally
fixed by an unrelated path in -next.
I'll be posting a series that should fix up all of this.
Johan
More information about the linux-arm-kernel
mailing list