When either rx_buf or tx_buf is not being used, i.e. for plain read-
or write operations, the atmel_spi uses a fixed-size DMA buffer
instead. If the transfer is longer than the size of this buffer, it is
split into multiple DMA transfers.

When the transfer is split like this, the atmel_spi driver ends up
using the same DMA address again and again even for the buffer that
came from the user, which is of course wrong. Fix this by adding the
number of bytes already transferred to the DMA address so that the
data ends up in the right place.

Thanks to Wu Xuan for discovering this bug.

Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
---
 drivers/spi/atmel_spi.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/atmel_spi.c b/drivers/spi/atmel_spi.c
index 1d8a2f6..8b2601d 100644
--- a/drivers/spi/atmel_spi.c
+++ b/drivers/spi/atmel_spi.c
@@ -113,16 +113,16 @@ static void atmel_spi_next_xfer(struct spi_master *master,
 
 	len = as->remaining_bytes;
 
-	tx_dma = xfer->tx_dma;
-	rx_dma = xfer->rx_dma;
+	tx_dma = xfer->tx_dma + xfer->len - len;
+	rx_dma = xfer->rx_dma + xfer->len - len;
 
 	/* use scratch buffer only when rx or tx data is unspecified */
-	if (rx_dma == INVALID_DMA_ADDRESS) {
+	if (!xfer->rx_buf) {
 		rx_dma = as->buffer_dma;
 		if (len > BUFFER_SIZE)
 			len = BUFFER_SIZE;
 	}
-	if (tx_dma == INVALID_DMA_ADDRESS) {
+	if (!xfer->tx_buf) {
 		tx_dma = as->buffer_dma;
 		if (len > BUFFER_SIZE)
 			len = BUFFER_SIZE;
-- 
1.4.4.4

-

[ message continues ]

... and this one must not assume that the relevant buffer
is always NULL when the tx_dma has been set up.  Keep the
test against INVALID_DMA_ADDRESS, but fetch the address
from the spi_transfer.

It's legit to set up cpu-virtual (for PIO) and dma addresses
for each buffer, since the upper layer driver has no way to
know if the underlying controller driver is DMA-capable, or


-

[ message continues ]

No it doesn't -- it's the other way around. It assumes that if
xfer->tx_buf is NULL, the user didn't provide a buffer so it has to
use the scratch buffer instead. The driver never does PIO transfers,
so if tx_buf is valid, tx_dma ought to be valid  too. This is taken
care of elsewhere by calling dma_map_single() if the user provided a
virtual pointer but not a dma address.

Checking for an invalid cpu-virtual pointer really should be
equivalent to checking for an invalid DMA address at this point.

I could perhaps add else blocks to those two ifs and add the offset to
the respective dma addresses there instead if it makes it clearer. The
tests are supposed to be the same as before, but I had to change them
one way or another because of the added offset (which would make an

Yes, but are there any drivers that will provide a valid dma address
and a NULL cpu-virtual pointer? That would indeed break my
assumptions, but it would also break any PIO-only drivers, wouldn't
it?

Haavard
-

[ message continues ]

I think I'll sign off on this as-is.



Potentially.  One scenario would be a block driver, which needs to
work with scatterlists.  dma_map_sg() is allowed to coalesce the
scatterlist entries, as with an IOMMU.  If it does that, there can
no longer be a one-to-one linkage between addreses provided to that
driver, and the dma addresess.  (Likewise, addresses in HIGHMEM are
not normally going to have kernel virtual addresses.)  So providing

Which is exactly why the current "mmc_spi" code doesn't use the
dma_map_sg() interface.  Instead, it goes in more byte-size chunks,
taking care to provide both dma and pio addresses.  It's a PITA,
but at least it's coded now.



-

[ message continues ]