summaryrefslogtreecommitdiffstats
path: root/drivers/spi/spi-fsl-dspi.c
diff options
context:
space:
mode:
authorVladimir Oltean <vladimir.oltean@nxp.com>2020-03-18 01:15:56 +0100
committerMark Brown <broonie@kernel.org>2020-03-18 23:44:57 +0100
commit0dedf901078074d6c5b41c297fac12c02c6dc41f (patch)
tree42cc105d125066c2db5d7c58b95b509e31530bee /drivers/spi/spi-fsl-dspi.c
parentspi: spi-fsl-dspi: Avoid reading more data than written in EOQ mode (diff)
downloadlinux-0dedf901078074d6c5b41c297fac12c02c6dc41f.tar.xz
linux-0dedf901078074d6c5b41c297fac12c02c6dc41f.zip
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>
Diffstat (limited to 'drivers/spi/spi-fsl-dspi.c')
-rw-r--r--drivers/spi/spi-fsl-dspi.c111
1 files changed, 52 insertions, 59 deletions
diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 51224b772680..f7e1e7085e31 100644
--- 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);