Re: [PATCH v3] serial: omap: fix rs485 half-duplex filtering

[Greg Kroah-Hartman]

On Thu, Apr 15, 2021 at 11:02:52PM +0200, Dario Binacchi wrote:
> Data received during half-duplex transmission must be filtered.
> If the target device responds quickly, emptying the FIFO at the end of
> the transmission can erase not only the echo characters but also part of
> the response message.
> By keeping the receive interrupt enabled even during transmission, it
> allows you to filter each echo character and only in a number equal to
> those transmitted.
> The issue was generated by a target device that started responding
> 240us later having received a request in communication at 115200bps.
> Sometimes, some messages received by the target were missing some of the
> first bytes.
> 
> Fixes: 3a13884abea0 ("tty/serial: omap: empty the RX FIFO at the end of half-duplex TX")
> Signed-off-by: Dario Binacchi <dariobin@xxxxxxxxx>
> 
> 
> ---
> 
> Changes in v3:
> - Add 'Fixes' tag
> 
> Changes in v2:
> - Fix compiling error
> 
>  drivers/tty/serial/omap-serial.c | 39 ++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 76b94d0ff586..c0df22b7ea5e 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -159,6 +159,8 @@ struct uart_omap_port {
>  	u32			calc_latency;
>  	struct work_struct	qos_work;
>  	bool			is_suspending;
> +
> +	atomic_t		rs485_tx_filter_count;

Why are you using an atomic variable?  What do you think this is
"protected from"?

>  };
>  
>  #define to_uart_omap_port(p) ((container_of((p), struct uart_omap_port, port)))
> @@ -328,19 +330,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
>  		serial_out(up, UART_IER, up->ier);
>  	}
>  
> -	if ((port->rs485.flags & SER_RS485_ENABLED) &&
> -	    !(port->rs485.flags & SER_RS485_RX_DURING_TX)) {
> -		/*
> -		 * Empty the RX FIFO, we are not interested in anything
> -		 * received during the half-duplex transmission.
> -		 */
> -		serial_out(up, UART_FCR, up->fcr | UART_FCR_CLEAR_RCVR);
> -		/* Re-enable RX interrupts */
> -		up->ier |= UART_IER_RLSI | UART_IER_RDI;
> -		up->port.read_status_mask |= UART_LSR_DR;
> -		serial_out(up, UART_IER, up->ier);
> -	}
> -
>  	pm_runtime_mark_last_busy(up->dev);
>  	pm_runtime_put_autosuspend(up->dev);
>  }
> @@ -366,6 +355,10 @@ static void transmit_chars(struct uart_omap_port *up, unsigned int lsr)
>  		serial_out(up, UART_TX, up->port.x_char);
>  		up->port.icount.tx++;
>  		up->port.x_char = 0;
> +		if ((up->port.rs485.flags & SER_RS485_ENABLED) &&
> +		    !(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> +			atomic_inc(&up->rs485_tx_filter_count);
> +
>  		return;
>  	}
>  	if (uart_circ_empty(xmit) || uart_tx_stopped(&up->port)) {
> @@ -377,6 +370,10 @@ static void transmit_chars(struct uart_omap_port *up, unsigned int lsr)
>  		serial_out(up, UART_TX, xmit->buf[xmit->tail]);
>  		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
>  		up->port.icount.tx++;
> +		if ((up->port.rs485.flags & SER_RS485_ENABLED) &&
> +		    !(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> +			atomic_inc(&up->rs485_tx_filter_count);
> +
>  		if (uart_circ_empty(xmit))
>  			break;
>  	} while (--count > 0);
> @@ -420,7 +417,7 @@ static void serial_omap_start_tx(struct uart_port *port)
>  
>  	if ((port->rs485.flags & SER_RS485_ENABLED) &&
>  	    !(port->rs485.flags & SER_RS485_RX_DURING_TX))
> -		serial_omap_stop_rx(port);
> +		atomic_set(&up->rs485_tx_filter_count, 0);
>  
>  	serial_omap_enable_ier_thri(up);
>  	pm_runtime_mark_last_busy(up->dev);
> @@ -491,8 +488,13 @@ static void serial_omap_rlsi(struct uart_omap_port *up, unsigned int lsr)
>  	 * Read one data character out to avoid stalling the receiver according
>  	 * to the table 23-246 of the omap4 TRM.
>  	 */
> -	if (likely(lsr & UART_LSR_DR))
> +	if (likely(lsr & UART_LSR_DR)) {
>  		serial_in(up, UART_RX);
> +		if ((up->port.rs485.flags & SER_RS485_ENABLED) &&
> +		    !(up->port.rs485.flags & SER_RS485_RX_DURING_TX) &&
> +		    atomic_read(&up->rs485_tx_filter_count))
> +			atomic_dec(&up->rs485_tx_filter_count);

You can not read and then decrement right afterward and expect this to
actually do what you think it is doing.

Just use a real lock if you need to protect access for this value, as it
is, this patch is totally wrong.

thanks,

greg k-h