linux-kernel - Re: [PATCH v3 1/6] serial: Do not hold the port lock when setting rx-during-tx GPIO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 11 Oct 2023 18:36:56 -0400
From:   Hugo Villeneuve <hugo@...ovil.com>
To:     Lino Sanfilippo <l.sanfilippo@...bus.com>
Cc:     gregkh@...uxfoundation.org, jirislaby@...nel.org,
        ilpo.jarvinen@...ux.intel.com, shawnguo@...nel.org,
        s.hauer@...gutronix.de, mcoquelin.stm32@...il.com,
        alexandre.torgue@...s.st.com, cniedermaier@...electronics.com,
        linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
        LinoSanfilippo@....de, lukas@...ner.de, p.rosenberger@...bus.com,
        stable@...r.kernel.org
Subject: Re: [PATCH v3 1/6] serial: Do not hold the port lock when setting
 rx-during-tx GPIO

On Wed, 11 Oct 2023 20:15:39 +0200
Lino Sanfilippo <l.sanfilippo@...bus.com> 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@...r.kernel.org
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@...bus.com>
> ---
>  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
>