| Message ID | 20201216144114.v2.1.I99ee04f0cb823415df59bd4f550d6ff5756e43d6@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/4] spi: spi-geni-qcom: Fix geni_spi_isr() NULL dereference in timeout case | expand |
Quoting Douglas Anderson (2020-12-16 14:41:49) > 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> > <transfer completes in hardware> > <hardware sets M_RX_FIFO_WATERMARK_EN in m_irq> > ... > handle_fifo_timeout() > spin_lock_irq(mas->lock) > mas->cur_xfer = NULL > geni_se_cancel_m_cmd() > spin_unlock_irq(mas->lock) > > geni_spi_isr() > spin_lock(mas->lock) > if (m_irq & M_RX_FIFO_WATERMARK_EN) > geni_spi_handle_rx() > mas->cur_xfer NULL dereference! > > tl;dr: Seriously delayed interrupts for RX/TX can lead to timeout > handling setting mas->cur_xfer to NULL. > > Let's check for the NULL transfer in the TX and RX cases and reset the > watermark or clear out the fifo respectively to put the hardware back > into a sane state. > > NOTE: things still could get confused if we get timeouts all the way > through handle_fifo_timeout() and then start a new transfer because > interrupts from the old transfer / cancel / abort could still be > pending. A future patch will help this corner case. > > Fixes: 561de45f72bd ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP") > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- Minor nits below. Reviewed-by: Stephen Boyd <swboyd@chromium.org> > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c > index 25810a7eef10..bf55abbd39f1 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) { > + 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,14 @@ 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) { This is still == NULL though. :( > + while (i++ < DIV_ROUND_UP(rx_bytes, bytes_per_fifo_word)) A for loop is also fine, but I think it would push the 80 character limit which is probably OK. I'm happy either way, but a for loop is usually easier to reason about. I suggested while to fit within 80 characters :/ for (i = 0; i < DIV_ROUND_UP(rx_bytes, bytes_per_fifo_word); i++) > + readl(se->base + SE_GENI_RX_FIFOn); > + return; > + }
diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c index 25810a7eef10..bf55abbd39f1 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) { + 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,14 @@ 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) { + while (i++ < DIV_ROUND_UP(rx_bytes, bytes_per_fifo_word)) + readl(se->base + SE_GENI_RX_FIFOn); + return; + } + if (mas->rx_rem_bytes < rx_bytes) rx_bytes = mas->rx_rem_bytes;
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> <transfer completes in hardware> <hardware sets M_RX_FIFO_WATERMARK_EN in m_irq> ... handle_fifo_timeout() spin_lock_irq(mas->lock) mas->cur_xfer = NULL geni_se_cancel_m_cmd() spin_unlock_irq(mas->lock) geni_spi_isr() spin_lock(mas->lock) if (m_irq & M_RX_FIFO_WATERMARK_EN) geni_spi_handle_rx() mas->cur_xfer NULL dereference! tl;dr: Seriously delayed interrupts for RX/TX can lead to timeout handling setting mas->cur_xfer to NULL. Let's check for the NULL transfer in the TX and RX cases and reset the watermark or clear out the fifo respectively to put the hardware back into a sane state. NOTE: things still could get confused if we get timeouts all the way through handle_fifo_timeout() and then start a new transfer because interrupts from the old transfer / cancel / abort could still be pending. A future patch will help this corner case. Fixes: 561de45f72bd ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP") Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Changes in v2: - (ptr == NULL) => (!ptr). - Addressed loop nits in geni_spi_handle_rx(). - Commit message rewording from Stephen. drivers/spi/spi-geni-qcom.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)