From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>
Closes: https://lore.kernel.org/linux-gpio/[email protected]/
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
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
On 2/20/24 12:34, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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 <[email protected]>
> Closes: https://lore.kernel.org/linux-gpio/[email protected]/
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> 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 <[email protected]>
Thanks
Patrice