[v2,2/2] spi: spi-ti-qspi: Handle truncated frames properly

Commit Message

Ben Hutchings April 12, 2016, 11:58 a.m. UTC

We clamp frame_len_words to a maximum of 4096, but do not actually
limit the number of words written or read through the DATA registers
or the length added to spi_message::actual_length.  This results in
silent data corruption for commands longer than this maximum.

Recalculate the length of each transfer, taking frame_len_words into
account.  Use this length in qspi_{read,write}_msg(), and to increment
spi_message::actual_length.

Cc: [e-mail]
Signed-off-by: Ben Hutchings <[e-mail]>
---
v2: Rebase; rename transfer_length to tranfer_len_words; improve commit
    message

 drivers/spi/spi-ti-qspi.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

Comments

Mark Brown April 13, 2016, 7:13 a.m. UTC | #1

On Tue, Apr 12, 2016 at 12:58:14PM +0100, Ben Hutchings wrote:

> -		ret = qspi_transfer_msg(qspi, t);
> +		wlen = t->bits_per_word >> 3;

I did ask you to fix this to just / 8 so it's more readable :/

Ben Hutchings April 13, 2016, 9:31 a.m. UTC | #2

On Wed, 2016-04-13 at 08:13 +0100, Mark Brown wrote:
> On Tue, Apr 12, 2016 at 12:58:14PM +0100, Ben Hutchings wrote:
> 
> > -		ret = qspi_transfer_msg(qspi, t);
> > +		wlen = t->bits_per_word >> 3;
> 
> I did ask you to fix this to just / 8 so it's more readable :/

Not explicitly.  And that would be inconsistent with every other place
this driver bits_per_word to a byte count.

Ben.

Mark Brown April 13, 2016, 1:12 p.m. UTC | #3

On Wed, Apr 13, 2016 at 10:31:33AM +0100, Ben Hutchings wrote:
> On Wed, 2016-04-13 at 08:13 +0100, Mark Brown wrote:
> > On Tue, Apr 12, 2016 at 12:58:14PM +0100, Ben Hutchings wrote:

> > > -		ret = qspi_transfer_msg(qspi, t);
> > > +		wlen = t->bits_per_word >> 3;

> > I did ask you to fix this to just / 8 so it's more readable :/

> Not explicitly.  And that would be inconsistent with every other place
> this driver bits_per_word to a byte count.

If someone is reviewing code and showing a different way of doing things
it is reasonable to interpret that as a request to change things.  It's
best to engage with rather than completely ignore the feedback, what I'd
have said would have been that it is better to have clear code than be
consistent with unclear code.

Patch

diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
index 2137f2112804..5044c6198332 100644
--- a/drivers/spi/spi-ti-qspi.c
+++ b/drivers/spi/spi-ti-qspi.c
@@ -225,16 +225,16 @@  static inline int ti_qspi_poll_wc(struct ti_qspi *qspi)
 	return  -ETIMEDOUT;
 }
 
-static int qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t)
+static int qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t,
+			  int count)
 {
-	int wlen, count, xfer_len;
+	int wlen, xfer_len;
 	unsigned int cmd;
 	const u8 *txbuf;
 	u32 data;
 
 	txbuf = t->tx_buf;
 	cmd = qspi->cmd | QSPI_WR_SNGL;
-	count = t->len;
 	wlen = t->bits_per_word >> 3;	/* in bytes */
 	xfer_len = wlen;
 
@@ -294,9 +294,10 @@  static int qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t)
 	return 0;
 }
 
-static int qspi_read_msg(struct ti_qspi *qspi, struct spi_transfer *t)
+static int qspi_read_msg(struct ti_qspi *qspi, struct spi_transfer *t,
+			 int count)
 {
-	int wlen, count;
+	int wlen;
 	unsigned int cmd;
 	u8 *rxbuf;
 
@@ -313,7 +314,6 @@  static int qspi_read_msg(struct ti_qspi *qspi, struct spi_transfer *t)
 		cmd |= QSPI_RD_SNGL;
 		break;
 	}
-	count = t->len;
 	wlen = t->bits_per_word >> 3;	/* in bytes */
 
 	while (count) {
@@ -344,12 +344,13 @@  static int qspi_read_msg(struct ti_qspi *qspi, struct spi_transfer *t)
 	return 0;
 }
 
-static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t)
+static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t,
+			     int count)
 {
 	int ret;
 
 	if (t->tx_buf) {
-		ret = qspi_write_msg(qspi, t);
+		ret = qspi_write_msg(qspi, t, count);
 		if (ret) {
 			dev_dbg(qspi->dev, "Error while writing\n");
 			return ret;
@@ -357,7 +358,7 @@  static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t)
 	}
 
 	if (t->rx_buf) {
-		ret = qspi_read_msg(qspi, t);
+		ret = qspi_read_msg(qspi, t, count);
 		if (ret) {
 			dev_dbg(qspi->dev, "Error while reading\n");
 			return ret;
@@ -374,7 +375,8 @@  static int ti_qspi_start_transfer_one(struct spi_master *master,
 	struct spi_device *spi = m->spi;
 	struct spi_transfer *t;
 	int status = 0, ret;
-	unsigned int frame_len_words;
+	unsigned int frame_len_words, transfer_len_words;
+	int wlen;
 
 	/* setup device control reg */
 	qspi->dc = 0;
@@ -404,14 +406,20 @@  static int ti_qspi_start_transfer_one(struct spi_master *master,
 		qspi->cmd = ((qspi->cmd & ~QSPI_WLEN_MASK) |
 			     QSPI_WLEN(t->bits_per_word));
 
-		ret = qspi_transfer_msg(qspi, t);
+		wlen = t->bits_per_word >> 3;
+		transfer_len_words = min(t->len / wlen, frame_len_words);
+
+		ret = qspi_transfer_msg(qspi, t, transfer_len_words * wlen);
 		if (ret) {
 			dev_dbg(qspi->dev, "transfer message failed\n");
 			mutex_unlock(&qspi->list_lock);
 			return -EINVAL;
 		}
 
-		m->actual_length += t->len;
+		m->actual_length += transfer_len_words * wlen;
+		frame_len_words -= transfer_len_words;
+		if (frame_len_words == 0)
+			break;
 	}
 
 	mutex_unlock(&qspi->list_lock);