Re: [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset

[Sean Wang <sean.wang@mediatek.com>]

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 <yuehaibing@huawei.com>
> >>> ---
> >>>  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.
	
	Sean

> > 
> > my test code is
> > 
> >          ring->dma = dma_zalloc_coherent(eth->dev,
> >                                           MTK_DMA_SIZE * sz,
> >                                           &ring->phys,
> >                                           GFP_ATOMIC);
> >         if (!ring->dma)
> >                 goto no_tx_mem;
> > 
> >         print_hex_dump(KERN_INFO, "mtk_tx_alloc:",
> >                        DUMP_PREFIX_OFFSET, 16, 1,
> >                        ring->dma, MTK_DMA_SIZE * sz, false);
> > 
> >         memset(ring->dma, 0, MTK_DMA_SIZE * sz);
> > 
> >         print_hex_dump(KERN_INFO, "mtk_tx_alloc2:",
> >                        DUMP_PREFIX_OFFSET, 16, 1,
> >                        ring->dma, MTK_DMA_SIZE * sz, false);
> > 
> > 
> > and the output 
> > 
> > ...
> > 
> > [  259.610413] mtk_tx_alloc:00000f40: 00 00 00 00 50 1f 00 bc 00 00 00 c0 00 00 00 00
> > [  259.617934] mtk_tx_alloc:00000f50: 00 00 00 00 60 1f 00 bc 00 00 00 c0 00 00 00 00
> > [  259.625470] mtk_tx_alloc:00000f60: 00 00 00 00 70 1f 00 bc 00 00 00 c0 00 00 00 00
> > [  259.633005] mtk_tx_alloc:00000f70: 00 00 00 00 80 1f 00 bc 00 00 00 c0 00 00 00 00
> > [  259.640539] mtk_tx_alloc:00000f80: 00 00 00 00 90 1f 00 bc 00 00 00 c0 00 00 00 00
> > [  259.648073] mtk_tx_alloc:00000f90: 00 00 00 00 a0 1f 00 bc 00 00 00 c0 00 00 00 00
> > [  259.655590] mtk_tx_alloc:00000fa0: 00 00 00 00 b0 1f 00 bc 00 00 00 c0 00 00 00 00
> > [  259.663124] mtk_tx_alloc:00000fb0: 00 00 00 00 c0 1f 00 bc 00 00 00 c0 00 00 00 00
> > [  259.670660] mtk_tx_alloc:00000fc0: 00 00 00 00 d0 1f 00 bc 00 00 00 c0 00 00 00 00
> > [  259.678196] mtk_tx_alloc:00000fd0: 00 00 00 00 e0 1f 00 bc 00 00 00 c0 00 00 00 00
> > [  259.685713] mtk_tx_alloc:00000fe0: 00 00 00 00 f0 1f 00 bc 00 00 00 c0 00 00 00 00
> > [  259.693247] mtk_tx_alloc:00000ff0: 02 c0 90 ad 00 10 00 bc 00 40 5a c0 00 00 00 02
> > [  259.700782] mtk_tx_alloc2:00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > [  259.708405] mtk_tx_alloc2:00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > [  259.716013] mtk_tx_alloc2:00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > [  259.723634] mtk_tx_alloc2:00000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > [  259.731253] mtk_tx_alloc2:00000040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > [  259.738875] mtk_tx_alloc2:00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > [  259.746481] mtk_tx_alloc2:00000060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > [  259.754103] mtk_tx_alloc2:00000070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > [  259.761723] mtk_tx_alloc2:00000080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > [  259.769344] mtk_tx_alloc2:00000090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > [  259.776951] mtk_tx_alloc2:000000a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 
> > ....
> > 
> > 
> > 	Sean
> > 
> > 
> > 
> > .
> > 
>