On Thu, May 30, 2024 at 03:45:58PM -0700, Douglas Anderson wrote: > On devices using Qualcomm's GENI UART it is possible to get the UART > stuck such that it no longer outputs data. Specifically, I could > reproduce this problem by logging in via an agetty on the debug serial > port (which was _not_ used for kernel console) and running: > cat /var/log/messages > ...and then (via an SSH session) forcing a few suspend/resume cycles. > > Digging into this showed a number of problems that are all related. > > The root of the problems was with qcom_geni_serial_stop_tx_fifo() > which is called as part of the suspend process. Specific problems with > that function: > - When we cancel an in-progress "tx" command it doesn't appear to > fully drain the FIFO. That meant qcom_geni_serial_tx_empty() > continued to report that the FIFO wasn't empty. The > qcom_geni_serial_start_tx_fifo() function didn't re-enable > interrupts in this case so we'd never start transferring again. > - We cancelled the current "tx" command but we forgot to zero out > "tx_remaining". This confused logic elsewhere in the driver > - From experimentation, it appears that cancelling the "tx" command > could drop some of the queued up bytes. While maybe not the end of > the world, it doesn't seem like we should be dropping bytes when > stopping the FIFO, which is defined more of a "pause". > > One idea to fix the above would be to add FIFO draining to > qcom_geni_serial_stop_tx_fifo(). However, digging into the > documentation in serial_core.h for stop_tx() makes this seem like the > wrong choice. Specifically stop_tx() is called with local interrupts > disabled. Waiting for a FIFO (which might be 64 bytes big) to drain at > 115.2 kbps doesn't seem like a wise move. > > Ideally qcom_geni_serial_stop_tx_fifo() would be able to pause the > transmitter, but nothing in the documentation for the GENI UART makes > me believe that is possible. > > Given the lack of better choices, we'll change > qcom_geni_serial_stop_tx_fifo() to simply disable the > TX_FIFO_WATERMARK interrupt and call it a day. This seems OK as per > the serial core docs since stop_tx() is supposed to stop transferring > bytes "as soon as possible" and there doesn't seem to be any possible > way to stop transferring sooner. As part of this, get rid of some of > the extra conditions on qcom_geni_serial_start_tx_fifo() which simply > weren't needed and are now getting in the way. It's always fine to > turn the interrupts on if we want to receive and it'll be up to the > IRQ handler to turn them back off if somehow they're not needed. This > works fine. > > Unfortunately, doing just the above change causes new/different > problems with suspend/resume. Now if you suspend while an active > transfer is happening you can find that after resume time you're no > longer receiving UART interrupts at all. It appears to be important to > drain the FIFO and send a "cancel" command if the UART is active to > avoid this. Since we've already decided that > qcom_geni_serial_stop_tx_fifo() shouldn't be doing this, let's add the > draining / cancelling logic to the shutdown() call where it should be > OK to delay a bit. This is called as part of the suspend process via > uart_suspend_port(). > > Finally, with all of the above, the test case where we're spamming the > UART with data and going through suspend/resume cycles doesn't kill > the UART and doesn't drop bytes. > > NOTE: though I haven't gone back and validated on ancient code, it > appears from code inspection that many of these problems have existed > since the start of the driver. In the very least, I could reproduce > the problems on vanilla v5.15. The problems don't seem to reproduce > when using the serial port for kernel console output and also don't > seem to reproduce if nothing is being printed to the console at > suspend time, so this is presumably why they were not noticed until > now. ... > + qcom_geni_serial_poll_bitfield(uport, SE_GENI_M_GP_LENGTH, 0xffffffff, It's easy to miscount f:s, GENMASK()? > + port->tx_total - port->tx_remaining); -- With Best Regards, Andy Shevchenko
