[PATCH] atmel_spi: Pass correct DMA address to controller

[PATCH] atmel_spi: Pass correct DMA address to controller

[Haavard Skinnemoen]

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

Re: [PATCH] atmel_spi: Pass correct DMA address to controller

[David Brownell]

On Wednesday 16 May 2007, Haavard Skinnemoen wrote:
> 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.

Yes, and needs fixing.  This is almost right, see below.


> 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) {

This test ...

>  		rx_dma = as->buffer_dma;
>  		if (len > BUFFER_SIZE)
>  			len = BUFFER_SIZE;
>  	}
> -	if (tx_dma == INVALID_DMA_ADDRESS) {
> +	if (!xfer->tx_buf) {

... 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
for that matter PIO-capable.

>  		tx_dma = as->buffer_dma;
>  		if (len > BUFFER_SIZE)
>  			len = BUFFER_SIZE;
> -- 
> 1.4.4.4
> 

Re: [PATCH] atmel_spi: Pass correct DMA address to controller

[Håvard Skinnemoen]

On 5/16/07, David Brownell <david-b@pacbell.net> wrote:
> On Wednesday 16 May 2007, Haavard Skinnemoen wrote:
> > -     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) {
>
> This test ...
>
> >               rx_dma = as->buffer_dma;
> >               if (len > BUFFER_SIZE)
> >                       len = BUFFER_SIZE;
> >       }
> > -     if (tx_dma == INVALID_DMA_ADDRESS) {
> > +     if (!xfer->tx_buf) {
>
> ... 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.

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
invalid dma address non-invalid.)

> 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
> for that matter PIO-capable.

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

Re: [PATCH] atmel_spi: Pass correct DMA address to controller

[David Brownell]

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


On Wednesday 16 May 2007, Håvard Skinnemoen wrote:
> On 5/16/07, David Brownell <david-b@pacbell.net> wrote:

> > 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
> > for that matter PIO-capable.
> 
> Yes, but are there any drivers that will provide a valid dma address
> and a NULL cpu-virtual pointer?

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
both types of address is no longer practical with scatterlists.


> 		That would indeed break my 
> assumptions, but it would also break any PIO-only drivers, wouldn't
> it?

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.

- Dave



> Haavard
>