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

From: Stephen Boyd
Date: Wed Dec 16 2020 - 22:45:18 EST


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@xxxxxxxxxxxx>
> ---

Minor nits below.

Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>

> 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;
> + }