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 <[email protected]>
---
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
Ben Nizette <[email protected]> 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 <[email protected]>
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 <[email protected]>
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
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 <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
---
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
Ben Nizette <[email protected]> wrote:
> From: Ben Nizette <[email protected]>
> 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 <[email protected]>
Excellent, thanks.
Acked-by: Haavard Skinnemoen <[email protected]>