[PATCH 1/2] dma: mv_xor: use proper dma memory management functions

[PATCH 1/2] dma: mv_xor: use proper dma memory management functions

[Lubomir Rintel]

Caught by self-check with DMA_API_DEBUG:
WARNING: at lib/dma-debug.c:878 check_unmap+0x37c/0x748()
mv_xor mv_xor.0: DMA-API: device driver frees DMA memory with wrong function [device address=0x000000001f3a1a40] [size=2000 bytes] [mapped as single] [unmapped as page]

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/dma/mv_xor.c |   22 +++++++++++++++-------
 1 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index ac71f55..39387df 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -306,9 +306,7 @@ mv_xor_run_tx_complete_actions(struct mv_xor_desc_slot *desc,
 			desc->async_tx.callback(
 				desc->async_tx.callback_param);
 
-		/* unmap dma addresses
-		 * (unmap_single vs unmap_page?)
-		 */
+		/* unmap dma addresses */
 		if (desc->group_head && desc->unmap_len) {
 			struct mv_xor_desc_slot *unmap = desc->group_head;
 			struct device *dev = mv_chan_to_devp(mv_chan);
@@ -327,7 +325,11 @@ mv_xor_run_tx_complete_actions(struct mv_xor_desc_slot *desc,
 					dir = DMA_BIDIRECTIONAL;
 				else
 					dir = DMA_FROM_DEVICE;
-				dma_unmap_page(dev, dest, len, dir);
+
+				if (flags & DMA_COMPL_DEST_UNMAP_SINGLE)
+					dma_unmap_single(dev, dest, len, dir);
+				else
+					dma_unmap_page(dev, dest, len, dir);
 			}
 
 			if (!(flags & DMA_COMPL_SKIP_SRC_UNMAP)) {
@@ -336,8 +338,12 @@ mv_xor_run_tx_complete_actions(struct mv_xor_desc_slot *desc,
 								    src_cnt);
 					if (addr == dest)
 						continue;
-					dma_unmap_page(dev, addr, len,
-						       DMA_TO_DEVICE);
+					if (flags & DMA_COMPL_SRC_UNMAP_SINGLE)
+						dma_unmap_single(dev, addr, len,
+							       DMA_TO_DEVICE);
+					else
+						dma_unmap_page(dev, addr, len,
+							       DMA_TO_DEVICE);
 				}
 			}
 			desc->group_head = NULL;
@@ -938,7 +944,9 @@ static int mv_xor_memcpy_self_test(struct mv_xor_chan *mv_chan)
 				 MV_XOR_TEST_SIZE, DMA_TO_DEVICE);
 
 	tx = mv_xor_prep_dma_memcpy(dma_chan, dest_dma, src_dma,
-				    MV_XOR_TEST_SIZE, 0);
+				    MV_XOR_TEST_SIZE,
+				    DMA_COMPL_SRC_UNMAP_SINGLE |
+				    DMA_COMPL_DEST_UNMAP_SINGLE);
 	cookie = mv_xor_tx_submit(tx);
 	mv_xor_issue_pending(dma_chan);
 	async_tx_ack(tx);
-- 
1.7.1

[PATCH 2/2] dma: mv_xor: do not sync the DMA buffer after being deallocated

[Lubomir Rintel]

DMA_CHECK was unhappy:
mv_xor mv_xor.0: DMA-API: device driver tries to sync DMA memory it has not

Also, the xor test was causing the mv_xor_slot_cleanup() to incorrectly
dealocate the destination buffer. This is fixed as well, since the deallocation
is done in mv_xor_probe() (with correct flags) now:
mv_xor mv_xor.0: DMA-API: device driver frees DMA memory with different direction [device address=0x000000001dea4000] [size=4096 bytes] [mapped with DMA_FROM_DEVICE] [unmapped with DMA_BIDIRECTIONAL]

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/dma/mv_xor.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 39387df..f642d8b 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -946,7 +946,7 @@ static int mv_xor_memcpy_self_test(struct mv_xor_chan *mv_chan)
 	tx = mv_xor_prep_dma_memcpy(dma_chan, dest_dma, src_dma,
 				    MV_XOR_TEST_SIZE,
 				    DMA_COMPL_SRC_UNMAP_SINGLE |
-				    DMA_COMPL_DEST_UNMAP_SINGLE);
+				    DMA_COMPL_SKIP_DEST_UNMAP);
 	cookie = mv_xor_tx_submit(tx);
 	mv_xor_issue_pending(dma_chan);
 	async_tx_ack(tx);
@@ -962,6 +962,8 @@ static int mv_xor_memcpy_self_test(struct mv_xor_chan *mv_chan)
 
 	dma_sync_single_for_cpu(dma_chan->device->dev, dest_dma,
 				MV_XOR_TEST_SIZE, DMA_FROM_DEVICE);
+	dma_unmap_single(dma_chan->device->dev, dest_dma, MV_XOR_TEST_SIZE,
+			 DMA_FROM_DEVICE);
 	if (memcmp(src, dest, MV_XOR_TEST_SIZE)) {
 		dev_err(dma_chan->device->dev,
 			"Self-test copy failed compare, disabling\n");
@@ -1039,7 +1041,8 @@ mv_xor_xor_self_test(struct mv_xor_chan *mv_chan)
 					   0, PAGE_SIZE, DMA_TO_DEVICE);
 
 	tx = mv_xor_prep_dma_xor(dma_chan, dest_dma, dma_srcs,
-				 MV_XOR_NUM_SRC_TEST, PAGE_SIZE, 0);
+				 MV_XOR_NUM_SRC_TEST, PAGE_SIZE,
+				 DMA_COMPL_SKIP_DEST_UNMAP);
 
 	cookie = mv_xor_tx_submit(tx);
 	mv_xor_issue_pending(dma_chan);
@@ -1056,6 +1059,8 @@ mv_xor_xor_self_test(struct mv_xor_chan *mv_chan)
 
 	dma_sync_single_for_cpu(dma_chan->device->dev, dest_dma,
 				PAGE_SIZE, DMA_FROM_DEVICE);
+	dma_unmap_page(dma_chan->device->dev, dest_dma, PAGE_SIZE,
+		       DMA_FROM_DEVICE);
 	for (i = 0; i < (PAGE_SIZE / sizeof(u32)); i++) {
 		u32 *ptr = page_address(dest);
 		if (ptr[i] != cmp_word) {
-- 
1.7.1

[PATCH 2/2] dma: mv_xor: do not sync the DMA buffer after being deallocated

[Thomas Petazzoni]

Dear Lubomir Rintel,

On Thu, 27 Dec 2012 20:23:48 +0100, Lubomir Rintel wrote:

>  	dma_sync_single_for_cpu(dma_chan->device->dev, dest_dma,
>  				MV_XOR_TEST_SIZE, DMA_FROM_DEVICE);
> +	dma_unmap_single(dma_chan->device->dev, dest_dma, MV_XOR_TEST_SIZE,
> +			 DMA_FROM_DEVICE);

I would assume that dma_unmap_single() implies a
dma_sync_single_for_cpu() since you're unmapping the DMA buffer. So if
you use dma_unmap_single(), I think you can remove the call to
dma_sync_single_for_cpu().


>  	dma_sync_single_for_cpu(dma_chan->device->dev, dest_dma,
>  				PAGE_SIZE, DMA_FROM_DEVICE);
> +	dma_unmap_page(dma_chan->device->dev, dest_dma, PAGE_SIZE,
> +		       DMA_FROM_DEVICE);

Ditto.

Also, the mv_xor_memcpy_self_test() function not only dma_map_single()
the destination buffer, but also the source buffer. So presumably, the
source buffer should also be dma_unmap_single()'d.

And for the mv_xor_xor_self_test() function, multiple source buffers
are dma_map_page()'d, so they should all be dma_unmap_page()'d I guess,
not only the destination buffer.

Does that make sense?

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/2] dma: mv_xor: do not sync the DMA buffer after being deallocated

[Lubomir Rintel]

On Fri, 2013-01-04 at 17:10 +0100, Thomas Petazzoni wrote:
> Dear Lubomir Rintel,
> 
> On Thu, 27 Dec 2012 20:23:48 +0100, Lubomir Rintel wrote:
> 
> >  	dma_sync_single_for_cpu(dma_chan->device->dev, dest_dma,
> >  				MV_XOR_TEST_SIZE, DMA_FROM_DEVICE);
> > +	dma_unmap_single(dma_chan->device->dev, dest_dma, MV_XOR_TEST_SIZE,
> > +			 DMA_FROM_DEVICE);
> 
> I would assume that dma_unmap_single() implies a
> dma_sync_single_for_cpu() since you're unmapping the DMA buffer. So if
> you use dma_unmap_single(), I think you can remove the call to
> dma_sync_single_for_cpu().

Yes, it indeed seems to be the case (comment for arm_dma_unmap_page():
"After this call, reads by the CPU to the buffer are guaranteed to see
whatever the device wrote there.".

> >  	dma_sync_single_for_cpu(dma_chan->device->dev, dest_dma,
> >  				PAGE_SIZE, DMA_FROM_DEVICE);
> > +	dma_unmap_page(dma_chan->device->dev, dest_dma, PAGE_SIZE,
> > +		       DMA_FROM_DEVICE);
> 
> Ditto.
> 
> Also, the mv_xor_memcpy_self_test() function not only dma_map_single()
> the destination buffer, but also the source buffer. So presumably, the
> source buffer should also be dma_unmap_single()'d.
> 
> And for the mv_xor_xor_self_test() function, multiple source buffers
> are dma_map_page()'d, so they should all be dma_unmap_page()'d I guess,
> not only the destination buffer.

Those get released by the mv_xor_run_tx_complete_actions() callback.

I will follow up with an updated patch.

Regards,
Lubo

[PATCH 2/2] dma: mv_xor: do not sync the DMA buffer after being deallocated

[Lubomir Rintel]

On Sun, 2013-01-13 at 14:18 +0100, Lubomir Rintel wrote:
> On Fri, 2013-01-04 at 17:10 +0100, Thomas Petazzoni wrote:
> > Dear Lubomir Rintel,
> > 
> > On Thu, 27 Dec 2012 20:23:48 +0100, Lubomir Rintel wrote:
> > 
> > >  	dma_sync_single_for_cpu(dma_chan->device->dev, dest_dma,
> > >  				MV_XOR_TEST_SIZE, DMA_FROM_DEVICE);
> > > +	dma_unmap_single(dma_chan->device->dev, dest_dma, MV_XOR_TEST_SIZE,
> > > +			 DMA_FROM_DEVICE);
> > 
> > I would assume that dma_unmap_single() implies a
> > dma_sync_single_for_cpu() since you're unmapping the DMA buffer. So if
> > you use dma_unmap_single(), I think you can remove the call to
> > dma_sync_single_for_cpu().
> 
> Yes, it indeed seems to be the case (comment for arm_dma_unmap_page():
> "After this call, reads by the CPU to the buffer are guaranteed to see
> whatever the device wrote there.".
> 
> > >  	dma_sync_single_for_cpu(dma_chan->device->dev, dest_dma,
> > >  				PAGE_SIZE, DMA_FROM_DEVICE);
> > > +	dma_unmap_page(dma_chan->device->dev, dest_dma, PAGE_SIZE,
> > > +		       DMA_FROM_DEVICE);
> > 
> > Ditto.
> > 
> > Also, the mv_xor_memcpy_self_test() function not only dma_map_single()
> > the destination buffer, but also the source buffer. So presumably, the
> > source buffer should also be dma_unmap_single()'d.
> > 
> > And for the mv_xor_xor_self_test() function, multiple source buffers
> > are dma_map_page()'d, so they should all be dma_unmap_page()'d I guess,
> > not only the destination buffer.
> 
> Those get released by the mv_xor_run_tx_complete_actions() callback.
> 
> I will follow up with an updated patch.

Actually, after cleaning up the sync a couple more selfcheck warnings
popped up. I'd be very thankful if you could take a look (patches
chained to this message).

[PATCH 1/3] dma: mv_xor: do not sync the DMA buffer after being deallocated

[Lubomir Rintel]

DMA_CHECK was unhappy:
mv_xor mv_xor.0: DMA-API: device driver tries to sync DMA memory it has not allocated [device address=0x000000001f3a1a40] [size=2000 bytes]

Deallocation, which involves a sync, happens automatically by
mv_xor_slot_cleanup() once the transfer is finished.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/dma/mv_xor.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index e666983..8b81a04 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -960,8 +960,6 @@ static int mv_xor_memcpy_self_test(struct mv_xor_chan *mv_chan)
 		goto free_resources;
 	}
 
-	dma_sync_single_for_cpu(dma_chan->device->dev, dest_dma,
-				MV_XOR_TEST_SIZE, DMA_FROM_DEVICE);
 	if (memcmp(src, dest, MV_XOR_TEST_SIZE)) {
 		dev_err(dma_chan->device->dev,
 			"Self-test copy failed compare, disabling\n");
@@ -1054,8 +1052,6 @@ mv_xor_xor_self_test(struct mv_xor_chan *mv_chan)
 		goto free_resources;
 	}
 
-	dma_sync_single_for_cpu(dma_chan->device->dev, dest_dma,
-				PAGE_SIZE, DMA_FROM_DEVICE);
 	for (i = 0; i < (PAGE_SIZE / sizeof(u32)); i++) {
 		u32 *ptr = page_address(dest);
 		if (ptr[i] != cmp_word) {
-- 
1.7.1

[PATCH 2/3] dma: mv_xor: fix DMA-API error handling sanity check

[Lubomir Rintel]

Add error checking.
---
 drivers/dma/mv_xor.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 8b81a04..d00a834 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -939,9 +939,21 @@ static int mv_xor_memcpy_self_test(struct mv_xor_chan *mv_chan)
 
 	dest_dma = dma_map_single(dma_chan->device->dev, dest,
 				  MV_XOR_TEST_SIZE, DMA_FROM_DEVICE);
+	if (dma_mapping_error(dma_chan->device->dev, dest_dma)) {
+		dev_err(dma_chan->device->dev,
+			"Could not map destination buffer, disabling\n");
+		err = -ENODEV;
+		goto free_resources;
+	}
 
 	src_dma = dma_map_single(dma_chan->device->dev, src,
 				 MV_XOR_TEST_SIZE, DMA_TO_DEVICE);
+	if (dma_mapping_error(dma_chan->device->dev, src_dma)) {
+		dev_err(dma_chan->device->dev,
+			"Could not map source buffer, disabling\n");
+		err = -ENODEV;
+		goto free_resources;
+	}
 
 	tx = mv_xor_prep_dma_memcpy(dma_chan, dest_dma, src_dma,
 				    MV_XOR_TEST_SIZE,
@@ -1031,10 +1043,23 @@ mv_xor_xor_self_test(struct mv_xor_chan *mv_chan)
 	/* test xor */
 	dest_dma = dma_map_page(dma_chan->device->dev, dest, 0, PAGE_SIZE,
 				DMA_FROM_DEVICE);
+	if (dma_mapping_error(dma_chan->device->dev, dest_dma)) {
+		dev_err(dma_chan->device->dev,
+			"Could not map destination page, disabling\n");
+		err = -ENODEV;
+		goto free_resources;
+	}
 
-	for (i = 0; i < MV_XOR_NUM_SRC_TEST; i++)
+	for (i = 0; i < MV_XOR_NUM_SRC_TEST; i++) {
 		dma_srcs[i] = dma_map_page(dma_chan->device->dev, xor_srcs[i],
 					   0, PAGE_SIZE, DMA_TO_DEVICE);
+		if (dma_mapping_error(dma_chan->device->dev, dma_srcs[i])) {
+			dev_err(dma_chan->device->dev,
+				"Could not map source page, disabling\n");
+			err = -ENODEV;
+			goto free_resources;
+		}
+	}
 
 	tx = mv_xor_prep_dma_xor(dma_chan, dest_dma, dma_srcs,
 				 MV_XOR_NUM_SRC_TEST, PAGE_SIZE, 0);
-- 
1.7.1

[PATCH 3/3] dma: mv_xor: get rid of a DMA-API sanity check warning

[Lubomir Rintel]

mv_xor mv_xor.0: DMA-API: device driver frees DMA memory with different direction [device address=0x000000001dea4000] [size=4096 bytes] [mapped with DMA_FROM_DEVICE] [unmapped with DMA_BIDIRECTIONAL]

Change xor self test destination buffer allocation direction to bidirectional,
as it's what mv_xor_run_tx_complete_actions() frees for multiple sources.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/dma/mv_xor.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index d00a834..1e90f5d 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -1042,7 +1042,7 @@ mv_xor_xor_self_test(struct mv_xor_chan *mv_chan)
 
 	/* test xor */
 	dest_dma = dma_map_page(dma_chan->device->dev, dest, 0, PAGE_SIZE,
-				DMA_FROM_DEVICE);
+				DMA_BIDIRECTIONAL);
 	if (dma_mapping_error(dma_chan->device->dev, dest_dma)) {
 		dev_err(dma_chan->device->dev,
 			"Could not map destination page, disabling\n");
-- 
1.7.1