lkml.org 
[lkml]   [2021]   [Jan]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 5.4 40/62] spi: spi-geni-qcom: Fix geni_spi_isr() NULL dereference in timeout case
    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.I99ee04f0cb823415df59bd4f550d6ff5756e43d6@changeid
    > 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.

    Cheers,
    Nathan

    \
     
     \ /
      Last update: 2021-01-16 19:51    [W:3.709 / U:0.712 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site