Re: [PATCH v5 2/8] serial: max310x: assert the transceiver during a break

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] serial: max310x: leaked RTS assertion if RS485 disabled mid-break
--

commit 7c2477330220cb7f9d2f88a0c967029ae3d062d8
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>
serial: max310x: assert the transceiver during a break

This commit manually asserts the RTS bit during a break on RS485 ports,
as the auto-RTS mode does not assert it for non-FIFO data. It disables 
auto-RTS for the break duration, tracking the break status in a variable 
so the transceiver is restored when the break ends.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index 4c1e10e0765f4..693decd04de10 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
[ ... ]
> @@ -929,9 +936,27 @@ static void max310x_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  
>  static void max310x_break_ctl(struct uart_port *port, int break_state)
>  {
> +	struct max310x_one *one = to_max310x_port(port);
> +
> +	one->tx_break = break_state;
> +
>  	max310x_port_update(port, MAX310X_LCR_REG,
>  			    MAX310X_LCR_TXBREAK_BIT,
>  			    break_state ? MAX310X_LCR_TXBREAK_BIT : 0);
> +
> +	if (!(port->rs485.flags & SER_RS485_ENABLED))
> +		return;

[Severity: High]
If a userspace application starts a break (TIOCSBRK), and then disables
RS-485 (TIOCSRS485) before ending the break (TIOCCBRK), won't this early
return bypass the restoration of the RTS bit? Could this leak the manual RTS
assertion and leave the transceiver permanently driving the bus, causing
collisions?

> +
> +	/*
> +	 * The chip's auto-RTS asserts the transceiver only while FIFO data is
> +	 * shifting out, and a break is not FIFO data. Disable auto-RTS for the
> +	 * break duration and drive RTS manually so the break reaches the wire;
> +	 * restore auto-RTS when the break ends.
> +	 */
> +	max310x_port_update(port, MAX310X_MODE1_REG,
> +			    MAX310X_MODE1_TRNSCVCTRL_BIT,
> +			    break_state ? 0 : MAX310X_MODE1_TRNSCVCTRL_BIT);
> +	max310x_rts_ctl(port, break_state);
>  }
[ ... ]
> @@ -1055,6 +1080,13 @@ static void max310x_rs_proc(struct work_struct *ws)
>  	struct max310x_one *one = container_of(ws, struct max310x_one, rs_work);
>  	unsigned int delay, mode1 = 0, mode2 = 0;
>  
[ ... ]
> -	max310x_port_update(&one->port, MAX310X_MODE1_REG,
> -			MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
> +	/*
> +	 * A break owns the transceiver: break_ctl() disabled auto-RTS and
> +	 * drives RTS manually, and restores it when the break ends. Leave
> +	 * MODE1 alone meanwhile or the break goes undriven mid-way.
> +	 */
> +	if (!one->tx_break)
> +		max310x_port_update(&one->port, MAX310X_MODE1_REG,
> +				    MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);

[Severity: High]
If the RS-485 state is changed mid-break, skipping this MODE1 update here 
means the transceiver might be left in the wrong state once the break
finishes. Should this new configuration be deferred or tracked so it can
be correctly applied when the break ends?

>  	max310x_port_update(&one->port, MAX310X_MODE2_REG,
>  			MAX310X_MODE2_ECHOSUPR_BIT, mode2);
>  }
> @@ -1090,6 +1128,7 @@ static int max310x_rs485_config(struct uart_port *port, struct ktermios *termios
>  
>  static int max310x_startup(struct uart_port *port)
>  {
> +	struct max310x_one *one = to_max310x_port(port);
>  	unsigned int val;
>  
>  	max310x_power(port, 1);
> @@ -1099,6 +1138,7 @@ static int max310x_startup(struct uart_port *port)
>  	 * closed with a break still asserted, and set_termios() no longer
>  	 * rewrites it.
>  	 */
> +	one->tx_break = false;
>  	max310x_port_update(port, MAX310X_LCR_REG, MAX310X_LCR_TXBREAK_BIT, 0);

[Severity: High]
While max310x_startup() correctly clears the latched TXBREAK bit here, does
it also need to clear the LCR_RTS_BIT? If a port is closed with an active
break, wouldn't the manual RTS assertion from max310x_break_ctl() remain
leaked on the next open?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929-max310x-rs485-sw-delay-v5-0-ae46afa583f2@vaisala.com?part=2