From: Ming Lei <[email protected]>
For edma, we should use DMA_BIDIRECTIONAL, or else use
DMA_FROM_DEVICE.
Signed-off-by: Ming Lei <[email protected]>
---
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
On 2010-05-15 3:31 AM, Ming Lei wrote:
> 2010/5/15 Felix Fietkau <[email protected]>:
>> On 2010-05-14 5:27 PM, Ming Lei wrote:
>>> 2010/5/14 Felix Fietkau <[email protected]>:
>>>> On 2010-05-14 3:16 PM, [email protected] wrote:
>>>>> From: Ming Lei <[email protected]>
>>>>>
>>>>> 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
On 2010-05-15 12:25 PM, Ming Lei wrote:
> From edd368e7436e7a80c5a43e7ad40cff1f3fa20806 Mon Sep 17 00:00:00 2001
> From: Ming Lei <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Felix Fietkau <[email protected]>
Sorry for the delay. I've tested your patch and it works for me.
Acked-by: Felix Fietkau <[email protected]>
2010/5/14 Felix Fietkau <[email protected]>:
> On 2010-05-14 3:16 PM, [email protected] wrote:
>> From: Ming Lei <[email protected]>
>>
>> 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
2010/5/15 Felix Fietkau <[email protected]>:
> 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
On 2010-05-14 5:27 PM, Ming Lei wrote:
> 2010/5/14 Felix Fietkau <[email protected]>:
>> On 2010-05-14 3:16 PM, [email protected] wrote:
>>> From: Ming Lei <[email protected]>
>>>
>>> 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
>From edd368e7436e7a80c5a43e7ad40cff1f3fa20806 Mon Sep 17 00:00:00 2001
From: Ming Lei <[email protected]>
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 <[email protected]>
Signed-off-by: Felix Fietkau <[email protected]>
---
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
On 2010-05-15 12:25 PM, Ming Lei wrote:
> From edd368e7436e7a80c5a43e7ad40cff1f3fa20806 Mon Sep 17 00:00:00 2001
> From: Ming Lei <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Felix Fietkau <[email protected]>
Thanks, I will test that later and let you know if it works.
- Felix
On 2010-05-14 3:16 PM, [email protected] wrote:
> From: Ming Lei <[email protected]>
>
> 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
From: Ming Lei <[email protected]>
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 <[email protected]>
---
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
2010/5/15 Felix Fietkau <[email protected]>:
> On 2010-05-14 5:27 PM, Ming Lei wrote:
>> 2010/5/14 Felix Fietkau <[email protected]>:
>>> On 2010-05-14 3:16 PM, [email protected] wrote:
>>>> From: Ming Lei <[email protected]>
>>>>
>>>> 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