On Wed, 11 Oct 2023 20:15:39 +0200
Lino Sanfilippo <l.sanfilippo@xxxxxxxxxx> wrote:
> Both the imx and stm32 driver set the rx-during-tx GPIO in the
> rs485_config() function by means of gpiod_set_value(). Since rs485_config()
> is called with the port lock held, this can be an problem in case that
> setting the GPIO line can sleep (e.g. if a GPIO expander is used which is
> connected via SPI or I2C).
>
> Avoid this issue by setting the GPIO outside of the port lock in the serial
> core and by using gpiod_set_value_cansleep() instead of gpiod_set_value().
Hi Lino,
it seems to me that both drivers were already using
gpiod_set_value_cansleep()? Maybe update your commit
message if this is the case.
>
> Since now both the term and the rx-during-tx GPIO are set within the serial
> core use a common function uart_set_rs485_gpios() to set both.
>
> With moving it into the serial core setting the rx-during-tx GPIO is now
> automatically done for all drivers that support such a GPIO.
>
> Fixes: c54d48543689 ("serial: stm32: Add support for rs485 RX_DURING_TX output GPIO")
> Fixes: ca530cfa968c ("serial: imx: Add support for RS485 RX_DURING_TX output GPIO")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@xxxxxxxxxx>
> ---
> drivers/tty/serial/imx.c | 4 ----
> drivers/tty/serial/serial_core.c | 10 ++++++----
> drivers/tty/serial/stm32-usart.c | 5 +----
> 3 files changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 13cb78340709..edb2ec6a5567 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1947,10 +1947,6 @@ static int imx_uart_rs485_config(struct uart_port *port, struct ktermios *termio
> rs485conf->flags & SER_RS485_RX_DURING_TX)
> imx_uart_start_rx(port);
>
> - if (port->rs485_rx_during_tx_gpio)
> - gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio,
> - !!(rs485conf->flags & SER_RS485_RX_DURING_TX));
> -
> return 0;
> }
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 7bdc21d5e13b..ef0500be3553 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1391,14 +1391,16 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
> memset(rs485->padding1, 0, sizeof(rs485->padding1));
> }
>
> -static void uart_set_rs485_termination(struct uart_port *port,
> - const struct serial_rs485 *rs485)
> +static void uart_set_rs485_gpios(struct uart_port *port,
> + const struct serial_rs485 *rs485)
> {
> if (!(rs485->flags & SER_RS485_ENABLED))
> return;
>
> gpiod_set_value_cansleep(port->rs485_term_gpio,
> !!(rs485->flags & SER_RS485_TERMINATE_BUS));
> + gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio,
> + !!(rs485->flags & SER_RS485_RX_DURING_TX));
> }
>
> static int uart_rs485_config(struct uart_port *port)
> @@ -1407,7 +1409,7 @@ static int uart_rs485_config(struct uart_port *port)
> int ret;
>
> uart_sanitize_serial_rs485(port, rs485);
> - uart_set_rs485_termination(port, rs485);
> + uart_set_rs485_gpios(port, rs485);
Suggestion: define a new function to handle rx_during_tx, to keep
uart_set_rs485_termination(), which is more self-documenting than
uart_set_rs485_gpios().
ex:
uart_set_rs485_termination(port, rs485);
+ uart_set_rs485_rx_during_tx(port, rs485);
Hugo.
>
> ret = port->rs485_config(port, NULL, rs485);
> if (ret)
> @@ -1449,7 +1451,7 @@ static int uart_set_rs485_config(struct tty_struct *tty, struct uart_port *port,
> if (ret)
> return ret;
> uart_sanitize_serial_rs485(port, &rs485);
> - uart_set_rs485_termination(port, &rs485);
> + uart_set_rs485_gpios(port, &rs485);
>
> spin_lock_irqsave(&port->lock, flags);
> ret = port->rs485_config(port, &tty->termios, &rs485);
> diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c
> index 5e9cf0c48813..8eb13bf055f2 100644
> --- a/drivers/tty/serial/stm32-usart.c
> +++ b/drivers/tty/serial/stm32-usart.c
> @@ -226,10 +226,7 @@ static int stm32_usart_config_rs485(struct uart_port *port, struct ktermios *ter
>
> stm32_usart_clr_bits(port, ofs->cr1, BIT(cfg->uart_enable_bit));
>
> - if (port->rs485_rx_during_tx_gpio)
> - gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio,
> - !!(rs485conf->flags & SER_RS485_RX_DURING_TX));
> - else
> + if (!port->rs485_rx_during_tx_gpio)
> rs485conf->flags |= SER_RS485_RX_DURING_TX;
>
> if (rs485conf->flags & SER_RS485_ENABLED) {
> --
> 2.40.1
>
