Re: [PATCH v3] serial: stm32: optimize spin lock usage
Hi Johan,
On Fri, Apr 16, 2021 at 10:10 PM Johan Hovold wrote:
>
> On Fri, Apr 16, 2021 at 06:10:41PM +0800, dillon.min...@gmail.com wrote:
> > From: dillon min
> >
> > This patch aims to fix two potential bug:
> > - no lock to protect uart register in this case
> >
> > stm32_usart_threaded_interrupt()
> > spin_lock(>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(>lock);
> > ...
> > uart_unlock_and_check_sysrq();
> > spin_unlock_irqrestore();
> > handle_sysrq(sysrq_ch);
> > stm32_usart_threaded_interrupt()//stm32_usart_interrupt() return
> >
> > Cc: Johan Hovold
> > Cc: Alexandre Torgue
> > Cc: Maxime Coquelin
> > Cc: Gerald Baeza
> > Cc: Erwan Le Ray
> > Reported-by: kernel test robot
> > Signed-off-by: dillon min
> > ---
> > 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 = _port->info->ofs;
> > + unsigned long flags;
> > u32 sr;
> >
> > - spin_lock(>lock);
> > + spin_lock_irqsave(>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(>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(>lock);
> > + spin_lock_irqsave(>lock, flags);
>
> This essentially turns the threaded handler into a non-threaded one,
> which is a bad idea.
This change is only to adapt for uart_unlock_and_check_sysrq() need flags.
Found your patch has removed this parameter from
uart_unlock_and_check_sysrq(), so this changes should be removed.
>
> > if (stm32_port->rx_ch)
> > stm32_usart_receive_chars(port, true);
> >
> > - spin_unlock(>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.
Yes, my submission is based on linux-5.12. based on the component's
next branch is a good idea , to avoid conflict. thanks.
>
> I'll be posting a series that should fix up all of this.
Thanks
>
> Johan
Re: [PATCH v3] serial: stm32: optimize spin lock usage
On Fri, Apr 16, 2021 at 06:10:41PM +0800, dillon.min...@gmail.com wrote:
> From: dillon min
>
> This patch aims to fix two potential bug:
> - no lock to protect uart register in this case
>
> stm32_usart_threaded_interrupt()
> spin_lock(>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(>lock);
> ...
> uart_unlock_and_check_sysrq();
> spin_unlock_irqrestore();
> handle_sysrq(sysrq_ch);
> stm32_usart_threaded_interrupt()//stm32_usart_interrupt() return
>
> Cc: Johan Hovold
> Cc: Alexandre Torgue
> Cc: Maxime Coquelin
> Cc: Gerald Baeza
> Cc: Erwan Le Ray
> Reported-by: kernel test robot
> Signed-off-by: dillon min
> ---
> 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 = _port->info->ofs;
> + unsigned long flags;
> u32 sr;
>
> - spin_lock(>lock);
> + spin_lock_irqsave(>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(>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(>lock);
> + spin_lock_irqsave(>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(>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
[PATCH v3] serial: stm32: optimize spin lock usage
From: dillon min
This patch aims to fix two potential bug:
- no lock to protect uart register in this case
stm32_usart_threaded_interrupt()
spin_lock(>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(>lock);
...
uart_unlock_and_check_sysrq();
spin_unlock_irqrestore();
handle_sysrq(sysrq_ch);
stm32_usart_threaded_interrupt()//stm32_usart_interrupt() return
Cc: Johan Hovold
Cc: Alexandre Torgue
Cc: Maxime Coquelin
Cc: Gerald Baeza
Cc: Erwan Le Ray
Reported-by: kernel test robot
Signed-off-by: dillon min
---
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 = _port->info->ofs;
+ unsigned long flags;
u32 sr;
- spin_lock(>lock);
+ spin_lock_irqsave(>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(>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(>lock);
+ spin_lock_irqsave(>lock, flags);
if (stm32_port->rx_ch)
stm32_usart_receive_chars(port, true);
- spin_unlock(>lock);
+ uart_unlock_and_check_sysrq(port, flags);
return IRQ_HANDLED;
}
@@ -1354,13 +1356,10 @@ static void stm32_usart_console_write(struct console
*co, const char *s,
u32 old_cr1, new_cr1;
int locked = 1;
- local_irq_save(flags);
- if (port->sysrq)
- locked = 0;
- else if (oops_in_progress)
- locked = spin_trylock(>lock);
+ if (oops_in_progress)
+ locked = spin_trylock_irqsave(>lock, flags);
else
- spin_lock(>lock);
+ spin_lock_irqsave(>lock, flags);
/* Save and disable interrupts, enable the transmitter */
old_cr1 = readl_relaxed(port->membase + ofs->cr1);
@@ -1374,8 +1373,7 @@ static void stm32_usart_console_write(struct console *co,
const char *s,
writel_relaxed(old_cr1, port->membase + ofs->cr1);
if (locked)
- spin_unlock(>lock);
- local_irq_restore(flags);
+ spin_unlock_irqrestore(>lock, flags);
}
static int stm32_usart_console_setup(struct console *co, char *options)
--
2.7.4
