[PATCH 0/3] serial: sysrq cleanup and stm32 fixes

[PATCH 0/3] serial: sysrq cleanup and stm32 fixes

[Johan Hovold]

The first patch cleans up the interrupt handlers that rely on deferred
sysrq processing by not needlessly saving the interrupt state.

The second fixes the threaded interrupt handling of the stm32 driver
and properly fixes a couple of deadlocks that were incidentally worked
around by a recent commit.

The third patch cleans up the stm32 console implementation by switching
to deferred sysrq processing, thereby making the console code more
robust and allowing it to be used with PREEMPT_RT.

This series is against tty-next and and have only been compile tested.

Johan


Johan Hovold (3):
  serial: do not restore interrupt state in sysrq helper
  serial: stm32: fix threaded interrupt handling
  serial: stm32: defer sysrq processing

 drivers/tty/serial/8250/8250_aspeed_vuart.c |  5 ++-
 drivers/tty/serial/8250/8250_fsl.c          | 11 ++++---
 drivers/tty/serial/8250/8250_omap.c         |  6 ++--
 drivers/tty/serial/8250/8250_port.c         |  6 ++--
 drivers/tty/serial/qcom_geni_serial.c       |  6 ++--
 drivers/tty/serial/stm32-usart.c            | 36 ++++++++++-----------
 include/linux/serial_core.h                 | 10 +++---
 7 files changed, 39 insertions(+), 41 deletions(-)

-- 
2.26.3

[PATCH 1/3] serial: do not restore interrupt state in sysrq helper

[Johan Hovold]

The uart_unlock_and_check_sysrq() helper can be used to defer processing
of sysrq until the interrupt handler has released the port lock and is
about to return.

Since commit 81e2073c175b ("genirq: Disable interrupts for force
threaded handlers") interrupt handlers that are not explicitly requested
as threaded are always called with interrupts disabled and there is no
need to save the interrupt state when taking the port lock.

Instead of adding another sysrq helper for when the interrupt state has
not needlessly been saved, drop the state parameter from
uart_unlock_and_check_sysrq() and update its callers to no longer
explicitly disable interrupts in their interrupt handlers.

Cc: Joel Stanley <joel@jms.id.au>
Cc: Andrew Jeffery <andrew@aj.id.au>
Cc: Andy Gross <agross@kernel.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c |  5 ++---
 drivers/tty/serial/8250/8250_fsl.c          | 11 ++++++-----
 drivers/tty/serial/8250/8250_omap.c         |  6 +++---
 drivers/tty/serial/8250/8250_port.c         |  6 +++---
 drivers/tty/serial/qcom_geni_serial.c       |  6 +++---
 include/linux/serial_core.h                 | 10 +++++-----
 6 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index 3c239d98747f..61550f24a2d3 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -320,7 +320,6 @@ static int aspeed_vuart_handle_irq(struct uart_port *port)
 {
 	struct uart_8250_port *up = up_to_u8250p(port);
 	unsigned int iir, lsr;
-	unsigned long flags;
 	int space, count;
 
 	iir = serial_port_in(port, UART_IIR);
@@ -328,7 +327,7 @@ static int aspeed_vuart_handle_irq(struct uart_port *port)
 	if (iir & UART_IIR_NO_INT)
 		return 0;
 
-	spin_lock_irqsave(&port->lock, flags);
+	spin_lock(&port->lock);
 
 	lsr = serial_port_in(port, UART_LSR);
 
@@ -364,7 +363,7 @@ static int aspeed_vuart_handle_irq(struct uart_port *port)
 	if (lsr & UART_LSR_THRE)
 		serial8250_tx_chars(up);
 
-	uart_unlock_and_check_sysrq(port, flags);
+	uart_unlock_and_check_sysrq(port);
 
 	return 1;
 }
diff --git a/drivers/tty/serial/8250/8250_fsl.c b/drivers/tty/serial/8250/8250_fsl.c
index cd19400b65ae..4e75d2e4f87c 100644
--- a/drivers/tty/serial/8250/8250_fsl.c
+++ b/drivers/tty/serial/8250/8250_fsl.c
@@ -30,15 +30,14 @@ struct fsl8250_data {
 int fsl8250_handle_irq(struct uart_port *port)
 {
 	unsigned char lsr, orig_lsr;
-	unsigned long flags;
 	unsigned int iir;
 	struct uart_8250_port *up = up_to_u8250p(port);
 
-	spin_lock_irqsave(&up->port.lock, flags);
+	spin_lock(&up->port.lock);
 
 	iir = port->serial_in(port, UART_IIR);
 	if (iir & UART_IIR_NO_INT) {
-		spin_unlock_irqrestore(&up->port.lock, flags);
+		spin_unlock(&up->port.lock);
 		return 0;
 	}
 
@@ -46,7 +45,7 @@ int fsl8250_handle_irq(struct uart_port *port)
 	if (unlikely(up->lsr_saved_flags & UART_LSR_BI)) {
 		up->lsr_saved_flags &= ~UART_LSR_BI;
 		port->serial_in(port, UART_RX);
-		spin_unlock_irqrestore(&up->port.lock, flags);
+		spin_unlock(&up->port.lock);
 		return 1;
 	}
 
@@ -82,7 +81,9 @@ int fsl8250_handle_irq(struct uart_port *port)
 		serial8250_tx_chars(up);
 
 	up->lsr_saved_flags = orig_lsr;
-	uart_unlock_and_check_sysrq(&up->port, flags);
+
+	uart_unlock_and_check_sysrq(&up->port);
+
 	return 1;
 }
 EXPORT_SYMBOL_GPL(fsl8250_handle_irq);
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 23e0decde33e..8ac11eaeca51 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -1143,7 +1143,6 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 	struct uart_8250_port *up = up_to_u8250p(port);
 	struct omap8250_priv *priv = up->port.private_data;
 	unsigned char status;
-	unsigned long flags;
 	u8 iir;
 
 	serial8250_rpm_get(up);
@@ -1154,7 +1153,7 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 		return IRQ_HANDLED;
 	}
 
-	spin_lock_irqsave(&port->lock, flags);
+	spin_lock(&port->lock);
 
 	status = serial_port_in(port, UART_LSR);
 
@@ -1179,7 +1178,8 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 		}
 	}
 
-	uart_unlock_and_check_sysrq(port, flags);
+	uart_unlock_and_check_sysrq(port);
+
 	serial8250_rpm_put(up);
 	return 1;
 }
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 9019f8f626bb..d45dab1ab316 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1879,14 +1879,13 @@ static bool handle_rx_dma(struct uart_8250_port *up, unsigned int iir)
 int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
 {
 	unsigned char status;
-	unsigned long flags;
 	struct uart_8250_port *up = up_to_u8250p(port);
 	bool skip_rx = false;
 
 	if (iir & UART_IIR_NO_INT)
 		return 0;
 
-	spin_lock_irqsave(&port->lock, flags);
+	spin_lock(&port->lock);
 
 	status = serial_port_in(port, UART_LSR);
 
@@ -1912,7 +1911,8 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
 		(up->ier & UART_IER_THRI))
 		serial8250_tx_chars(up);
 
-	uart_unlock_and_check_sysrq(port, flags);
+	uart_unlock_and_check_sysrq(port);
+
 	return 1;
 }
 EXPORT_SYMBOL_GPL(serial8250_handle_irq);
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 0d85b55ea823..00bb88a71606 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -818,7 +818,6 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
 	u32 s_irq_status;
 	u32 geni_status;
 	struct uart_port *uport = dev;
-	unsigned long flags;
 	bool drop_rx = false;
 	struct tty_port *tport = &uport->state->port;
 	struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
@@ -826,7 +825,8 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
 	if (uport->suspended)
 		return IRQ_NONE;
 
-	spin_lock_irqsave(&uport->lock, flags);
+	spin_lock(&uport->lock);
+
 	m_irq_status = readl(uport->membase + SE_GENI_M_IRQ_STATUS);
 	s_irq_status = readl(uport->membase + SE_GENI_S_IRQ_STATUS);
 	geni_status = readl(uport->membase + SE_GENI_STATUS);
@@ -861,7 +861,7 @@ static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
 		qcom_geni_serial_handle_rx(uport, drop_rx);
 
 out_unlock:
-	uart_unlock_and_check_sysrq(uport, flags);
+	uart_unlock_and_check_sysrq(uport);
 
 	return IRQ_HANDLED;
 }
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index e1b684e33841..d7ed00f1594e 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -500,19 +500,19 @@ static inline int uart_prepare_sysrq_char(struct uart_port *port, unsigned int c
 	return 0;
 }
 
-static inline void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
+static inline void uart_unlock_and_check_sysrq(struct uart_port *port)
 {
 	int sysrq_ch;
 
 	if (!port->has_sysrq) {
-		spin_unlock_irqrestore(&port->lock, irqflags);
+		spin_unlock(&port->lock);
 		return;
 	}
 
 	sysrq_ch = port->sysrq_ch;
 	port->sysrq_ch = 0;
 
-	spin_unlock_irqrestore(&port->lock, irqflags);
+	spin_unlock(&port->lock);
 
 	if (sysrq_ch)
 		handle_sysrq(sysrq_ch);
@@ -526,9 +526,9 @@ static inline int uart_prepare_sysrq_char(struct uart_port *port, unsigned int c
 {
 	return 0;
 }
-static inline void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
+static inline void uart_unlock_and_check_sysrq(struct uart_port *port)
 {
-	spin_unlock_irqrestore(&port->lock, irqflags);
+	spin_unlock(&port->lock);
 }
 #endif	/* CONFIG_MAGIC_SYSRQ_SERIAL */
 
-- 
2.26.3

[PATCH 2/3] serial: stm32: fix threaded interrupt handling

[Johan Hovold]

When DMA is enabled the receive handler runs in a threaded handler, but
the primary handler up until very recently neither disabled interrupts
in the device or used IRQF_ONESHOT. This would lead to a deadlock if an
interrupt comes in while the threaded receive handler is running under
the port lock.

Commit ad7676812437 ("serial: stm32: fix a deadlock condition with
wakeup event") claimed to fix an unrelated deadlock, but unfortunately
also disabled interrupts in the threaded handler. While this prevents
the deadlock mentioned in the previous paragraph it also defeats the
purpose of using a threaded handler in the first place.

Fix this by making the interrupt one-shot and not disabling interrupts
in the threaded handler.

Note that (receive) DMA must not be used for a console port as the
threaded handler could be interrupted while holding the port lock,
something which could lead to a deadlock in case an interrupt handler
ends up calling printk.

Fixes: ad7676812437 ("serial: stm32: fix a deadlock condition with wakeup event")
Fixes: 3489187204eb ("serial: stm32: adding dma support")
Cc: stable@vger.kernel.org      # 4.9
Cc: Alexandre TORGUE <alexandre.torgue@st.com>
Cc: Gerald Baeza <gerald.baeza@st.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/stm32-usart.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index 4d277804c63e..3524ed2c0c73 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -214,14 +214,11 @@ static void stm32_usart_receive_chars(struct uart_port *port, bool threaded)
 	struct tty_port *tport = &port->state->port;
 	struct stm32_port *stm32_port = to_stm32_port(port);
 	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
-	unsigned long c, flags;
+	unsigned long c;
 	u32 sr;
 	char flag;
 
-	if (threaded)
-		spin_lock_irqsave(&port->lock, flags);
-	else
-		spin_lock(&port->lock);
+	spin_lock(&port->lock);
 
 	while (stm32_usart_pending_rx(port, &sr, &stm32_port->last_res,
 				      threaded)) {
@@ -278,10 +275,7 @@ static void stm32_usart_receive_chars(struct uart_port *port, bool threaded)
 		uart_insert_char(port, sr, USART_SR_ORE, c, flag);
 	}
 
-	if (threaded)
-		spin_unlock_irqrestore(&port->lock, flags);
-	else
-		spin_unlock(&port->lock);
+	spin_unlock(&port->lock);
 
 	tty_flip_buffer_push(tport);
 }
@@ -667,7 +661,8 @@ static int stm32_usart_startup(struct uart_port *port)
 
 	ret = request_threaded_irq(port->irq, stm32_usart_interrupt,
 				   stm32_usart_threaded_interrupt,
-				   IRQF_NO_SUSPEND, name, port);
+				   IRQF_ONESHOT | IRQF_NO_SUSPEND,
+				   name, port);
 	if (ret)
 		return ret;
 
@@ -1156,6 +1151,13 @@ static int stm32_usart_of_dma_rx_probe(struct stm32_port *stm32port,
 	struct dma_async_tx_descriptor *desc = NULL;
 	int ret;
 
+	/*
+	 * Using DMA and threaded handler for the console could lead to
+	 * deadlocks.
+	 */
+	if (uart_console(port))
+		return -ENODEV;
+
 	/* Request DMA RX channel */
 	stm32port->rx_ch = dma_request_slave_channel(dev, "rx");
 	if (!stm32port->rx_ch) {
-- 
2.26.3

Re: [PATCH 2/3] serial: stm32: fix threaded interrupt handling

[Johan Hovold]

On Fri, Apr 16, 2021 at 04:05:56PM +0200, Johan Hovold wrote:
> When DMA is enabled the receive handler runs in a threaded handler, but
> the primary handler up until very recently neither disabled interrupts

Scratch the "up until very recently" bit here since the driver still
doesn't disable interrupt in the device (it just disables all interrupts
in the threaded handler). The rest stands as is.

> in the device or used IRQF_ONESHOT. This would lead to a deadlock if an
> interrupt comes in while the threaded receive handler is running under
> the port lock.
> 
> Commit ad7676812437 ("serial: stm32: fix a deadlock condition with
> wakeup event") claimed to fix an unrelated deadlock, but unfortunately
> also disabled interrupts in the threaded handler. While this prevents
> the deadlock mentioned in the previous paragraph it also defeats the
> purpose of using a threaded handler in the first place.
> 
> Fix this by making the interrupt one-shot and not disabling interrupts
> in the threaded handler.
> 
> Note that (receive) DMA must not be used for a console port as the
> threaded handler could be interrupted while holding the port lock,
> something which could lead to a deadlock in case an interrupt handler
> ends up calling printk.

Johan

Re: [PATCH 2/3] serial: stm32: fix threaded interrupt handling

[Johan Hovold]

On Fri, Apr 16, 2021 at 10:35:25PM +0800, dillon min wrote:
> Hi Johan
> 
> Thanks for share your patch.
> 
> Johan Hovold <johan@kernel.org>于2021年4月16日 周五22:11写道：
> 
> > When DMA is enabled the receive handler runs in a threaded handler, but
> > the primary handler up until very recently neither disabled interrupts
> > in the device or used IRQF_ONESHOT. This would lead to a deadlock if an
> > interrupt comes in while the threaded receive handler is running under
> > the port lock.
> >
> Greg told me there was a patch fixed this case. In case hard irq &
> threaded_fn both offered. The local_irq_save() will be executed before call
> driver’s threaded handler.
> 
> Post the original mail from Greg
> 
> Please see 81e2073c175b ("genirq: Disable interrupts for force threaded
> handlers") for when threaded irq handlers have irqs disabled, isn't that
> the case you are trying to "protect" from here?
> 
> Why is the "threaded" flag used at all?  The driver should not care.
> 
> Also see 9baedb7baeda ("serial: imx: drop workaround for forced irq
> threading") in linux-next for an example of how this was fixed up in a
> serial driver.

Neither of these commits are (directly) related to the problem this
patch addresses (they are about force-threaded handlers, this is about a
normal threaded handler which run with interrupts enabled).

Johan

Re: [PATCH 2/3] serial: stm32: fix threaded interrupt handling

[Valentin CARON - foss]

Hi, Johan

On 4/16/21 4:05 PM, Johan Hovold wrote:
> When DMA is enabled the receive handler runs in a threaded handler, but
> the primary handler up until very recently neither disabled interrupts
> in the device or used IRQF_ONESHOT. This would lead to a deadlock if an
> interrupt comes in while the threaded receive handler is running under
> the port lock.
>
> Commit ad7676812437 ("serial: stm32: fix a deadlock condition with
> wakeup event") claimed to fix an unrelated deadlock, but unfortunately
> also disabled interrupts in the threaded handler. While this prevents
> the deadlock mentioned in the previous paragraph it also defeats the
> purpose of using a threaded handler in the first place.
>
> Fix this by making the interrupt one-shot and not disabling interrupts
> in the threaded handler.
>
> Note that (receive) DMA must not be used for a console port as the
> threaded handler could be interrupted while holding the port lock,
> something which could lead to a deadlock in case an interrupt handler
> ends up calling printk.
>
> Fixes: ad7676812437 ("serial: stm32: fix a deadlock condition with wakeup event")
> Fixes: 3489187204eb ("serial: stm32: adding dma support")
> Cc: stable@vger.kernel.org      # 4.9
> Cc: Alexandre TORGUE <alexandre.torgue@st.com>
> Cc: Gerald Baeza <gerald.baeza@st.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Reviewed-by: Valentin Caron<valentin.caron@foss.st.com>

> ---
>   drivers/tty/serial/stm32-usart.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index 4d277804c63e..3524ed2c0c73 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -214,14 +214,11 @@ static void stm32_usart_receive_chars(struct uart_port *port, bool threaded)
>   	struct tty_port *tport = &port->state->port;
>   	struct stm32_port *stm32_port = to_stm32_port(port);
>   	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
> -	unsigned long c, flags;
> +	unsigned long c;
>   	u32 sr;
>   	char flag;
>   
> -	if (threaded)
> -		spin_lock_irqsave(&port->lock, flags);
> -	else
> -		spin_lock(&port->lock);
> +	spin_lock(&port->lock);
>   
>   	while (stm32_usart_pending_rx(port, &sr, &stm32_port->last_res,
>   				      threaded)) {
> @@ -278,10 +275,7 @@ static void stm32_usart_receive_chars(struct uart_port *port, bool threaded)
>   		uart_insert_char(port, sr, USART_SR_ORE, c, flag);
>   	}
>   
> -	if (threaded)
> -		spin_unlock_irqrestore(&port->lock, flags);
> -	else
> -		spin_unlock(&port->lock);
> +	spin_unlock(&port->lock);
>   
>   	tty_flip_buffer_push(tport);
>   }
> @@ -667,7 +661,8 @@ static int stm32_usart_startup(struct uart_port *port)
>   
>   	ret = request_threaded_irq(port->irq, stm32_usart_interrupt,
>   				   stm32_usart_threaded_interrupt,
> -				   IRQF_NO_SUSPEND, name, port);
> +				   IRQF_ONESHOT | IRQF_NO_SUSPEND,
> +				   name, port);
>   	if (ret)
>   		return ret;
>   
> @@ -1156,6 +1151,13 @@ static int stm32_usart_of_dma_rx_probe(struct stm32_port *stm32port,
>   	struct dma_async_tx_descriptor *desc = NULL;
>   	int ret;
>   
> +	/*
> +	 * Using DMA and threaded handler for the console could lead to
> +	 * deadlocks.
> +	 */
> +	if (uart_console(port))
> +		return -ENODEV;
> +
>   	/* Request DMA RX channel */
>   	stm32port->rx_ch = dma_request_slave_channel(dev, "rx");
>   	if (!stm32port->rx_ch) {

[PATCH 3/3] serial: stm32: defer sysrq processing

[Johan Hovold]

Use the uart_unlock_and_check_sysrq() helper to defer sysrq processing
until receive processing is done and the port lock has been released.

This allows cleaning up the console_write() implementation by not having
to work around the recursive sysrq case (by dropping locking completely)
and also makes the console code work with PREEMPT_RT by no longer
relying on local_irq_save().

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/tty/serial/stm32-usart.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index 3524ed2c0c73..24a1dfe7058b 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -270,12 +270,12 @@ 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);
 	}
 
-	spin_unlock(&port->lock);
+	uart_unlock_and_check_sysrq(port);
 
 	tty_flip_buffer_push(tport);
 }
@@ -1430,13 +1430,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);
@@ -1450,8 +1447,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.26.3

Re: [PATCH 3/3] serial: stm32: defer sysrq processing

[Valentin CARON - foss]

Hi,

On 4/16/21 4:05 PM, Johan Hovold wrote:
> Use the uart_unlock_and_check_sysrq() helper to defer sysrq processing
> until receive processing is done and the port lock has been released.
>
> This allows cleaning up the console_write() implementation by not having
> to work around the recursive sysrq case (by dropping locking completely)
> and also makes the console code work with PREEMPT_RT by no longer
> relying on local_irq_save().
>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Reviewed-by: Valentin Caron<valentin.caron@foss.st.com>

> ---
>   drivers/tty/serial/stm32-usart.c | 16 ++++++----------
>   1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index 3524ed2c0c73..24a1dfe7058b 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -270,12 +270,12 @@ 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);
>   	}
>   
> -	spin_unlock(&port->lock);
> +	uart_unlock_and_check_sysrq(port);
>   
>   	tty_flip_buffer_push(tport);
>   }
> @@ -1430,13 +1430,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);
> @@ -1450,8 +1447,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)

Re: [PATCH 3/3] serial: stm32: defer sysrq processing

[kernel test robot]

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

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210416140557.25177-4-johan@kernel.org>
References: <20210416140557.25177-4-johan@kernel.org>
TO: Johan Hovold <johan@kernel.org>
TO: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>
CC: Jiri Slaby <jirislaby@kernel.org>
CC: Maxime Coquelin <mcoquelin.stm32@gmail.com>
CC: Alexandre Torgue <alexandre.torgue@foss.st.com>
CC: dillon.minfei(a)gmail.com
CC: Erwan Le Ray <erwan.leray@foss.st.com>
CC: linux-serial(a)vger.kernel.org
CC: linux-kernel(a)vger.kernel.org
CC: Johan Hovold <johan@kernel.org>

Hi Johan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on next-20210420]
[cannot apply to stm32/stm32-next usb/usb-testing v5.12-rc8]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Johan-Hovold/serial-sysrq-cleanup-and-stm32-fixes/20210416-221336
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
:::::: branch date: 4 days ago
:::::: commit date: 4 days ago
config: arm64-randconfig-s032-20210420 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-341-g8af24329-dirty
        # https://github.com/0day-ci/linux/commit/4554917dbd6d9c8d915616e748d7d1471d3b1366
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Johan-Hovold/serial-sysrq-cleanup-and-stm32-fixes/20210416-221336
        git checkout 4554917dbd6d9c8d915616e748d7d1471d3b1366
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
   drivers/tty/serial/stm32-usart.c: note: in included file:
   drivers/tty/serial/stm32-usart.h:42:25: sparse: sparse: symbol 'stm32f4_info' was not declared. Should it be static?
   drivers/tty/serial/stm32-usart.h:63:25: sparse: sparse: symbol 'stm32f7_info' was not declared. Should it be static?
   drivers/tty/serial/stm32-usart.h:85:25: sparse: sparse: symbol 'stm32h7_info' was not declared. Should it be static?
>> drivers/tty/serial/stm32-usart.c:1449:9: sparse: sparse: context imbalance in 'stm32_usart_console_write' - different lock contexts for basic block

vim +/stm32_usart_console_write +1449 drivers/tty/serial/stm32-usart.c

48a6092fb41fab Maxime Coquelin  2015-06-10  1421  
56f9a76c27b51b Erwan Le Ray     2021-01-06  1422  static void stm32_usart_console_write(struct console *co, const char *s,
92fc00238675a1 Erwan Le Ray     2021-01-06  1423  				      unsigned int cnt)
48a6092fb41fab Maxime Coquelin  2015-06-10  1424  {
48a6092fb41fab Maxime Coquelin  2015-06-10  1425  	struct uart_port *port = &stm32_ports[co->index].port;
ada8618ff3bfe1 Alexandre TORGUE 2016-09-15  1426  	struct stm32_port *stm32_port = to_stm32_port(port);
d825f0bea20f49 Stephen Boyd     2021-01-22  1427  	const struct stm32_usart_offsets *ofs = &stm32_port->info->ofs;
d825f0bea20f49 Stephen Boyd     2021-01-22  1428  	const struct stm32_usart_config *cfg = &stm32_port->info->cfg;
48a6092fb41fab Maxime Coquelin  2015-06-10  1429  	unsigned long flags;
48a6092fb41fab Maxime Coquelin  2015-06-10  1430  	u32 old_cr1, new_cr1;
48a6092fb41fab Maxime Coquelin  2015-06-10  1431  	int locked = 1;
48a6092fb41fab Maxime Coquelin  2015-06-10  1432  
4554917dbd6d9c Johan Hovold     2021-04-16  1433  	if (oops_in_progress)
4554917dbd6d9c Johan Hovold     2021-04-16  1434  		locked = spin_trylock_irqsave(&port->lock, flags);
48a6092fb41fab Maxime Coquelin  2015-06-10  1435  	else
4554917dbd6d9c Johan Hovold     2021-04-16  1436  		spin_lock_irqsave(&port->lock, flags);
48a6092fb41fab Maxime Coquelin  2015-06-10  1437  
87f1f809c9b909 Alexandre TORGUE 2016-09-15  1438  	/* Save and disable interrupts, enable the transmitter */
ada8618ff3bfe1 Alexandre TORGUE 2016-09-15  1439  	old_cr1 = readl_relaxed(port->membase + ofs->cr1);
48a6092fb41fab Maxime Coquelin  2015-06-10  1440  	new_cr1 = old_cr1 & ~USART_CR1_IE_MASK;
87f1f809c9b909 Alexandre TORGUE 2016-09-15  1441  	new_cr1 |=  USART_CR1_TE | BIT(cfg->uart_enable_bit);
ada8618ff3bfe1 Alexandre TORGUE 2016-09-15  1442  	writel_relaxed(new_cr1, port->membase + ofs->cr1);
48a6092fb41fab Maxime Coquelin  2015-06-10  1443  
56f9a76c27b51b Erwan Le Ray     2021-01-06  1444  	uart_console_write(port, s, cnt, stm32_usart_console_putchar);
48a6092fb41fab Maxime Coquelin  2015-06-10  1445  
48a6092fb41fab Maxime Coquelin  2015-06-10  1446  	/* Restore interrupt state */
ada8618ff3bfe1 Alexandre TORGUE 2016-09-15  1447  	writel_relaxed(old_cr1, port->membase + ofs->cr1);
48a6092fb41fab Maxime Coquelin  2015-06-10  1448  
48a6092fb41fab Maxime Coquelin  2015-06-10 @1449  	if (locked)
4554917dbd6d9c Johan Hovold     2021-04-16  1450  		spin_unlock_irqrestore(&port->lock, flags);
48a6092fb41fab Maxime Coquelin  2015-06-10  1451  }
48a6092fb41fab Maxime Coquelin  2015-06-10  1452  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 43421 bytes --]