From mboxrd@z Thu Jan 1 00:00:00 1970 From: YueHaibing Subject: Re: [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset Date: Fri, 20 Jul 2018 20:13:02 +0800 Message-ID: <3fcf3f71-fd63-773b-c863-e4a7beade1ef@huawei.com> 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> <1532069663.8953.259.camel@mtkswgap22> <20180720092555.GA28941@n2100.armlinux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20180720092555.GA28941-l+eeeJia6m9URfEZ8mYm6t73F7V6hmMc@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+glpam-linux-mediatek=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Russell King - ARM Linux , Sean Wang Cc: nbd-p3rKhJxN3npAfugRpC6u6w@public.gmane.org, nelson.chang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, john-Pj+rj9U5foFAfugRpC6u6w@public.gmane.org, matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: linux-mediatek@lists.infradead.org On 2018/7/20 17:25, Russell King - ARM Linux wrote: > On Fri, Jul 20, 2018 at 02:54:23PM +0800, Sean Wang wrote: >> On Fri, 2018-07-20 at 14:30 +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? >>> >> >> I'm not sure if it's true for every armv7. or it's only happening on my >> device. >> >> anyway, i think we can replace all occurrences in the driver for >> dma_alloc_coherent with __GFP_ZERO by dma_zalloc_coherent, and but >> keep the extra memset as is. > > No, a bug in the allocator has been found that needs fixing. The > right solution is to fix the allocator and remove what should be > unnecessary memset()s. Agree, will send v2. > > This should have been reported when the __GFP_ZERO flag was not > being honoured and memset() was initially found to be required - > all that dma_zalloc_coherent() does is set the __GFP_ZERO flag > before calling dma_alloc_coherent(). >