[PATCH] atmel_spi: fix dma addr calculation for len > BUFFER_SIZE

[PATCH] atmel_spi: fix dma addr calculation for len > BUFFER_SIZE

[Ben Nizette]

If len > BUFFER_LEN and !xfer->rx_buf we end up calculating the tx
buffer address as

*tx_dma = xfer->tx_dma + xfer->len - BUFFER_SIZE;

which is constant; i.e. we just send the last BUFFER_SIZE data over
again until we've reached the right number of bytes.

This patch gets around this by using the /requested/ length when
calculating addresses.

Note there's no way len != *plen when we calculate the rx buffer address
but conceptually we should be using *plen and I don't want someone to
come through later, see the calculations for rx and tx are different and
"clean up" back to what we had.

Signed-off-by: Ben Nizette <bn@niasdigital.com>

---
 drivers/spi/atmel_spi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/atmel_spi.c b/drivers/spi/atmel_spi.c
index f5b3fdb..8ce70cb 100644
--- a/drivers/spi/atmel_spi.c
+++ b/drivers/spi/atmel_spi.c
@@ -189,14 +189,14 @@ static void atmel_spi_next_xfer_data(struct spi_master *master,
 
 	/* use scratch buffer only when rx or tx data is unspecified */
 	if (xfer->rx_buf)
-		*rx_dma = xfer->rx_dma + xfer->len - len;
+		*rx_dma = xfer->rx_dma + xfer->len - *plen;
 	else {
 		*rx_dma = as->buffer_dma;
 		if (len > BUFFER_SIZE)
 			len = BUFFER_SIZE;
 	}
 	if (xfer->tx_buf)
-		*tx_dma = xfer->tx_dma + xfer->len - len;
+		*tx_dma = xfer->tx_dma + xfer->len - *plen;
 	else {
 		*tx_dma = as->buffer_dma;
 		if (len > BUFFER_SIZE)
-- 
1.6.0.4

Re: [PATCH] atmel_spi: fix dma addr calculation for len > BUFFER_SIZE

[Haavard Skinnemoen]

Ben Nizette <bn@niasdigital.com> wrote:
> If len > BUFFER_LEN and !xfer->rx_buf we end up calculating the tx
> buffer address as
> 
> *tx_dma = xfer->tx_dma + xfer->len - BUFFER_SIZE;
> 
> which is constant; i.e. we just send the last BUFFER_SIZE data over
> again until we've reached the right number of bytes.
> 
> This patch gets around this by using the /requested/ length when
> calculating addresses.
> 
> Note there's no way len != *plen when we calculate the rx buffer address
> but conceptually we should be using *plen and I don't want someone to
> come through later, see the calculations for rx and tx are different and
> "clean up" back to what we had.
> 
> Signed-off-by: Ben Nizette <bn@niasdigital.com>

Wow, that is subtle. I had to stare at it for a long while before I
understood what's going on, but I believe you're right.

Acked-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>

While you're at it, could you send another patch renaming 'len' to
'next_len'? I think that would make it a bit more obvious why your
patch is correct and prevent similar mistakes in the future.

Haavard

Re: [PATCH] atmel_spi: fix dma addr calculation for len > BUFFER_SIZE

[Ben Nizette]

On Mon, 2009-09-28 at 09:12 +0200, Haavard Skinnemoen wrote:

> Wow, that is subtle. I had to stare at it for a long while before I
> understood what's going on, but I believe you're right.

Prolly just means my changelog was crappy ;-)

> 
> Acked-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
> 
> While you're at it, could you send another patch renaming 'len' to
> 'next_len'? I think that would make it a bit more obvious why your
> patch is correct and prevent similar mistakes in the future.

Good plan, below.

Thx,
	--Ben.
> 
> Haavard

---8<---

From: Ben Nizette <bn@niasdigital.com>
Subject: [PATCH] atmel_spi: make "len" variable name less ambiguous in dma addr calculation

"[PATCH] atmel_spi: fix dma addr calculation for len > BUFFER_SIZE"
fixed a bug where the "len" variable in atmel_spi_next_xfer_data() was
taken to be the total number of bytes remaining in the transfer but it
actually represents the number of bytes which will be sent this dma
chunk.  While the 2 will be the same if there is less than 1 chunk to go
(or if we aren't using a scratch buffer and therefore aren't breaking
the transfers in to chunks), they won't be the same in general.

s/len/next_len to reflect the true nature and usage of this variable.

Signed-off-by: Ben Nizette <bn@niasdigital.com>
---
 drivers/spi/atmel_spi.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/atmel_spi.c b/drivers/spi/atmel_spi.c
index 8ce70cb..5d94fca 100644
--- a/drivers/spi/atmel_spi.c
+++ b/drivers/spi/atmel_spi.c
@@ -185,28 +185,28 @@ static void atmel_spi_next_xfer_data(struct spi_master *master,
 				u32 *plen)
 {
 	struct atmel_spi	*as = spi_master_get_devdata(master);
-	u32			len = *plen;
+	u32			next_len = *plen;
 
 	/* use scratch buffer only when rx or tx data is unspecified */
 	if (xfer->rx_buf)
 		*rx_dma = xfer->rx_dma + xfer->len - *plen;
 	else {
 		*rx_dma = as->buffer_dma;
-		if (len > BUFFER_SIZE)
-			len = BUFFER_SIZE;
+		if (next_len > BUFFER_SIZE)
+			next_len = BUFFER_SIZE;
 	}
 	if (xfer->tx_buf)
 		*tx_dma = xfer->tx_dma + xfer->len - *plen;
 	else {
 		*tx_dma = as->buffer_dma;
-		if (len > BUFFER_SIZE)
-			len = BUFFER_SIZE;
-		memset(as->buffer, 0, len);
+		if (next_len > BUFFER_SIZE)
+			next_len = BUFFER_SIZE;
+		memset(as->buffer, 0, next_len);
 		dma_sync_single_for_device(&as->pdev->dev,
-				as->buffer_dma, len, DMA_TO_DEVICE);
+				as->buffer_dma, next_len, DMA_TO_DEVICE);
 	}
 
-	*plen = len;
+	*plen = next_len;
 }
 
 /*
-- 
1.6.0.4

Re: [PATCH] atmel_spi: fix dma addr calculation for len > BUFFER_SIZE

[Haavard Skinnemoen]

Ben Nizette <bn@niasdigital.com> wrote:
> From: Ben Nizette <bn@niasdigital.com>
> Subject: [PATCH] atmel_spi: make "len" variable name less ambiguous in dma addr calculation
> 
> "[PATCH] atmel_spi: fix dma addr calculation for len > BUFFER_SIZE"
> fixed a bug where the "len" variable in atmel_spi_next_xfer_data() was
> taken to be the total number of bytes remaining in the transfer but it
> actually represents the number of bytes which will be sent this dma
> chunk.  While the 2 will be the same if there is less than 1 chunk to go
> (or if we aren't using a scratch buffer and therefore aren't breaking
> the transfers in to chunks), they won't be the same in general.
> 
> s/len/next_len to reflect the true nature and usage of this variable.
> 
> Signed-off-by: Ben Nizette <bn@niasdigital.com>

Excellent, thanks.

Acked-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>