Re: [PATCH v5 4/8] serial: max310x: wait for TX to drain before powering down in shutdown

[Hugo Villeneuve]

Hi Tapio,

On Tue, 29 Sep 2026 09:37:58 +0000
Tapio Reijonen <tapio.reijonen@xxxxxxxxxxx> wrote:

> max310x_tx_empty() reports the chip TX FIFO level and nothing else, so
> both tcdrain() and the tty layer's wait-until-sent on close() return
> while the final character is still clocking out of the transmit shift
> register. max310x_shutdown() then powers the port down mid-character
> and the last byte is truncated on the wire. At 9600 baud the ~1 ms
> window is easy to miss; at 1200 baud a write()-then-close() reliably
> corrupts the final byte (observed on the wire: 0x24 transmitted as a
> 0x04 frame with a framing error).
> 
> Wait in shutdown() for the FIFO to drain, bounded by one character
> duration per FIFO word, plus one more character for the byte in the
> shift register, before powering the port down. The per-character
> duration is computed in set_termios() from the frame size and baud
> rate.
> 
> Fixes: f65444187a66 ("serial: New serial driver MAX310X")
> Signed-off-by: Tapio Reijonen <tapio.reijonen@xxxxxxxxxxx>
> ---
>  drivers/tty/serial/max310x.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index f8dad37d017afe5c0b1d36d6239ba05fc5e7aff0..cd3b1913aaadba94805f4728b5eba18e970e34c8 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
> @@ -302,6 +302,7 @@ struct max310x_one {
>  	struct work_struct	md_work;
>  	struct work_struct	rs_work;
>  	struct regmap		*regmap;
> +	unsigned int		one_char_duration_us;

char_time_us?


>  	unsigned int		baud;
>  	bool			tx_break;	/* break_ctl() owns the transceiver */
>  
> @@ -1020,6 +1021,7 @@ static void max310x_set_termios(struct uart_port *port,
>  				struct ktermios *termios,
>  				const struct ktermios *old)
>  {
> +	unsigned int frame_bits = tty_get_frame_size(termios->c_cflag);
>  	unsigned int lcr = 0, flow = 0;
>  	int baud;
>  
> @@ -1132,10 +1134,13 @@ static void max310x_set_termios(struct uart_port *port,
>  	uart_update_timeout(port, termios->c_cflag, baud);
>  
>  	/*
> -	 * Cache the new baud rate and reprogram the RS485 RTS delays, whose
> -	 * millisecond-to-bit-time conversion depends on it.
> +	 * Cache the new baud rate and the time it takes to clock out one
> +	 * character, then reprogram the RS485 RTS delays, whose
> +	 * millisecond-to-bit-time conversion depends on the baud rate.
>  	 */
>  	to_max310x_port(port)->baud = baud;
> +	to_max310x_port(port)->one_char_duration_us =
> +		DIV_ROUND_UP(USEC_PER_SEC * frame_bits, baud);

Would it be a good idea to if you moved these two lines after
max310x_set_rts_ctl_params(), then you could probably leave the
original comments and simply add a new comment to indicate "Compute
time it takes to clock out one character", simplifying the diff
(review) and readability?

>  	max310x_set_rts_ctl_params(to_max310x_port(port));
>  }
>  
> @@ -1231,6 +1236,20 @@ static int max310x_startup(struct uart_port *port)
>  
>  static void max310x_shutdown(struct uart_port *port)
>  {
> +	struct max310x_one *one = to_max310x_port(port);
> +	unsigned int loops = port->fifosize + 1;

tries?

> +
> +	/*
> +	 * The tty layer waits for tx_empty() before close(), but tx_empty()
> +	 * only reflects the chip TX FIFO - the last character may still be in
> +	 * the transmit shift register. Let the FIFO drain and the final
> +	 * character clock out before the port is powered down, otherwise
> +	 * close() truncates the last byte on the wire.
> +	 */

Based on these comments, does it mean that the FIFO has already been
validated empty at this point by the tty layer, so you don't need the
loop at all, just the unconditional last fsleep()?

> +	while (!max310x_tx_empty(port) && loops-- > 0)
> +		fsleep(one->one_char_duration_us);

For certain combinations of large fifo_sizes and high-baud rates, that
could mean a lot of I2C/SPI transactions?


> +	fsleep(one->one_char_duration_us);
> +
>  	/* Disable all interrupts */
>  	max310x_port_write(port, MAX310X_IRQEN_REG, 0);
>  
> 
> -- 
> 2.47.3
> 
> 
> 


-- 
Hugo Villeneuve