linux-kernel - Re: [PATCH] serial: qcom-geni: Show '@' characters if we have a FIFO underrun

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: <2024071001-justice-catfight-37f0@gregkh>
Date: Wed, 10 Jul 2024 07:35:24 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Douglas Anderson <dianders@...omium.org>
Cc: Johan Hovold <johan@...nel.org>, Jiri Slaby <jirislaby@...nel.org>,
	Johan Hovold <johan+linaro@...nel.org>,
	Vijaya Krishna Nivarthi <quic_vnivarth@...cinc.com>,
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-serial@...r.kernel.org
Subject: Re: [PATCH] serial: qcom-geni: Show '@' characters if we have a FIFO
 underrun

On Tue, Jul 09, 2024 at 04:28:45PM -0700, Douglas Anderson wrote:
> As of commit 2ac33975abda ("serial: qcom-geni: do not kill the machine
> on fifo underrun") a FIFO underrun will no longer hard lockup the
> machine. Instead, a FIFO underrun will cause the UART to output a
> bunch of '\0' characters. The '\0' characters don't seem to show up on
> most terminal programs and this hides the fact that we had an
> underrun. An underrun is aq sign of problems in the driver and
> should be obvious / debugged.
> 
> Change the driver to put '@' characters in the case of an underrun
> which should make it much more obvious.
> 
> Adding this extra initialization doesn't add any real overhead. In
> fact, this patch reduces code size because the code was calling
> memset() to init 4 bytes of data. Disassembling the new code shows
> that early in the function w22 is setup to hold the '@@@@' constant:
>   mov     w22, #0x40404040
> 
> Each time through the loop w22 is simply stored:
>   str     w22, [sp, #4]
> 
> Cc: Johan Hovold <johan@...nel.org>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
> 
>  drivers/tty/serial/qcom_geni_serial.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 69a632fefc41..332eaa2faa2b 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -872,10 +872,10 @@ static void qcom_geni_serial_send_chunk_fifo(struct uart_port *uport,
>  {
>  	struct qcom_geni_serial_port *port = to_dev_port(uport);
>  	unsigned int tx_bytes, remaining = chunk;
> -	u8 buf[BYTES_PER_FIFO_WORD];
>  
>  	while (remaining) {
> -		memset(buf, 0, sizeof(buf));
> +		u8 buf[BYTES_PER_FIFO_WORD] = { '@', '@', '@', '@' };

Why is '@' a valid character for an underrun?  Why would any characters
be ok?  Where is this now documented?

And shouldn't you use a memset to get the BYTES_PER_FIFO_WORD amount of
'@' here?

thanks,

greg k-h