All of lore.kernel.org
 help / color / mirror / Atom feed
From: dillon.minfei@gmail.com
To: gregkh@linuxfoundation.org, jirislaby@kernel.org,
	mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com,
	lkp@intel.com, johan@kernel.org, gerald.baeza@foss.st.com,
	erwan.leray@foss.st.com
Cc: linux-serial@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, kbuild-all@lists.01.org,
	clang-built-linux@googlegroups.com,
	dillon min <dillon.minfei@gmail.com>
Subject: [PATCH v3] serial: stm32: optimize spin lock usage
Date: Fri, 16 Apr 2021 18:10:41 +0800	[thread overview]
Message-ID: <1618567841-18546-1-git-send-email-dillon.minfei@gmail.com> (raw)

From: dillon min <dillon.minfei@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@kernel.org>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Gerald Baeza <gerald.baeza@foss.st.com>
Cc: Erwan Le Ray <erwan.leray@foss.st.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: dillon min <dillon.minfei@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);
 
 	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;
 }
@@ -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(&port->lock);
+	if (oops_in_progress)
+		locked = spin_trylock_irqsave(&port->lock, flags);
 	else
-		spin_lock(&port->lock);
+		spin_lock_irqsave(&port->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(&port->lock);
-	local_irq_restore(flags);
+		spin_unlock_irqrestore(&port->lock, flags);
 }
 
 static int stm32_usart_console_setup(struct console *co, char *options)
-- 
2.7.4


WARNING: multiple messages have this Message-ID (diff)
From: dillon.minfei@gmail.com
To: gregkh@linuxfoundation.org, jirislaby@kernel.org,
	mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com,
	lkp@intel.com, johan@kernel.org, gerald.baeza@foss.st.com,
	erwan.leray@foss.st.com
Cc: linux-serial@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, kbuild-all@lists.01.org,
	clang-built-linux@googlegroups.com,
	dillon min <dillon.minfei@gmail.com>
Subject: [PATCH v3] serial: stm32: optimize spin lock usage
Date: Fri, 16 Apr 2021 18:10:41 +0800	[thread overview]
Message-ID: <1618567841-18546-1-git-send-email-dillon.minfei@gmail.com> (raw)

From: dillon min <dillon.minfei@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@kernel.org>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Gerald Baeza <gerald.baeza@foss.st.com>
Cc: Erwan Le Ray <erwan.leray@foss.st.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: dillon min <dillon.minfei@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);
 
 	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;
 }
@@ -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(&port->lock);
+	if (oops_in_progress)
+		locked = spin_trylock_irqsave(&port->lock, flags);
 	else
-		spin_lock(&port->lock);
+		spin_lock_irqsave(&port->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(&port->lock);
-	local_irq_restore(flags);
+		spin_unlock_irqrestore(&port->lock, flags);
 }
 
 static int stm32_usart_console_setup(struct console *co, char *options)
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
From: dillon.minfei@gmail.com
To: kbuild-all@lists.01.org
Subject: [PATCH v3] serial: stm32: optimize spin lock usage
Date: Fri, 16 Apr 2021 18:10:41 +0800	[thread overview]
Message-ID: <1618567841-18546-1-git-send-email-dillon.minfei@gmail.com> (raw)

[-- Attachment #1: Type: text/plain, Size: 4411 bytes --]

From: dillon min <dillon.minfei@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@kernel.org>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Gerald Baeza <gerald.baeza@foss.st.com>
Cc: Erwan Le Ray <erwan.leray@foss.st.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: dillon min <dillon.minfei@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);
 
 	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;
 }
@@ -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(&port->lock);
+	if (oops_in_progress)
+		locked = spin_trylock_irqsave(&port->lock, flags);
 	else
-		spin_lock(&port->lock);
+		spin_lock_irqsave(&port->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(&port->lock);
-	local_irq_restore(flags);
+		spin_unlock_irqrestore(&port->lock, flags);
 }
 
 static int stm32_usart_console_setup(struct console *co, char *options)
-- 
2.7.4

             reply	other threads:[~2021-04-16 10:10 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-16 10:10 dillon.minfei [this message]
2021-04-16 10:10 ` [PATCH v3] serial: stm32: optimize spin lock usage dillon.minfei
2021-04-16 10:10 ` dillon.minfei
2021-04-16 14:10 ` Johan Hovold
2021-04-16 14:10   ` Johan Hovold
2021-04-16 14:10   ` Johan Hovold
2021-04-17 13:07   ` dillon min
2021-04-17 13:07     ` dillon min
2021-04-17 13:07     ` dillon min

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1618567841-18546-1-git-send-email-dillon.minfei@gmail.com \
    --to=dillon.minfei@gmail.com \
    --cc=alexandre.torgue@foss.st.com \
    --cc=clang-built-linux@googlegroups.com \
    --cc=erwan.leray@foss.st.com \
    --cc=gerald.baeza@foss.st.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jirislaby@kernel.org \
    --cc=johan@kernel.org \
    --cc=kbuild-all@lists.01.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=linux-stm32@st-md-mailman.stormreply.com \
    --cc=lkp@intel.com \
    --cc=mcoquelin.stm32@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.