linux-kernel - RE: [PATCH] serial: imx: also enable Transmit Complete interrupt in rs232 mode

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]

Message-ID: <AS8PR04MB8404D066C247F5B2979CBD1F92BBA@AS8PR04MB8404.eurprd04.prod.outlook.com>
Date:   Tue, 21 Nov 2023 06:37:27 +0000
From:   Sherry Sun <sherry.sun@....com>
To:     Rasmus Villemoes <rasmus.villemoes@...vas.dk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        dl-linux-imx <linux-imx@....com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH] serial: imx: also enable Transmit Complete interrupt in
 rs232 mode



> -----Original Message-----
> From: Rasmus Villemoes <rasmus.villemoes@...vas.dk>
> Sent: 2023年11月20日 21:23
> To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>; Jiri Slaby
> <jirislaby@...nel.org>; Shawn Guo <shawnguo@...nel.org>; Sascha Hauer
> <s.hauer@...gutronix.de>; Pengutronix Kernel Team
> <kernel@...gutronix.de>; Fabio Estevam <festevam@...il.com>; dl-linux-
> imx <linux-imx@....com>
> Cc: Rasmus Villemoes <rasmus.villemoes@...vas.dk>; linux-
> kernel@...r.kernel.org; linux-serial@...r.kernel.org; linux-arm-
> kernel@...ts.infradead.org
> Subject: [PATCH] serial: imx: also enable Transmit Complete interrupt in
> rs232 mode
> 
> Currently, if one switches to rs232 mode, writes something to the device, and
> then switches to rs485 mode, the imx_port's ->tx_state is left as SEND. This
> then prevents a subsequent write in rs485 mode from properly asserting the
> rts pin (i.e. enabling the transceiver), because imx_uart_start_rx() does not
> enter the "if (sport->tx_state == OFF)" branch. Hence nothing is actually
> transmitted.
> 
> The problem is that in rs232 mode, ->tx_state never gets set to OFF, due to
> 
> 	usr2 = imx_uart_readl(sport, USR2);
> 	if (!(usr2 & USR2_TXDC)) {
> 		/* The shifter is still busy, so retry once TC triggers */
> 		return;
> 	}
> 
> in imx_uart_stop_tx(), and TC never triggers because the Transmit Complete
> interrupt is not enabled for rs232.

Hi Rasmus,
I am afraid this is not right, USR2_TXDC is just a flag, it is not affected by whether UCR4_TCEN is set or not, UCR4_TCEN only determines if USR2_TXDC flag can generate a interrupt or not.
I tried on imx8mp-evk board, for rs232, sport->tx_state can get set to OFF even we don’t set UCR4_TCEN.

Best Regards
Sherry

> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@...vas.dk>
> ---
> I'm not sure this is the best fix.
> 
> At first I considered doing something much more targeted, but definitely also
> more hacky: In imx_uart_rs485_config(), if switching on rs485 mode, simply
> add "sport->tx_state = OFF;".
> 
> If someone has a better suggestion, I'm all ears.
> 
>  drivers/tty/serial/imx.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c index
> 7341d060f85c..ffee157e13cd 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -708,25 +708,19 @@ static void imx_uart_start_tx(struct uart_port
> *port)
>  		    || sport->tx_state == WAIT_AFTER_RTS) {
> 
>  			hrtimer_try_to_cancel(&sport->trigger_stop_tx);
> -
> -			/*
> -			 * Enable transmitter and shifter empty irq only if
> DMA
> -			 * is off.  In the DMA case this is done in the
> -			 * tx-callback.
> -			 */
> -			if (!sport->dma_is_enabled) {
> -				u32 ucr4 = imx_uart_readl(sport, UCR4);
> -				ucr4 |= UCR4_TCEN;
> -				imx_uart_writel(sport, ucr4, UCR4);
> -			}
> -
> -			sport->tx_state = SEND;
>  		}
> -	} else {
> -		sport->tx_state = SEND;
>  	}
> 
> +	sport->tx_state = SEND;
> +
> +	/*
> +	 * Enable transmitter and shifter empty irq only if DMA is
> +	 * off.  In the DMA case this is done in the tx-callback.
> +	 */
>  	if (!sport->dma_is_enabled) {
> +		u32 ucr4 = imx_uart_readl(sport, UCR4);
> +		ucr4 |= UCR4_TCEN;
> +		imx_uart_writel(sport, ucr4, UCR4);
>  		ucr1 = imx_uart_readl(sport, UCR1);
>  		imx_uart_writel(sport, ucr1 | UCR1_TRDYEN, UCR1);
>  	}
> --
> 2.40.1.1.g1c60b9335d