spi: spi-fsl-dspi: Protect against races on dspi->words_in_flight

dspi->words_in_flight is a variable populated in the *_write functions
and used in the dspi_fifo_read function. It is also used in
dspi_fifo_write, immediately after transmission, to update the
message->actual_length variable used by higher layers such as spi-mem
for integrity checking.

But it may happen that the IRQ which calls dspi_fifo_read to be
triggered before the updating of message->actual_length takes place. In
that case, dspi_fifo_read will decrement dspi->words_in_flight to -1,
and that will cause an invalid modification of message->actual_length.

For that, we make the simplest fix possible: to not decrement the actual
shared variable in dspi->words_in_flight from dspi_fifo_read, but
actually a copy of it which is on stack.

But even if dspi_fifo_read from the next IRQ does not interfere with the
dspi_fifo_write of the current chunk, the *next* dspi_fifo_write still
can. So we must assume that everything after the last write to the TX
FIFO can be preempted by the "TX complete" IRQ, and the dspi_fifo_write
function must be safe against that. This means refactoring the 2
flavours of FIFO writes (for EOQ and XSPI) such that the calculation of
the number of words to be written is common and happens a priori. This
way, the code for updating the message->actual_length variable works
with a copy and not with the volatile dspi->words_in_flight.

After some interior debate, the dspi->progress variable used for
software timestamping was *not* backed up against preemption in a copy
on stack. Because if preemption does occur between
spi_take_timestamp_pre and spi_take_timestamp_post, there's really no
point in trying to save anything. The first-in-time
spi_take_timestamp_post call with a dspi->progress higher than the
requested xfer->ptp_sts_word_post will trigger xfer->timestamped = true
anyway and will close the deal.

To understand the above a bit better, consider a transfer with
xfer->ptp_sts_word_pre = xfer->ptp_sts_word_post = 3, and
xfer->bits_per_words = 8 (so byte 3 needs to be timestamped). The DSPI
controller timestamps in chunks of 4 bytes at a time, and preemption
occurs in the middle of timestamping the first chunk:

  spi_take_timestamp_pre(0)
    .
    . (preemption)
    .
    . spi_take_timestamp_pre(4)
    .
    . spi_take_timestamp_post(7)
    .
  spi_take_timestamp_post(3)

So the reason I'm not bothering to back up dspi->progress for that
spi_take_timestamp_post(3) is that spi_take_timestamp_post(7) is going
to (a) be more honest, (b) provide better accuracy and (c) already
render the spi_take_timestamp_post(3) into a noop by setting
xfer->timestamped = true anyway.

Fixes: d59c90a2400f ("spi: spi-fsl-dspi: Convert TCFQ users to XSPI FIFO mode")
Reported-by: Michael Walle <michael@walle.cc>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Michael Walle <michael@walle.cc>
Link: https://lore.kernel.org/r/20200318001603.9650-6-olteanv@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 51224b7..f7e1e70 100644 (file)
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -669,17 +669,26 @@ static void dspi_pushr_txdata_write(struct fsl_dspi *dspi, u16 txdata)
        regmap_write(dspi->regmap_pushr, dspi->pushr_tx, txdata);
 }
 
-static void dspi_xspi_write(struct fsl_dspi *dspi, int cnt, bool eoq)
+static void dspi_xspi_fifo_write(struct fsl_dspi *dspi, int num_words)
 {
+       int num_bytes = num_words * dspi->oper_word_size;
        u16 tx_cmd = dspi->tx_cmd;
 
-       if (eoq)
+       /*
+        * If the PCS needs to de-assert (i.e. we're at the end of the buffer
+        * and cs_change does not want the PCS to stay on), then we need a new
+        * PUSHR command, since this one (for the body of the buffer)
+        * necessarily has the CONT bit set.
+        * So send one word less during this go, to force a split and a command
+        * with a single word next time, when CONT will be unset.
+        */
+       if (!(dspi->tx_cmd & SPI_PUSHR_CMD_CONT) && num_bytes == dspi->len)
                tx_cmd |= SPI_PUSHR_CMD_EOQ;
 
        /* Update CTARE */
        regmap_write(dspi->regmap, SPI_CTARE(0),
                     SPI_FRAME_EBITS(dspi->oper_bits_per_word) |
-                    SPI_CTARE_DTCP(cnt));
+                    SPI_CTARE_DTCP(num_words));
 
        /*
         * Write the CMD FIFO entry first, and then the two
@@ -688,7 +697,7 @@ static void dspi_xspi_write(struct fsl_dspi *dspi, int cnt, bool eoq)
        dspi_pushr_cmd_write(dspi, tx_cmd);
 
        /* Fill TX FIFO with as many transfers as possible */
-       while (cnt--) {
+       while (num_words--) {
                u32 data = dspi_pop_tx(dspi);
 
                dspi_pushr_txdata_write(dspi, data & 0xFFFF);
@@ -697,58 +706,15 @@ static void dspi_xspi_write(struct fsl_dspi *dspi, int cnt, bool eoq)
        }
 }
 
-static void dspi_xspi_fifo_write(struct fsl_dspi *dspi)
-{
-       int num_fifo_entries = dspi->devtype_data->fifo_size;
-       int bytes_in_flight;
-       bool eoq = false;
-
-       /* In XSPI mode each 32-bit word occupies 2 TX FIFO entries */
-       if (dspi->oper_word_size == 4)
-               num_fifo_entries /= 2;
-
-       /*
-        * Integer division intentionally trims off odd (or non-multiple of 4)
-        * numbers of bytes at the end of the buffer, which will be sent next
-        * time using a smaller oper_word_size.
-        */
-       dspi->words_in_flight = dspi->len / dspi->oper_word_size;
-
-       if (dspi->words_in_flight > num_fifo_entries)
-               dspi->words_in_flight = num_fifo_entries;
-
-       bytes_in_flight = dspi->words_in_flight * dspi->oper_word_size;
-
-       /*
-        * If the PCS needs to de-assert (i.e. we're at the end of the buffer
-        * and cs_change does not want the PCS to stay on), then we need a new
-        * PUSHR command, since this one (for the body of the buffer)
-        * necessarily has the CONT bit set.
-        * So send one word less during this go, to force a split and a command
-        * with a single word next time, when CONT will be unset.
-        */
-       if (!(dspi->tx_cmd & SPI_PUSHR_CMD_CONT) &&
-           bytes_in_flight == dspi->len)
-               eoq = true;
-
-       dspi_xspi_write(dspi, dspi->words_in_flight, eoq);
-}
-
-static void dspi_eoq_fifo_write(struct fsl_dspi *dspi)
+static void dspi_eoq_fifo_write(struct fsl_dspi *dspi, int num_words)
 {
-       int num_fifo_entries = dspi->devtype_data->fifo_size;
        u16 xfer_cmd = dspi->tx_cmd;
 
-       if (num_fifo_entries * dspi->oper_word_size > dspi->len)
-               num_fifo_entries = dspi->len / dspi->oper_word_size;
-
-       dspi->words_in_flight = num_fifo_entries;
-
        /* Fill TX FIFO with as many transfers as possible */
-       while (num_fifo_entries--) {
+       while (num_words--) {
                dspi->tx_cmd = xfer_cmd;
                /* Request EOQF for last transfer in FIFO */
-               if (num_fifo_entries == 0)
+               if (num_words == 0)
                        dspi->tx_cmd |= SPI_PUSHR_CMD_EOQ;
                /* Write combined TX FIFO and CMD FIFO entry */
                dspi_pushr_write(dspi);
@@ -765,8 +731,10 @@ static u32 dspi_popr_read(struct fsl_dspi *dspi)
 
 static void dspi_fifo_read(struct fsl_dspi *dspi)
 {
+       int num_fifo_entries = dspi->words_in_flight;
+
        /* Read one FIFO entry and push to rx buffer */
-       while (dspi->words_in_flight--)
+       while (num_fifo_entries--)
                dspi_push_rx(dspi, dspi_popr_read(dspi));
 }
 
@@ -832,23 +800,48 @@ no_accel:
 
 static void dspi_fifo_write(struct fsl_dspi *dspi)
 {
+       int num_fifo_entries = dspi->devtype_data->fifo_size;
        struct spi_transfer *xfer = dspi->cur_transfer;
        struct spi_message *msg = dspi->cur_msg;
-       int bytes_sent;
+       int num_words, num_bytes;
 
        dspi_setup_accel(dspi);
 
+       /* In XSPI mode each 32-bit word occupies 2 TX FIFO entries */
+       if (dspi->oper_word_size == 4)
+               num_fifo_entries /= 2;
+
+       /*
+        * Integer division intentionally trims off odd (or non-multiple of 4)
+        * numbers of bytes at the end of the buffer, which will be sent next
+        * time using a smaller oper_word_size.
+        */
+       num_words = dspi->len / dspi->oper_word_size;
+       if (num_words > num_fifo_entries)
+               num_words = num_fifo_entries;
+
+       /* Update total number of bytes that were transferred */
+       num_bytes = num_words * dspi->oper_word_size;
+       msg->actual_length += num_bytes;
+       dspi->progress += num_bytes / DIV_ROUND_UP(xfer->bits_per_word, 8);
+
+       /*
+        * Update shared variable for use in the next interrupt (both in
+        * dspi_fifo_read and in dspi_fifo_write).
+        */
+       dspi->words_in_flight = num_words;
+
        spi_take_timestamp_pre(dspi->ctlr, xfer, dspi->progress, !dspi->irq);
 
        if (dspi->devtype_data->trans_mode == DSPI_EOQ_MODE)
-               dspi_eoq_fifo_write(dspi);
+               dspi_eoq_fifo_write(dspi, num_words);
        else
-               dspi_xspi_fifo_write(dspi);
-
-       /* Update total number of bytes that were transferred */
-       bytes_sent = dspi->words_in_flight * dspi->oper_word_size;
-       msg->actual_length += bytes_sent;
-       dspi->progress += bytes_sent / DIV_ROUND_UP(xfer->bits_per_word, 8);
+               dspi_xspi_fifo_write(dspi, num_words);
+       /*
+        * Everything after this point is in a potential race with the next
+        * interrupt, so we must never use dspi->words_in_flight again since it
+        * might already be modified by the next dspi_fifo_write.
+        */
 
        spi_take_timestamp_post(dspi->ctlr, dspi->cur_transfer,
                                dspi->progress, !dspi->irq);