Re: [PATCH] serial: 8250_omap: Avoid FIFO corruption caused by MDR1 access

From: Vignesh Raghavendra <vigneshr@ti.com>
To: Alexander Sverdlin <alexander.sverdlin@gmail.com>,
	<linux-serial@vger.kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	Peter Ujfalusi <peter.ujfalusi@ti.com>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	<linux-kernel@vger.kernel.org>, <stable@vger.kernel.org>
Subject: Re: [PATCH] serial: 8250_omap: Avoid FIFO corruption caused by MDR1 access
Date: Fri, 11 Dec 2020 19:21:50 +0530	[thread overview]
Message-ID: <b6bc434b-d75d-1f04-9bb2-457333c46a92@ti.com> (raw)
In-Reply-To: <20201210055257.1053028-1-alexander.sverdlin@gmail.com>

Hi,

On 12/10/20 11:22 AM, Alexander Sverdlin wrote:
> It has been observed that once per 300-1300 port openings the first
> transmitted byte is being corrupted on AM3352 ("v" written to FIFO appeared
> as "e" on the wire). It only happened if single byte has been transmitted
> right after port open, which means, DMA is not used for this transfer and
> the corruption never happened afterwards.
> 
> Therefore I've carefully re-read the MDR1 errata (link below), which says
> "when accessing the MDR1 registers that causes a dummy under-run condition
> that will freeze the UART in IrDA transmission. In UART mode, this may
> corrupt the transferred data". Strictly speaking,
> omap_8250_mdr1_errataset() performs a read access and if the value is the
> same as should be written, exits without errata-recommended FIFO reset.
> 
> A brief check of the serial_omap_mdr1_errataset() from the competing
> omap-serial driver showed it has no read access of MDR1. After removing the
> read access from omap_8250_mdr1_errataset() the data corruption never
> happened any more.
> 
> Link: https://www.ti.com/lit/er/sprz360i/sprz360i.pdf
> Fixes: 61929cf0169d ("tty: serial: Add 8250-core based omap driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

Thanks for the fix.

Reviewed-by: Vignesh Raghavendra <vigneshr@ti.com>

> ---
>  drivers/tty/serial/8250/8250_omap.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index 562087df7d33..0cc6d35a0815 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -184,11 +184,6 @@ static void omap_8250_mdr1_errataset(struct uart_8250_port *up,
>  				     struct omap8250_priv *priv)
>  {
>  	u8 timeout = 255;
> -	u8 old_mdr1;
> -
> -	old_mdr1 = serial_in(up, UART_OMAP_MDR1);
> -	if (old_mdr1 == priv->mdr1)
> -		return;
>  
>  	serial_out(up, UART_OMAP_MDR1, priv->mdr1);
>  	udelay(2);
> 