On Sat, Jan 16, 2021 at 11:48:51AM -0700, Nathan Chancellor wrote:
On Fri, Jan 15, 2021 at 01:28:02PM +0100, Greg Kroah-Hartman wrote:
From: Douglas Anderson dianders@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 dianders@chromium.org Reviewed-by: Stephen Boyd swboyd@chromium.org Link: https://lore.kernel.org/r/20201217142842.v3.1.I99ee04f0cb823415df59bd4f550d6... Signed-off-by: Mark Brown broonie@kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@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.
Argh, I thought I had dropped this before, but no. Good catch, I've dropped it now.
And yes, that might be a good patch to backport (the gcc one), I'll queue that up next round, thanks.
greg k-h