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@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> > Link: https://lore.kernel.org/r/20201217142842.v3.1.I99ee04f0cb823415df59bd4f550d6ff5756e43d6@changeid
> > Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >
> > ---
> > 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
