Re: [PATCH next v2 1/4] serial: 8250: Split out IER from rs485_start_tx()

[Petr Mladek <pmladek@suse.com>]

On Fri 2024-09-13 16:11:35, John Ogness wrote:
> Move IER handling out of rs485_start_tx() callback and into a new
> wrapper serial8250_rs485_start_tx(). Replace all callback call sites
> with wrapper, except for the console write() callback, where it is
> inappropriate to modify IER.

Sigh, I am trying to review this patch but I am not familiar with the
code. Feel free to ignore me when the questions are completely off.

> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1370,7 +1370,6 @@ static void serial8250_stop_rx(struct uart_port *port)
>  	serial8250_rpm_get(up);
>  
>  	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
> -	up->port.read_status_mask &= ~UART_LSR_DR;
>  	serial_port_out(port, UART_IER, up->ier);
>  
>  	serial8250_rpm_put(up);
> @@ -1543,16 +1542,20 @@ static inline void __start_tx(struct uart_port *port)
>   *
>   * Generic callback usable by 8250 uart drivers to start rs485 transmission.
>   * Assumes that setting the RTS bit in the MCR register means RTS is high.
> - * (Some chips use inverse semantics.)  Further assumes that reception is
> - * stoppable by disabling the UART_IER_RDI interrupt. (Some chips set the
> - * UART_LSR_DR bit even when UART_IER_RDI is disabled, foiling this approach.)
> + * (Some chips use inverse semantics.)
> + * It does not disable RX interrupts. Use the wrapper function
> + * serial8250_rs485_start_tx() if that is also needed.
>   */
>  void serial8250_em485_start_tx(struct uart_8250_port *up)
>  {
>  	unsigned char mcr = serial8250_in_MCR(up);
>  
> +	/*
> +	 * Some chips set the UART_LSR_DR bit even when UART_IER_RDI is
> +	 * disabled, so explicitly mask it.
> +	 */
>  	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> -		serial8250_stop_rx(&up->port);
> +		up->port.read_status_mask &= ~UART_LSR_DR;

This change is related to disabling UART_IER_RDI but we do not longer
disable it in this code path.

Why do we need to do it here, please?
Why is it needed only in the em485-specific path, please?


I tried to understand the code and am in doubts:

On one hand, the comment talks about UART_LSR_DR and UART_IER_RDI
so seems to be relater.

But the "Some chips set..." comment has been added by the commit
058bc104f7ca5c83d81 ("serial: 8250: Generalize rs485 software emulation").
And I do not see any explanation why it was added in this code path
even though UART_LSR_DR and UART_IER_RDI were manipulated in
serial8250_stop_rx() which can be called also in other code
paths via uport->ops->stop_rx().

Also the comment suggests that this fixes a bug in some chips but
the line has been added into 1.1.60 back in 2007.

--- a/drivers/char/ChangeLog
+++ b/drivers/char/ChangeLog
@@ -1,3 +1,28 @@
+Sat Oct 29 18:17:34 1994  Theodore Y. Ts'o  (tytso@rt-11)
+
+	* serial.c (rs_ioctl, get_lsr_info): Added patch suggested by Arne
+		Riiber so that user mode programs can tell when the
+		transmitter shift register is empty.
+
+Thu Oct 27 23:14:29 1994  Theodore Y. Ts'o  (tytso@rt-11)
+
+	* tty_ioctl.c (wait_until_sent): Added debugging printk statements
+		(under the #ifdef TTY_DEBUG_WAIT_UNTL_SENT)  
+
+	* serial.c (rs_interrupt, rs_interrupt_single, receive_chars,
+		change_speed, rs_close): rs_close now disables receiver
+		interrupts when closing the serial port.  This allows the
+		serial port to close quickly when Linux and a modem (or a
+		mouse) are engaged in an echo war; when closing the serial
+		port, we now first stop listening to incoming characters,
+		and *then* wait for the transmit buffer to drain.  
+
+		In order to make this change, the info->read_status_mask
+		is now used to control what bits of the line status
+		register are looked at in the interrupt routine in all
+		cases; previously it was only used in receive_chars to
+		select a few of the status bits.
+
--- a/drivers/char/serial.c
+++ b/drivers/char/serial.c
[...]
@@ -1780,6 +1830,15 @@ static void rs_close(struct tty_struct *tty, struct file * filp)
 		info->normal_termios = *tty->termios;
 	if (info->flags & ASYNC_CALLOUT_ACTIVE)
 		info->callout_termios = *tty->termios;
+	/*
+	 * At this point we stop accepting input.  To do this, we
+	 * disable the receive line status interrupts, and tell the
+	 * interrut driver to stop checking the data ready bit in the
+	 * line status register.
+	 */
+	info->IER &= ~UART_IER_RLSI;
+	serial_out(info, UART_IER, info->IER);
+	info->read_status_mask &= ~UART_LSR_DR;
 	if (info->flags & ASYNC_INITIALIZED) {
 		wait_until_sent(tty, 3000); /* 30 seconds timeout */
 		/*

    => It looks like it was not a fix for a "buggy chips". It looks
       like it was part of the design.

>  
>  	if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>  		mcr |= UART_MCR_RTS;
> @@ -1562,6 +1565,18 @@ void serial8250_em485_start_tx(struct uart_8250_port *up)
>  }
>  EXPORT_SYMBOL_GPL(serial8250_em485_start_tx);
>  
> +/**
> + * serial8250_rs485_start_tx() - stop rs485 reception, enable transmission
> + * @up: uart 8250 port
> + */
> +void serial8250_rs485_start_tx(struct uart_8250_port *up)
> +{
> +	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> +		serial8250_stop_rx(&up->port);
> +
> +	up->rs485_start_tx(up);
> +}
> +
>  /* Returns false, if start_tx_timer was setup to defer TX start */
>  static bool start_tx_rs485(struct uart_port *port)
>  {
> @@ -1585,7 +1600,7 @@ static bool start_tx_rs485(struct uart_port *port)
>  	if (em485->tx_stopped) {
>  		em485->tx_stopped = false;
>  
> -		up->rs485_start_tx(up);
> +		serial8250_rs485_start_tx(up);

If I get this correctly then this keeps the existing behavior when

    up->rs485_start_tx == serial8250_em485_start_tx

Is this always the case, please?

The callback has been added by the commit 058bc104f7ca5c83d81
("serial: 8250: Generalize rs485 software emulation") because
8250_bcm2835aux.c driver needed to do something else.

Can start_tx_rs485() be called for the 8250_bcm2835aux.c driver?
Will it still work as expected?

>  
>  		if (up->port.rs485.delay_rts_before_send > 0) {
>  			em485->active_timer = &em485->start_tx_timer;

Best Regards,
Petr