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

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

[Dario Binacchi]

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@libero.it>


---

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;
 };
 
 #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);
+	}
 
 	up->port.icount.rx++;
 	flag = TTY_NORMAL;
@@ -543,6 +545,13 @@ static void serial_omap_rdi(struct uart_omap_port *up, unsigned int lsr)
 		return;
 
 	ch = 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);
+		return;
+	}
+
 	flag = TTY_NORMAL;
 	up->port.icount.rx++;
 
-- 
2.17.1

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@libero.it>
> 
> 
> ---
> 
> 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

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

[Dario Binacchi]

> Il 16/04/2021 08:46 Greg Kroah-Hartman <gregkh@linuxfoundation.org> ha scritto:
> 
>  
> 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@libero.it>
> > 
> > 
> > ---
> > 
> > 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"?

You are right. They are already protected. All the functions affected by the patch 
are already protected by a lock, even going up in the serial_core for the 
serial_omap_start_tx().

Thanks and regards,
Dario

> 
> >  };
> >  
> >  #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