> 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
