* [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
@ 2018-07-19 14:09 ` YueHaibing
0 siblings, 0 replies; 27+ messages in thread
From: YueHaibing @ 2018-07-19 14:09 UTC (permalink / raw)
To: davem, nbd, john, sean.wang, nelson.chang
Cc: linux-kernel, netdev, matthias.bgg, linux-arm-kernel,
linux-mediatek, YueHaibing
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);
for (i = 0; i < MTK_DMA_SIZE; i++) {
int next = (i + 1) % MTK_DMA_SIZE;
u32 next_ptr = ring->phys + next * sz;
--
2.7.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
@ 2018-07-19 14:09 ` YueHaibing
0 siblings, 0 replies; 27+ messages in thread
From: YueHaibing @ 2018-07-19 14:09 UTC (permalink / raw)
To: davem, nbd, john, sean.wang, nelson.chang
Cc: linux-kernel, netdev, matthias.bgg, linux-arm-kernel,
linux-mediatek, YueHaibing
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);
for (i = 0; i < MTK_DMA_SIZE; i++) {
int next = (i + 1) % MTK_DMA_SIZE;
u32 next_ptr = ring->phys + next * sz;
--
2.7.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
@ 2018-07-19 14:09 ` YueHaibing
0 siblings, 0 replies; 27+ messages in thread
From: YueHaibing @ 2018-07-19 14:09 UTC (permalink / raw)
To: linux-arm-kernel
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);
for (i = 0; i < MTK_DMA_SIZE; i++) {
int next = (i + 1) % MTK_DMA_SIZE;
u32 next_ptr = ring->phys + next * sz;
--
2.7.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
2018-07-19 14:09 ` YueHaibing
@ 2018-07-19 14:17 ` Russell King - ARM Linux
-1 siblings, 0 replies; 27+ messages in thread
From: Russell King - ARM Linux @ 2018-07-19 14:17 UTC (permalink / raw)
To: YueHaibing
Cc: davem, nbd, john, sean.wang, nelson.chang, netdev, linux-kernel,
linux-mediatek, matthias.bgg, linux-arm-kernel
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. ;)
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
@ 2018-07-19 14:17 ` Russell King - ARM Linux
0 siblings, 0 replies; 27+ messages in thread
From: Russell King - ARM Linux @ 2018-07-19 14:17 UTC (permalink / raw)
To: linux-arm-kernel
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. ;)
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
2018-07-19 14:17 ` Russell King - ARM Linux
@ 2018-07-19 17:02 ` Sean Wang
-1 siblings, 0 replies; 27+ messages in thread
From: Sean Wang @ 2018-07-19 17:02 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: nbd, nelson.chang, netdev, YueHaibing, linux-kernel,
linux-mediatek, john, matthias.bgg, davem, linux-arm-kernel
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.
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
@ 2018-07-19 17:02 ` Sean Wang
0 siblings, 0 replies; 27+ messages in thread
From: Sean Wang @ 2018-07-19 17:02 UTC (permalink / raw)
To: linux-arm-kernel
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.
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
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
2018-07-19 17:02 ` Sean Wang
@ 2018-07-20 6:30 ` YueHaibing
-1 siblings, 0 replies; 27+ messages in thread
From: YueHaibing @ 2018-07-20 6:30 UTC (permalink / raw)
To: Sean Wang, Russell King - ARM Linux
Cc: nbd, nelson.chang, netdev, linux-kernel, linux-mediatek, john,
matthias.bgg, davem, linux-arm-kernel
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?
>
> 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
>
>
>
> .
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
@ 2018-07-20 6:30 ` YueHaibing
0 siblings, 0 replies; 27+ messages in thread
From: YueHaibing @ 2018-07-20 6:30 UTC (permalink / raw)
To: linux-arm-kernel
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?
>
> 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
>
>
>
> .
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
2018-07-20 6:30 ` YueHaibing
@ 2018-07-20 6:54 ` Sean Wang
-1 siblings, 0 replies; 27+ messages in thread
From: Sean Wang @ 2018-07-20 6:54 UTC (permalink / raw)
To: YueHaibing
Cc: nbd, nelson.chang, netdev, Russell King - ARM Linux,
linux-kernel, linux-mediatek, john, matthias.bgg, davem,
linux-arm-kernel
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
> >
> >
> >
> > .
> >
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
@ 2018-07-20 6:54 ` Sean Wang
0 siblings, 0 replies; 27+ messages in thread
From: Sean Wang @ 2018-07-20 6:54 UTC (permalink / raw)
To: linux-arm-kernel
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
> >
> >
> >
> > .
> >
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
2018-07-20 6:30 ` YueHaibing
@ 2018-07-20 8:13 ` Russell King - ARM Linux
-1 siblings, 0 replies; 27+ messages in thread
From: Russell King - ARM Linux @ 2018-07-20 8:13 UTC (permalink / raw)
To: YueHaibing
Cc: nbd-p3rKhJxN3npAfugRpC6u6w, nelson.chang-NuS5LvNUpcJWk0Htik3J/w,
netdev-u79uwXL29TY76Z2rM5mHXA, Sean Wang,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
john-Pj+rj9U5foFAfugRpC6u6w, matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w,
davem-fT/PcQaiUtIeIZ0/mPfg9Q,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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 <yuehaibing-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> >>> ---
> >>> 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.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
^ permalink raw reply [flat|nested] 27+ messages in thread
* [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
@ 2018-07-20 8:13 ` Russell King - ARM Linux
0 siblings, 0 replies; 27+ messages in thread
From: Russell King - ARM Linux @ 2018-07-20 8:13 UTC (permalink / raw)
To: linux-arm-kernel
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 <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?
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.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
2018-07-20 6:54 ` Sean Wang
@ 2018-07-20 9:25 ` Russell King - ARM Linux
-1 siblings, 0 replies; 27+ messages in thread
From: Russell King - ARM Linux @ 2018-07-20 9:25 UTC (permalink / raw)
To: Sean Wang
Cc: nbd, nelson.chang, netdev, YueHaibing, linux-kernel,
linux-mediatek, john, matthias.bgg, davem, linux-arm-kernel
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 <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.
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.
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().
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
^ permalink raw reply [flat|nested] 27+ messages in thread
* [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
@ 2018-07-20 9:25 ` Russell King - ARM Linux
0 siblings, 0 replies; 27+ messages in thread
From: Russell King - ARM Linux @ 2018-07-20 9:25 UTC (permalink / raw)
To: linux-arm-kernel
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 <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.
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.
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().
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
2018-07-20 9:25 ` Russell King - ARM Linux
@ 2018-07-20 12:13 ` YueHaibing
-1 siblings, 0 replies; 27+ messages in thread
From: YueHaibing @ 2018-07-20 12:13 UTC (permalink / raw)
To: Russell King - ARM Linux, Sean Wang
Cc: nbd-p3rKhJxN3npAfugRpC6u6w, nelson.chang-NuS5LvNUpcJWk0Htik3J/w,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
john-Pj+rj9U5foFAfugRpC6u6w, matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w,
davem-fT/PcQaiUtIeIZ0/mPfg9Q,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
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 <yuehaibing-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>>>>>> ---
>>>>>> 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().
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
@ 2018-07-20 12:13 ` YueHaibing
0 siblings, 0 replies; 27+ messages in thread
From: YueHaibing @ 2018-07-20 12:13 UTC (permalink / raw)
To: linux-arm-kernel
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 <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.
>
> 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().
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
2018-07-20 8:13 ` Russell King - ARM Linux
@ 2018-07-20 13:33 ` YueHaibing
-1 siblings, 0 replies; 27+ messages in thread
From: YueHaibing @ 2018-07-20 13:33 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: nbd-p3rKhJxN3npAfugRpC6u6w, nelson.chang-NuS5LvNUpcJWk0Htik3J/w,
netdev-u79uwXL29TY76Z2rM5mHXA, Sean Wang,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
john-Pj+rj9U5foFAfugRpC6u6w, matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w,
davem-fT/PcQaiUtIeIZ0/mPfg9Q,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On 2018/7/20 16:13, 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 <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?
>
> 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);
}
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
@ 2018-07-20 13:33 ` YueHaibing
0 siblings, 0 replies; 27+ messages in thread
From: YueHaibing @ 2018-07-20 13:33 UTC (permalink / raw)
To: linux-arm-kernel
On 2018/7/20 16:13, 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 <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?
>
> 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);
}
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
2018-07-20 8:13 ` Russell King - ARM Linux
@ 2018-07-20 13:48 ` Sean Wang
-1 siblings, 0 replies; 27+ messages in thread
From: Sean Wang @ 2018-07-20 13:48 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: nbd, nelson.chang, netdev, YueHaibing, linux-kernel,
linux-mediatek, john, matthias.bgg, davem, linux-arm-kernel
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 <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?
>
> 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.
>
Yes, your guess is right. the allocator is from pool. I show the full
stack as below when calling dma_zalloc_coherent
[ 54.358310] [<c0113ca8>] (unwind_backtrace) from [<c010e558>] (show_stack+0x20/0x24)
[ 54.366012] [<c010e558>] (show_stack) from [<c0927f58>] (dump_stack+0x98/0xac)
[ 54.373196] [<c0927f58>] (dump_stack) from [<c011c1f8>] (pool_allocator_alloc+0x20/0x30)
[ 54.381238] [<c011c1f8>] (pool_allocator_alloc) from [<c011a238>] (__dma_alloc+0x1b8/0x344)
[ 54.389538] [<c011a238>] (__dma_alloc) from [<c011a45c>] (arm_dma_alloc+0x50/0x58)
[ 54.397063] [<c011a45c>] (arm_dma_alloc) from [<c05dc208>] (mtk_open+0xf4/0x710)
[ 54.404416] [<c05dc208>] (mtk_open) from [<c076d110>] (__dev_open+0xdc/0x160)
[ 54.411507] [<c076d110>] (__dev_open) from [<c076d530>] (__dev_change_flags+0x178/0x1c4)
[ 54.419547] [<c076d530>] (__dev_change_flags) from [<c076d5a4>] (dev_change_flags+0x28/0x58)
[ 54.427934] [<c076d5a4>] (dev_change_flags) from [<c07ea7c0>] (devinet_ioctl+0x630/0x720)
[ 54.436062] [<c07ea7c0>] (devinet_ioctl) from [<c07ed070>] (inet_ioctl+0x210/0x3a8)
[ 54.443674] [<c07ed070>] (inet_ioctl) from [<c0747094>] (sock_ioctl+0x234/0x4dc)
[ 54.451029] [<c0747094>] (sock_ioctl) from [<c028206c>] (do_vfs_ioctl+0xc0/0x914)
[ 54.458468] [<c028206c>] (do_vfs_ioctl) from [<c0282904>] (SyS_ioctl+0x44/0x6c)
[ 54.465733] [<c0282904>] (SyS_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x54)
^ permalink raw reply [flat|nested] 27+ messages in thread
* [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
@ 2018-07-20 13:48 ` Sean Wang
0 siblings, 0 replies; 27+ messages in thread
From: Sean Wang @ 2018-07-20 13:48 UTC (permalink / raw)
To: linux-arm-kernel
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 <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?
>
> 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.
>
Yes, your guess is right. the allocator is from pool. I show the full
stack as below when calling dma_zalloc_coherent
[ 54.358310] [<c0113ca8>] (unwind_backtrace) from [<c010e558>] (show_stack+0x20/0x24)
[ 54.366012] [<c010e558>] (show_stack) from [<c0927f58>] (dump_stack+0x98/0xac)
[ 54.373196] [<c0927f58>] (dump_stack) from [<c011c1f8>] (pool_allocator_alloc+0x20/0x30)
[ 54.381238] [<c011c1f8>] (pool_allocator_alloc) from [<c011a238>] (__dma_alloc+0x1b8/0x344)
[ 54.389538] [<c011a238>] (__dma_alloc) from [<c011a45c>] (arm_dma_alloc+0x50/0x58)
[ 54.397063] [<c011a45c>] (arm_dma_alloc) from [<c05dc208>] (mtk_open+0xf4/0x710)
[ 54.404416] [<c05dc208>] (mtk_open) from [<c076d110>] (__dev_open+0xdc/0x160)
[ 54.411507] [<c076d110>] (__dev_open) from [<c076d530>] (__dev_change_flags+0x178/0x1c4)
[ 54.419547] [<c076d530>] (__dev_change_flags) from [<c076d5a4>] (dev_change_flags+0x28/0x58)
[ 54.427934] [<c076d5a4>] (dev_change_flags) from [<c07ea7c0>] (devinet_ioctl+0x630/0x720)
[ 54.436062] [<c07ea7c0>] (devinet_ioctl) from [<c07ed070>] (inet_ioctl+0x210/0x3a8)
[ 54.443674] [<c07ed070>] (inet_ioctl) from [<c0747094>] (sock_ioctl+0x234/0x4dc)
[ 54.451029] [<c0747094>] (sock_ioctl) from [<c028206c>] (do_vfs_ioctl+0xc0/0x914)
[ 54.458468] [<c028206c>] (do_vfs_ioctl) from [<c0282904>] (SyS_ioctl+0x44/0x6c)
[ 54.465733] [<c0282904>] (SyS_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x54)
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
2018-07-20 13:33 ` YueHaibing
@ 2018-07-20 13:57 ` Sean Wang
-1 siblings, 0 replies; 27+ messages in thread
From: Sean Wang @ 2018-07-20 13:57 UTC (permalink / raw)
To: YueHaibing
Cc: nbd, nelson.chang, netdev, Russell King - ARM Linux,
linux-kernel, linux-mediatek, john, matthias.bgg, davem,
linux-arm-kernel
On Fri, 2018-07-20 at 21:33 +0800, YueHaibing wrote:
> On 2018/7/20 16:13, 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 <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?
> >
> > 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);
> }
>
it should work. but don't we need to compare gfp against __GFP_ZERO
before zeros buffer ?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 27+ messages in thread
* [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
@ 2018-07-20 13:57 ` Sean Wang
0 siblings, 0 replies; 27+ messages in thread
From: Sean Wang @ 2018-07-20 13:57 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 2018-07-20 at 21:33 +0800, YueHaibing wrote:
> On 2018/7/20 16:13, 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 <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?
> >
> > 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);
> }
>
it should work. but don't we need to compare gfp against __GFP_ZERO
before zeros buffer ?
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
2018-07-20 13:48 ` Sean Wang
@ 2018-07-20 13:59 ` YueHaibing
-1 siblings, 0 replies; 27+ messages in thread
From: YueHaibing @ 2018-07-20 13:59 UTC (permalink / raw)
To: Sean Wang, Russell King - ARM Linux
Cc: nbd, nelson.chang, netdev, linux-kernel, linux-mediatek, john,
matthias.bgg, davem, linux-arm-kernel
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 <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?
>>
>> 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] [<c0113ca8>] (unwind_backtrace) from [<c010e558>] (show_stack+0x20/0x24)
> [ 54.366012] [<c010e558>] (show_stack) from [<c0927f58>] (dump_stack+0x98/0xac)
> [ 54.373196] [<c0927f58>] (dump_stack) from [<c011c1f8>] (pool_allocator_alloc+0x20/0x30)
> [ 54.381238] [<c011c1f8>] (pool_allocator_alloc) from [<c011a238>] (__dma_alloc+0x1b8/0x344)
> [ 54.389538] [<c011a238>] (__dma_alloc) from [<c011a45c>] (arm_dma_alloc+0x50/0x58)
> [ 54.397063] [<c011a45c>] (arm_dma_alloc) from [<c05dc208>] (mtk_open+0xf4/0x710)
> [ 54.404416] [<c05dc208>] (mtk_open) from [<c076d110>] (__dev_open+0xdc/0x160)
> [ 54.411507] [<c076d110>] (__dev_open) from [<c076d530>] (__dev_change_flags+0x178/0x1c4)
> [ 54.419547] [<c076d530>] (__dev_change_flags) from [<c076d5a4>] (dev_change_flags+0x28/0x58)
> [ 54.427934] [<c076d5a4>] (dev_change_flags) from [<c07ea7c0>] (devinet_ioctl+0x630/0x720)
> [ 54.436062] [<c07ea7c0>] (devinet_ioctl) from [<c07ed070>] (inet_ioctl+0x210/0x3a8)
> [ 54.443674] [<c07ed070>] (inet_ioctl) from [<c0747094>] (sock_ioctl+0x234/0x4dc)
> [ 54.451029] [<c0747094>] (sock_ioctl) from [<c028206c>] (do_vfs_ioctl+0xc0/0x914)
> [ 54.458468] [<c028206c>] (do_vfs_ioctl) from [<c0282904>] (SyS_ioctl+0x44/0x6c)
> [ 54.465733] [<c0282904>] (SyS_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x54)
>
>
>
>
> .
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
@ 2018-07-20 13:59 ` YueHaibing
0 siblings, 0 replies; 27+ messages in thread
From: YueHaibing @ 2018-07-20 13:59 UTC (permalink / raw)
To: linux-arm-kernel
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 <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?
>>
>> 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] [<c0113ca8>] (unwind_backtrace) from [<c010e558>] (show_stack+0x20/0x24)
> [ 54.366012] [<c010e558>] (show_stack) from [<c0927f58>] (dump_stack+0x98/0xac)
> [ 54.373196] [<c0927f58>] (dump_stack) from [<c011c1f8>] (pool_allocator_alloc+0x20/0x30)
> [ 54.381238] [<c011c1f8>] (pool_allocator_alloc) from [<c011a238>] (__dma_alloc+0x1b8/0x344)
> [ 54.389538] [<c011a238>] (__dma_alloc) from [<c011a45c>] (arm_dma_alloc+0x50/0x58)
> [ 54.397063] [<c011a45c>] (arm_dma_alloc) from [<c05dc208>] (mtk_open+0xf4/0x710)
> [ 54.404416] [<c05dc208>] (mtk_open) from [<c076d110>] (__dev_open+0xdc/0x160)
> [ 54.411507] [<c076d110>] (__dev_open) from [<c076d530>] (__dev_change_flags+0x178/0x1c4)
> [ 54.419547] [<c076d530>] (__dev_change_flags) from [<c076d5a4>] (dev_change_flags+0x28/0x58)
> [ 54.427934] [<c076d5a4>] (dev_change_flags) from [<c07ea7c0>] (devinet_ioctl+0x630/0x720)
> [ 54.436062] [<c07ea7c0>] (devinet_ioctl) from [<c07ed070>] (inet_ioctl+0x210/0x3a8)
> [ 54.443674] [<c07ed070>] (inet_ioctl) from [<c0747094>] (sock_ioctl+0x234/0x4dc)
> [ 54.451029] [<c0747094>] (sock_ioctl) from [<c028206c>] (do_vfs_ioctl+0xc0/0x914)
> [ 54.458468] [<c028206c>] (do_vfs_ioctl) from [<c0282904>] (SyS_ioctl+0x44/0x6c)
> [ 54.465733] [<c0282904>] (SyS_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x54)
>
>
>
>
> .
>
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
2018-07-20 13:57 ` Sean Wang
@ 2018-07-20 14:16 ` YueHaibing
-1 siblings, 0 replies; 27+ messages in thread
From: YueHaibing @ 2018-07-20 14:16 UTC (permalink / raw)
To: Sean Wang
Cc: nbd, nelson.chang, netdev, Russell King - ARM Linux,
linux-kernel, linux-mediatek, john, matthias.bgg, davem,
linux-arm-kernel
On 2018/7/20 21:57, Sean Wang wrote:
> On Fri, 2018-07-20 at 21:33 +0800, YueHaibing wrote:
>> On 2018/7/20 16:13, 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 <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?
>>>
>>> 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);
>> }
>>
>
> it should work. but don't we need to compare gfp against __GFP_ZERO
> before zeros buffer ?
>
arm64 zero it unconditionally in __alloc_from_pool.
>
>
> .
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 27+ messages in thread
* [SPAM]Re: [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset
@ 2018-07-20 14:16 ` YueHaibing
0 siblings, 0 replies; 27+ messages in thread
From: YueHaibing @ 2018-07-20 14:16 UTC (permalink / raw)
To: linux-arm-kernel
On 2018/7/20 21:57, Sean Wang wrote:
> On Fri, 2018-07-20 at 21:33 +0800, YueHaibing wrote:
>> On 2018/7/20 16:13, 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 <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?
>>>
>>> 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);
>> }
>>
>
> it should work. but don't we need to compare gfp against __GFP_ZERO
> before zeros buffer ?
>
arm64 zero it unconditionally in __alloc_from_pool.
>
>
> .
>
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2018-07-20 14:16 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-19 14:09 [PATCH] net: mediatek: use dma_zalloc_coherent instead of allocator/memset YueHaibing
2018-07-19 14:09 ` YueHaibing
2018-07-19 14:09 ` YueHaibing
2018-07-19 14:17 ` Russell King - ARM Linux
2018-07-19 14:17 ` Russell King - ARM Linux
2018-07-19 17:02 ` [SPAM]Re: " Sean Wang
2018-07-19 17:02 ` Sean Wang
2018-07-20 6:30 ` YueHaibing
2018-07-20 6:30 ` YueHaibing
2018-07-20 6:54 ` Sean Wang
2018-07-20 6:54 ` Sean Wang
2018-07-20 9:25 ` Russell King - ARM Linux
2018-07-20 9:25 ` Russell King - ARM Linux
[not found] ` <20180720092555.GA28941-l+eeeJia6m9URfEZ8mYm6t73F7V6hmMc@public.gmane.org>
2018-07-20 12:13 ` YueHaibing
2018-07-20 12:13 ` YueHaibing
[not found] ` <9a4cdfc6-7487-5908-b584-f3bf6158c4d4-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2018-07-20 8:13 ` Russell King - ARM Linux
2018-07-20 8:13 ` Russell King - ARM Linux
[not found] ` <20180720081334.GO17271-l+eeeJia6m9URfEZ8mYm6t73F7V6hmMc@public.gmane.org>
2018-07-20 13:33 ` YueHaibing
2018-07-20 13:33 ` YueHaibing
2018-07-20 13:57 ` Sean Wang
2018-07-20 13:57 ` Sean Wang
2018-07-20 14:16 ` YueHaibing
2018-07-20 14:16 ` YueHaibing
2018-07-20 13:48 ` Sean Wang
2018-07-20 13:48 ` Sean Wang
2018-07-20 13:59 ` YueHaibing
2018-07-20 13:59 ` YueHaibing
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.