[PATCH 1/3] net: lpc_eth: Replace WARN() trace with simple pr_warn()

A WARN() trace indicating a "BUG!" was identified as a "normal" case in the
xmit function in case all TX descriptors are occupied already. In this case,
NETDEV_TX_BUSY is returned, nothing buggy at all.

Signed-off-by: Roland Stigge <redacted>
Tested-by: Alexandre Pereira da Silva <redacted>

---
 drivers/net/ethernet/nxp/lpc_eth.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-2.6.orig/drivers/net/ethernet/nxp/lpc_eth.c
+++ linux-2.6/drivers/net/ethernet/nxp/lpc_eth.c

@@ -1114,7 +1114,7 @@ static int lpc_eth_hard_start_xmit(struc
 		   buffers */
 		netif_stop_queue(ndev);
 		spin_unlock_irq(&pldat->lock);
-		WARN(1, "BUG! TX request when no free TX buffers!\n");
+		pr_warn("Note: TX request when no free TX buffers.\n");
 		return NETDEV_TX_BUSY;
 	}
 

Since we have enough SRAM, we can increase the number of TX descriptors, so the
"BUSY" warning about occupied TX descriptors doesn't need to show up as often.

Signed-off-by: Roland Stigge <redacted>
Tested-by: Alexandre Pereira da Silva <redacted>

---
 drivers/net/ethernet/nxp/lpc_eth.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-2.6.orig/drivers/net/ethernet/nxp/lpc_eth.c
+++ linux-2.6/drivers/net/ethernet/nxp/lpc_eth.c

@@ -56,7 +56,7 @@
 
 #define ENET_MAXF_SIZE 1536
 #define ENET_RX_DESC 48
-#define ENET_TX_DESC 16
+#define ENET_TX_DESC 32
 
 #define NAPI_WEIGHT 16
 

This patch removes some nowadays superfluous definitions (one unused define and
an obsolete function forward declaration) and corrects a netdev_err() to
netdev_dbg().

Signed-off-by: Roland Stigge <redacted>
Signed-off-by: Alexandre Pereira da Silva <redacted>

---
 drivers/net/ethernet/nxp/lpc_eth.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

--- linux-2.6.orig/drivers/net/ethernet/nxp/lpc_eth.c
+++ linux-2.6/drivers/net/ethernet/nxp/lpc_eth.c

@@ -52,7 +52,6 @@
 
 #define MODNAME "lpc-eth"
 #define DRV_VERSION "1.00"
-#define PHYDEF_ADDR 0x00
 
 #define ENET_MAXF_SIZE 1536
 #define ENET_RX_DESC 48

@@ -416,9 +415,6 @@ static bool use_iram_for_net(struct devi
 #define TXDESC_CONTROL_LAST		(1 << 30)
 #define TXDESC_CONTROL_INT		(1 << 31)
 
-static int lpc_eth_hard_start_xmit(struct sk_buff *skb,
-				   struct net_device *ndev);
-
 /*
  * Structure of a TX/RX descriptors and RX status
  */

@@ -1441,7 +1437,7 @@ static int lpc_eth_drv_probe(struct plat
 			res->start);
 	netdev_dbg(ndev, "IO address size      :%d\n",
 			res->end - res->start + 1);
-	netdev_err(ndev, "IO address (mapped)  :0x%p\n",
+	netdev_dbg(ndev, "IO address (mapped)  :0x%p\n",
 			pldat->net_base);
 	netdev_dbg(ndev, "IRQ number           :%d\n", ndev->irq);
 	netdev_dbg(ndev, "DMA buffer size      :%d\n", pldat->dma_buff_size);

From: Roland Stigge <redacted>
Date: Mon, 11 Jun 2012 10:03:11 +0200

A WARN() trace indicating a "BUG!" was identified as a "normal" case in the
xmit function in case all TX descriptors are occupied already. In this case,
NETDEV_TX_BUSY is returned, nothing buggy at all.

Signed-off-by: Roland Stigge <redacted>
Tested-by: Alexandre Pereira da Silva <redacted>

This is not normal.

Read the comment above this code you are changing.  If we are
out of TX descriptors, the queue must be stopped, and therefore
if the queue is stopped this transmit method should not be
invoked.

It is a hard error condition, should never occur, and indicates
a very serious error condition in the driver.

From: Roland Stigge <redacted>
Date: Mon, 11 Jun 2012 10:03:12 +0200

Since we have enough SRAM, we can increase the number of TX descriptors, so the
"BUSY" warning about occupied TX descriptors doesn't need to show up as often.

Signed-off-by: Roland Stigge <redacted>
Tested-by: Alexandre Pereira da Silva <redacted>

This is way too terse, and as I described in my reply to your first
patch it is not normal for the transmit function to be invoked when
there are no TX descriptors available.  That's a bug if it is happening.

On Mon, 2012-06-11 at 10:03 +0200, Roland Stigge wrote:

Sorry, this is not the right fix.

You only lower probability of the bug.

What is this BUSY warning mentioned in changelog ?

On Mon, 2012-06-11 at 10:03 +0200, Roland Stigge wrote:

Entering this path is a bug, don't hide it...

Please share with us how this bug was identified as a "normal case" ?

Hi Dave and Eric,

thanks for your feedback!

On 06/11/2012 10:25 AM, Eric Dumazet wrote:

On Mon, 2012-06-11 at 10:03 +0200, Roland Stigge wrote:

quoted

A WARN() trace indicating a "BUG!" was identified as a "normal" case in the
xmit function in case all TX descriptors are occupied already. In this case,
NETDEV_TX_BUSY is returned, nothing buggy at all.

Signed-off-by: Roland Stigge <redacted>
Tested-by: Alexandre Pereira da Silva <redacted>

---
 drivers/net/ethernet/nxp/lpc_eth.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-2.6.orig/drivers/net/ethernet/nxp/lpc_eth.c
+++ linux-2.6/drivers/net/ethernet/nxp/lpc_eth.c

@@ -1114,7 +1114,7 @@ static int lpc_eth_hard_start_xmit(struc
 		   buffers */
 		netif_stop_queue(ndev);
 		spin_unlock_irq(&pldat->lock);
-		WARN(1, "BUG! TX request when no free TX buffers!\n");
+		pr_warn("Note: TX request when no free TX buffers.\n");
 		return NETDEV_TX_BUSY;
 	}
 

Entering this path is a bug, don't hide it...

Please share with us how this bug was identified as a "normal case" ?

I encountered cases where this happened for me on a custom board under
heavy load.

I discussed this with Kevin Wells, the original driver author. We
identified the case of xmit()'s TX request (from .ndo_start_xmit) with
full TX driver buffers as valid when ethernet is busy.

But maybe this is wrong. Can you please give me a hint how the net
subsystem makes sure that this doesn't happen under normal circumstances?

Thanks in advance!

Roland

On Mon, 2012-06-11 at 10:25 +0200, Eric Dumazet wrote:

On Mon, 2012-06-11 at 10:03 +0200, Roland Stigge wrote:

quoted

A WARN() trace indicating a "BUG!" was identified as a "normal" case in the
xmit function in case all TX descriptors are occupied already. In this case,
NETDEV_TX_BUSY is returned, nothing buggy at all.

Signed-off-by: Roland Stigge <redacted>
Tested-by: Alexandre Pereira da Silva <redacted>

---
 drivers/net/ethernet/nxp/lpc_eth.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-2.6.orig/drivers/net/ethernet/nxp/lpc_eth.c
+++ linux-2.6/drivers/net/ethernet/nxp/lpc_eth.c

@@ -1114,7 +1114,7 @@ static int lpc_eth_hard_start_xmit(struc
 		   buffers */
 		netif_stop_queue(ndev);
 		spin_unlock_irq(&pldat->lock);
-		WARN(1, "BUG! TX request when no free TX buffers!\n");
+		pr_warn("Note: TX request when no free TX buffers.\n");
 		return NETDEV_TX_BUSY;
 	}
 

Entering this path is a bug, don't hide it...

Please share with us how this bug was identified as a "normal case" ?

There is an skb leak in this driver, maybe it's the real problem.

diff --git a/drivers/net/ethernet/nxp/lpc_eth.c b/drivers/net/ethernet/nxp/lpc_eth.c
index 8d2666f..0d0f4cb 100644
--- a/drivers/net/ethernet/nxp/lpc_eth.c
+++ b/drivers/net/ethernet/nxp/lpc_eth.c

@@ -946,10 +946,8 @@ static void __lpc_handle_xmit(struct net_device *ndev)
 			/* Update stats */
 			ndev->stats.tx_packets++;
 			ndev->stats.tx_bytes += skb->len;
-
-			/* Free buffer */
-			dev_kfree_skb_irq(skb);
 		}
+		dev_kfree_skb_irq(skb);
 
 		txcidx = readl(LPC_ENET_TXCONSUMEINDEX(pldat->net_base));
 	}

On Mon, 2012-06-11 at 10:36 +0200, Roland Stigge wrote:

I encountered cases where this happened for me on a custom board under
heavy load.

I discussed this with Kevin Wells, the original driver author. We
identified the case of xmit()'s TX request (from .ndo_start_xmit) with
full TX driver buffers as valid when ethernet is busy.

But maybe this is wrong. Can you please give me a hint how the net
subsystem makes sure that this doesn't happen under normal circumstances?

When TX ring is about to be filler, driver lpc_eth_hard_start_xmit()
calls netif_stop_queue(ndev);

So network stack should not call again lpc_eth_hard_start_xmit().

I would say the bug(s) come from __lpc_handle_xmit(), since it does :

if (netif_queue_stopped(ndev))
	netif_wake_queue(ndev);

without making sure some room is available in TX ring.

cumulative patch :

diff --git a/drivers/net/ethernet/nxp/lpc_eth.c b/drivers/net/ethernet/nxp/lpc_eth.c
index 8d2666f..59b37c8 100644
--- a/drivers/net/ethernet/nxp/lpc_eth.c
+++ b/drivers/net/ethernet/nxp/lpc_eth.c

@@ -946,16 +946,16 @@ static void __lpc_handle_xmit(struct net_device *ndev)
 			/* Update stats */
 			ndev->stats.tx_packets++;
 			ndev->stats.tx_bytes += skb->len;
-
-			/* Free buffer */
-			dev_kfree_skb_irq(skb);
 		}
+		dev_kfree_skb_irq(skb);
 
 		txcidx = readl(LPC_ENET_TXCONSUMEINDEX(pldat->net_base));
 	}
 
-	if (netif_queue_stopped(ndev))
-		netif_wake_queue(ndev);
+	if (pldat->num_used_tx_buffs <= ENET_TX_DESC/2) { 
+		if (netif_queue_stopped(ndev))
+			netif_wake_queue(ndev);
+	}
 }
 
 static int __lpc_handle_recv(struct net_device *ndev, int budget)

From: Roland Stigge <redacted>
Date: Mon, 11 Jun 2012 10:36:45 +0200

But maybe this is wrong. Can you please give me a hint how the net
subsystem makes sure that this doesn't happen under normal circumstances?

Well if you are asking this question then you didn't read my feedback,
because I explained exactly what prevents this.

Hi!

On 06/11/2012 11:03 AM, David Miller wrote:

From: Roland Stigge <redacted>
Date: Mon, 11 Jun 2012 10:36:45 +0200

quoted

But maybe this is wrong. Can you please give me a hint how the net
subsystem makes sure that this doesn't happen under normal circumstances?

Well if you are asking this question then you didn't read my feedback,
because I explained exactly what prevents this.

Re-reading your feedback, you are right, sorry!

My question was based on the assumption that the driver is doing
correctly, which was wrong.

Thank you and Eric for clarifying!

Eric's second (cumulative) patch works fine for now, and I can't
reproduce the issue. Will do more test runs now and will reply back
later with an updated patch set.

Is it sensible at this point to increase the TX buffers anyway? For
different reasons of course: We have enough SRAM available and TX
buffers (16->32) are still more than RX buffers (48).

Roland

From: Eric Dumazet <edumazet@google.com>

__lpc_handle_xmit() has two bugs :

1) It can leak skbs in case TXSTATUS_ERROR is set

2) It can wake up txqueue while no slot was freed.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Roland Stigge <redacted>
Tested-by: Roland Stigge <redacted>
Cc: Kevin Wells <redacted>
---
 drivers/net/ethernet/nxp/lpc_eth.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/nxp/lpc_eth.c b/drivers/net/ethernet/nxp/lpc_eth.c
index 8d2666f..ff76c21 100644
--- a/drivers/net/ethernet/nxp/lpc_eth.c
+++ b/drivers/net/ethernet/nxp/lpc_eth.c

@@ -946,16 +946,16 @@ static void __lpc_handle_xmit(struct net_device *ndev)
 			/* Update stats */
 			ndev->stats.tx_packets++;
 			ndev->stats.tx_bytes += skb->len;
-
-			/* Free buffer */
-			dev_kfree_skb_irq(skb);
 		}
+		dev_kfree_skb_irq(skb);
 
 		txcidx = readl(LPC_ENET_TXCONSUMEINDEX(pldat->net_base));
 	}
 
-	if (netif_queue_stopped(ndev))
-		netif_wake_queue(ndev);
+	if (pldat->num_used_tx_buffs <= ENET_TX_DESC/2) {
+		if (netif_queue_stopped(ndev))
+			netif_wake_queue(ndev);
+	}
 }
 
 static int __lpc_handle_recv(struct net_device *ndev, int budget)

On 11/06/12 19:21, Eric Dumazet wrote:

From: Eric Dumazet <edumazet@google.com>

__lpc_handle_xmit() has two bugs :

1) It can leak skbs in case TXSTATUS_ERROR is set

2) It can wake up txqueue while no slot was freed.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Roland Stigge <redacted>
Tested-by: Roland Stigge <redacted>
Cc: Kevin Wells <redacted>

Thanks!

Would be good for v3.5 and for stable v3.4 also.

On Mon, 2012-06-11 at 11:26 +0200, Roland Stigge wrote:

Is it sensible at this point to increase the TX buffers anyway? For
different reasons of course: We have enough SRAM available and TX
buffers (16->32) are still more than RX buffers (48).

I doubt it has any impact on performance for a 100Mbit link ?

One thing that could be done would be to free skbs in
lpc_eth_hard_start_xmit() instead of __lpc_handle_xmit()

From: Eric Dumazet <redacted>
Date: Mon, 11 Jun 2012 19:21:36 +0200

From: Eric Dumazet <edumazet@google.com>

__lpc_handle_xmit() has two bugs :

1) It can leak skbs in case TXSTATUS_ERROR is set

2) It can wake up txqueue while no slot was freed.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Roland Stigge <redacted>
Tested-by: Roland Stigge <redacted>

Applied and queued up for -stable.

On Mon, 2012-06-11 at 21:18 +0200, Eric Dumazet wrote:

On Mon, 2012-06-11 at 11:26 +0200, Roland Stigge wrote:

quoted

Is it sensible at this point to increase the TX buffers anyway? For
different reasons of course: We have enough SRAM available and TX
buffers (16->32) are still more than RX buffers (48).

I doubt it has any impact on performance for a 100Mbit link ?

One thing that could be done would be to free skbs in
lpc_eth_hard_start_xmit() instead of __lpc_handle_xmit()

Here is the patch I was thinking about

(on top of latest net-next)

Could you please test it ?

 drivers/net/ethernet/nxp/lpc_eth.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/nxp/lpc_eth.c b/drivers/net/ethernet/nxp/lpc_eth.c
index 083d671..426f14c 100644
--- a/drivers/net/ethernet/nxp/lpc_eth.c
+++ b/drivers/net/ethernet/nxp/lpc_eth.c

@@ -440,7 +440,7 @@ struct netdata_local {
 	spinlock_t		lock;
 	void __iomem		*net_base;
 	u32			msg_enable;
-	struct sk_buff		*skb[ENET_TX_DESC];
+	unsigned int		skblen[ENET_TX_DESC];
 	unsigned int		last_tx_idx;
 	unsigned int		num_used_tx_buffs;
 	struct mii_bus		*mii_bus;

@@ -908,7 +908,7 @@ static void __lpc_handle_xmit(struct net_device *ndev)
 
 	txcidx = readl(LPC_ENET_TXCONSUMEINDEX(pldat->net_base));
 	while (pldat->last_tx_idx != txcidx) {
-		skb = pldat->skb[pldat->last_tx_idx];
+		unsigned int skblen = pldat->skblen[pldat->last_tx_idx];
 
 		/* A buffer is available, get buffer status */
 		ptxstat = &pldat->tx_stat_v[pldat->last_tx_idx];

@@ -945,9 +945,8 @@ static void __lpc_handle_xmit(struct net_device *ndev)
 		} else {
 			/* Update stats */
 			ndev->stats.tx_packets++;
-			ndev->stats.tx_bytes += skb->len;
+			ndev->stats.tx_bytes += skblen;
 		}
-		dev_kfree_skb_irq(skb);
 
 		txcidx = readl(LPC_ENET_TXCONSUMEINDEX(pldat->net_base));
 	}

@@ -1132,7 +1131,7 @@ static int lpc_eth_hard_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	memcpy(pldat->tx_buff_v + txidx * ENET_MAXF_SIZE, skb->data, len);
 
 	/* Save the buffer and increment the buffer counter */
-	pldat->skb[txidx] = skb;
+	pldat->skblen[txidx] = len;
 	pldat->num_used_tx_buffs++;
 
 	/* Start transmit */

@@ -1147,6 +1146,7 @@ static int lpc_eth_hard_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 
 	spin_unlock_irq(&pldat->lock);
 
+	dev_kfree_skb(skb);
 	return NETDEV_TX_OK;
 }
 

On 06/13/2012 08:16 AM, Eric Dumazet wrote:

Works fine for a while now.

We can remove the unused variable skb from __lpc_handle_xmit() now,
maybe just do in your patch?

Thanks!

Tested-by: Roland Stigge <redacted>

From: Eric Dumazet <edumazet@google.com>

Transmitted skbs can be freed immediately in lpc_eth_hard_start_xmit()
instead of at TX completion, since driver copies the frames in DMA area.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Tested-by: Roland Stigge <redacted>
---
 drivers/net/ethernet/nxp/lpc_eth.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/nxp/lpc_eth.c b/drivers/net/ethernet/nxp/lpc_eth.c
index 083d671..028dde4 100644
--- a/drivers/net/ethernet/nxp/lpc_eth.c
+++ b/drivers/net/ethernet/nxp/lpc_eth.c

@@ -440,7 +440,7 @@ struct netdata_local {
 	spinlock_t		lock;
 	void __iomem		*net_base;
 	u32			msg_enable;
-	struct sk_buff		*skb[ENET_TX_DESC];
+	unsigned int		skblen[ENET_TX_DESC];
 	unsigned int		last_tx_idx;
 	unsigned int		num_used_tx_buffs;
 	struct mii_bus		*mii_bus;

@@ -903,12 +903,11 @@ err_out:
 static void __lpc_handle_xmit(struct net_device *ndev)
 {
 	struct netdata_local *pldat = netdev_priv(ndev);
-	struct sk_buff *skb;
 	u32 txcidx, *ptxstat, txstat;
 
 	txcidx = readl(LPC_ENET_TXCONSUMEINDEX(pldat->net_base));
 	while (pldat->last_tx_idx != txcidx) {
-		skb = pldat->skb[pldat->last_tx_idx];
+		unsigned int skblen = pldat->skblen[pldat->last_tx_idx];
 
 		/* A buffer is available, get buffer status */
 		ptxstat = &pldat->tx_stat_v[pldat->last_tx_idx];

@@ -945,9 +944,8 @@ static void __lpc_handle_xmit(struct net_device *ndev)
 		} else {
 			/* Update stats */
 			ndev->stats.tx_packets++;
-			ndev->stats.tx_bytes += skb->len;
+			ndev->stats.tx_bytes += skblen;
 		}
-		dev_kfree_skb_irq(skb);
 
 		txcidx = readl(LPC_ENET_TXCONSUMEINDEX(pldat->net_base));
 	}

@@ -1132,7 +1130,7 @@ static int lpc_eth_hard_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	memcpy(pldat->tx_buff_v + txidx * ENET_MAXF_SIZE, skb->data, len);
 
 	/* Save the buffer and increment the buffer counter */
-	pldat->skb[txidx] = skb;
+	pldat->skblen[txidx] = len;
 	pldat->num_used_tx_buffs++;
 
 	/* Start transmit */

@@ -1147,6 +1145,7 @@ static int lpc_eth_hard_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 
 	spin_unlock_irq(&pldat->lock);
 
+	dev_kfree_skb(skb);
 	return NETDEV_TX_OK;
 }
 

From: Eric Dumazet <redacted>
Date: Wed, 13 Jun 2012 11:58:16 +0200

From: Eric Dumazet <edumazet@google.com>

Transmitted skbs can be freed immediately in lpc_eth_hard_start_xmit()
instead of at TX completion, since driver copies the frames in DMA area.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Tested-by: Roland Stigge <redacted>

Applied.