linux-kernel - Re: [PATCH] serial: 8250_omap: remove wait loop from Errata i202 workaround

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 17 Oct 2022 11:12:41 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Matthias Schiffer <matthias.schiffer@...tq-group.com>
cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Tony Lindgren <tony@...mide.com>,
        Peter Hurley <peter@...leysoftware.com>,
        linux-serial <linux-serial@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] serial: 8250_omap: remove wait loop from Errata i202
 workaround

On Thu, 13 Oct 2022, Matthias Schiffer wrote:

> We were occasionally seeing the "Errata i202: timedout" on an AM335x
> board when repeatedly opening and closing a UART connected to an active
> sender. As new input may arrive at any time, it is possible to miss the
> "RX FIFO empty" condition, forcing the loop to wait until it times out.

I can see this problem could occur and why your patch fixes it.

> Nothing in the i202 Advisory states that such a wait is even necessary;
> other FIFO clear functions like serial8250_clear_fifos() do not wait
> either. For this reason, it seems safe to remove the wait, fixing the
> mentioned issue.

Checking the commit that added this driver and the loop along with it, 
there was no information why it would be needed there either.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>

Thanks.

-- 
 i.


> Fixes: 61929cf0169d ("tty: serial: Add 8250-core based omap driver")
> Signed-off-by: Matthias Schiffer <matthias.schiffer@...tq-group.com>
> ---
>  drivers/tty/serial/8250/8250_omap.c | 17 -----------------
>  1 file changed, 17 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 41b8c6b27136..484f791617af 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -193,27 +193,10 @@ static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  static void omap_8250_mdr1_errataset(struct uart_8250_port *up,
>  				     struct omap8250_priv *priv)
>  {
> -	u8 timeout = 255;
> -
>  	serial_out(up, UART_OMAP_MDR1, priv->mdr1);
>  	udelay(2);
>  	serial_out(up, UART_FCR, up->fcr | UART_FCR_CLEAR_XMIT |
>  			UART_FCR_CLEAR_RCVR);
> -	/*
> -	 * Wait for FIFO to empty: when empty, RX_FIFO_E bit is 0 and
> -	 * TX_FIFO_E bit is 1.
> -	 */
> -	while (UART_LSR_THRE != (serial_in(up, UART_LSR) &
> -				(UART_LSR_THRE | UART_LSR_DR))) {
> -		timeout--;
> -		if (!timeout) {
> -			/* Should *never* happen. we warn and carry on */
> -			dev_crit(up->port.dev, "Errata i202: timedout %x\n",
> -				 serial_in(up, UART_LSR));
> -			break;
> -		}
> -		udelay(1);
> -	}
>  }
>  
>  static void omap_8250_get_divisor(struct uart_port *port, unsigned int baud,
>