[PATCH] stmmac: explicitly zero des0 & des1 on init

[PATCH] stmmac: explicitly zero des0 & des1 on init

[Alexey Brodkin]

Current implementtion of descriptor init procedure only takes care about
ownership flag. While it is perfectly possible to have underlying memory
filled with garbage on boot or driver installation.

And randomly set flags in non-zeroed des0 and des1 fields may lead to
unpredictable behavior of the GMAC DMA block.

Solution to this problem is as simple as explicit zeroing of both des0
and des1 fields of all buffer descriptors.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: arc-linux-dev@synopsys.com
Cc: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org
---
 drivers/net/ethernet/stmicro/stmmac/descs.h     | 2 ++
 drivers/net/ethernet/stmicro/stmmac/enh_desc.c  | 3 ++-
 drivers/net/ethernet/stmicro/stmmac/norm_desc.c | 3 ++-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/descs.h b/drivers/net/ethernet/stmicro/stmmac/descs.h
index ad39960..799c292 100644
--- a/drivers/net/ethernet/stmicro/stmmac/descs.h
+++ b/drivers/net/ethernet/stmicro/stmmac/descs.h
@@ -158,6 +158,8 @@ struct dma_desc {
 			u32 buffer2_size:13;
 			u32 reserved4:3;
 		} etx;		/* -- enhanced -- */
+
+		u64 all_flags;
 	} des01;
 	unsigned int des2;
 	unsigned int des3;
diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
index 1e2bcf5..7d94444 100644
--- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
@@ -240,6 +240,7 @@ static int enh_desc_get_rx_status(void *data, struct stmmac_extra_stats *x,
 static void enh_desc_init_rx_desc(struct dma_desc *p, int disable_rx_ic,
 				  int mode, int end)
 {
+	p->des01.all_flags = 0;
 	p->des01.erx.own = 1;
 	p->des01.erx.buffer1_size = BUF_SIZE_8KiB - 1;
 
@@ -254,7 +255,7 @@ static void enh_desc_init_rx_desc(struct dma_desc *p, int disable_rx_ic,
 
 static void enh_desc_init_tx_desc(struct dma_desc *p, int mode, int end)
 {
-	p->des01.etx.own = 0;
+	p->des01.all_flags = 0;
 	if (mode == STMMAC_CHAIN_MODE)
 		ehn_desc_tx_set_on_chain(p, end);
 	else
diff --git a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
index 35ad4f4..48c3456 100644
--- a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
@@ -123,6 +123,7 @@ static int ndesc_get_rx_status(void *data, struct stmmac_extra_stats *x,
 static void ndesc_init_rx_desc(struct dma_desc *p, int disable_rx_ic, int mode,
 			       int end)
 {
+	p->des01.all_flags = 0;
 	p->des01.rx.own = 1;
 	p->des01.rx.buffer1_size = BUF_SIZE_2KiB - 1;
 
@@ -137,7 +138,7 @@ static void ndesc_init_rx_desc(struct dma_desc *p, int disable_rx_ic, int mode,
 
 static void ndesc_init_tx_desc(struct dma_desc *p, int mode, int end)
 {
-	p->des01.tx.own = 0;
+	p->des01.all_flags = 0;
 	if (mode == STMMAC_CHAIN_MODE)
 		ndesc_tx_set_on_chain(p, end);
 	else
-- 
2.4.2

Re: [arc-linux-dev] [PATCH] stmmac: explicitly zero des0 & des1 on init

[Vineet Gupta]

+CC linux-arch, linux-mm, Arnd and Marek

On Tuesday 16 June 2015 11:11 PM, Alexey Brodkin wrote:

Current implementtion of descriptor init procedure only takes care about
ownership flag. While it is perfectly possible to have underlying memory
filled with garbage on boot or driver installation.

And randomly set flags in non-zeroed des0 and des1 fields may lead to
unpredictable behavior of the GMAC DMA block.

Solution to this problem is as simple as explicit zeroing of both des0
and des1 fields of all buffer descriptors.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com><mailto:abrodkin@synopsys.com>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com><mailto:peppe.cavallaro@st.com>
Cc: arc-linux-dev@synopsys.com<mailto:arc-linux-dev@synopsys.com>
Cc: linux-kernel@vger.kernel.org<mailto:linux-kernel@vger.kernel.org>
Cc: stable@vger.kernel.org<mailto:stable@vger.kernel.org>

FWIW, this was causing sporadic/random networking flakiness on ARC SDP platform (scheduled for upstream inclusion in next window)

This also leads to an interesting question - should arch/*/dma_alloc_coherent() and friends unconditionally zero out memory (vs. the current semantics of letting only doing it based on gfp, as requested by driver). This is the second instance we ran into stale descriptor memory, the first one was in dw_mmc driver which was recently fixed in upstream as well (although debugged independently by Alexey and using the upstream fix)

http://www.spinics.net/lists/linux-mmc/msg31600.html

The pros is better out of box experience (despite buggy drivers) while the cons are they remain broken and perhaps increased boot time due to extra memzero....

Thx,
-Vineet

Re: [arc-linux-dev] [PATCH] stmmac: explicitly zero des0 & des1 on init

[Alexey Brodkin]

Hi all,

On Wed, 2015-06-17 at 07:03 +0000, Vineet Gupta wrote:
+CC linux-arch, linux-mm, Arnd and Marek

On Tuesday 16 June 2015 11:11 PM, Alexey Brodkin wrote:

Current implementtion of descriptor init procedure only takes care about
ownership flag. While it is perfectly possible to have underlying memory
filled with garbage on boot or driver installation.

And randomly set flags in non-zeroed des0 and des1 fields may lead to
unpredictable behavior of the GMAC DMA block.

Solution to this problem is as simple as explicit zeroing of both des0
and des1 fields of all buffer descriptors.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com><mailto:abrodkin@synopsys.com>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com><mailto:peppe.cavallaro@st.com>
Cc: arc-linux-dev@synopsys.com<mailto:arc-linux-dev@synopsys.com>
Cc: linux-kernel@vger.kernel.org<mailto:linux-kernel@vger.kernel.org>
Cc: stable@vger.kernel.org<mailto:stable@vger.kernel.org>

FWIW, this was causing sporadic/random networking flakiness on ARC SDP platform (scheduled for upstream inclusion in next window)

This also leads to an interesting question - should arch/*/dma_alloc_coherent() and friends unconditionally zero out memory (vs. the current semantics of letting only doing it based on gfp, as requested by driver). This is the second instance we ran into stale descriptor memory, the first one was in dw_mmc driver which was recently fixed in upstream as well (although debugged independently by Alexey and using the upstream fix)

http://www.spinics.net/lists/linux-mmc/msg31600.html

The pros is better out of box experience (despite buggy drivers) while the cons are they remain broken and perhaps increased boot time due to extra memzero....

Probably if we already have dma_zalloc_coherent() that does explicit zeroing of returned memory then there's no need to do implicit zeroing in dma_alloc_coherent()?

-Alexey

Re: [arc-linux-dev] [PATCH] stmmac: explicitly zero des0 & des1 on init

[Vineet Gupta]

On Monday 22 June 2015 01:38 PM, Alexey Brodkin wrote:
> Hi all,
> 
> On Wed, 2015-06-17 at 07:03 +0000, Vineet Gupta wrote:
> +CC linux-arch, linux-mm, Arnd and Marek
> 
> On Tuesday 16 June 2015 11:11 PM, Alexey Brodkin wrote:
> 
> Current implementtion of descriptor init procedure only takes care about
> ownership flag. While it is perfectly possible to have underlying memory
> filled with garbage on boot or driver installation.
> 
> And randomly set flags in non-zeroed des0 and des1 fields may lead to
> unpredictable behavior of the GMAC DMA block.
> 
> Solution to this problem is as simple as explicit zeroing of both des0
> and des1 fields of all buffer descriptors.
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com><mailto:abrodkin@synopsys.com>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com><mailto:peppe.cavallaro@st.com>
> Cc: arc-linux-dev@synopsys.com<mailto:arc-linux-dev@synopsys.com>
> Cc: linux-kernel@vger.kernel.org<mailto:linux-kernel@vger.kernel.org>
> Cc: stable@vger.kernel.org<mailto:stable@vger.kernel.org>
> 
> FWIW, this was causing sporadic/random networking flakiness on ARC SDP platform (scheduled for upstream inclusion in next window)
> 
> This also leads to an interesting question - should arch/*/dma_alloc_coherent() and friends unconditionally zero out memory (vs. the current semantics of letting only doing it based on gfp, as requested by driver). This is the second instance we ran into stale descriptor memory, the first one was in dw_mmc driver which was recently fixed in upstream as well (although debugged independently by Alexey and using the upstream fix)
> 
> http://www.spinics.net/lists/linux-mmc/msg31600.html
> 
> The pros is better out of box experience (despite buggy drivers) while the cons are they remain broken and perhaps increased boot time due to extra memzero....
> 
> Probably if we already have dma_zalloc_coherent() that does explicit zeroing of returned memory then there's no need to do implicit zeroing in dma_alloc_coherent()?


The question is, when drivers don't have dma_zalloc_coherent() - meaning they
don't pass __GFP_ZERO, which causes these random issues, do we need to be more
conservative in arch code (ARC at least is) or do we need to debug and fix these
drivers - one by one.

FWIW, ARC needs to fix __GFP_ZERO case, since we are doing memzero twice.

-Vineet

Re: [PATCH] stmmac: explicitly zero des0 & des1 on init

[David Miller]

From: Alexey Brodkin <Alexey.Brodkin@synopsys.com>
Date: Tue, 16 Jun 2015 20:40:41 +0300

> Current implementtion of descriptor init procedure only takes care about
> ownership flag. While it is perfectly possible to have underlying memory
> filled with garbage on boot or driver installation.
> 
> And randomly set flags in non-zeroed des0 and des1 fields may lead to
> unpredictable behavior of the GMAC DMA block.
> 
> Solution to this problem is as simple as explicit zeroing of both des0
> and des1 fields of all buffer descriptors.
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>

If you need the memory zero initialized, use dma_zalloc_coherent().

Re: [PATCH] stmmac: explicitly zero des0 & des1 on init

[Alexey Brodkin]

Hi David,

On Sun, 2015-06-21 at 09:29 -0700, David Miller wrote:
> From: Alexey Brodkin <Alexey.Brodkin@synopsys.com>
> Date: Tue, 16 Jun 2015 20:40:41 +0300
> 
> > Current implementtion of descriptor init procedure only takes care 
> > about
> > ownership flag. While it is perfectly possible to have underlying 
> > memory
> > filled with garbage on boot or driver installation.
> > 
> > And randomly set flags in non-zeroed des0 and des1 fields may lead 
> > to
> > unpredictable behavior of the GMAC DMA block.
> > 
> > Solution to this problem is as simple as explicit zeroing of both 
> > des0
> > and des1 fields of all buffer descriptors.
> > 
> > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> 
> If you need the memory zero initialized, use dma_zalloc_coherent().

Indeed usage of dma_zalloc_coherent() will resolve observed issue.
But since buffer descriptors are reused extensively I would say that
explicit zeroing of fields with flags is useful. Probably I need to add
this clarification in commit message.

And then if we do that explicit zeroing of flags and other fields which
hold data size and addresses of data buffer and the next descriptor in
chain are all get set later we may not care about allocation of zeroed
memory.

-Alexey--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] stmmac: explicitly zero des0 & des1 on init

[Alexey Brodkin]

Hi David,

On Mon, 2015-06-22 at 09:43 +0300, Alexey Brodkin wrote:
> Hi David,
> 
> On Sun, 2015-06-21 at 09:29 -0700, David Miller wrote:
> > From: Alexey Brodkin <Alexey.Brodkin@synopsys.com>
> > Date: Tue, 16 Jun 2015 20:40:41 +0300
> > 
> > > Current implementtion of descriptor init procedure only takes 
> > > care 
> > > about
> > > ownership flag. While it is perfectly possible to have underlying 
> > > 
> > > memory
> > > filled with garbage on boot or driver installation.
> > > 
> > > And randomly set flags in non-zeroed des0 and des1 fields may 
> > > lead 
> > > to
> > > unpredictable behavior of the GMAC DMA block.
> > > 
> > > Solution to this problem is as simple as explicit zeroing of both 
> > > 
> > > des0
> > > and des1 fields of all buffer descriptors.
> > > 
> > > Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> > 
> > If you need the memory zero initialized, use dma_zalloc_coherent().
> 
> Indeed usage of dma_zalloc_coherent() will resolve observed issue.
> But since buffer descriptors are reused extensively I would say that
> explicit zeroing of fields with flags is useful. Probably I need to 
> add
> this clarification in commit message.
> 
> And then if we do that explicit zeroing of flags and other fields 
> which
> hold data size and addresses of data buffer and the next descriptor 
> in
> chain are all get set later we may not care about allocation of 
> zeroed
> memory.

I'm wondering if my comment makes sense and should I just change commit
message or you'd prefer to still use dma_zalloc_coherent() during
driver probe?

-Alexey

Re: [PATCH] stmmac: explicitly zero des0 & des1 on init

[David Miller]

From: Alexey Brodkin <Alexey.Brodkin@synopsys.com>
Date: Tue, 23 Jun 2015 15:04:31 +0000

> I'm wondering if my comment makes sense and should I just change commit
> message or you'd prefer to still use dma_zalloc_coherent() during
> driver probe?

I think you should do both, convert to dma_zalloc_coherent() and perform
the explicit clearing in the routine in question.

[PATCH v2] stmmac: troubleshoot unexpected bits in des0 & des1

[Alexey Brodkin]

Current implementation of descriptor init procedure only takes
care about setting/clearing ownership flag in "des0"/"des1"
fields while it is perfectly possible to get unexpected bits
set because of the following factors:

 [1] On driver probe underlying memory allocated with
     dma_alloc_coherent() might not be zeroed and so
     it will be filled with garbage.

 [2] During driver operation some bits could be set by SD/MMC
     controller (for example error flags etc).

And unexpected and/or randomly set flags in "des0"/"des1"
fields may lead to unpredictable behavior of GMAC DMA block.

This change addresses both items above with:

 [1] Use of dma_zalloc_coherent() instead of simple
     dma_alloc_coherent() to make sure allocated memory is
     zeroed. That shouldn't affect performance because
     this allocation only happens once on driver probe.

 [2] Do explicit zeroing of both "des0" and "des1" fields
     of all buffer descriptors during initialization of
     DMA transfer.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: arc-linux-dev@synopsys.com
Cc: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org
Cc: David Miller <davem@davemloft.net>
---
 drivers/net/ethernet/stmicro/stmmac/descs.h       | 2 ++
 drivers/net/ethernet/stmicro/stmmac/enh_desc.c    | 3 ++-
 drivers/net/ethernet/stmicro/stmmac/norm_desc.c   | 3 ++-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++----
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/descs.h b/drivers/net/ethernet/stmicro/stmmac/descs.h
index ad39960..799c292 100644
--- a/drivers/net/ethernet/stmicro/stmmac/descs.h
+++ b/drivers/net/ethernet/stmicro/stmmac/descs.h
@@ -158,6 +158,8 @@ struct dma_desc {
 			u32 buffer2_size:13;
 			u32 reserved4:3;
 		} etx;		/* -- enhanced -- */
+
+		u64 all_flags;
 	} des01;
 	unsigned int des2;
 	unsigned int des3;
diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
index 1e2bcf5..7d94444 100644
--- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
@@ -240,6 +240,7 @@ static int enh_desc_get_rx_status(void *data, struct stmmac_extra_stats *x,
 static void enh_desc_init_rx_desc(struct dma_desc *p, int disable_rx_ic,
 				  int mode, int end)
 {
+	p->des01.all_flags = 0;
 	p->des01.erx.own = 1;
 	p->des01.erx.buffer1_size = BUF_SIZE_8KiB - 1;
 
@@ -254,7 +255,7 @@ static void enh_desc_init_rx_desc(struct dma_desc *p, int disable_rx_ic,
 
 static void enh_desc_init_tx_desc(struct dma_desc *p, int mode, int end)
 {
-	p->des01.etx.own = 0;
+	p->des01.all_flags = 0;
 	if (mode == STMMAC_CHAIN_MODE)
 		ehn_desc_tx_set_on_chain(p, end);
 	else
diff --git a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
index 35ad4f4..48c3456 100644
--- a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
@@ -123,6 +123,7 @@ static int ndesc_get_rx_status(void *data, struct stmmac_extra_stats *x,
 static void ndesc_init_rx_desc(struct dma_desc *p, int disable_rx_ic, int mode,
 			       int end)
 {
+	p->des01.all_flags = 0;
 	p->des01.rx.own = 1;
 	p->des01.rx.buffer1_size = BUF_SIZE_2KiB - 1;
 
@@ -137,7 +138,7 @@ static void ndesc_init_rx_desc(struct dma_desc *p, int disable_rx_ic, int mode,
 
 static void ndesc_init_tx_desc(struct dma_desc *p, int mode, int end)
 {
-	p->des01.tx.own = 0;
+	p->des01.all_flags = 0;
 	if (mode == STMMAC_CHAIN_MODE)
 		ndesc_tx_set_on_chain(p, end);
 	else
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 18c46bb..3309b98 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1172,7 +1172,7 @@ static int alloc_dma_desc_resources(struct stmmac_priv *priv)
 		goto err_tx_skbuff;
 
 	if (priv->extend_desc) {
-		priv->dma_erx = dma_alloc_coherent(priv->device, rxsize *
+		priv->dma_erx = dma_zalloc_coherent(priv->device, rxsize *
 						   sizeof(struct
 							  dma_extended_desc),
 						   &priv->dma_rx_phy,
@@ -1180,7 +1180,7 @@ static int alloc_dma_desc_resources(struct stmmac_priv *priv)
 		if (!priv->dma_erx)
 			goto err_dma;
 
-		priv->dma_etx = dma_alloc_coherent(priv->device, txsize *
+		priv->dma_etx = dma_zalloc_coherent(priv->device, txsize *
 						   sizeof(struct
 							  dma_extended_desc),
 						   &priv->dma_tx_phy,
@@ -1192,14 +1192,14 @@ static int alloc_dma_desc_resources(struct stmmac_priv *priv)
 			goto err_dma;
 		}
 	} else {
-		priv->dma_rx = dma_alloc_coherent(priv->device, rxsize *
+		priv->dma_rx = dma_zalloc_coherent(priv->device, rxsize *
 						  sizeof(struct dma_desc),
 						  &priv->dma_rx_phy,
 						  GFP_KERNEL);
 		if (!priv->dma_rx)
 			goto err_dma;
 
-		priv->dma_tx = dma_alloc_coherent(priv->device, txsize *
+		priv->dma_tx = dma_zalloc_coherent(priv->device, txsize *
 						  sizeof(struct dma_desc),
 						  &priv->dma_tx_phy,
 						  GFP_KERNEL);
-- 
2.4.2

Re: [PATCH v2] stmmac: troubleshoot unexpected bits in des0 & des1

[David Miller]

From: Alexey Brodkin <Alexey.Brodkin@synopsys.com>
Date: Wed, 24 Jun 2015 11:07:26 +0300

>  
> -		priv->dma_tx = dma_alloc_coherent(priv->device, txsize *
> +		priv->dma_tx = dma_zalloc_coherent(priv->device, txsize *
>  						  sizeof(struct dma_desc),
>  						  &priv->dma_tx_phy,
>  						  GFP_KERNEL);

When you change the column at which the openning parenthesis of the
function call occurs, you must indent the subsequent lines of the
function call such that the all are adjusted to properly start
at the very next column after that openning parenthesis.'

Please fix this up for all of the dma_zalloc_coherent()
transformations.

Thanks.

Re: [PATCH v2] stmmac: troubleshoot unexpected bits in des0 & des1

[Alexey Brodkin]

Hi David,

On Wed, 2015-06-24 at 01:44 -0700, David Miller wrote:
> From: Alexey Brodkin <Alexey.Brodkin@synopsys.com>
> Date: Wed, 24 Jun 2015 11:07:26 +0300
> 
> >  
> > -           priv->dma_tx = dma_alloc_coherent(priv->device, 
> > txsize *
> > +           priv->dma_tx = dma_zalloc_coherent(priv->device, 
> > txsize *
> >                                               sizeof(struct 
> > dma_desc),
> >                                               &priv
> > ->dma_tx_phy,
> >                                               GFP_KERNEL);
> 
> When you change the column at which the openning parenthesis of the
> function call occurs, you must indent the subsequent lines of the
> function call such that the all are adjusted to properly start
> at the very next column after that openning parenthesis.'
> 
> Please fix this up for all of the dma_zalloc_coherent()
> transformations.

I thought about that and actually made this change initially but later
reverted it because I noticed dma_free_coherent() had parameters not
-aligned/idented properly.

So do you think if I may fix identation of dma_free_coherent() while at
it as well or I need to send a separate patch that takes care about
dma_free_coherent() solely?

-Alexey