[PATCH v3] serial: st-asc: don't get/put GPIOs in atomic context

[PATCH v3] serial: st-asc: don't get/put GPIOs in atomic context

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Since commit 1f2bcb8c8ccd ("gpio: protect the descriptor label with
SRCU") gpiod_set_consumer_name() calls synchronize_srcu() which led to
a "sleeping in atomic context" smatch warning.

This function (along with gpiod_get/put() and all other GPIO APIs apart
from gpiod_get/set_value() and gpiod_direction_input/output()) should
have never been called with a spinlock taken. We're only fixing this now
as GPIOLIB has been rebuilt to use SRCU for access serialization which
uncovered this problem.

Move the calls to gpiod_get/put() outside the spinlock critical section.

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/linux-gpio/deee1438-efc1-47c4-8d80-0ab2cf01d60a@moroto.mountain/
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
v2 -> v3:
- we only need to change the GPIO configuration for RTS in certain situations
  so use a separate variable for storing that information; if we don't then we
  may end up putting the descriptor when setting a different option
- I dropped Linus tag as the code change significantly

v1 -> v2:
- initialize the 'manual_rts' variable to false as we don't always get to
  the place where it's set

 drivers/tty/serial/st-asc.c | 40 ++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
index bbb5595d7e24..a23e59551848 100644
--- a/drivers/tty/serial/st-asc.c
+++ b/drivers/tty/serial/st-asc.c
@@ -465,6 +465,7 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios,
 			    const struct ktermios *old)
 {
 	struct asc_port *ascport = to_asc_port(port);
+	bool manual_rts, toggle_rts = false;
 	struct gpio_desc *gpiod;
 	unsigned int baud;
 	u32 ctrl_val;
@@ -518,25 +519,13 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios,
 
 		/* If flow-control selected, stop handling RTS manually */
 		if (ascport->rts) {
-			devm_gpiod_put(port->dev, ascport->rts);
-			ascport->rts = NULL;
-
-			pinctrl_select_state(ascport->pinctrl,
-					     ascport->states[DEFAULT]);
+			toggle_rts = true;
+			manual_rts = false;
 		}
 	} else {
 		/* If flow-control disabled, it's safe to handle RTS manually */
-		if (!ascport->rts && ascport->states[NO_HW_FLOWCTRL]) {
-			pinctrl_select_state(ascport->pinctrl,
-					     ascport->states[NO_HW_FLOWCTRL]);
-
-			gpiod = devm_gpiod_get(port->dev, "rts", GPIOD_OUT_LOW);
-			if (!IS_ERR(gpiod)) {
-				gpiod_set_consumer_name(gpiod,
-						port->dev->of_node->name);
-				ascport->rts = gpiod;
-			}
-		}
+		if (!ascport->rts && ascport->states[NO_HW_FLOWCTRL])
+			manual_rts = toggle_rts = true;
 	}
 
 	if ((baud < 19200) && !ascport->force_m1) {
@@ -595,6 +584,25 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios,
 	asc_out(port, ASC_CTL, (ctrl_val | ASC_CTL_RUN));
 
 	uart_port_unlock_irqrestore(port, flags);
+
+	if (toggle_rts) {
+		if (manual_rts) {
+			pinctrl_select_state(ascport->pinctrl,
+					     ascport->states[NO_HW_FLOWCTRL]);
+
+			gpiod = devm_gpiod_get(port->dev, "rts", GPIOD_OUT_LOW);
+			if (!IS_ERR(gpiod)) {
+				gpiod_set_consumer_name(gpiod,
+							port->dev->of_node->name);
+				ascport->rts = gpiod;
+			}
+		} else {
+				devm_gpiod_put(port->dev, ascport->rts);
+				ascport->rts = NULL;
+				pinctrl_select_state(ascport->pinctrl,
+						     ascport->states[DEFAULT]);
+		}
+	}
 }
 
 static const char *asc_type(struct uart_port *port)
-- 
2.40.1

Re: [PATCH v3] serial: st-asc: don't get/put GPIOs in atomic context

[Patrice CHOTARD]

On 2/20/24 12:34, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Since commit 1f2bcb8c8ccd ("gpio: protect the descriptor label with
> SRCU") gpiod_set_consumer_name() calls synchronize_srcu() which led to
> a "sleeping in atomic context" smatch warning.
> 
> This function (along with gpiod_get/put() and all other GPIO APIs apart
> from gpiod_get/set_value() and gpiod_direction_input/output()) should
> have never been called with a spinlock taken. We're only fixing this now
> as GPIOLIB has been rebuilt to use SRCU for access serialization which
> uncovered this problem.
> 
> Move the calls to gpiod_get/put() outside the spinlock critical section.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/linux-gpio/deee1438-efc1-47c4-8d80-0ab2cf01d60a@moroto.mountain/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> v2 -> v3:
> - we only need to change the GPIO configuration for RTS in certain situations
>   so use a separate variable for storing that information; if we don't then we
>   may end up putting the descriptor when setting a different option
> - I dropped Linus tag as the code change significantly
> 
> v1 -> v2:
> - initialize the 'manual_rts' variable to false as we don't always get to
>   the place where it's set
> 
>  drivers/tty/serial/st-asc.c | 40 ++++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> index bbb5595d7e24..a23e59551848 100644
> --- a/drivers/tty/serial/st-asc.c
> +++ b/drivers/tty/serial/st-asc.c
> @@ -465,6 +465,7 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios,
>  			    const struct ktermios *old)
>  {
>  	struct asc_port *ascport = to_asc_port(port);
> +	bool manual_rts, toggle_rts = false;
>  	struct gpio_desc *gpiod;
>  	unsigned int baud;
>  	u32 ctrl_val;
> @@ -518,25 +519,13 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios,
>  
>  		/* If flow-control selected, stop handling RTS manually */
>  		if (ascport->rts) {
> -			devm_gpiod_put(port->dev, ascport->rts);
> -			ascport->rts = NULL;
> -
> -			pinctrl_select_state(ascport->pinctrl,
> -					     ascport->states[DEFAULT]);
> +			toggle_rts = true;
> +			manual_rts = false;
>  		}
>  	} else {
>  		/* If flow-control disabled, it's safe to handle RTS manually */
> -		if (!ascport->rts && ascport->states[NO_HW_FLOWCTRL]) {
> -			pinctrl_select_state(ascport->pinctrl,
> -					     ascport->states[NO_HW_FLOWCTRL]);
> -
> -			gpiod = devm_gpiod_get(port->dev, "rts", GPIOD_OUT_LOW);
> -			if (!IS_ERR(gpiod)) {
> -				gpiod_set_consumer_name(gpiod,
> -						port->dev->of_node->name);
> -				ascport->rts = gpiod;
> -			}
> -		}
> +		if (!ascport->rts && ascport->states[NO_HW_FLOWCTRL])
> +			manual_rts = toggle_rts = true;
>  	}
>  
>  	if ((baud < 19200) && !ascport->force_m1) {
> @@ -595,6 +584,25 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios,
>  	asc_out(port, ASC_CTL, (ctrl_val | ASC_CTL_RUN));
>  
>  	uart_port_unlock_irqrestore(port, flags);
> +
> +	if (toggle_rts) {
> +		if (manual_rts) {
> +			pinctrl_select_state(ascport->pinctrl,
> +					     ascport->states[NO_HW_FLOWCTRL]);
> +
> +			gpiod = devm_gpiod_get(port->dev, "rts", GPIOD_OUT_LOW);
> +			if (!IS_ERR(gpiod)) {
> +				gpiod_set_consumer_name(gpiod,
> +							port->dev->of_node->name);
> +				ascport->rts = gpiod;
> +			}
> +		} else {
> +				devm_gpiod_put(port->dev, ascport->rts);
> +				ascport->rts = NULL;
> +				pinctrl_select_state(ascport->pinctrl,
> +						     ascport->states[DEFAULT]);
> +		}
> +	}
>  }
>  
>  static const char *asc_type(struct uart_port *port)

Reviewed-by: Patrice Chotard <patrice.chotard@foss.st.com>

Thanks
Patrice