[PATCH 1/2] spi: spi-geni-qcom: Fix geni_spi_isr() NULL dereference in timeout case

From: Douglas Anderson
Date: Mon Dec 14 2020 - 19:32:34 EST


In commit 7ba9bdcb91f6 ("spi: spi-geni-qcom: Don't keep a local state
variable") we changed handle_fifo_timeout() so that we set
"mas->cur_xfer" to NULL to make absolutely sure that we don't mess
with the buffers from the previous transfer in the timeout case.

Unfortunately, this caused the IRQ handler to dereference NULL in some
cases. One case:

CPU0 CPU1
---- ----
setup_fifo_xfer()
...
geni_se_setup_m_cmd()
<hardware starts transfer>
<unrelated interrupt storm> spin_unlock_irq()
<continued interrupt storm> <time passes>
<continued interrupt storm> <transfer complets in hardware>
<continued interrupt storm> <hardware sets M_RX_FIFO_WATERMARK_EN>
<continued interrupt storm> <time passes>
<continued interrupt storm> handle_fifo_timeout()
<continued interrupt storm> spin_lock_irq()
<continued interrupt storm> mas->cur_xfer = NULL
<continued interrupt storm> geni_se_cancel_m_cmd()
<continued interrupt storm> spin_unlock_irq()
<continued interrupt storm> wait_for_completion_timeout() => timeout
<continued interrupt storm> spin_lock_irq()
<continued interrupt storm> geni_se_abort_m_cmd()
<continued interrupt storm> spin_unlock_irq()
<continued interrupt storm> wait_for_completion_timeout() => timeout
<interrupt storm ends>
geni_spi_isr()
spin_lock()
if (m_irq & M_RX_FIFO_WATERMARK_EN)
geni_spi_handle_rx()
mas->cur_xfer NULL derefrence

Specifically it should be noted that the RX/TX interrupts are still
shown asserted even when a CANCEL/ABORT interrupt has asserted.

Let's check for the NULL transfer in the TX and RX cases.

NOTE: things still could get confused if we get timeouts all the way
through handle_fifo_timeout(), meaning that interrupts are still
pending. A future patch will help these corner cases.

Fixes: 561de45f72bd ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP")
Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
---

drivers/spi/spi-geni-qcom.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 25810a7eef10..6f736e94e9f4 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -354,6 +354,12 @@ static bool geni_spi_handle_tx(struct spi_geni_master *mas)
unsigned int bytes_per_fifo_word = geni_byte_per_fifo_word(mas);
unsigned int i = 0;

+ /* Stop the watermark IRQ if nothing to send */
+ if (mas->cur_xfer == NULL) {
+ writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
+ return false;
+ }
+
max_bytes = (mas->tx_fifo_depth - mas->tx_wm) * bytes_per_fifo_word;
if (mas->tx_rem_bytes < max_bytes)
max_bytes = mas->tx_rem_bytes;
@@ -396,6 +402,17 @@ static void geni_spi_handle_rx(struct spi_geni_master *mas)
if (rx_last_byte_valid && rx_last_byte_valid < 4)
rx_bytes -= bytes_per_fifo_word - rx_last_byte_valid;
}
+
+ /* Clear out the FIFO and bail if nowhere to put it */
+ if (mas->cur_xfer == NULL) {
+ unsigned int words = DIV_ROUND_UP(rx_bytes, bytes_per_fifo_word);
+
+ for (i = 0; i < words; i++)
+ readl(se->base + SE_GENI_RX_FIFOn);
+
+ return;
+ }
+
if (mas->rx_rem_bytes < rx_bytes)
rx_bytes = mas->rx_rem_bytes;

--
2.29.2.684.gfbc64c5ab5-goog