From mboxrd@z Thu Jan 1 00:00:00 1970 From: YueHaibing Subject: Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset Date: Fri, 20 Jul 2018 21:59:06 +0800 Message-ID: References: <20180719140955.19444-1-yuehaibing@huawei.com> <20180719141723.GM17271@n2100.armlinux.org.uk> <1532019744.8953.248.camel@mtkswgap22> <9a4cdfc6-7487-5908-b584-f3bf6158c4d4@huawei.com> <20180720081334.GO17271@n2100.armlinux.org.uk> <1532094504.8953.262.camel@mtkswgap22> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Cc: nbd@openwrt.org, nelson.chang@mediatek.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org, john@phrozen.org, matthias.bgg@gmail.com, davem@davemloft.net, linux-arm-kernel@lists.infradead.org To: Sean Wang , Russell King - ARM Linux Return-path: In-Reply-To: <1532094504.8953.262.camel@mtkswgap22> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org List-Id: netdev.vger.kernel.org CgpPbiAyMDE4LzcvMjAgMjE6NDgsIFNlYW4gV2FuZyB3cm90ZToKPiBPbiBGcmksIDIwMTgtMDct MjAgYXQgMDk6MTMgKzAxMDAsIFJ1c3NlbGwgS2luZyAtIEFSTSBMaW51eCB3cm90ZToKPj4gT24g RnJpLCBKdWwgMjAsIDIwMTggYXQgMDI6MzA6NTNQTSArMDgwMCwgWXVlSGFpYmluZyB3cm90ZToK Pj4+IE9uIDIwMTgvNy8yMCAxOjAyLCBTZWFuIFdhbmcgd3JvdGU6Cj4+Pj4gT24gVGh1LCAyMDE4 LTA3LTE5IGF0IDE1OjE3ICswMTAwLCBSdXNzZWxsIEtpbmcgLSBBUk0gTGludXggd3JvdGU6Cj4+ Pj4+IE9uIFRodSwgSnVsIDE5LCAyMDE4IGF0IDEwOjA5OjU1UE0gKzA4MDAsIFl1ZUhhaWJpbmcg d3JvdGU6Cj4+Pj4+PiBVc2UgZG1hX3phbGxvY19jb2hlcmVudCBpbnN0ZWFkIG9mIGRtYV9hbGxv Y19jb2hlcmVudAo+Pj4+Pj4gZm9sbG93ZWQgYnkgbWVtc2V0IDAuCj4+Pj4+Pgo+Pj4+Pj4gU2ln bmVkLW9mZi1ieTogWXVlSGFpYmluZyA8eXVlaGFpYmluZ0BodWF3ZWkuY29tPgo+Pj4+Pj4gLS0t Cj4+Pj4+PiAgZHJpdmVycy9uZXQvZXRoZXJuZXQvbWVkaWF0ZWsvbXRrX2V0aF9zb2MuYyB8IDcg KystLS0tLQo+Pj4+Pj4gIDEgZmlsZSBjaGFuZ2VkLCAyIGluc2VydGlvbnMoKyksIDUgZGVsZXRp b25zKC0pCj4+Pj4+Pgo+Pj4+Pj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvbmV0L2V0aGVybmV0L21l ZGlhdGVrL210a19ldGhfc29jLmMgYi9kcml2ZXJzL25ldC9ldGhlcm5ldC9tZWRpYXRlay9tdGtf ZXRoX3NvYy5jCj4+Pj4+PiBpbmRleCBkOGViZjBhLi5mYmRiM2UzIDEwMDY0NAo+Pj4+Pj4gLS0t IGEvZHJpdmVycy9uZXQvZXRoZXJuZXQvbWVkaWF0ZWsvbXRrX2V0aF9zb2MuYwo+Pj4+Pj4gKysr IGIvZHJpdmVycy9uZXQvZXRoZXJuZXQvbWVkaWF0ZWsvbXRrX2V0aF9zb2MuYwo+Pj4+Pj4gQEAg LTEyMjEsMTQgKzEyMjEsMTEgQEAgc3RhdGljIGludCBtdGtfdHhfYWxsb2Moc3RydWN0IG10a19l dGggKmV0aCkKPj4+Pj4+ICAJaWYgKCFyaW5nLT5idWYpCj4+Pj4+PiAgCQlnb3RvIG5vX3R4X21l bTsKPj4+Pj4+ICAKPj4+Pj4+IC0JcmluZy0+ZG1hID0gZG1hX2FsbG9jX2NvaGVyZW50KGV0aC0+ ZGV2LAo+Pj4+Pj4gLQkJCQkJICBNVEtfRE1BX1NJWkUgKiBzeiwKPj4+Pj4+IC0JCQkJCSAgJnJp bmctPnBoeXMsCj4+Pj4+PiAtCQkJCQkgIEdGUF9BVE9NSUMgfCBfX0dGUF9aRVJPKTsKPj4+Pj4+ ICsJcmluZy0+ZG1hID0gZG1hX3phbGxvY19jb2hlcmVudChldGgtPmRldiwgTVRLX0RNQV9TSVpF ICogc3osCj4+Pj4+PiArCQkJCQkmcmluZy0+cGh5cywgR0ZQX0FUT01JQyB8IF9fR0ZQX1pFUk8p Owo+Pj4+Pj4gIAlpZiAoIXJpbmctPmRtYSkKPj4+Pj4+ICAJCWdvdG8gbm9fdHhfbWVtOwo+Pj4+ Pj4gIAo+Pj4+Pj4gLQltZW1zZXQocmluZy0+ZG1hLCAwLCBNVEtfRE1BX1NJWkUgKiBzeik7Cj4+ Pj4+Cj4+Pj4+IEkgaGF2ZSB0byB3b25kZXIgd2hldGhlciB0aGlzIGNvZGUgbmVlZHMgdHdvIGZv cm1zIG9mIHplcm9pbmcuLi4gaW4KPj4+Pj4gdGhlIG9yaWdpbmFsIGNvZGUsIF9fR0ZQX1pFUk8g X2FuZF8gYSBjYWxsIHRvIG1lbXNldCgpIGp1c3QgaW4gY2FzZQo+Pj4+PiBfX0dGUF9aRVJPIGZh aWxlZCB0byBkbyBpdHMgam9iLCBhbmQgaW4gdGhlIHJlcGxhY2VtZW50IGNvZGUsIGp1c3QKPj4+ Pj4gaW4gY2FzZSBkbWFfemFsbG9jX2NvaGVyZW50KCkgaGFzbid0IGdvdCB0aGUgaWRlYS4uLgo+ Pj4+Pgo+Pj4+PiBJIHRoaW5rIHlvdSBjYW4gZHJvcCB0aGUgX19HRlBfWkVSTy4gOykKPj4+Pj4K Pj4+Pgo+Pj4+IEp1c3Qgbm93IEkgZGlkIGFuIGV4cGVyaW1lbnQgb24gNC4xNC41NiBvbiBhcm12 Ny4gSSBmb3VuZCB0aGF0Cj4+Pj4gZG1hX3phbGxvY19jb2hlcmVudCBkb2VzIG5vdCBndWFyYW50 ZWUgdGhhdCB0aGUgYnVmZmVyIHdlIGdldAo+Pj4+IGlzIGFsbCBmaWxsZWQgd2l0aCAwLgo+Pj4+ Cj4+Pj4KPj4+PiBJIHJlYWxseSB0aGluayBpdCdzIGEgbGl0dGxlIGJpdCB3ZWlyZCBPUiB3aGF0 IHdhcyBJIG1pc3Npbmcgc29tZXRoaW5nCj4+Pj4gZm9yIGVuYWJsaW5nIGRtYV96YWxsb2NfY29o ZXJlbnQgPyBUaGUgcmVzdWx0IHNlZW1zIHRvIHRlbGwgdGhhdCB3ZQo+Pj4+IGNhbid0IHJlbW92 ZSBmcmVlbHkgdGhlIG1lbXNldCB3aXRoIDAgYXQgdGhpcyBtb21lbnQgdW50aWwgd2UgZ2V0IGEK Pj4+PiBjYXVzZS4KPj4+Pgo+Pj4KPj4+IFRoYXQgbWVhbnMgZG1hX3phbGxvY19jb2hlcmVudCBk b2Vzbid0IHdvcmsgYXMgZXhwZWN0IG9uIGFybXY3Pwo+Pgo+PiBDYW4gc29tZW9uZSB3b3JrIG91 dCB3aGljaCB1bmRlcmx5aW5nIGFsbG9jYXRvciBpcyBiZWluZyB1c2VkIC0gdGhlCj4+IHBvc3Np YmlsaXRpZXMgYXJlOgo+Pgo+PiAtIGRtYV9hbGxvY19mcm9tX2Rldl9jb2hlcmVudAo+PiAtIGNt YQo+PiAtIHNpbXBsZQo+PiAtIHJlbWFwCj4+IC0gcG9vbAo+Pgo+PiBMb29raW5nIGF0IHRoZSBj b2RlLCBJJ2QgZ3Vlc3MgaXQncyB0aGUgcG9vbCBhbGxvY2F0b3IsIGFzIEkgZG9uJ3Qgc2VlCj4+ IGFueXRoaW5nIHdoaWNoIHplcm9zIG1lbW9yeSB0aGVyZSwgYW5kIGl0IGRvZXNuJ3QgaG9ub3Ig dGhlIF9fR0ZQX1pFUk8KPj4gZmxhZy4gIFRoaXMgaXMgZGVmaW5pdGVseSBhbiBhbGxvY2F0b3Ig YnVnLgo+PgoKU2VhbiwKCmNhbiB5b3UgdGVzdCBiZWxsb3cgcGF0Y2jvvJoKCmRpZmYgLS1naXQg YS9hcmNoL2FybS9tbS9kbWEtbWFwcGluZy5jIGIvYXJjaC9hcm0vbW0vZG1hLW1hcHBpbmcuYwpp bmRleCBiZTBmYTdlLi41MjAyOWJiIDEwMDY0NAotLS0gYS9hcmNoL2FybS9tbS9kbWEtbWFwcGlu Zy5jCisrKyBiL2FyY2gvYXJtL21tL2RtYS1tYXBwaW5nLmMKQEAgLTU2NCw2ICs1NjQsNyBAQCBz dGF0aWMgdm9pZCAqX19hbGxvY19mcm9tX3Bvb2woc2l6ZV90IHNpemUsIHN0cnVjdCBwYWdlICoq cmV0X3BhZ2UpCgoqcmV0X3BhZ2UgPSBwaHlzX3RvX3BhZ2UocGh5cyk7CnB0ciA9ICh2b2lkICop dmFsOworIG1lbXNldChwdHIsIDAsIHNpemUpOwp9Cgo+IAo+IFllcywgeW91ciBndWVzcyBpcyBy aWdodC4gdGhlIGFsbG9jYXRvciBpcyBmcm9tIHBvb2wuIEkgc2hvdyB0aGUgZnVsbAo+IHN0YWNr IGFzIGJlbG93IHdoZW4gY2FsbGluZyBkbWFfemFsbG9jX2NvaGVyZW50Cj4gCj4gWyAgIDU0LjM1 ODMxMF0gWzxjMDExM2NhOD5dICh1bndpbmRfYmFja3RyYWNlKSBmcm9tIFs8YzAxMGU1NTg+XSAo c2hvd19zdGFjaysweDIwLzB4MjQpCj4gWyAgIDU0LjM2NjAxMl0gWzxjMDEwZTU1OD5dIChzaG93 X3N0YWNrKSBmcm9tIFs8YzA5MjdmNTg+XSAoZHVtcF9zdGFjaysweDk4LzB4YWMpCj4gWyAgIDU0 LjM3MzE5Nl0gWzxjMDkyN2Y1OD5dIChkdW1wX3N0YWNrKSBmcm9tIFs8YzAxMWMxZjg+XSAocG9v bF9hbGxvY2F0b3JfYWxsb2MrMHgyMC8weDMwKQo+IFsgICA1NC4zODEyMzhdIFs8YzAxMWMxZjg+ XSAocG9vbF9hbGxvY2F0b3JfYWxsb2MpIGZyb20gWzxjMDExYTIzOD5dIChfX2RtYV9hbGxvYysw eDFiOC8weDM0NCkKPiBbICAgNTQuMzg5NTM4XSBbPGMwMTFhMjM4Pl0gKF9fZG1hX2FsbG9jKSBm cm9tIFs8YzAxMWE0NWM+XSAoYXJtX2RtYV9hbGxvYysweDUwLzB4NTgpCj4gWyAgIDU0LjM5NzA2 M10gWzxjMDExYTQ1Yz5dIChhcm1fZG1hX2FsbG9jKSBmcm9tIFs8YzA1ZGMyMDg+XSAobXRrX29w ZW4rMHhmNC8weDcxMCkKPiBbICAgNTQuNDA0NDE2XSBbPGMwNWRjMjA4Pl0gKG10a19vcGVuKSBm cm9tIFs8YzA3NmQxMTA+XSAoX19kZXZfb3BlbisweGRjLzB4MTYwKQo+IFsgICA1NC40MTE1MDdd IFs8YzA3NmQxMTA+XSAoX19kZXZfb3BlbikgZnJvbSBbPGMwNzZkNTMwPl0gKF9fZGV2X2NoYW5n ZV9mbGFncysweDE3OC8weDFjNCkKPiBbICAgNTQuNDE5NTQ3XSBbPGMwNzZkNTMwPl0gKF9fZGV2 X2NoYW5nZV9mbGFncykgZnJvbSBbPGMwNzZkNWE0Pl0gKGRldl9jaGFuZ2VfZmxhZ3MrMHgyOC8w eDU4KQo+IFsgICA1NC40Mjc5MzRdIFs8YzA3NmQ1YTQ+XSAoZGV2X2NoYW5nZV9mbGFncykgZnJv bSBbPGMwN2VhN2MwPl0gKGRldmluZXRfaW9jdGwrMHg2MzAvMHg3MjApCj4gWyAgIDU0LjQzNjA2 Ml0gWzxjMDdlYTdjMD5dIChkZXZpbmV0X2lvY3RsKSBmcm9tIFs8YzA3ZWQwNzA+XSAoaW5ldF9p b2N0bCsweDIxMC8weDNhOCkKPiBbICAgNTQuNDQzNjc0XSBbPGMwN2VkMDcwPl0gKGluZXRfaW9j dGwpIGZyb20gWzxjMDc0NzA5ND5dIChzb2NrX2lvY3RsKzB4MjM0LzB4NGRjKQo+IFsgICA1NC40 NTEwMjldIFs8YzA3NDcwOTQ+XSAoc29ja19pb2N0bCkgZnJvbSBbPGMwMjgyMDZjPl0gKGRvX3Zm c19pb2N0bCsweGMwLzB4OTE0KQo+IFsgICA1NC40NTg0NjhdIFs8YzAyODIwNmM+XSAoZG9fdmZz X2lvY3RsKSBmcm9tIFs8YzAyODI5MDQ+XSAoU3lTX2lvY3RsKzB4NDQvMHg2YykKPiBbICAgNTQu NDY1NzMzXSBbPGMwMjgyOTA0Pl0gKFN5U19pb2N0bCkgZnJvbSBbPGMwMTAxMDAwPl0gKHJldF9m YXN0X3N5c2NhbGwrMHgwLzB4NTQpCj4gCj4gCj4gCj4gCj4gLgo+IAoKCl9fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmxpbnV4LWFybS1rZXJuZWwgbWFpbGlu ZyBsaXN0CmxpbnV4LWFybS1rZXJuZWxAbGlzdHMuaW5mcmFkZWFkLm9yZwpodHRwOi8vbGlzdHMu aW5mcmFkZWFkLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2xpbnV4LWFybS1rZXJuZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: yuehaibing@huawei.com (YueHaibing) Date: Fri, 20 Jul 2018 21:59:06 +0800 Subject: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset In-Reply-To: <1532094504.8953.262.camel@mtkswgap22> References: <20180719140955.19444-1-yuehaibing@huawei.com> <20180719141723.GM17271@n2100.armlinux.org.uk> <1532019744.8953.248.camel@mtkswgap22> <9a4cdfc6-7487-5908-b584-f3bf6158c4d4@huawei.com> <20180720081334.GO17271@n2100.armlinux.org.uk> <1532094504.8953.262.camel@mtkswgap22> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 2018/7/20 21:48, Sean Wang wrote: > On Fri, 2018-07-20 at 09:13 +0100, Russell King - ARM Linux wrote: >> On Fri, Jul 20, 2018 at 02:30:53PM +0800, YueHaibing wrote: >>> On 2018/7/20 1:02, Sean Wang wrote: >>>> On Thu, 2018-07-19 at 15:17 +0100, Russell King - ARM Linux wrote: >>>>> On Thu, Jul 19, 2018 at 10:09:55PM +0800, YueHaibing wrote: >>>>>> Use dma_zalloc_coherent instead of dma_alloc_coherent >>>>>> followed by memset 0. >>>>>> >>>>>> Signed-off-by: YueHaibing >>>>>> --- >>>>>> drivers/net/ethernet/mediatek/mtk_eth_soc.c | 7 ++----- >>>>>> 1 file changed, 2 insertions(+), 5 deletions(-) >>>>>> >>>>>> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c >>>>>> index d8ebf0a..fbdb3e3 100644 >>>>>> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c >>>>>> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c >>>>>> @@ -1221,14 +1221,11 @@ static int mtk_tx_alloc(struct mtk_eth *eth) >>>>>> if (!ring->buf) >>>>>> goto no_tx_mem; >>>>>> >>>>>> - ring->dma = dma_alloc_coherent(eth->dev, >>>>>> - MTK_DMA_SIZE * sz, >>>>>> - &ring->phys, >>>>>> - GFP_ATOMIC | __GFP_ZERO); >>>>>> + ring->dma = dma_zalloc_coherent(eth->dev, MTK_DMA_SIZE * sz, >>>>>> + &ring->phys, GFP_ATOMIC | __GFP_ZERO); >>>>>> if (!ring->dma) >>>>>> goto no_tx_mem; >>>>>> >>>>>> - memset(ring->dma, 0, MTK_DMA_SIZE * sz); >>>>> >>>>> I have to wonder whether this code needs two forms of zeroing... in >>>>> the original code, __GFP_ZERO _and_ a call to memset() just in case >>>>> __GFP_ZERO failed to do its job, and in the replacement code, just >>>>> in case dma_zalloc_coherent() hasn't got the idea... >>>>> >>>>> I think you can drop the __GFP_ZERO. ;) >>>>> >>>> >>>> Just now I did an experiment on 4.14.56 on armv7. I found that >>>> dma_zalloc_coherent does not guarantee that the buffer we get >>>> is all filled with 0. >>>> >>>> >>>> I really think it's a little bit weird OR what was I missing something >>>> for enabling dma_zalloc_coherent ? The result seems to tell that we >>>> can't remove freely the memset with 0 at this moment until we get a >>>> cause. >>>> >>> >>> That means dma_zalloc_coherent doesn't work as expect on armv7? >> >> Can someone work out which underlying allocator is being used - the >> possibilities are: >> >> - dma_alloc_from_dev_coherent >> - cma >> - simple >> - remap >> - pool >> >> Looking at the code, I'd guess it's the pool allocator, as I don't see >> anything which zeros memory there, and it doesn't honor the __GFP_ZERO >> flag. This is definitely an allocator bug. >> Sean, can you test bellow patch? diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index be0fa7e..52029bb 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -564,6 +564,7 @@ static void *__alloc_from_pool(size_t size, struct page **ret_page) *ret_page = phys_to_page(phys); ptr = (void *)val; + memset(ptr, 0, size); } > > Yes, your guess is right. the allocator is from pool. I show the full > stack as below when calling dma_zalloc_coherent > > [ 54.358310] [] (unwind_backtrace) from [] (show_stack+0x20/0x24) > [ 54.366012] [] (show_stack) from [] (dump_stack+0x98/0xac) > [ 54.373196] [] (dump_stack) from [] (pool_allocator_alloc+0x20/0x30) > [ 54.381238] [] (pool_allocator_alloc) from [] (__dma_alloc+0x1b8/0x344) > [ 54.389538] [] (__dma_alloc) from [] (arm_dma_alloc+0x50/0x58) > [ 54.397063] [] (arm_dma_alloc) from [] (mtk_open+0xf4/0x710) > [ 54.404416] [] (mtk_open) from [] (__dev_open+0xdc/0x160) > [ 54.411507] [] (__dev_open) from [] (__dev_change_flags+0x178/0x1c4) > [ 54.419547] [] (__dev_change_flags) from [] (dev_change_flags+0x28/0x58) > [ 54.427934] [] (dev_change_flags) from [] (devinet_ioctl+0x630/0x720) > [ 54.436062] [] (devinet_ioctl) from [] (inet_ioctl+0x210/0x3a8) > [ 54.443674] [] (inet_ioctl) from [] (sock_ioctl+0x234/0x4dc) > [ 54.451029] [] (sock_ioctl) from [] (do_vfs_ioctl+0xc0/0x914) > [ 54.458468] [] (do_vfs_ioctl) from [] (SyS_ioctl+0x44/0x6c) > [ 54.465733] [] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x54) > > > > > . >