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
| ||
|
Date: Mon, 10 Aug 2020 18:01:54 +0530 From: Akash Asthana <akashast@...eaurora.org> To: Douglas Anderson <dianders@...omium.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org> Cc: kgdb-bugreport@...ts.sourceforge.net, Mukesh Savaliya <msavaliy@...eaurora.org>, Andy Gross <agross@...nel.org>, Bjorn Andersson <bjorn.andersson@...aro.org>, Evan Green <evgreen@...omium.org>, Jiri Slaby <jirislaby@...nel.org>, linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org Subject: Re: [PATCH] serial: qcom_geni_serial: Fix recent kdb hang Hi Doug, On 8/7/2020 10:49 AM, Douglas Anderson wrote: > The commit e42d6c3ec0c7 ("serial: qcom_geni_serial: Make kgdb work > even if UART isn't console") worked pretty well and I've been doing a > lot of debugging with it. However, recently I typed "dmesg" in kdb > and then held the space key down to scroll through the pagination. My > device hung. This was repeatable and I found that it was introduced > with the aforementioned commit. > > It turns out that there are some strange boundary cases in geni where > in some weird situations it will signal RX_LAST but then will put 0 in > RX_LAST_BYTE. This means that the entire last FIFO entry is valid. IMO that means we received a word in RX_FIFO and it is the last word hence RX_LAST bit is set. RX_LAST_BYTE is 0 means none of the bytes are valid in the last word. In such scenario we should just read RX_FIFO buffer (to empty it), discard the word and return NO_POLL_CHAR. Something like below. --------------------------------------------------------------------------------------------------------------------------------------------------------- else private_data->poll_cached_bytes_cnt = 4; private_data->poll_cached_bytes = readl(uport->membase + SE_GENI_RX_FIFOn); } + if (!private_data->poll_cached_bytes_cnt) + return NO_POLL_CHAR; private_data->poll_cached_bytes_cnt--; ret = private_data->poll_cached_bytes & 0xff; ------------------------------------------------------------------------------------------------------------------------------------------------------------- Please let me know whether above code helps. I am not sure about what all scenario can leads to this behavior from hardware, I will try to get an answer from hardware team. Any error bit was set for SE_GENI_S_IRQ_STATUS & SE_GENI_M_IRQ_STATUS registers? I guess the hang was seen because *poll_cached_bytes_cnt* is unsigned int and it's value was 0, when it's decremented by 1 it's value become '4294967295' (very large) and dummy RX (0x00) would happen that many times before reading any actual RX transfers/bytes. Regards, Akash > This weird corner case is handled in qcom_geni_serial_handle_rx() > where you can see that we only honor RX_LAST_BYTE if RX_LAST is set > _and_ RX_LAST_BYTE is non-zero. If either of these is not true we use > BYTES_PER_FIFO_WORD (4) for the size of the last FIFO word. > > Let's fix kgdb. While at it, also use the proper #define for 4. > > Fixes: e42d6c3ec0c7 ("serial: qcom_geni_serial: Make kgdb work even if UART isn't console") > Signed-off-by: Douglas Anderson <dianders@...omium.org> > --- > > drivers/tty/serial/qcom_geni_serial.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index 07b7b6b05b8b..e27077656939 100644 > --- a/drivers/tty/serial/qcom_geni_serial.c > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -361,11 +361,16 @@ static int qcom_geni_serial_get_char(struct uart_port *uport) > return NO_POLL_CHAR; > > if (word_cnt == 1 && (status & RX_LAST)) > + /* > + * NOTE: If RX_LAST_BYTE_VALID is 0 it needs to be > + * treated as if it was BYTES_PER_FIFO_WORD. > + */ > private_data->poll_cached_bytes_cnt = > (status & RX_LAST_BYTE_VALID_MSK) >> > RX_LAST_BYTE_VALID_SHFT; > - else > - private_data->poll_cached_bytes_cnt = 4; > + > + if (private_data->poll_cached_bytes_cnt == 0) > + private_data->poll_cached_bytes_cnt = BYTES_PER_FIFO_WORD; > > private_data->poll_cached_bytes = > readl(uport->membase + SE_GENI_RX_FIFOn); -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project
Powered by blists - more mailing lists