[PATCH 1/2] ath9k: fix dma direction for map/unmap in ath_rx_tasklet

[PATCH 1/2] ath9k: fix dma direction for map/unmap in ath_rx_tasklet

[tom.leiming]

From: Ming Lei <tom.leiming@gmail.com>

For edma, we should use DMA_BIDIRECTIONAL, or else use
DMA_FROM_DEVICE.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/net/wireless/ath/ath9k/recv.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index ba13913..a3fe6e1 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -838,9 +838,9 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
 	int dma_type;
 
 	if (edma)
-		dma_type = DMA_FROM_DEVICE;
-	else
 		dma_type = DMA_BIDIRECTIONAL;
+	else
+		dma_type = DMA_FROM_DEVICE;
 
 	qtype = hp ? ATH9K_RX_QUEUE_HP : ATH9K_RX_QUEUE_LP;
 	spin_lock_bh(&sc->rx.rxbuflock);
-- 
1.6.2.5

[PATCH 2/2] ath9k: fix dma sync in rx path

[tom.leiming]

From: Ming Lei <tom.leiming@gmail.com>

If buffer is to be accessed by cpu after dma transfer is over, but
between dma mapping and dma unmapping, we should use
dma_sync_single_for_cpu to sync the buffer between cpu with
device. And dma_sync_single_for_device is used to let
device gain the buffer again.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/net/wireless/ath/ath9k/recv.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index a3fe6e1..96f5d83 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -694,7 +694,7 @@ static bool ath_edma_get_buffers(struct ath_softc *sc,
 	bf = SKB_CB_ATHBUF(skb);
 	BUG_ON(!bf);
 
-	dma_sync_single_for_device(sc->dev, bf->bf_buf_addr,
+	dma_sync_single_for_cpu(sc->dev, bf->bf_buf_addr,
 				common->rx_bufsize, DMA_FROM_DEVICE);
 
 	ret = ath9k_hw_process_rxdesc_edma(ah, NULL, skb->data);
@@ -808,7 +808,7 @@ static struct ath_buf *ath_get_next_rx_buf(struct ath_softc *sc,
 	 * 1. accessing the frame
 	 * 2. requeueing the same buffer to h/w
 	 */
-	dma_sync_single_for_device(sc->dev, bf->bf_buf_addr,
+	dma_sync_single_for_cpu(sc->dev, bf->bf_buf_addr,
 			common->rx_bufsize,
 			DMA_FROM_DEVICE);
 
-- 
1.6.2.5

Re: [PATCH 2/2] ath9k: fix dma sync in rx path

[Felix Fietkau]

On 2010-05-14 3:16 PM, tom.leiming@gmail.com wrote:
> From: Ming Lei <tom.leiming@gmail.com>
> 
> If buffer is to be accessed by cpu after dma transfer is over, but
> between dma mapping and dma unmapping, we should use
> dma_sync_single_for_cpu to sync the buffer between cpu with
> device. And dma_sync_single_for_device is used to let
> device gain the buffer again.
I think this patch is wrong. On most MIPS devices,
dma_sync_single_for_cpu is a no-op. In fact, with this patch, the rx
path fails very quickly.
I believe keeping the dma_sync_single_for_device variant is necessary
for all syncs.

- Felix

Re: [PATCH 2/2] ath9k: fix dma sync in rx path

[Ming Lei]

2010/5/14 Felix Fietkau <nbd@openwrt.org>:
> On 2010-05-14 3:16 PM, tom.leiming@gmail.com wrote:
>> From: Ming Lei <tom.leiming@gmail.com>
>>
>> If buffer is to be accessed by cpu after dma transfer is over, but
>> between dma mapping and dma unmapping, we should use
>> dma_sync_single_for_cpu to sync the buffer between cpu with
>> device. And dma_sync_single_for_device is used to let
>> device gain the buffer again.
> I think this patch is wrong. On most MIPS devices,
> dma_sync_single_for_cpu is a no-op. In fact, with this patch, the rx
> path fails very quickly.

Sorry for  my  bad email client.

On most MIPS devices,  dma_sync_single_for_cpu does same things
almost with dma_unmap_single(plat_unmap_dma_mem is no-op). If
dma_unmap_single is enough, dma_sync_single_for_cpu is certainly
enough,
isn't it?

For the usage of dma_sync_single_for_cpu or dma_sync_single_for_device,
Documentation/DMA-API*.txt give more details.

> I believe keeping the dma_sync_single_for_device variant is necessary
> for all syncs.

Thanks,

-- 
Lei Ming

Re: [PATCH 2/2] ath9k: fix dma sync in rx path

[Felix Fietkau]

On 2010-05-14 5:27 PM, Ming Lei wrote:
> 2010/5/14 Felix Fietkau <nbd@openwrt.org>:
>> On 2010-05-14 3:16 PM, tom.leiming@gmail.com wrote:
>>> From: Ming Lei <tom.leiming@gmail.com>
>>>
>>> If buffer is to be accessed by cpu after dma transfer is over, but
>>> between dma mapping and dma unmapping, we should use
>>> dma_sync_single_for_cpu to sync the buffer between cpu with
>>> device. And dma_sync_single_for_device is used to let
>>> device gain the buffer again.
>> I think this patch is wrong. On most MIPS devices,
>> dma_sync_single_for_cpu is a no-op. In fact, with this patch, the rx
>> path fails very quickly.
> 
> Sorry for  my  bad email client.
> 
> On most MIPS devices,  dma_sync_single_for_cpu does same things
> almost with dma_unmap_single(plat_unmap_dma_mem is no-op). If
> dma_unmap_single is enough, dma_sync_single_for_cpu is certainly
> enough,
> isn't it?
Because I did some testing with these functions while writing the code,
I already know that dma_sync_single_for_cpu is not enough in this case.

Maybe we need to place the dma_sync_single_for_device call elsewhere and
then move the dma_sync_single_for_cpu call there afterwads, but simply
replacing this instance as is done in your patch *will* cause regressions.

- Felix

Re: [PATCH 2/2] ath9k: fix dma sync in rx path

[Ming Lei]

2010/5/15 Felix Fietkau <nbd@openwrt.org>:
> On 2010-05-14 5:27 PM, Ming Lei wrote:
>> 2010/5/14 Felix Fietkau <nbd@openwrt.org>:
>>> On 2010-05-14 3:16 PM, tom.leiming@gmail.com wrote:
>>>> From: Ming Lei <tom.leiming@gmail.com>
>>>>
>>>> If buffer is to be accessed by cpu after dma transfer is over, but
>>>> between dma mapping and dma unmapping, we should use
>>>> dma_sync_single_for_cpu to sync the buffer between cpu with
>>>> device. And dma_sync_single_for_device is used to let
>>>> device gain the buffer again.
>>> I think this patch is wrong. On most MIPS devices,
>>> dma_sync_single_for_cpu is a no-op. In fact, with this patch, the rx
>>> path fails very quickly.
>>
>> Sorry for  my  bad email client.
>>
>> On most MIPS devices,  dma_sync_single_for_cpu does same things
>> almost with dma_unmap_single(plat_unmap_dma_mem is no-op). If
>> dma_unmap_single is enough, dma_sync_single_for_cpu is certainly
>> enough,
>> isn't it?
> Because I did some testing with these functions while writing the code,
> I already know that dma_sync_single_for_cpu is not enough in this case.

In theory, dma_sync_single_for_cpu is enough in this case, as explained
in the commit log.

On MIPS, cache invalidation is done in dma mapping for DMA_FROM_DEVICE
direction, so it is correct that dma_sync_single_for_cpu or dma unmapping
is only a no-op since access from CPU will trigger a refetch of the buffer
from memory into cache.

Also dma_sync_single_for_device still does cache invalidation for
DMA_FROM_DEVICE direction, if it has to be done in this case to avoid rx
failure, I guess there is some code which does touch the dma buffer
after dma mapping but before dma_sync_single_for_device(should be
dma_sync_single_for_cpu), maybe before dma transfer is over.
If so, it should be a bug since it violates the dma api constraint, maybe
we should have a careful review to check the dma api rule.

>
> Maybe we need to place the dma_sync_single_for_device call elsewhere and
> then move the dma_sync_single_for_cpu call there afterwads, but simply
> replacing this instance as is done in your patch *will* cause regressions.
>
> - Felix
>

-- 
Lei Ming

Re: [PATCH 2/2] ath9k: fix dma sync in rx path

[Felix Fietkau]

On 2010-05-15 3:31 AM, Ming Lei wrote:
> 2010/5/15 Felix Fietkau <nbd@openwrt.org>:
>> On 2010-05-14 5:27 PM, Ming Lei wrote:
>>> 2010/5/14 Felix Fietkau <nbd@openwrt.org>:
>>>> On 2010-05-14 3:16 PM, tom.leiming@gmail.com wrote:
>>>>> From: Ming Lei <tom.leiming@gmail.com>
>>>>>
>>>>> If buffer is to be accessed by cpu after dma transfer is over, but
>>>>> between dma mapping and dma unmapping, we should use
>>>>> dma_sync_single_for_cpu to sync the buffer between cpu with
>>>>> device. And dma_sync_single_for_device is used to let
>>>>> device gain the buffer again.
>>>> I think this patch is wrong. On most MIPS devices,
>>>> dma_sync_single_for_cpu is a no-op. In fact, with this patch, the rx
>>>> path fails very quickly.
>>>
>>> Sorry for  my  bad email client.
>>>
>>> On most MIPS devices,  dma_sync_single_for_cpu does same things
>>> almost with dma_unmap_single(plat_unmap_dma_mem is no-op). If
>>> dma_unmap_single is enough, dma_sync_single_for_cpu is certainly
>>> enough,
>>> isn't it?
>> Because I did some testing with these functions while writing the code,
>> I already know that dma_sync_single_for_cpu is not enough in this case.
> 
> In theory, dma_sync_single_for_cpu is enough in this case, as explained
> in the commit log.
> 
> On MIPS, cache invalidation is done in dma mapping for DMA_FROM_DEVICE
> direction, so it is correct that dma_sync_single_for_cpu or dma unmapping
> is only a no-op since access from CPU will trigger a refetch of the buffer
> from memory into cache.
> 
> Also dma_sync_single_for_device still does cache invalidation for
> DMA_FROM_DEVICE direction, if it has to be done in this case to avoid rx
> failure, I guess there is some code which does touch the dma buffer
> after dma mapping but before dma_sync_single_for_device(should be
> dma_sync_single_for_cpu), maybe before dma transfer is over.
> If so, it should be a bug since it violates the dma api constraint, maybe
> we should have a careful review to check the dma api rule.
The second part of your patch might be OK then (I'm not really sure).
But the first part is definitely wrong, since with EDMA Rx, the
descriptor data is part of the buffer, so the cache needs to be
invalidated for every access until the status bit shows that it has been
completed.
This could probably be fixed by running dma_sync_single_for_device after
an unsuccessful status bit check and also after the descriptor part has
been zero'd out before passing the buffer on to the hw.

- Felix

Re: [PATCH 2/2] ath9k: fix dma sync in rx path

[Ming Lei]

2010/5/15 Felix Fietkau <nbd@openwrt.org>:
> The second part of your patch might be OK then (I'm not really sure).
> But the first part is definitely wrong, since with EDMA Rx, the
> descriptor data is part of the buffer, so the cache needs to be
> invalidated for every access until the status bit shows that it has been
> completed.
> This could probably be fixed by running dma_sync_single_for_device after
> an unsuccessful status bit check and also after the descriptor part has

Yes, dma_sync_single_for_device is needed to return buffer to device
if an unsuccessful status bit check is found.

> been zero'd out before passing the buffer on to the hw.

The patch does not touch this dma_sync_single_for_device.


-- 
Lei Ming

Re: [PATCH 2/2] ath9k: fix dma sync in rx path

[Ming Lei]

>From edd368e7436e7a80c5a43e7ad40cff1f3fa20806 Mon Sep 17 00:00:00 2001
From: Ming Lei <tom.leiming@gmail.com>
Date: Fri, 14 May 2010 17:35:51 +0800
Subject: [PATCH 2/2] ath9k: fix dma sync in rx path(v2)

If buffer is to be accessed by cpu after dma is over, but
between dma mapping and dma unmapping, we should use
dma_sync_single_for_cpu to sync the buffer between cpu with
device. And dma_sync_single_for_device is used to let
device gain the buffer again.

v2: Felix pointed out dma_sync_single_for_device is needed to return
buffer to device if an unsuccessful status bit check is found.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
Signed-off-by: Felix Fietkau <nbd@openwrt.org>
---
 drivers/net/wireless/ath/ath9k/recv.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c
index a3fe6e1..f4453f0 100644
--- a/drivers/net/wireless/ath/ath9k/recv.c
+++ b/drivers/net/wireless/ath/ath9k/recv.c
@@ -694,12 +694,16 @@ static bool ath_edma_get_buffers(struct ath_softc *sc,
 	bf = SKB_CB_ATHBUF(skb);
 	BUG_ON(!bf);
 
-	dma_sync_single_for_device(sc->dev, bf->bf_buf_addr,
+	dma_sync_single_for_cpu(sc->dev, bf->bf_buf_addr,
 				common->rx_bufsize, DMA_FROM_DEVICE);
 
 	ret = ath9k_hw_process_rxdesc_edma(ah, NULL, skb->data);
-	if (ret == -EINPROGRESS)
+	if (ret == -EINPROGRESS) {
+		/*let device gain the buffer again*/
+		dma_sync_single_for_device(sc->dev, bf->bf_buf_addr,
+				common->rx_bufsize, DMA_FROM_DEVICE);
 		return false;
+	}
 
 	__skb_unlink(skb, &rx_edma->rx_fifo);
 	if (ret == -EINVAL) {
@@ -808,7 +812,7 @@ static struct ath_buf *ath_get_next_rx_buf(struct ath_softc *sc,
 	 * 1. accessing the frame
 	 * 2. requeueing the same buffer to h/w
 	 */
-	dma_sync_single_for_device(sc->dev, bf->bf_buf_addr,
+	dma_sync_single_for_cpu(sc->dev, bf->bf_buf_addr,
 			common->rx_bufsize,
 			DMA_FROM_DEVICE);
 
-- 
1.6.2.5

Re: [PATCH 2/2] ath9k: fix dma sync in rx path

[Felix Fietkau]

On 2010-05-15 12:25 PM, Ming Lei wrote:
> From edd368e7436e7a80c5a43e7ad40cff1f3fa20806 Mon Sep 17 00:00:00 2001
> From: Ming Lei <tom.leiming@gmail.com>
> Date: Fri, 14 May 2010 17:35:51 +0800
> Subject: [PATCH 2/2] ath9k: fix dma sync in rx path(v2)
> 
> If buffer is to be accessed by cpu after dma is over, but
> between dma mapping and dma unmapping, we should use
> dma_sync_single_for_cpu to sync the buffer between cpu with
> device. And dma_sync_single_for_device is used to let
> device gain the buffer again.
> 
> v2: Felix pointed out dma_sync_single_for_device is needed to return
> buffer to device if an unsuccessful status bit check is found.
> 
> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
Thanks, I will test that later and let you know if it works.

- Felix

Re: [PATCH 2/2] ath9k: fix dma sync in rx path

[Felix Fietkau]

On 2010-05-15 12:25 PM, Ming Lei wrote:
> From edd368e7436e7a80c5a43e7ad40cff1f3fa20806 Mon Sep 17 00:00:00 2001
> From: Ming Lei <tom.leiming@gmail.com>
> Date: Fri, 14 May 2010 17:35:51 +0800
> Subject: [PATCH 2/2] ath9k: fix dma sync in rx path(v2)
> 
> If buffer is to be accessed by cpu after dma is over, but
> between dma mapping and dma unmapping, we should use
> dma_sync_single_for_cpu to sync the buffer between cpu with
> device. And dma_sync_single_for_device is used to let
> device gain the buffer again.
> 
> v2: Felix pointed out dma_sync_single_for_device is needed to return
> buffer to device if an unsuccessful status bit check is found.
> 
> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
Sorry for the delay. I've tested your patch and it works for me.

Acked-by: Felix Fietkau <nbd@openwrt.org>