On Fri, Jan 15, 2021 at 01:28:02PM +0100, Greg Kroah-Hartman wrote:
> From: Douglas Anderson <diand...@chromium.org>
>
> commit 4aa1464acbe3697710279a4bd65cb4801ed30425 upstream.
>
> 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 <diand...@chromium.org>
> Reviewed-by: Stephen Boyd <swb...@chromium.org>
> Link:
> https://lore.kernel.org/r/20201217142842.v3.1.I99ee04f0cb823415df59bd4f550d6ff5756e43d6@changeid
> Signed-off-by: Mark Brown <broo...@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
>
> ---
> drivers/spi/spi-geni-qcom.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -415,6 +415,12 @@ static void geni_spi_handle_tx(struct sp
> 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;
> @@ -454,6 +460,14 @@ static void geni_spi_handle_rx(struct sp
> 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) {
> + for (i = 0; i < DIV_ROUND_UP(rx_bytes, bytes_per_fifo_word);
> i++)
> + readl(se->base + SE_GENI_RX_FIFOn);
> + return;
> + }
> +
> if (mas->rx_rem_bytes < rx_bytes)
> rx_bytes = mas->rx_rem_bytes;
>
>
>
This commit breaks the build with clang:
drivers/spi/spi-geni-qcom.c:421:3: error: void function
'geni_spi_handle_tx' should not return a value [-Wreturn-type]
return false;
^ ~~~~~
1 error generated.
It looks like commit 6d66507d9b55 ("spi: spi-geni-qcom: Don't wait to
start 1st transfer if transmitting") would resolve this.
It might be worth picking up commit 172aad81a882 ("kbuild: enforce
-Werror=return-type") so that GCC behaves like clang does.
Cheers,
Nathan
