From mboxrd@z Thu Jan 1 00:00:00 1970 From: Felix Fietkau Date: Tue, 12 Jul 2011 12:36:06 +0800 Subject: [ath9k-devel] [PATCH v2 07/46] net/wireless: ath9k: fix DMA API usage In-Reply-To: <280ad9176e6532f231e054b38b952b20580874c5.1310339688.git.mirq-linux@rere.qmqm.pl> References: <280ad9176e6532f231e054b38b952b20580874c5.1310339688.git.mirq-linux@rere.qmqm.pl> Message-ID: <4E1BCF36.2010506@openwrt.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: ath9k-devel@lists.ath9k.org On 2011-07-11 8:52 AM, Micha? Miros?aw wrote: > Also constify buf_addr for ath9k_hw_process_rxdesc_edma() to verify > assumptions --- dma_sync_single_for_device() call can be removed. > > Signed-off-by: Micha? Miros?aw > --- > drivers/net/wireless/ath/ath9k/ar9003_mac.c | 4 ++-- > drivers/net/wireless/ath/ath9k/ar9003_mac.h | 2 +- > drivers/net/wireless/ath/ath9k/recv.c | 10 +++------- > 3 files changed, 6 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c > index 70dc8ec..c5f46d5 100644 > --- a/drivers/net/wireless/ath/ath9k/recv.c > +++ b/drivers/net/wireless/ath/ath9k/recv.c > @@ -684,15 +684,11 @@ static bool ath_edma_get_buffers(struct ath_softc *sc, > BUG_ON(!bf); > > dma_sync_single_for_cpu(sc->dev, bf->bf_buf_addr, > - common->rx_bufsize, DMA_FROM_DEVICE); > + common->rx_bufsize, DMA_BIDIRECTIONAL); > > ret = ath9k_hw_process_rxdesc_edma(ah, NULL, skb->data); > - 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); > + if (ret == -EINPROGRESS) > return false; > - } > > __skb_unlink(skb,&rx_edma->rx_fifo); > if (ret == -EINVAL) { I have strong doubts about this change. On most MIPS devices, dma_sync_single_for_cpu is a no-op, whereas dma_sync_single_for_device flushes the cache range. With this change, the CPU could cache the DMA status part behind skb->data and that cache entry would not be flushed inbetween calls to this functions on the same buffer, likely leading to rx stalls. - Felix From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from nbd.name ([46.4.11.11]:44795 "EHLO nbd.name" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750831Ab1GLEgS (ORCPT ); Tue, 12 Jul 2011 00:36:18 -0400 Message-ID: <4E1BCF36.2010506@openwrt.org> (sfid-20110712_063722_719569_CC4E02B6) Date: Tue, 12 Jul 2011 12:36:06 +0800 From: Felix Fietkau MIME-Version: 1.0 To: =?UTF-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= CC: netdev@vger.kernel.org, linux-wireless@vger.kernel.org, Jouni Malinen , Senthil Balasubramanian , ath9k-devel@lists.ath9k.org, Vasanthakumar Thiagarajan Subject: Re: [ath9k-devel] [PATCH v2 07/46] net/wireless: ath9k: fix DMA API usage References: <280ad9176e6532f231e054b38b952b20580874c5.1310339688.git.mirq-linux@rere.qmqm.pl> In-Reply-To: <280ad9176e6532f231e054b38b952b20580874c5.1310339688.git.mirq-linux@rere.qmqm.pl> Content-Type: text/plain; charset=UTF-8; format=flowed Sender: linux-wireless-owner@vger.kernel.org List-ID: On 2011-07-11 8:52 AM, Michał Mirosław wrote: > Also constify buf_addr for ath9k_hw_process_rxdesc_edma() to verify > assumptions --- dma_sync_single_for_device() call can be removed. > > Signed-off-by: Michał Mirosław > --- > drivers/net/wireless/ath/ath9k/ar9003_mac.c | 4 ++-- > drivers/net/wireless/ath/ath9k/ar9003_mac.h | 2 +- > drivers/net/wireless/ath/ath9k/recv.c | 10 +++------- > 3 files changed, 6 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless/ath/ath9k/recv.c > index 70dc8ec..c5f46d5 100644 > --- a/drivers/net/wireless/ath/ath9k/recv.c > +++ b/drivers/net/wireless/ath/ath9k/recv.c > @@ -684,15 +684,11 @@ static bool ath_edma_get_buffers(struct ath_softc *sc, > BUG_ON(!bf); > > dma_sync_single_for_cpu(sc->dev, bf->bf_buf_addr, > - common->rx_bufsize, DMA_FROM_DEVICE); > + common->rx_bufsize, DMA_BIDIRECTIONAL); > > ret = ath9k_hw_process_rxdesc_edma(ah, NULL, skb->data); > - 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); > + if (ret == -EINPROGRESS) > return false; > - } > > __skb_unlink(skb,&rx_edma->rx_fifo); > if (ret == -EINVAL) { I have strong doubts about this change. On most MIPS devices, dma_sync_single_for_cpu is a no-op, whereas dma_sync_single_for_device flushes the cache range. With this change, the CPU could cache the DMA status part behind skb->data and that cache entry would not be flushed inbetween calls to this functions on the same buffer, likely leading to rx stalls. - Felix From mboxrd@z Thu Jan 1 00:00:00 1970 From: Felix Fietkau Subject: Re: [PATCH v2 07/46] net/wireless: ath9k: fix DMA API usage Date: Tue, 12 Jul 2011 12:36:06 +0800 Message-ID: <4E1BCF36.2010506@openwrt.org> References: <280ad9176e6532f231e054b38b952b20580874c5.1310339688.git.mirq-linux@rere.qmqm.pl> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Cc: netdev@vger.kernel.org, linux-wireless@vger.kernel.org, Jouni Malinen , Senthil Balasubramanian , ath9k-devel@lists.ath9k.org, Vasanthakumar Thiagarajan To: =?UTF-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= Return-path: In-Reply-To: <280ad9176e6532f231e054b38b952b20580874c5.1310339688.git.mirq-linux@rere.qmqm.pl> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: ath9k-devel-bounces@lists.ath9k.org Errors-To: ath9k-devel-bounces@lists.ath9k.org List-Id: netdev.vger.kernel.org T24gMjAxMS0wNy0xMSA4OjUyIEFNLCBNaWNoYcWCIE1pcm9zxYJhdyB3cm90ZToKPiBBbHNvIGNv bnN0aWZ5IGJ1Zl9hZGRyIGZvciBhdGg5a19od19wcm9jZXNzX3J4ZGVzY19lZG1hKCkgdG8gdmVy aWZ5Cj4gYXNzdW1wdGlvbnMgLS0tIGRtYV9zeW5jX3NpbmdsZV9mb3JfZGV2aWNlKCkgY2FsbCBj YW4gYmUgcmVtb3ZlZC4KPgo+IFNpZ25lZC1vZmYtYnk6IE1pY2hhxYIgTWlyb3PFgmF3PG1pcnEt bGludXhAcmVyZS5xbXFtLnBsPgo+IC0tLQo+ICAgZHJpdmVycy9uZXQvd2lyZWxlc3MvYXRoL2F0 aDlrL2FyOTAwM19tYWMuYyB8ICAgIDQgKystLQo+ICAgZHJpdmVycy9uZXQvd2lyZWxlc3MvYXRo L2F0aDlrL2FyOTAwM19tYWMuaCB8ICAgIDIgKy0KPiAgIGRyaXZlcnMvbmV0L3dpcmVsZXNzL2F0 aC9hdGg5ay9yZWN2LmMgICAgICAgfCAgIDEwICsrKy0tLS0tLS0KPiAgIDMgZmlsZXMgY2hhbmdl ZCwgNiBpbnNlcnRpb25zKCspLCAxMCBkZWxldGlvbnMoLSkKPgo+IGRpZmYgLS1naXQgYS9kcml2 ZXJzL25ldC93aXJlbGVzcy9hdGgvYXRoOWsvcmVjdi5jIGIvZHJpdmVycy9uZXQvd2lyZWxlc3Mv YXRoL2F0aDlrL3JlY3YuYwo+IGluZGV4IDcwZGM4ZWMuLmM1ZjQ2ZDUgMTAwNjQ0Cj4gLS0tIGEv ZHJpdmVycy9uZXQvd2lyZWxlc3MvYXRoL2F0aDlrL3JlY3YuYwo+ICsrKyBiL2RyaXZlcnMvbmV0 L3dpcmVsZXNzL2F0aC9hdGg5ay9yZWN2LmMKPiBAQCAtNjg0LDE1ICs2ODQsMTEgQEAgc3RhdGlj IGJvb2wgYXRoX2VkbWFfZ2V0X2J1ZmZlcnMoc3RydWN0IGF0aF9zb2Z0YyAqc2MsCj4gICAJQlVH X09OKCFiZik7Cj4KPiAgIAlkbWFfc3luY19zaW5nbGVfZm9yX2NwdShzYy0+ZGV2LCBiZi0+YmZf YnVmX2FkZHIsCj4gLQkJCQljb21tb24tPnJ4X2J1ZnNpemUsIERNQV9GUk9NX0RFVklDRSk7Cj4g KwkJCQljb21tb24tPnJ4X2J1ZnNpemUsIERNQV9CSURJUkVDVElPTkFMKTsKPgo+ICAgCXJldCA9 IGF0aDlrX2h3X3Byb2Nlc3NfcnhkZXNjX2VkbWEoYWgsIE5VTEwsIHNrYi0+ZGF0YSk7Cj4gLQlp ZiAocmV0ID09IC1FSU5QUk9HUkVTUykgewo+IC0JCS8qbGV0IGRldmljZSBnYWluIHRoZSBidWZm ZXIgYWdhaW4qLwo+IC0JCWRtYV9zeW5jX3NpbmdsZV9mb3JfZGV2aWNlKHNjLT5kZXYsIGJmLT5i Zl9idWZfYWRkciwKPiAtCQkJCWNvbW1vbi0+cnhfYnVmc2l6ZSwgRE1BX0ZST01fREVWSUNFKTsK PiArCWlmIChyZXQgPT0gLUVJTlBST0dSRVNTKQo+ICAgCQlyZXR1cm4gZmFsc2U7Cj4gLQl9Cj4K PiAgIAlfX3NrYl91bmxpbmsoc2tiLCZyeF9lZG1hLT5yeF9maWZvKTsKPiAgIAlpZiAocmV0ID09 IC1FSU5WQUwpIHsKSSBoYXZlIHN0cm9uZyBkb3VidHMgYWJvdXQgdGhpcyBjaGFuZ2UuIE9uIG1v c3QgTUlQUyBkZXZpY2VzLCAKZG1hX3N5bmNfc2luZ2xlX2Zvcl9jcHUgaXMgYSBuby1vcCwgd2hl cmVhcyBkbWFfc3luY19zaW5nbGVfZm9yX2RldmljZSAKZmx1c2hlcyB0aGUgY2FjaGUgcmFuZ2Uu IFdpdGggdGhpcyBjaGFuZ2UsIHRoZSBDUFUgY291bGQgY2FjaGUgdGhlIERNQSAKc3RhdHVzIHBh cnQgYmVoaW5kIHNrYi0+ZGF0YSBhbmQgdGhhdCBjYWNoZSBlbnRyeSB3b3VsZCBub3QgYmUgZmx1 c2hlZCAKaW5iZXR3ZWVuIGNhbGxzIHRvIHRoaXMgZnVuY3Rpb25zIG9uIHRoZSBzYW1lIGJ1ZmZl ciwgbGlrZWx5IGxlYWRpbmcgdG8gCnJ4IHN0YWxscy4KCi0gRmVsaXgKX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYXRoOWstZGV2ZWwgbWFpbGluZyBsaXN0 CmF0aDlrLWRldmVsQGxpc3RzLmF0aDlrLm9yZwpodHRwczovL2xpc3RzLmF0aDlrLm9yZy9tYWls bWFuL2xpc3RpbmZvL2F0aDlrLWRldmVsCg==