Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1166292 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] stmmac: explicitly zero des0 & des1 on init Alexey Brodkin <Alexey.Brodkin@synopsys.com> - 2015-06-16 19:50 +0200
    Re: [PATCH] stmmac: explicitly zero des0 & des1 on init David Miller <davem@davemloft.net> - 2015-06-21 18:20 +0200
      Re: [PATCH] stmmac: explicitly zero des0 & des1 on init Alexey Brodkin <Alexey.Brodkin@synopsys.com> - 2015-06-22 08:50 +0200

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1169606

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().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1169735

Hi David,

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

Indeed usage of dma+AF8-zalloc+AF8-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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web