Andy,
Unfortunately this will re-introduce the bug that it was attempting to
solve, that is ensuring that the buffer in the kernel and the buffer
on the chip are clear before going into shutdown on the chip.
Breaking at the beginning of the loop means that the kernel has
written everything to the internal buffer on the chip, but until the
LSR bits are clear the bytes have not been transmitted yet.
I'm not positive that the uart_circ_empty needs to be checked in the
first place; I had put it in because the serial8250_tx_chars does that
before stopping the tx, and I assume that there could be a potential
race condition where the kernel has not yet written all the data to
the exar, but the exar has finished transmitting all the data in its
transmit buffer(I am not sure how likely this is to happen).
-Robert Middleton
On Mon, Aug 5, 2019 at 6:05 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> First of all, boolean variable should be assigned with boolean values.
> Second, it's not needed at all in this case.
>
> Drop unneeded boolean variable and use 'break' statement instead.
>
> While here, change iterations to be more visible by moving the number of them
> to the variable definition block.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
> v2: Check kernel buffer first as in the original conditional (Robert)
> drivers/tty/serial/8250/8250_exar.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 873aa6b0c2f3..8f9baae92831 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -169,19 +169,18 @@ static void xr17v35x_set_divisor(struct uart_port *p, unsigned int baud,
> static void exar_shutdown(struct uart_port *port)
> {
> unsigned char lsr;
> - bool tx_complete = 0;
> struct uart_8250_port *up = up_to_u8250p(port);
> struct circ_buf *xmit = &port->state->xmit;
> - int i = 0;
> + unsigned int retries = 1000;
>
> do {
> + if (uart_circ_empty(xmit))
> + break;
> lsr = serial_in(up, UART_LSR);
> if (lsr & (UART_LSR_TEMT | UART_LSR_THRE))
> - tx_complete = 1;
> - else
> - tx_complete = 0;
> + break;
> msleep(1);
> - } while (!uart_circ_empty(xmit) && !tx_complete && i++ < 1000);
> + } while (--retries);
>
> serial8250_do_shutdown(port);
> }
> --
> 2.20.1
>
