[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-pV1zxKMKwgg3AZtZ2NlBNQ@public.gmane.org> wrote:
> From: Ben Nizette <bn-pV1zxKMKwgg3AZtZ2NlBNQ@public.gmane.org>
> 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-pV1zxKMKwgg3AZtZ2NlBNQ@public.gmane.org>

Excellent, thanks.

Acked-by: Haavard Skinnemoen <haavard.skinnemoen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

------------------------------------------------------------------------------
Come build with us! The BlackBerry&reg; Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay 
ahead of the curve. Join us from November 9&#45;12, 2009. Register now&#33;
http://p.sf.net/sfu/devconf