[PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Marek Vasut]

Requesting an interrupt with IRQF_ONESHOT will run the primary handler
in the hard-IRQ context even in the force-threaded mode. The
force-threaded mode is used by PREEMPT_RT in order to avoid acquiring
sleeping locks (spinlock_t) in hard-IRQ context. This combination
makes it impossible and leads to "sleeping while atomic" warnings.

Use one interrupt handler for both handlers (primary and secondary)
and drop the IRQF_ONESHOT flag which is not needed.

Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling")
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
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: 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
---
V2: - Update patch subject, was:
      serial: stm32: Move hard IRQ handling to threaded interrupt context
    - Use request_irq() instead, rename the IRQ handler function
V3: - Update the commit message per suggestion from Sebastian
    - Add RB from Sebastian
    - Add Fixes tag
---
 drivers/tty/serial/stm32-usart.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index dfdbcf092facc..bbbab8dc2bfa9 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -752,8 +752,9 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
 	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;
-	u32 sr;
+	unsigned long flags;
 	unsigned int size;
+	u32 sr;
 
 	sr = readl_relaxed(port->membase + ofs->isr);
 
@@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
 	}
 
 	if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) {
-		spin_lock(&port->lock);
+		spin_lock_irqsave(&port->lock, flags);
 		stm32_usart_transmit_chars(port);
-		spin_unlock(&port->lock);
+		spin_unlock_irqrestore(&port->lock, flags);
 	}
 
-	if (stm32_usart_rx_dma_enabled(port))
-		return IRQ_WAKE_THREAD;
-	else
-		return IRQ_HANDLED;
-}
-
-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;
-	unsigned long flags;
-
 	/* Receiver timeout irq for DMA RX */
-	if (!stm32_port->throttled) {
+	if (stm32_usart_rx_dma_enabled(port) && !stm32_port->throttled) {
 		spin_lock_irqsave(&port->lock, flags);
 		size = stm32_usart_receive_chars(port, false);
 		uart_unlock_and_check_sysrq_irqrestore(port, flags);
@@ -1016,10 +1003,8 @@ static int stm32_usart_startup(struct uart_port *port)
 	u32 val;
 	int ret;
 
-	ret = request_threaded_irq(port->irq, stm32_usart_interrupt,
-				   stm32_usart_threaded_interrupt,
-				   IRQF_ONESHOT | IRQF_NO_SUSPEND,
-				   name, port);
+	ret = request_irq(port->irq, stm32_usart_interrupt,
+			  IRQF_NO_SUSPEND, name, port);
 	if (ret)
 		return ret;
 
-- 
2.35.1

[PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Marek Vasut]

Requesting an interrupt with IRQF_ONESHOT will run the primary handler
in the hard-IRQ context even in the force-threaded mode. The
force-threaded mode is used by PREEMPT_RT in order to avoid acquiring
sleeping locks (spinlock_t) in hard-IRQ context. This combination
makes it impossible and leads to "sleeping while atomic" warnings.

Use one interrupt handler for both handlers (primary and secondary)
and drop the IRQF_ONESHOT flag which is not needed.

Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling")
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
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: 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
---
V2: - Update patch subject, was:
      serial: stm32: Move hard IRQ handling to threaded interrupt context
    - Use request_irq() instead, rename the IRQ handler function
V3: - Update the commit message per suggestion from Sebastian
    - Add RB from Sebastian
    - Add Fixes tag
---
 drivers/tty/serial/stm32-usart.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
index dfdbcf092facc..bbbab8dc2bfa9 100644
--- a/drivers/tty/serial/stm32-usart.c
+++ b/drivers/tty/serial/stm32-usart.c
@@ -752,8 +752,9 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
 	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;
-	u32 sr;
+	unsigned long flags;
 	unsigned int size;
+	u32 sr;
 
 	sr = readl_relaxed(port->membase + ofs->isr);
 
@@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
 	}
 
 	if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) {
-		spin_lock(&port->lock);
+		spin_lock_irqsave(&port->lock, flags);
 		stm32_usart_transmit_chars(port);
-		spin_unlock(&port->lock);
+		spin_unlock_irqrestore(&port->lock, flags);
 	}
 
-	if (stm32_usart_rx_dma_enabled(port))
-		return IRQ_WAKE_THREAD;
-	else
-		return IRQ_HANDLED;
-}
-
-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;
-	unsigned long flags;
-
 	/* Receiver timeout irq for DMA RX */
-	if (!stm32_port->throttled) {
+	if (stm32_usart_rx_dma_enabled(port) && !stm32_port->throttled) {
 		spin_lock_irqsave(&port->lock, flags);
 		size = stm32_usart_receive_chars(port, false);
 		uart_unlock_and_check_sysrq_irqrestore(port, flags);
@@ -1016,10 +1003,8 @@ static int stm32_usart_startup(struct uart_port *port)
 	u32 val;
 	int ret;
 
-	ret = request_threaded_irq(port->irq, stm32_usart_interrupt,
-				   stm32_usart_threaded_interrupt,
-				   IRQF_ONESHOT | IRQF_NO_SUSPEND,
-				   name, port);
+	ret = request_irq(port->irq, stm32_usart_interrupt,
+			  IRQF_NO_SUSPEND, name, port);
 	if (ret)
 		return ret;
 
-- 
2.35.1


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

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Johan Hovold]

On Fri, Dec 16, 2022 at 12:53:38PM +0100, Marek Vasut wrote:
> Requesting an interrupt with IRQF_ONESHOT will run the primary handler
> in the hard-IRQ context even in the force-threaded mode. The
> force-threaded mode is used by PREEMPT_RT in order to avoid acquiring
> sleeping locks (spinlock_t) in hard-IRQ context. This combination
> makes it impossible and leads to "sleeping while atomic" warnings.
> 
> Use one interrupt handler for both handlers (primary and secondary)
> and drop the IRQF_ONESHOT flag which is not needed.
> 
> Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling")

I don't think a Fixes tag is warranted as this is only needed due to
this undocumented quirk of PREEMPT_RT.

And this should not be backported in any case.

> Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 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: 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
> ---
> V2: - Update patch subject, was:
>       serial: stm32: Move hard IRQ handling to threaded interrupt context
>     - Use request_irq() instead, rename the IRQ handler function
> V3: - Update the commit message per suggestion from Sebastian
>     - Add RB from Sebastian
>     - Add Fixes tag
> ---
>  drivers/tty/serial/stm32-usart.c | 29 +++++++----------------------
>  1 file changed, 7 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index dfdbcf092facc..bbbab8dc2bfa9 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -752,8 +752,9 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
>  	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;
> -	u32 sr;
> +	unsigned long flags;
>  	unsigned int size;
> +	u32 sr;
>  
>  	sr = readl_relaxed(port->membase + ofs->isr);
>  
> @@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
>  	}
>  
>  	if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) {
> -		spin_lock(&port->lock);
> +		spin_lock_irqsave(&port->lock, flags);
>  		stm32_usart_transmit_chars(port);
> -		spin_unlock(&port->lock);
> +		spin_unlock_irqrestore(&port->lock, flags);

This is not needed as the handler runs with interrupts disabled.

>  	}
>  
> -	if (stm32_usart_rx_dma_enabled(port))
> -		return IRQ_WAKE_THREAD;
> -	else
> -		return IRQ_HANDLED;
> -}
> -
> -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;
> -	unsigned long flags;
> -
>  	/* Receiver timeout irq for DMA RX */
> -	if (!stm32_port->throttled) {
> +	if (stm32_usart_rx_dma_enabled(port) && !stm32_port->throttled) {
>  		spin_lock_irqsave(&port->lock, flags);

But you could change this to spin_lock() now.

>  		size = stm32_usart_receive_chars(port, false);
>  		uart_unlock_and_check_sysrq_irqrestore(port, flags);
> @@ -1016,10 +1003,8 @@ static int stm32_usart_startup(struct uart_port *port)
>  	u32 val;
>  	int ret;
>  
> -	ret = request_threaded_irq(port->irq, stm32_usart_interrupt,
> -				   stm32_usart_threaded_interrupt,
> -				   IRQF_ONESHOT | IRQF_NO_SUSPEND,
> -				   name, port);
> +	ret = request_irq(port->irq, stm32_usart_interrupt,
> +			  IRQF_NO_SUSPEND, name, port);
>  	if (ret)
>  		return ret;

You should also remove

	/*
	 * Using DMA and threaded handler for the console could lead to
	 * deadlocks.
	 */
	if (uart_console(port))
		return -ENODEV;

from stm32_usart_of_dma_rx_probe() when removing the threaded handler.

Johan

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Johan Hovold]

On Fri, Dec 16, 2022 at 12:53:38PM +0100, Marek Vasut wrote:
> Requesting an interrupt with IRQF_ONESHOT will run the primary handler
> in the hard-IRQ context even in the force-threaded mode. The
> force-threaded mode is used by PREEMPT_RT in order to avoid acquiring
> sleeping locks (spinlock_t) in hard-IRQ context. This combination
> makes it impossible and leads to "sleeping while atomic" warnings.
> 
> Use one interrupt handler for both handlers (primary and secondary)
> and drop the IRQF_ONESHOT flag which is not needed.
> 
> Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling")

I don't think a Fixes tag is warranted as this is only needed due to
this undocumented quirk of PREEMPT_RT.

And this should not be backported in any case.

> Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 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: 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
> ---
> V2: - Update patch subject, was:
>       serial: stm32: Move hard IRQ handling to threaded interrupt context
>     - Use request_irq() instead, rename the IRQ handler function
> V3: - Update the commit message per suggestion from Sebastian
>     - Add RB from Sebastian
>     - Add Fixes tag
> ---
>  drivers/tty/serial/stm32-usart.c | 29 +++++++----------------------
>  1 file changed, 7 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index dfdbcf092facc..bbbab8dc2bfa9 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -752,8 +752,9 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
>  	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;
> -	u32 sr;
> +	unsigned long flags;
>  	unsigned int size;
> +	u32 sr;
>  
>  	sr = readl_relaxed(port->membase + ofs->isr);
>  
> @@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
>  	}
>  
>  	if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) {
> -		spin_lock(&port->lock);
> +		spin_lock_irqsave(&port->lock, flags);
>  		stm32_usart_transmit_chars(port);
> -		spin_unlock(&port->lock);
> +		spin_unlock_irqrestore(&port->lock, flags);

This is not needed as the handler runs with interrupts disabled.

>  	}
>  
> -	if (stm32_usart_rx_dma_enabled(port))
> -		return IRQ_WAKE_THREAD;
> -	else
> -		return IRQ_HANDLED;
> -}
> -
> -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;
> -	unsigned long flags;
> -
>  	/* Receiver timeout irq for DMA RX */
> -	if (!stm32_port->throttled) {
> +	if (stm32_usart_rx_dma_enabled(port) && !stm32_port->throttled) {
>  		spin_lock_irqsave(&port->lock, flags);

But you could change this to spin_lock() now.

>  		size = stm32_usart_receive_chars(port, false);
>  		uart_unlock_and_check_sysrq_irqrestore(port, flags);
> @@ -1016,10 +1003,8 @@ static int stm32_usart_startup(struct uart_port *port)
>  	u32 val;
>  	int ret;
>  
> -	ret = request_threaded_irq(port->irq, stm32_usart_interrupt,
> -				   stm32_usart_threaded_interrupt,
> -				   IRQF_ONESHOT | IRQF_NO_SUSPEND,
> -				   name, port);
> +	ret = request_irq(port->irq, stm32_usart_interrupt,
> +			  IRQF_NO_SUSPEND, name, port);
>  	if (ret)
>  		return ret;

You should also remove

	/*
	 * Using DMA and threaded handler for the console could lead to
	 * deadlocks.
	 */
	if (uart_console(port))
		return -ENODEV;

from stm32_usart_of_dma_rx_probe() when removing the threaded handler.

Johan

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

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Valentin CARON]

Hi Marek,

It is OK for me.

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

Thanks,
Valentin

On 12/16/22 12:53, Marek Vasut wrote:
> Requesting an interrupt with IRQF_ONESHOT will run the primary handler
> in the hard-IRQ context even in the force-threaded mode. The
> force-threaded mode is used by PREEMPT_RT in order to avoid acquiring
> sleeping locks (spinlock_t) in hard-IRQ context. This combination
> makes it impossible and leads to "sleeping while atomic" warnings.
>
> Use one interrupt handler for both handlers (primary and secondary)
> and drop the IRQF_ONESHOT flag which is not needed.
>
> Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling")
> Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 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: 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
> ---
> V2: - Update patch subject, was:
>        serial: stm32: Move hard IRQ handling to threaded interrupt context
>      - Use request_irq() instead, rename the IRQ handler function
> V3: - Update the commit message per suggestion from Sebastian
>      - Add RB from Sebastian
>      - Add Fixes tag
> ---
>   drivers/tty/serial/stm32-usart.c | 29 +++++++----------------------
>   1 file changed, 7 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index dfdbcf092facc..bbbab8dc2bfa9 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -752,8 +752,9 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
>   	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;
> -	u32 sr;
> +	unsigned long flags;
>   	unsigned int size;
> +	u32 sr;
>   
>   	sr = readl_relaxed(port->membase + ofs->isr);
>   
> @@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
>   	}
>   
>   	if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) {
> -		spin_lock(&port->lock);
> +		spin_lock_irqsave(&port->lock, flags);
>   		stm32_usart_transmit_chars(port);
> -		spin_unlock(&port->lock);
> +		spin_unlock_irqrestore(&port->lock, flags);
>   	}
>   
> -	if (stm32_usart_rx_dma_enabled(port))
> -		return IRQ_WAKE_THREAD;
> -	else
> -		return IRQ_HANDLED;
> -}
> -
> -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;
> -	unsigned long flags;
> -
>   	/* Receiver timeout irq for DMA RX */
> -	if (!stm32_port->throttled) {
> +	if (stm32_usart_rx_dma_enabled(port) && !stm32_port->throttled) {
>   		spin_lock_irqsave(&port->lock, flags);
>   		size = stm32_usart_receive_chars(port, false);
>   		uart_unlock_and_check_sysrq_irqrestore(port, flags);
> @@ -1016,10 +1003,8 @@ static int stm32_usart_startup(struct uart_port *port)
>   	u32 val;
>   	int ret;
>   
> -	ret = request_threaded_irq(port->irq, stm32_usart_interrupt,
> -				   stm32_usart_threaded_interrupt,
> -				   IRQF_ONESHOT | IRQF_NO_SUSPEND,
> -				   name, port);
> +	ret = request_irq(port->irq, stm32_usart_interrupt,
> +			  IRQF_NO_SUSPEND, name, port);
>   	if (ret)
>   		return ret;
>   

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Valentin CARON]

Hi Marek,

It is OK for me.

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

Thanks,
Valentin

On 12/16/22 12:53, Marek Vasut wrote:
> Requesting an interrupt with IRQF_ONESHOT will run the primary handler
> in the hard-IRQ context even in the force-threaded mode. The
> force-threaded mode is used by PREEMPT_RT in order to avoid acquiring
> sleeping locks (spinlock_t) in hard-IRQ context. This combination
> makes it impossible and leads to "sleeping while atomic" warnings.
>
> Use one interrupt handler for both handlers (primary and secondary)
> and drop the IRQF_ONESHOT flag which is not needed.
>
> Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling")
> Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 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: 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
> ---
> V2: - Update patch subject, was:
>        serial: stm32: Move hard IRQ handling to threaded interrupt context
>      - Use request_irq() instead, rename the IRQ handler function
> V3: - Update the commit message per suggestion from Sebastian
>      - Add RB from Sebastian
>      - Add Fixes tag
> ---
>   drivers/tty/serial/stm32-usart.c | 29 +++++++----------------------
>   1 file changed, 7 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index dfdbcf092facc..bbbab8dc2bfa9 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -752,8 +752,9 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
>   	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;
> -	u32 sr;
> +	unsigned long flags;
>   	unsigned int size;
> +	u32 sr;
>   
>   	sr = readl_relaxed(port->membase + ofs->isr);
>   
> @@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
>   	}
>   
>   	if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) {
> -		spin_lock(&port->lock);
> +		spin_lock_irqsave(&port->lock, flags);
>   		stm32_usart_transmit_chars(port);
> -		spin_unlock(&port->lock);
> +		spin_unlock_irqrestore(&port->lock, flags);
>   	}
>   
> -	if (stm32_usart_rx_dma_enabled(port))
> -		return IRQ_WAKE_THREAD;
> -	else
> -		return IRQ_HANDLED;
> -}
> -
> -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;
> -	unsigned long flags;
> -
>   	/* Receiver timeout irq for DMA RX */
> -	if (!stm32_port->throttled) {
> +	if (stm32_usart_rx_dma_enabled(port) && !stm32_port->throttled) {
>   		spin_lock_irqsave(&port->lock, flags);
>   		size = stm32_usart_receive_chars(port, false);
>   		uart_unlock_and_check_sysrq_irqrestore(port, flags);
> @@ -1016,10 +1003,8 @@ static int stm32_usart_startup(struct uart_port *port)
>   	u32 val;
>   	int ret;
>   
> -	ret = request_threaded_irq(port->irq, stm32_usart_interrupt,
> -				   stm32_usart_threaded_interrupt,
> -				   IRQF_ONESHOT | IRQF_NO_SUSPEND,
> -				   name, port);
> +	ret = request_irq(port->irq, stm32_usart_interrupt,
> +			  IRQF_NO_SUSPEND, name, port);
>   	if (ret)
>   		return ret;
>   

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

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Marek Vasut]

On 12/27/22 15:56, Johan Hovold wrote:

[...]

>> @@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
>>   	}
>>   
>>   	if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) {
>> -		spin_lock(&port->lock);
>> +		spin_lock_irqsave(&port->lock, flags);
>>   		stm32_usart_transmit_chars(port);
>> -		spin_unlock(&port->lock);
>> +		spin_unlock_irqrestore(&port->lock, flags);
> 
> This is not needed as the handler runs with interrupts disabled.

On SMP system, another thread on another core can call 
stm32_usart_transmit_chars() . I don't think removing the locking is 
correct ?

> 
>>   	}
>>   
>> -	if (stm32_usart_rx_dma_enabled(port))
>> -		return IRQ_WAKE_THREAD;
>> -	else
>> -		return IRQ_HANDLED;
>> -}
>> -
>> -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;
>> -	unsigned long flags;
>> -
>>   	/* Receiver timeout irq for DMA RX */
>> -	if (!stm32_port->throttled) {
>> +	if (stm32_usart_rx_dma_enabled(port) && !stm32_port->throttled) {
>>   		spin_lock_irqsave(&port->lock, flags);
> 
> But you could change this to spin_lock() now.

[...]

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Marek Vasut]

On 12/27/22 15:56, Johan Hovold wrote:

[...]

>> @@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
>>   	}
>>   
>>   	if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) {
>> -		spin_lock(&port->lock);
>> +		spin_lock_irqsave(&port->lock, flags);
>>   		stm32_usart_transmit_chars(port);
>> -		spin_unlock(&port->lock);
>> +		spin_unlock_irqrestore(&port->lock, flags);
> 
> This is not needed as the handler runs with interrupts disabled.

On SMP system, another thread on another core can call 
stm32_usart_transmit_chars() . I don't think removing the locking is 
correct ?

> 
>>   	}
>>   
>> -	if (stm32_usart_rx_dma_enabled(port))
>> -		return IRQ_WAKE_THREAD;
>> -	else
>> -		return IRQ_HANDLED;
>> -}
>> -
>> -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;
>> -	unsigned long flags;
>> -
>>   	/* Receiver timeout irq for DMA RX */
>> -	if (!stm32_port->throttled) {
>> +	if (stm32_usart_rx_dma_enabled(port) && !stm32_port->throttled) {
>>   		spin_lock_irqsave(&port->lock, flags);
> 
> But you could change this to spin_lock() now.

[...]

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

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Marek Vasut]

On 1/5/23 15:56, Valentin CARON wrote:
> Hi Marek,

Hello Valentin,

> It is OK for me.
> 
> Tested-by: Valentin Caron <valentin.caron@foss.st.com>

Thank you. I might need one more test for V4 in a bit.

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Marek Vasut]

On 1/5/23 15:56, Valentin CARON wrote:
> Hi Marek,

Hello Valentin,

> It is OK for me.
> 
> Tested-by: Valentin Caron <valentin.caron@foss.st.com>

Thank you. I might need one more test for V4 in a bit.

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

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Johan Hovold]

On Thu, Jan 05, 2023 at 09:46:57PM +0100, Marek Vasut wrote:
> On 12/27/22 15:56, Johan Hovold wrote:
> 
> [...]
> 
> >> @@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
> >>   	}
> >>   
> >>   	if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) {
> >> -		spin_lock(&port->lock);
> >> +		spin_lock_irqsave(&port->lock, flags);
> >>   		stm32_usart_transmit_chars(port);
> >> -		spin_unlock(&port->lock);
> >> +		spin_unlock_irqrestore(&port->lock, flags);
> > 
> > This is not needed as the handler runs with interrupts disabled.
> 
> On SMP system, another thread on another core can call 
> stm32_usart_transmit_chars() . I don't think removing the locking is 
> correct ?

I didn't say that you should remove the locking, which is very much
needed. There's just no need to disable interrupts in a (non-threaded)
interrupt handler as that has already been done by IRQ core (and, yes,
that is also the case with forced threading).

Johan

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Johan Hovold]

On Thu, Jan 05, 2023 at 09:46:57PM +0100, Marek Vasut wrote:
> On 12/27/22 15:56, Johan Hovold wrote:
> 
> [...]
> 
> >> @@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
> >>   	}
> >>   
> >>   	if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) {
> >> -		spin_lock(&port->lock);
> >> +		spin_lock_irqsave(&port->lock, flags);
> >>   		stm32_usart_transmit_chars(port);
> >> -		spin_unlock(&port->lock);
> >> +		spin_unlock_irqrestore(&port->lock, flags);
> > 
> > This is not needed as the handler runs with interrupts disabled.
> 
> On SMP system, another thread on another core can call 
> stm32_usart_transmit_chars() . I don't think removing the locking is 
> correct ?

I didn't say that you should remove the locking, which is very much
needed. There's just no need to disable interrupts in a (non-threaded)
interrupt handler as that has already been done by IRQ core (and, yes,
that is also the case with forced threading).

Johan

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

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Sebastian Andrzej Siewior]

On 2022-12-27 15:56:47 [+0100], Johan Hovold wrote:
> On Fri, Dec 16, 2022 at 12:53:38PM +0100, Marek Vasut wrote:
> > Requesting an interrupt with IRQF_ONESHOT will run the primary handler
> > in the hard-IRQ context even in the force-threaded mode. The
> > force-threaded mode is used by PREEMPT_RT in order to avoid acquiring
> > sleeping locks (spinlock_t) in hard-IRQ context. This combination
> > makes it impossible and leads to "sleeping while atomic" warnings.
> > 
> > Use one interrupt handler for both handlers (primary and secondary)
> > and drop the IRQF_ONESHOT flag which is not needed.
> > 
> > Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling")
> 
> I don't think a Fixes tag is warranted as this is only needed due to
> this undocumented quirk of PREEMPT_RT.

It is not an undocumented quirk of PREEMPT_RT. The part that might not
be well documented is that IRQF_ONESHOT won't run the primary handler as
a threaded handler. This is also the case for IRQF_NO_THREAD and
IRQF_PERCPU but might be more obvious.
Anyway, if the primary handler is not threaded then it runs in HARDIRQ
context and here you must not use a spinlock_t. This is documented in
Documentation/locking/locktypes.rst and there is also a LOCKDEP warning
for this on !RT which is off by default because it triggers with printk
(and this is worked on).

> And this should not be backported in any case.

Such things have been backported via -stable in the past. If you
disagree, please keep me in loop while is merged so I can poke people to
backport it as part of the RT patch for the relevant kernels.

> Johan

Sebastian

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Sebastian Andrzej Siewior]

On 2022-12-27 15:56:47 [+0100], Johan Hovold wrote:
> On Fri, Dec 16, 2022 at 12:53:38PM +0100, Marek Vasut wrote:
> > Requesting an interrupt with IRQF_ONESHOT will run the primary handler
> > in the hard-IRQ context even in the force-threaded mode. The
> > force-threaded mode is used by PREEMPT_RT in order to avoid acquiring
> > sleeping locks (spinlock_t) in hard-IRQ context. This combination
> > makes it impossible and leads to "sleeping while atomic" warnings.
> > 
> > Use one interrupt handler for both handlers (primary and secondary)
> > and drop the IRQF_ONESHOT flag which is not needed.
> > 
> > Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling")
> 
> I don't think a Fixes tag is warranted as this is only needed due to
> this undocumented quirk of PREEMPT_RT.

It is not an undocumented quirk of PREEMPT_RT. The part that might not
be well documented is that IRQF_ONESHOT won't run the primary handler as
a threaded handler. This is also the case for IRQF_NO_THREAD and
IRQF_PERCPU but might be more obvious.
Anyway, if the primary handler is not threaded then it runs in HARDIRQ
context and here you must not use a spinlock_t. This is documented in
Documentation/locking/locktypes.rst and there is also a LOCKDEP warning
for this on !RT which is off by default because it triggers with printk
(and this is worked on).

> And this should not be backported in any case.

Such things have been backported via -stable in the past. If you
disagree, please keep me in loop while is merged so I can poke people to
backport it as part of the RT patch for the relevant kernels.

> Johan

Sebastian

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

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Marek Vasut]

On 1/6/23 11:56, Johan Hovold wrote:
> On Thu, Jan 05, 2023 at 09:46:57PM +0100, Marek Vasut wrote:
>> On 12/27/22 15:56, Johan Hovold wrote:
>>
>> [...]
>>
>>>> @@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
>>>>    	}
>>>>    
>>>>    	if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) {
>>>> -		spin_lock(&port->lock);
>>>> +		spin_lock_irqsave(&port->lock, flags);
>>>>    		stm32_usart_transmit_chars(port);
>>>> -		spin_unlock(&port->lock);
>>>> +		spin_unlock_irqrestore(&port->lock, flags);
>>>
>>> This is not needed as the handler runs with interrupts disabled.
>>
>> On SMP system, another thread on another core can call
>> stm32_usart_transmit_chars() . I don't think removing the locking is
>> correct ?
> 
> I didn't say that you should remove the locking, which is very much
> needed. There's just no need to disable interrupts in a (non-threaded)
> interrupt handler as that has already been done by IRQ core (and, yes,
> that is also the case with forced threading).

Ah, understood.

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Marek Vasut]

On 1/6/23 11:56, Johan Hovold wrote:
> On Thu, Jan 05, 2023 at 09:46:57PM +0100, Marek Vasut wrote:
>> On 12/27/22 15:56, Johan Hovold wrote:
>>
>> [...]
>>
>>>> @@ -793,27 +794,13 @@ static irqreturn_t stm32_usart_interrupt(int irq, void *ptr)
>>>>    	}
>>>>    
>>>>    	if ((sr & USART_SR_TXE) && !(stm32_port->tx_ch)) {
>>>> -		spin_lock(&port->lock);
>>>> +		spin_lock_irqsave(&port->lock, flags);
>>>>    		stm32_usart_transmit_chars(port);
>>>> -		spin_unlock(&port->lock);
>>>> +		spin_unlock_irqrestore(&port->lock, flags);
>>>
>>> This is not needed as the handler runs with interrupts disabled.
>>
>> On SMP system, another thread on another core can call
>> stm32_usart_transmit_chars() . I don't think removing the locking is
>> correct ?
> 
> I didn't say that you should remove the locking, which is very much
> needed. There's just no need to disable interrupts in a (non-threaded)
> interrupt handler as that has already been done by IRQ core (and, yes,
> that is also the case with forced threading).

Ah, understood.

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

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Johan Hovold]

On Mon, Jan 09, 2023 at 11:13:15AM +0100, Sebastian Andrzej Siewior wrote:
> On 2022-12-27 15:56:47 [+0100], Johan Hovold wrote:
> > On Fri, Dec 16, 2022 at 12:53:38PM +0100, Marek Vasut wrote:
> > > Requesting an interrupt with IRQF_ONESHOT will run the primary handler
> > > in the hard-IRQ context even in the force-threaded mode. The
> > > force-threaded mode is used by PREEMPT_RT in order to avoid acquiring
> > > sleeping locks (spinlock_t) in hard-IRQ context. This combination
> > > makes it impossible and leads to "sleeping while atomic" warnings.
> > > 
> > > Use one interrupt handler for both handlers (primary and secondary)
> > > and drop the IRQF_ONESHOT flag which is not needed.
> > > 
> > > Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling")
> > 
> > I don't think a Fixes tag is warranted as this is only needed due to
> > this undocumented quirk of PREEMPT_RT.
> 
> It is not an undocumented quirk of PREEMPT_RT. The part that might not
> be well documented is that IRQF_ONESHOT won't run the primary handler as
> a threaded handler. This is also the case for IRQF_NO_THREAD and
> IRQF_PERCPU but might be more obvious.

Yeah, that not being documented seems like an oversight as generally
drivers should not need be changed to continue working on PREEMPT_RT (or
with forced-threading generally).

> Anyway, if the primary handler is not threaded then it runs in HARDIRQ
> context and here you must not use a spinlock_t. This is documented in
> Documentation/locking/locktypes.rst and there is also a LOCKDEP warning
> for this on !RT which is off by default because it triggers with printk
> (and this is worked on).

All interrupt handlers typically run in hard interrupt context unless
explicitly requested to be threaded on !RT and drivers still use
spinlock_t for that so not sure how that lockdep warning is supposed to
work. Or do you mean that you have a lockdep warning specifically for
IRQF_ONESHOT primary handlers?
 
> > And this should not be backported in any case.
> 
> Such things have been backported via -stable in the past. If you
> disagree, please keep me in loop while is merged so I can poke people to
> backport it as part of the RT patch for the relevant kernels.

The author did not seem to think this was stable material as there is no
cc-stable tag and it also seems fairly intrusive.

But if the ST guys or whoever cares about this driver are fine with this
being backported, I don't really mind either.

Johan

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Johan Hovold]

On Mon, Jan 09, 2023 at 11:13:15AM +0100, Sebastian Andrzej Siewior wrote:
> On 2022-12-27 15:56:47 [+0100], Johan Hovold wrote:
> > On Fri, Dec 16, 2022 at 12:53:38PM +0100, Marek Vasut wrote:
> > > Requesting an interrupt with IRQF_ONESHOT will run the primary handler
> > > in the hard-IRQ context even in the force-threaded mode. The
> > > force-threaded mode is used by PREEMPT_RT in order to avoid acquiring
> > > sleeping locks (spinlock_t) in hard-IRQ context. This combination
> > > makes it impossible and leads to "sleeping while atomic" warnings.
> > > 
> > > Use one interrupt handler for both handlers (primary and secondary)
> > > and drop the IRQF_ONESHOT flag which is not needed.
> > > 
> > > Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling")
> > 
> > I don't think a Fixes tag is warranted as this is only needed due to
> > this undocumented quirk of PREEMPT_RT.
> 
> It is not an undocumented quirk of PREEMPT_RT. The part that might not
> be well documented is that IRQF_ONESHOT won't run the primary handler as
> a threaded handler. This is also the case for IRQF_NO_THREAD and
> IRQF_PERCPU but might be more obvious.

Yeah, that not being documented seems like an oversight as generally
drivers should not need be changed to continue working on PREEMPT_RT (or
with forced-threading generally).

> Anyway, if the primary handler is not threaded then it runs in HARDIRQ
> context and here you must not use a spinlock_t. This is documented in
> Documentation/locking/locktypes.rst and there is also a LOCKDEP warning
> for this on !RT which is off by default because it triggers with printk
> (and this is worked on).

All interrupt handlers typically run in hard interrupt context unless
explicitly requested to be threaded on !RT and drivers still use
spinlock_t for that so not sure how that lockdep warning is supposed to
work. Or do you mean that you have a lockdep warning specifically for
IRQF_ONESHOT primary handlers?
 
> > And this should not be backported in any case.
> 
> Such things have been backported via -stable in the past. If you
> disagree, please keep me in loop while is merged so I can poke people to
> backport it as part of the RT patch for the relevant kernels.

The author did not seem to think this was stable material as there is no
cc-stable tag and it also seems fairly intrusive.

But if the ST guys or whoever cares about this driver are fine with this
being backported, I don't really mind either.

Johan

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

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Marek Vasut]

On 1/12/23 14:13, Johan Hovold wrote:
> On Mon, Jan 09, 2023 at 11:13:15AM +0100, Sebastian Andrzej Siewior wrote:
>> On 2022-12-27 15:56:47 [+0100], Johan Hovold wrote:
>>> On Fri, Dec 16, 2022 at 12:53:38PM +0100, Marek Vasut wrote:
>>>> Requesting an interrupt with IRQF_ONESHOT will run the primary handler
>>>> in the hard-IRQ context even in the force-threaded mode. The
>>>> force-threaded mode is used by PREEMPT_RT in order to avoid acquiring
>>>> sleeping locks (spinlock_t) in hard-IRQ context. This combination
>>>> makes it impossible and leads to "sleeping while atomic" warnings.
>>>>
>>>> Use one interrupt handler for both handlers (primary and secondary)
>>>> and drop the IRQF_ONESHOT flag which is not needed.
>>>>
>>>> Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling")
>>>
>>> I don't think a Fixes tag is warranted as this is only needed due to
>>> this undocumented quirk of PREEMPT_RT.
>>
>> It is not an undocumented quirk of PREEMPT_RT. The part that might not
>> be well documented is that IRQF_ONESHOT won't run the primary handler as
>> a threaded handler. This is also the case for IRQF_NO_THREAD and
>> IRQF_PERCPU but might be more obvious.
> 
> Yeah, that not being documented seems like an oversight as generally
> drivers should not need be changed to continue working on PREEMPT_RT (or
> with forced-threading generally).
> 
>> Anyway, if the primary handler is not threaded then it runs in HARDIRQ
>> context and here you must not use a spinlock_t. This is documented in
>> Documentation/locking/locktypes.rst and there is also a LOCKDEP warning
>> for this on !RT which is off by default because it triggers with printk
>> (and this is worked on).
> 
> All interrupt handlers typically run in hard interrupt context unless
> explicitly requested to be threaded on !RT and drivers still use
> spinlock_t for that so not sure how that lockdep warning is supposed to
> work. Or do you mean that you have a lockdep warning specifically for
> IRQF_ONESHOT primary handlers?
>   
>>> And this should not be backported in any case.
>>
>> Such things have been backported via -stable in the past. If you
>> disagree, please keep me in loop while is merged so I can poke people to
>> backport it as part of the RT patch for the relevant kernels.
> 
> The author did not seem to think this was stable material as there is no
> cc-stable tag and it also seems fairly intrusive.

The author does not have enough experience with preempt-rt to make that 
determination, hence deferred to Sebastian for better judgement.

> But if the ST guys or whoever cares about this driver are fine with this
> being backported, I don't really mind either.

[...]

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

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Marek Vasut]

On 1/12/23 14:13, Johan Hovold wrote:
> On Mon, Jan 09, 2023 at 11:13:15AM +0100, Sebastian Andrzej Siewior wrote:
>> On 2022-12-27 15:56:47 [+0100], Johan Hovold wrote:
>>> On Fri, Dec 16, 2022 at 12:53:38PM +0100, Marek Vasut wrote:
>>>> Requesting an interrupt with IRQF_ONESHOT will run the primary handler
>>>> in the hard-IRQ context even in the force-threaded mode. The
>>>> force-threaded mode is used by PREEMPT_RT in order to avoid acquiring
>>>> sleeping locks (spinlock_t) in hard-IRQ context. This combination
>>>> makes it impossible and leads to "sleeping while atomic" warnings.
>>>>
>>>> Use one interrupt handler for both handlers (primary and secondary)
>>>> and drop the IRQF_ONESHOT flag which is not needed.
>>>>
>>>> Fixes: e359b4411c283 ("serial: stm32: fix threaded interrupt handling")
>>>
>>> I don't think a Fixes tag is warranted as this is only needed due to
>>> this undocumented quirk of PREEMPT_RT.
>>
>> It is not an undocumented quirk of PREEMPT_RT. The part that might not
>> be well documented is that IRQF_ONESHOT won't run the primary handler as
>> a threaded handler. This is also the case for IRQF_NO_THREAD and
>> IRQF_PERCPU but might be more obvious.
> 
> Yeah, that not being documented seems like an oversight as generally
> drivers should not need be changed to continue working on PREEMPT_RT (or
> with forced-threading generally).
> 
>> Anyway, if the primary handler is not threaded then it runs in HARDIRQ
>> context and here you must not use a spinlock_t. This is documented in
>> Documentation/locking/locktypes.rst and there is also a LOCKDEP warning
>> for this on !RT which is off by default because it triggers with printk
>> (and this is worked on).
> 
> All interrupt handlers typically run in hard interrupt context unless
> explicitly requested to be threaded on !RT and drivers still use
> spinlock_t for that so not sure how that lockdep warning is supposed to
> work. Or do you mean that you have a lockdep warning specifically for
> IRQF_ONESHOT primary handlers?
>   
>>> And this should not be backported in any case.
>>
>> Such things have been backported via -stable in the past. If you
>> disagree, please keep me in loop while is merged so I can poke people to
>> backport it as part of the RT patch for the relevant kernels.
> 
> The author did not seem to think this was stable material as there is no
> cc-stable tag and it also seems fairly intrusive.

The author does not have enough experience with preempt-rt to make that 
determination, hence deferred to Sebastian for better judgement.

> But if the ST guys or whoever cares about this driver are fine with this
> being backported, I don't really mind either.

[...]

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Johan Hovold]

On Thu, Jan 12, 2023 at 05:38:48PM +0100, Marek Vasut wrote:
> On 1/12/23 14:13, Johan Hovold wrote:
  
> > The author did not seem to think this was stable material as there is no
> > cc-stable tag and it also seems fairly intrusive.
> 
> The author does not have enough experience with preempt-rt to make that 
> determination, hence deferred to Sebastian for better judgement.

Fair enough. And it's not obvious that the stable team should backport
patches that only concern PREEMPT_RT either (e.g. as parts of it are
still out-of-tree).

The stable tag is still missing from the final revision though.

> > But if the ST guys or whoever cares about this driver are fine with this
> > being backported, I don't really mind either.

Johan

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

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Johan Hovold]

On Thu, Jan 12, 2023 at 05:38:48PM +0100, Marek Vasut wrote:
> On 1/12/23 14:13, Johan Hovold wrote:
  
> > The author did not seem to think this was stable material as there is no
> > cc-stable tag and it also seems fairly intrusive.
> 
> The author does not have enough experience with preempt-rt to make that 
> determination, hence deferred to Sebastian for better judgement.

Fair enough. And it's not obvious that the stable team should backport
patches that only concern PREEMPT_RT either (e.g. as parts of it are
still out-of-tree).

The stable tag is still missing from the final revision though.

> > But if the ST guys or whoever cares about this driver are fine with this
> > being backported, I don't really mind either.

Johan

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Marek Vasut]

On 1/12/23 18:09, Johan Hovold wrote:
> On Thu, Jan 12, 2023 at 05:38:48PM +0100, Marek Vasut wrote:
>> On 1/12/23 14:13, Johan Hovold wrote:
>    
>>> The author did not seem to think this was stable material as there is no
>>> cc-stable tag and it also seems fairly intrusive.
>>
>> The author does not have enough experience with preempt-rt to make that
>> determination, hence deferred to Sebastian for better judgement.
> 
> Fair enough. And it's not obvious that the stable team should backport
> patches that only concern PREEMPT_RT either (e.g. as parts of it are
> still out-of-tree).
> 
> The stable tag is still missing from the final revision though.

Please pardon my ignorance, which stable tag is missing ?

Can you maybe just comment on the V4 and point this out to me ? I'll 
send a V5 then.

Thanks !

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

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Marek Vasut]

On 1/12/23 18:09, Johan Hovold wrote:
> On Thu, Jan 12, 2023 at 05:38:48PM +0100, Marek Vasut wrote:
>> On 1/12/23 14:13, Johan Hovold wrote:
>    
>>> The author did not seem to think this was stable material as there is no
>>> cc-stable tag and it also seems fairly intrusive.
>>
>> The author does not have enough experience with preempt-rt to make that
>> determination, hence deferred to Sebastian for better judgement.
> 
> Fair enough. And it's not obvious that the stable team should backport
> patches that only concern PREEMPT_RT either (e.g. as parts of it are
> still out-of-tree).
> 
> The stable tag is still missing from the final revision though.

Please pardon my ignorance, which stable tag is missing ?

Can you maybe just comment on the V4 and point this out to me ? I'll 
send a V5 then.

Thanks !

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Johan Hovold]

On Thu, Jan 12, 2023 at 06:50:34PM +0100, Marek Vasut wrote:
> On 1/12/23 18:09, Johan Hovold wrote:

> > Fair enough. And it's not obvious that the stable team should backport
> > patches that only concern PREEMPT_RT either (e.g. as parts of it are
> > still out-of-tree).
> > 
> > The stable tag is still missing from the final revision though.
> 
> Please pardon my ignorance, which stable tag is missing ?
> 
> Can you maybe just comment on the V4 and point this out to me ? I'll 
> send a V5 then.

It's gone from my inbox.

But as per Documentation/process/stable-kernel-rules.rst:

    To have the patch automatically included in the stable tree, add the tag
    
    .. code-block:: none
    
         Cc: stable@vger.kernel.org
    
    in the sign-off area. Once the patch is merged it will be applied to
    the stable tree without anything else needing to be done by the author
    or subsystem maintainer.

A Fixes tag only indicates which commit introduced a bug, not
necessarily that the patch should be backported to stable (even if
autosel is likely to pick it up these days).

Johan

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

Re: [PATCH v3] serial: stm32: Merge hard IRQ and threaded IRQ handling into single IRQ handler

[Johan Hovold]

On Thu, Jan 12, 2023 at 06:50:34PM +0100, Marek Vasut wrote:
> On 1/12/23 18:09, Johan Hovold wrote:

> > Fair enough. And it's not obvious that the stable team should backport
> > patches that only concern PREEMPT_RT either (e.g. as parts of it are
> > still out-of-tree).
> > 
> > The stable tag is still missing from the final revision though.
> 
> Please pardon my ignorance, which stable tag is missing ?
> 
> Can you maybe just comment on the V4 and point this out to me ? I'll 
> send a V5 then.

It's gone from my inbox.

But as per Documentation/process/stable-kernel-rules.rst:

    To have the patch automatically included in the stable tree, add the tag
    
    .. code-block:: none
    
         Cc: stable@vger.kernel.org
    
    in the sign-off area. Once the patch is merged it will be applied to
    the stable tree without anything else needing to be done by the author
    or subsystem maintainer.

A Fixes tag only indicates which commit introduced a bug, not
necessarily that the patch should be backported to stable (even if
autosel is likely to pick it up these days).

Johan