drivers/uart: stm32: fix dead lock on poll_out

A dead lock could happen if 2 threads with differents priorities use
poll_out. In fact, the lock data->tx_lock could be lock by a thread with
lower priority and then a thread with higher priority can't take the
lock. There was a race condition here:

/* Wait for TXE flag to be raised */
while (1) {
	if (atomic_cas(&data->tx_lock, 0, 1)) {
		/* !!!!!!!! RACE CONDITION !!!!!!!!!!!!!!
		if (LL_USART_IsActiveFlag_TXE(UartInstance)) {
			break;
		}
		atomic_set(&data->tx_lock, 0);
	}
}

To fix race condition, the interrupts are locked in poll_out.

Signed-off-by: Julien D'ascenzio <julien.dascenzio@paratronic.fr>

drivers/serial/uart_stm32.c

@@ -525,21 +525,36 @@ static void uart_stm32_poll_out(const struct device *dev,
 					unsigned char c)
 {
 	USART_TypeDef *UartInstance = UART_STRUCT(dev);
+#ifdef CONFIG_PM
 	struct uart_stm32_data *data = DEV_DATA(dev);
+#endif
+	int key;
 
-	/* Wait for TXE flag to be raised */
+	/* Wait for TXE flag to be raised
+	 * When TXE flag is raised, we lock interrupts to prevent interrupts (notably that of usart)
+	 * or thread switch. Then, we can safely send our character. The character sent will be
+	 * interlaced with the characters potentially send with interrupt transmission API
+	 */
 	while (1) {
-		if (atomic_cas(&data->tx_lock, 0, 1)) {
+		if (LL_USART_IsActiveFlag_TXE(UartInstance)) {
+			key = irq_lock();
 			if (LL_USART_IsActiveFlag_TXE(UartInstance)) {
 				break;
 			}
-			atomic_set(&data->tx_lock, 0);
+			irq_unlock(key);
+		}
+		if (!k_is_in_isr()) {
+			/* yield execution to another thread of the same or higher priority. */
+			k_yield();
 		}
 	}
 
 #ifdef CONFIG_PM
 
-	if (!data->tx_poll_stream_on) {
+	/* If an interrupt transmission is in progress, the pm constraint is already managed by the
+	 * call of uart_stm32_irq_tx_[en|dis]able
+	 */
+	if (!data->tx_poll_stream_on && !data->tx_int_stream_on) {
 		data->tx_poll_stream_on = true;
 
 		/* Don't allow system to suspend until stream
@@ -555,7 +570,7 @@ static void uart_stm32_poll_out(const struct device *dev,
 #endif /* CONFIG_PM */
 
 	LL_USART_TransmitData8(UartInstance, (uint8_t)c);
-	atomic_set(&data->tx_lock, 0);
+	irq_unlock(key);
 }
 
 static int uart_stm32_err_check(const struct device *dev)
@@ -615,6 +630,14 @@ static int uart_stm32_fifo_fill(const struct device *dev,
 {
 	USART_TypeDef *UartInstance = UART_STRUCT(dev);
 	uint8_t num_tx = 0U;
+	int key;
+
+	if (!LL_USART_IsActiveFlag_TXE(UartInstance)) {
+		return num_tx;
+	}
+
+	/* Lock interrupts to prevent nested interrupts or thread switch */
+	key = irq_lock();
 
 	while ((size - num_tx > 0) &&
 	       LL_USART_IsActiveFlag_TXE(UartInstance)) {
@@ -624,6 +647,8 @@ static int uart_stm32_fifo_fill(const struct device *dev,
 		LL_USART_TransmitData8(UartInstance, tx_data[num_tx++]);
 	}
 
+	irq_unlock(key);
+
 	return num_tx;
 }
 
@@ -652,29 +677,43 @@ static int uart_stm32_fifo_read(const struct device *dev, uint8_t *rx_data,
 static void uart_stm32_irq_tx_enable(const struct device *dev)
 {
 	USART_TypeDef *UartInstance = UART_STRUCT(dev);
+#ifdef CONFIG_PM
 	struct uart_stm32_data *data = DEV_DATA(dev);
-
+	int key;
-	while (atomic_cas(&data->tx_lock, 0, 1) == false) {
+#endif
-	}
 
 #ifdef CONFIG_PM
+	key = irq_lock();
 	data->tx_poll_stream_on = false;
+	data->tx_int_stream_on = true;
 	uart_stm32_pm_constraint_set(dev);
 #endif
 	LL_USART_EnableIT_TC(UartInstance);
+
+#ifdef CONFIG_PM
+	irq_unlock(key);
+#endif
 }
 
 static void uart_stm32_irq_tx_disable(const struct device *dev)
 {
 	USART_TypeDef *UartInstance = UART_STRUCT(dev);
+#ifdef CONFIG_PM
 	struct uart_stm32_data *data = DEV_DATA(dev);
+	int key;
+
+	key = irq_lock();
+#endif
 
 	LL_USART_DisableIT_TC(UartInstance);
 
-	atomic_set(&data->tx_lock, 0);
+#ifdef CONFIG_PM
+	data->tx_int_stream_on = false;
+	uart_stm32_pm_constraint_release(dev);
+#endif
 
 #ifdef CONFIG_PM
-	uart_stm32_pm_constraint_release(dev);
+	irq_unlock(key);
 #endif
 }
 

drivers/serial/uart_stm32.h

@@ -57,7 +57,6 @@ struct uart_stm32_data {
 	uint32_t baud_rate;
 	/* clock device */
 	const struct device *clock;
-	atomic_t tx_lock;
 #ifdef CONFIG_UART_INTERRUPT_DRIVEN
 	uart_irq_callback_user_data_t user_cb;
 	void *user_data;
@@ -74,6 +73,7 @@ struct uart_stm32_data {
 #endif
 #ifdef CONFIG_PM
 	bool tx_poll_stream_on;
+	bool tx_int_stream_on;
 	bool pm_constraint_on;
 #endif
 };