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
