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

[Dario Binacchi]

> Il 16/04/2021 08:46 Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> 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@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"?

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