linux-kernel - RE: [PATCH v2 tty] tty: serial: fsl_lpuart: LS1021A has a FIFO size of 16 words, like LS1028A

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] [day] [month] [year] [list]

Message-ID: <AM8PR04MB7315545206E9F66BACBAE391FF1A0@AM8PR04MB7315.eurprd04.prod.outlook.com>
Date:   Fri, 23 Oct 2020 01:48:47 +0000
From:   Andy Duan <fugang.duan@....com>
To:     Vladimir Oltean <vladimir.oltean@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Michael Walle <michael@...le.cc>, Peng Fan <peng.fan@....com>
Subject: RE: [PATCH v2 tty] tty: serial: fsl_lpuart: LS1021A has a FIFO size
 of 16 words, like LS1028A

From: Vladimir Oltean <vladimir.oltean@....com> Sent: Friday, October 23, 2020 9:34 AM
> Prior to the commit that this one fixes, the FIFO size was derived from the
> read-only register LPUARTx_FIFO[TXFIFOSIZE] using the following
> formula:
> 
> TX FIFO size = 2 ^ (LPUARTx_FIFO[TXFIFOSIZE] - 1)
> 
> The documentation for LS1021A is a mess. Under chapter 26.1.3 LS1021A
> LPUART module special consideration, it mentions TXFIFO_SZ and RXFIFO_SZ
> being equal to 4, and in the register description for LPUARTx_FIFO, it shows the
> out-of-reset value of TXFIFOSIZE and RXFIFOSIZE fields as "011", even though
> these registers read as "101" in reality.
> 
> And when LPUART on LS1021A was working, the "101" value did correspond to
> "16 datawords", by applying the formula above, even though the
> documentation is wrong again (!!!!) and says that "101" means 64 datawords
> (hint: it doesn't).
> 
> So the "new" formula created by commit f77ebb241ce0 has all the premises of
> being wrong for LS1021A, because it relied only on false data and no actual
> experimentation.
> 
> Interestingly, in commit c2f448cff22a ("tty: serial: fsl_lpuart: add LS1028A
> support"), Michael Walle applied a workaround to this by manually setting the
> FIFO widths for LS1028A. It looks like the same values are used by LS1021A as
> well, in fact.
> 
> When the driver thinks that it has a deeper FIFO than it really has, getty (user
> space) output gets truncated.
> 
> Many thanks to Michael for pointing out where to look.
> 
> Fixes: f77ebb241ce0 ("tty: serial: fsl_lpuart: correct the FIFO depth size")
> Suggested-by: Michael Walle <michael@...le.cc>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> Changes in v2:
> Reworded commit message.

For the v2 with commit message change: 
Reviewed-by：Fugang Duan <fugang.duan@....com>
> 
>  drivers/tty/serial/fsl_lpuart.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c index
> ff4b88c637d0..bd047e1f9bea 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -314,9 +314,10 @@ MODULE_DEVICE_TABLE(of, lpuart_dt_ids);
>  /* Forward declare this for the dma callbacks*/  static void
> lpuart_dma_tx_complete(void *arg);
> 
> -static inline bool is_ls1028a_lpuart(struct lpuart_port *sport)
> +static inline bool is_layerscape_lpuart(struct lpuart_port *sport)
>  {
> -	return sport->devtype == LS1028A_LPUART;
> +	return (sport->devtype == LS1021A_LPUART ||
> +		sport->devtype == LS1028A_LPUART);
>  }
> 
>  static inline bool is_imx8qxp_lpuart(struct lpuart_port *sport) @@ -1701,11
> +1702,11 @@ static int lpuart32_startup(struct uart_port *port)
>  					    UARTFIFO_FIFOSIZE_MASK);
> 
>  	/*
> -	 * The LS1028A has a fixed length of 16 words. Although it supports the
> -	 * RX/TXSIZE fields their encoding is different. Eg the reference manual
> -	 * states 0b101 is 16 words.
> +	 * The LS1021A and LS1028A have a fixed FIFO depth of 16 words.
> +	 * Although they support the RX/TXSIZE fields, their encoding is
> +	 * different. Eg the reference manual states 0b101 is 16 words.
>  	 */
> -	if (is_ls1028a_lpuart(sport)) {
> +	if (is_layerscape_lpuart(sport)) {
>  		sport->rxfifo_size = 16;
>  		sport->txfifo_size = 16;
>  		sport->port.fifosize = sport->txfifo_size;
> --
> 2.25.1