[PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Lucas Stach]

This reverts commit 3f104c38259dcb3e5443c246f0805bc04d887cc3.

In preparaton to revert the other NAPI related changes.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 73195f6..afb52eb 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -1490,7 +1490,6 @@ fec_enet_close(struct net_device *ndev)
 	struct fec_enet_private *fep = netdev_priv(ndev);
 
 	/* Don't know what to do yet. */
-	napi_disable(&fep->napi);
 	fep->opened = 0;
 	netif_stop_queue(ndev);
 	fec_stop(ndev);
-- 
1.8.2.rc2

[PATCH resend 2/3] Revert "net: fec: put tx to napi poll function to fix dead lock"

[Lucas Stach]

This reverts commit de5fb0a053482d89262c3284b67884cd2c621adc.

This commit changed how ringbuffers are tracked. I suspect this
to break in subtle ways, which I unfortunately wasn't able to track
down to a single-line change.

Also in preparation to revert the initial NAPI introduction commit.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec.c | 86 ++++++++++++++++++------------------
 drivers/net/ethernet/freescale/fec.h |  3 ++
 2 files changed, 47 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index afb52eb..fa6a999 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -246,13 +246,14 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	struct bufdesc *bdp;
 	void *bufaddr;
 	unsigned short	status;
-	unsigned int index;
+	unsigned long flags;
 
 	if (!fep->link) {
 		/* Link is down or autonegotiation is in progress. */
 		return NETDEV_TX_BUSY;
 	}
 
+	spin_lock_irqsave(&fep->hw_lock, flags);
 	/* Fill in a Tx ring entry */
 	bdp = fep->cur_tx;
 
@@ -263,6 +264,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		 * This should not happen, since ndev->tbusy should be set.
 		 */
 		printk("%s: tx queue full!.\n", ndev->name);
+		spin_unlock_irqrestore(&fep->hw_lock, flags);
 		return NETDEV_TX_BUSY;
 	}
 
@@ -278,13 +280,13 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	 * 4-byte boundaries. Use bounce buffers to copy data
 	 * and get it aligned. Ugh.
 	 */
-	if (fep->bufdesc_ex)
-		index = (struct bufdesc_ex *)bdp -
-			(struct bufdesc_ex *)fep->tx_bd_base;
-	else
-		index = bdp - fep->tx_bd_base;
-
 	if (((unsigned long) bufaddr) & FEC_ALIGNMENT) {
+		unsigned int index;
+		if (fep->bufdesc_ex)
+			index = (struct bufdesc_ex *)bdp -
+				(struct bufdesc_ex *)fep->tx_bd_base;
+		else
+			index = bdp - fep->tx_bd_base;
 		memcpy(fep->tx_bounce[index], skb->data, skb->len);
 		bufaddr = fep->tx_bounce[index];
 	}
@@ -298,7 +300,10 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 		swap_buffer(bufaddr, skb->len);
 
 	/* Save skb pointer */
-	fep->tx_skbuff[index] = skb;
+	fep->tx_skbuff[fep->skb_cur] = skb;
+
+	ndev->stats.tx_bytes += skb->len;
+	fep->skb_cur = (fep->skb_cur+1) & TX_RING_MOD_MASK;
 
 	/* Push the data cache so the CPM does not get stale memory
 	 * data.
@@ -326,22 +331,26 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 			ebdp->cbd_esc = BD_ENET_TX_INT;
 		}
 	}
+	/* Trigger transmission start */
+	writel(0, fep->hwp + FEC_X_DES_ACTIVE);
+
 	/* If this was the last BD in the ring, start at the beginning again. */
 	if (status & BD_ENET_TX_WRAP)
 		bdp = fep->tx_bd_base;
 	else
 		bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
 
-	fep->cur_tx = bdp;
-
-	if (fep->cur_tx == fep->dirty_tx)
+	if (bdp == fep->dirty_tx) {
+		fep->tx_full = 1;
 		netif_stop_queue(ndev);
+	}
 
-	/* Trigger transmission start */
-	writel(0, fep->hwp + FEC_X_DES_ACTIVE);
+	fep->cur_tx = bdp;
 
 	skb_tx_timestamp(skb);
 
+	spin_unlock_irqrestore(&fep->hw_lock, flags);
+
 	return NETDEV_TX_OK;
 }
 
@@ -373,7 +382,7 @@ static void fec_enet_bd_init(struct net_device *dev)
 
 	/* ...and the same for transmit */
 	bdp = fep->tx_bd_base;
-	fep->cur_tx = bdp;
+	fep->dirty_tx = fep->cur_tx = bdp;
 	for (i = 0; i < TX_RING_SIZE; i++) {
 
 		/* Initialize the BD for every fragment in the page. */
@@ -389,7 +398,6 @@ static void fec_enet_bd_init(struct net_device *dev)
 	/* Set the last buffer to wrap */
 	bdp = fec_enet_get_prevdesc(bdp, fep->bufdesc_ex);
 	bdp->cbd_sc |= BD_SC_WRAP;
-	fep->dirty_tx = bdp;
 }
 
 /* This function is called to start or restart the FEC during a link
@@ -446,7 +454,8 @@ fec_restart(struct net_device *ndev, int duplex)
 		writel((unsigned long)fep->bd_dma + sizeof(struct bufdesc)
 			* RX_RING_SIZE,	fep->hwp + FEC_X_DES_START);
 
-
+	/* Reset SKB transmit buffers. */
+	fep->skb_cur = fep->skb_dirty = 0;
 	for (i = 0; i <= TX_RING_MOD_MASK; i++) {
 		if (fep->tx_skbuff[i]) {
 			dev_kfree_skb_any(fep->tx_skbuff[i]);
@@ -609,35 +618,20 @@ fec_enet_tx(struct net_device *ndev)
 	struct bufdesc *bdp;
 	unsigned short status;
 	struct	sk_buff	*skb;
-	int	index = 0;
 
 	fep = netdev_priv(ndev);
+	spin_lock(&fep->hw_lock);
 	bdp = fep->dirty_tx;
 
-	/* get next bdp of dirty_tx */
-	if (bdp->cbd_sc & BD_ENET_TX_WRAP)
-		bdp = fep->tx_bd_base;
-	else
-		bdp = fec_enet_get_nextdesc(bdp, fep->bufdesc_ex);
-
 	while (((status = bdp->cbd_sc) & BD_ENET_TX_READY) == 0) {
-
-		/* current queue is empty */
-		if (bdp == fep->cur_tx)
+		if (bdp == fep->cur_tx && fep->tx_full == 0)
 			break;
 
-		if (fep->bufdesc_ex)
-			index = (struct bufdesc_ex *)bdp -
-				(struct bufdesc_ex *)fep->tx_bd_base;
-		else
-			index = bdp - fep->tx_bd_base;
-
 		dma_unmap_single(&fep->pdev->dev, bdp->cbd_bufaddr,
 				FEC_ENET_TX_FRSIZE, DMA_TO_DEVICE);
 		bdp->cbd_bufaddr = 0;
 
-		skb = fep->tx_skbuff[index];
-
+		skb = fep->tx_skbuff[fep->skb_dirty];
 		/* Check for errors. */
 		if (status & (BD_ENET_TX_HB | BD_ENET_TX_LC |
 				   BD_ENET_TX_RL | BD_ENET_TX_UN |
@@ -682,9 +676,8 @@ fec_enet_tx(struct net_device *ndev)
 
 		/* Free the sk buffer associated with this last transmit */
 		dev_kfree_skb_any(skb);
-		fep->tx_skbuff[index] = NULL;
-
-		fep->dirty_tx = bdp;
+		fep->tx_skbuff[fep->skb_dirty] = NULL;
+		fep->skb_dirty = (fep->skb_dirty + 1) & TX_RING_MOD_MASK;
 
 		/* Update pointer to next buffer descriptor to be transmitted */
 		if (status & BD_ENET_TX_WRAP)
@@ -694,12 +687,14 @@ fec_enet_tx(struct net_device *ndev)
 
 		/* Since we have freed up a buffer, the ring is no longer full
 		 */
-		if (fep->dirty_tx != fep->cur_tx) {
+		if (fep->tx_full) {
+			fep->tx_full = 0;
 			if (netif_queue_stopped(ndev))
 				netif_wake_queue(ndev);
 		}
 	}
-	return;
+	fep->dirty_tx = bdp;
+	spin_unlock(&fep->hw_lock);
 }
 
 
@@ -866,7 +861,7 @@ fec_enet_interrupt(int irq, void *dev_id)
 		int_events = readl(fep->hwp + FEC_IEVENT);
 		writel(int_events, fep->hwp + FEC_IEVENT);
 
-		if (int_events & (FEC_ENET_RXF | FEC_ENET_TXF)) {
+		if (int_events & FEC_ENET_RXF) {
 			ret = IRQ_HANDLED;
 
 			/* Disable the RX interrupt */
@@ -877,6 +872,15 @@ fec_enet_interrupt(int irq, void *dev_id)
 			}
 		}
 
+		/* Transmit OK, or non-fatal error. Update the buffer
+		 * descriptors. FEC handles all errors, we just discover
+		 * them as part of the transmit process.
+		 */
+		if (int_events & FEC_ENET_TXF) {
+			ret = IRQ_HANDLED;
+			fec_enet_tx(ndev);
+		}
+
 		if (int_events & FEC_ENET_MII) {
 			ret = IRQ_HANDLED;
 			complete(&fep->mdio_done);
@@ -892,8 +896,6 @@ static int fec_enet_rx_napi(struct napi_struct *napi, int budget)
 	int pkts = fec_enet_rx(ndev, budget);
 	struct fec_enet_private *fep = netdev_priv(ndev);
 
-	fec_enet_tx(ndev);
-
 	if (pkts < budget) {
 		napi_complete(napi);
 		writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index eb43729..b70c3cd 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -213,6 +213,8 @@ struct fec_enet_private {
 	unsigned char *tx_bounce[TX_RING_SIZE];
 	struct	sk_buff *tx_skbuff[TX_RING_SIZE];
 	struct	sk_buff *rx_skbuff[RX_RING_SIZE];
+	ushort	skb_cur;
+	ushort	skb_dirty;
 
 	/* CPM dual port RAM relative addresses */
 	dma_addr_t	bd_dma;
@@ -224,6 +226,7 @@ struct fec_enet_private {
 	/* The ring entries to be free()ed */
 	struct bufdesc	*dirty_tx;
 
+	uint	tx_full;
 	/* hold while accessing the HW like ringbuffer for tx/rx but not MAC */
 	spinlock_t hw_lock;
 
-- 
1.8.2.rc2

[PATCH resend 3/3] Revert "net: fec: add napi support to improve proformance"

[Lucas Stach]

This reverts commit dc975382d2ef36be7e78fac3717927de1a5abcd8.

This change caused trouble up to kernel OOPSes with NULL-ptr
dereferences. The only sane way to get back to a working state
is this revert.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec.c | 41 ++++++------------------------------
 drivers/net/ethernet/freescale/fec.h |  2 --
 2 files changed, 7 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index fa6a999..3d51163 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -67,7 +67,6 @@
 #endif
 
 #define DRIVER_NAME	"fec"
-#define FEC_NAPI_WEIGHT	64
 
 /* Pause frame feild and FIFO threshold */
 #define FEC_ENET_FCE	(1 << 5)
@@ -169,7 +168,6 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
 #define FEC_ENET_EBERR	((uint)0x00400000)	/* SDMA bus error */
 
 #define FEC_DEFAULT_IMASK (FEC_ENET_TXF | FEC_ENET_RXF | FEC_ENET_MII)
-#define FEC_RX_DISABLED_IMASK (FEC_DEFAULT_IMASK & (~FEC_ENET_RXF))
 
 /* The FEC stores dest/src/type, data, and checksum for receive packets.
  */
@@ -703,8 +701,8 @@ fec_enet_tx(struct net_device *ndev)
  * not been given to the system, we just set the empty indicator,
  * effectively tossing the packet.
  */
-static int
-fec_enet_rx(struct net_device *ndev, int budget)
+static void
+fec_enet_rx(struct net_device *ndev)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	const struct platform_device_id *id_entry =
@@ -714,12 +712,13 @@ fec_enet_rx(struct net_device *ndev, int budget)
 	struct	sk_buff	*skb;
 	ushort	pkt_len;
 	__u8 *data;
-	int	pkt_received = 0;
 
 #ifdef CONFIG_M532x
 	flush_cache_all();
 #endif
 
+	spin_lock(&fep->hw_lock);
+
 	/* First, grab all of the stats for the incoming packet.
 	 * These get messed up if we get called due to a busy condition.
 	 */
@@ -727,10 +726,6 @@ fec_enet_rx(struct net_device *ndev, int budget)
 
 	while (!((status = bdp->cbd_sc) & BD_ENET_RX_EMPTY)) {
 
-		if (pkt_received >= budget)
-			break;
-		pkt_received++;
-
 		/* Since we have allocated space to hold a complete frame,
 		 * the last indicator should be set.
 		 */
@@ -812,7 +807,7 @@ fec_enet_rx(struct net_device *ndev, int budget)
 			}
 
 			if (!skb_defer_rx_timestamp(skb))
-				napi_gro_receive(&fep->napi, skb);
+				netif_rx(skb);
 		}
 
 		bdp->cbd_bufaddr = dma_map_single(&fep->pdev->dev, data,
@@ -846,7 +841,7 @@ rx_processing_done:
 	}
 	fep->cur_rx = bdp;
 
-	return pkt_received;
+	spin_unlock(&fep->hw_lock);
 }
 
 static irqreturn_t
@@ -863,13 +858,7 @@ fec_enet_interrupt(int irq, void *dev_id)
 
 		if (int_events & FEC_ENET_RXF) {
 			ret = IRQ_HANDLED;
-
-			/* Disable the RX interrupt */
-			if (napi_schedule_prep(&fep->napi)) {
-				writel(FEC_RX_DISABLED_IMASK,
-					fep->hwp + FEC_IMASK);
-				__napi_schedule(&fep->napi);
-			}
+			fec_enet_rx(ndev);
 		}
 
 		/* Transmit OK, or non-fatal error. Update the buffer
@@ -890,18 +879,7 @@ fec_enet_interrupt(int irq, void *dev_id)
 	return ret;
 }
 
-static int fec_enet_rx_napi(struct napi_struct *napi, int budget)
-{
-	struct net_device *ndev = napi->dev;
-	int pkts = fec_enet_rx(ndev, budget);
-	struct fec_enet_private *fep = netdev_priv(ndev);
 
-	if (pkts < budget) {
-		napi_complete(napi);
-		writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
-	}
-	return pkts;
-}
 
 /* ------------------------------------------------------------------------- */
 static void fec_get_mac(struct net_device *ndev)
@@ -1464,8 +1442,6 @@ fec_enet_open(struct net_device *ndev)
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	int ret;
 
-	napi_enable(&fep->napi);
-
 	/* I should reset the ring buffers here, but I don't yet know
 	 * a simple way to do that.
 	 */
@@ -1677,9 +1653,6 @@ static int fec_enet_init(struct net_device *ndev)
 	ndev->netdev_ops = &fec_netdev_ops;
 	ndev->ethtool_ops = &fec_enet_ethtool_ops;
 
-	writel(FEC_RX_DISABLED_IMASK, fep->hwp + FEC_IMASK);
-	netif_napi_add(ndev, &fep->napi, fec_enet_rx_napi, FEC_NAPI_WEIGHT);
-
 	fec_restart(ndev, 0);
 
 	return 0;
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index b70c3cd..ff79610 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -249,8 +249,6 @@ struct fec_enet_private {
 	int	bufdesc_ex;
 	int	pause_flag;
 
-	struct	napi_struct napi;
-
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info ptp_caps;
 	unsigned long last_overflow_check;
-- 
1.8.2.rc2

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Frank Li]

2013/4/26 Lucas Stach <l.stach@pengutronix.de>:
> This reverts commit 3f104c38259dcb3e5443c246f0805bc04d887cc3.
>
> In preparaton to revert the other NAPI related changes.
>
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>

David:

Please hold apply this patch for some days.
Let me found a good way to fix the issue that lucas met.

This patch may cause spin_lock dead lock when using 1588 ptp.

best regards
Frank Li

> ---
>  drivers/net/ethernet/freescale/fec.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> index 73195f6..afb52eb 100644
> --- a/drivers/net/ethernet/freescale/fec.c
> +++ b/drivers/net/ethernet/freescale/fec.c
> @@ -1490,7 +1490,6 @@ fec_enet_close(struct net_device *ndev)
>         struct fec_enet_private *fep = netdev_priv(ndev);
>
>         /* Don't know what to do yet. */
> -       napi_disable(&fep->napi);
>         fep->opened = 0;
>         netif_stop_queue(ndev);
>         fec_stop(ndev);
> --
> 1.8.2.rc2
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Robert Schwebel]

Frank,

On Fri, Apr 26, 2013 at 05:32:02PM +0800, Frank Li wrote:
> 2013/4/26 Lucas Stach <l.stach@pengutronix.de>:
> > This reverts commit 3f104c38259dcb3e5443c246f0805bc04d887cc3.
> >
> > In preparaton to revert the other NAPI related changes.
> >
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>
> David:
>
> Please hold apply this patch for some days.
> Let me found a good way to fix the issue that lucas met.
>
> This patch may cause spin_lock dead lock when using 1588 ptp.

Seriously - it's friday, and 3.9 is expected to come out this weekend.
You broke the driver in mainline with your patches, and the current
state is that it isn't fixable before the release.

So in order not to break the FEC in 3.9-final, please let's go with the
reverts for now; if 1588 PTP is broken, it was broken in 3.8 as well,
correct? After the release, there will be a lot of time to make proper
patches that fix the issues.

rsc
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[David Miller]

From: Robert Schwebel <r.schwebel@pengutronix.de>
Date: Fri, 26 Apr 2013 15:44:15 +0200

> Seriously - it's friday, and 3.9 is expected to come out this
> weekend.

Seriously, it took you how long to notice the breakage and report
it in sufficient detail for the author to make an attempt at a fix?

I thnk Frank's request is reasonable given the circumstances, please
work closely with him on the fix.

Thanks.

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Robert Schwebel]

On Fri, Apr 26, 2013 at 02:33:09PM -0400, David Miller wrote:
> From: Robert Schwebel <r.schwebel@pengutronix.de>
> Date: Fri, 26 Apr 2013 15:44:15 +0200
>
> > Seriously - it's friday, and 3.9 is expected to come out this
> > weekend.
>
> Seriously, it took you how long to notice the breakage and report
> it in sufficient detail for the author to make an attempt at a fix?
>
> I thnk Frank's request is reasonable given the circumstances, please
> work closely with him on the fix.

The FEC driver has worked fine in 3.8.x.

Frank's patches for the 3.9 cycle...

- remove locking in a way that memory is freed which is in use
- break the driver, up to a point where the kernel oopses when the link
  goes away
- mix up different changes (queue handling) and should have been split up
  into separate patches

Unfortunately, the breakage happens only on multicore (MX6Q) and if you
change the link status; that's probably the reason why it hasn't been
noticed earlier.

We have really tried to find a "quick fix which does it right", but it
has turned out that this isn't possible in such a short time, because it
is more complex than just re-adding locks. We feel that the results of
last week's activities are not good enough that they could be merged
without further breakage.

Please consider to merge the reverts. Otherwhise FEC will be broken in 3.9.

Of course, we can help with a real solution, but please after 3.9.final.

Thanks,
Robert
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Frank Li]

2013/4/27 Robert Schwebel <r.schwebel@pengutronix.de>:
> On Fri, Apr 26, 2013 at 02:33:09PM -0400, David Miller wrote:
>> From: Robert Schwebel <r.schwebel@pengutronix.de>
>> Date: Fri, 26 Apr 2013 15:44:15 +0200
>>
>> > Seriously - it's friday, and 3.9 is expected to come out this
>> > weekend.
>>
>> Seriously, it took you how long to notice the breakage and report
>> it in sufficient detail for the author to make an attempt at a fix?
>>
>> I thnk Frank's request is reasonable given the circumstances, please
>> work closely with him on the fix.
>
> The FEC driver has worked fine in 3.8.x.
>
> Frank's patches for the 3.9 cycle...
>
> - remove locking in a way that memory is freed which is in use

The purpose of remove lock is that fix dead lock.
It is true I miss a execute path to reset BD descript.
But the possibility is very low.

You report this problem without detail reproduce steps.

> - break the driver, up to a point where the kernel oopses when the link
>   goes away

run script while [ 1 ]; do ethtool -s eth0 autoneg off;sleep 3;ethtool
-s eth0 autoneg on; sleep 4; done;

I just capture one oops during 1 hours.
I am try to fix this issue without add back lock.

I plan use below way to fix this issue.  I am running my stress test.
if you have time, you can run it at your sit.

diff --git a/drivers/net/ethernet/freescale/fec.c
b/drivers/net/ethernet/freescale/fe
index 73195f6..c945bb7 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -403,6 +403,11 @@ fec_restart(struct net_device *ndev, int duplex)
        const struct platform_device_id *id_entry =
                                platform_get_device_id(fep->pdev);
        int i;
+       if (netif_running(ndev)) {
+               napi_disable(&fep->napi);
+               netif_stop_queue(ndev);
+       }
+
        u32 temp_mac[2];
        u32 rcntl = OPT_FRAME_SIZE | 0x04;
        u32 ecntl = 0x2; /* ETHEREN */
@@ -559,6 +564,11 @@ fec_restart(struct net_device *ndev, int duplex)

        /* Enable interrupts we wish to service */
        writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
+
+       if (netif_running(ndev)) {
+               napi_enable(&fep->napi);
+               netif_wake_queue(ndev);
+       }
 }


> - mix up different changes (queue handling) and should have been split up
>   into separate patches
>
> Unfortunately, the breakage happens only on multicore (MX6Q) and if you
> change the link status; that's probably the reason why it hasn't been
> noticed earlier.
>
> We have really tried to find a "quick fix which does it right", but it
> has turned out that this isn't possible in such a short time, because it
> is more complex than just re-adding locks. We feel that the results of
> last week's activities are not good enough that they could be merged
> without further breakage.
>
> Please consider to merge the reverts. Otherwhise FEC will be broken in 3.9.
>
> Of course, we can help with a real solution, but please after 3.9.final.
>
> Thanks,
> Robert
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Francois Romieu]

Frank Li <lznuaa@gmail.com> :
[...]
> diff --git a/drivers/net/ethernet/freescale/fec.c
> b/drivers/net/ethernet/freescale/fe
> index 73195f6..c945bb7 100644
> --- a/drivers/net/ethernet/freescale/fec.c
> +++ b/drivers/net/ethernet/freescale/fec.c
> @@ -403,6 +403,11 @@ fec_restart(struct net_device *ndev, int duplex)
>         const struct platform_device_id *id_entry =
>                                 platform_get_device_id(fep->pdev);
>         int i;
> +       if (netif_running(ndev)) {
> +               napi_disable(&fep->napi);

napi_disable may sleep.

fec_restart can be called with spinlock held in fec_enet_adjust_link

-- 
Ueimor

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Frank Li]

2013/4/27, Francois Romieu <romieu@fr.zoreil.com>:
> Frank Li <lznuaa@gmail.com> :
> [...]
>> diff --git a/drivers/net/ethernet/freescale/fec.c
>> b/drivers/net/ethernet/freescale/fe
>> index 73195f6..c945bb7 100644
>> --- a/drivers/net/ethernet/freescale/fec.c
>> +++ b/drivers/net/ethernet/freescale/fec.c
>> @@ -403,6 +403,11 @@ fec_restart(struct net_device *ndev, int duplex)
>>         const struct platform_device_id *id_entry =
>>                                 platform_get_device_id(fep->pdev);
>>         int i;
>> +       if (netif_running(ndev)) {
>> +               napi_disable(&fep->napi);
>
> napi_disable may sleep.
>
> fec_restart can be called with spinlock held in fec_enet_adjust_link

You are right. Spin lock in FEC enet adjust link can be removed when
remove spinlock in tx and RX.

>
> --
> Ueimor
>

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Francois Romieu]

Frank Li <lznuaa@gmail.com> :
> 2013/4/27, Francois Romieu <romieu@fr.zoreil.com>:
[...]
> > napi_disable may sleep.
> >
> > fec_restart can be called with spinlock held in fec_enet_adjust_link
> 
> You are right. Spin lock in FEC enet adjust link can be removed when
> remove spinlock in tx and RX.

fec_restart is also called from the netdev watchdog handler (fec_timeout)
and tasklet can't sleep either. You should imho schedule_work from
fec_timeout.

How is the driver supposed to avoid the napi context
fec_enet_tx:netif_queue_stopped vs fec_enet_start_xmit:netif_stop_queue
race since both can run on different cpu without any read/write ordering
enforcement between one thread netif_* and its peer {dirty/cur}_tx ?

-- 
Ueimor

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Frank Li]

2013/4/28 Francois Romieu <romieu@fr.zoreil.com>:
> Frank Li <lznuaa@gmail.com> :
>> 2013/4/27, Francois Romieu <romieu@fr.zoreil.com>:
> [...]
>> > napi_disable may sleep.
>> >
>> > fec_restart can be called with spinlock held in fec_enet_adjust_link
>>
>> You are right. Spin lock in FEC enet adjust link can be removed when
>> remove spinlock in tx and RX.
>
> fec_restart is also called from the netdev watchdog handler (fec_timeout)
> and tasklet can't sleep either. You should imho schedule_work from
> fec_timeout.

I will add delay_work to fix this issue.

>
> How is the driver supposed to avoid the napi context
> fec_enet_tx:netif_queue_stopped vs fec_enet_start_xmit:netif_stop_queue
> race since both can run on different cpu without any read/write ordering
> enforcement between one thread netif_* and its peer {dirty/cur}_tx ?

This is lockless design if only one read and one write. It likes kfifo.

Assume CPU1 run xmit.  CPU2 run napi fec_enet_tx

(1)       if (fep->cur_tx == fep->dirty_tx)
(2)                 netif_stop_queue(ndev);

if CPU2 update fep->dirty_tx before CPU1 run (1).  the condition is false.
if CPU2 update fep->dirty_tx after (1) before (2),   netif_stop_queue
will be called.  There are not problem at this time even though queue
is not full here.  queue is not empty for sure here. fec_enet_tx will
be called again when NAPI trigger by one frame finished transfer,
fec_enet_tx will wake up send queue.

Most worse case is waste one BD script.

>
> --
> Ueimor

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Frank Li]

2013/4/28 Frank Li <lznuaa@gmail.com>:
> 2013/4/28 Francois Romieu <romieu@fr.zoreil.com>:
>> Frank Li <lznuaa@gmail.com> :
>>> 2013/4/27, Francois Romieu <romieu@fr.zoreil.com>:
>> [...]
>>> > napi_disable may sleep.
>>> >
>>> > fec_restart can be called with spinlock held in fec_enet_adjust_link
>>>
>>> You are right. Spin lock in FEC enet adjust link can be removed when
>>> remove spinlock in tx and RX.
>>
>> fec_restart is also called from the netdev watchdog handler (fec_timeout)
>> and tasklet can't sleep either. You should imho schedule_work from
>> fec_timeout.
>
> I will add delay_work to fix this issue.
>
>>
>> How is the driver supposed to avoid the napi context
>> fec_enet_tx:netif_queue_stopped vs fec_enet_start_xmit:netif_stop_queue
>> race since both can run on different cpu without any read/write ordering
>> enforcement between one thread netif_* and its peer {dirty/cur}_tx ?
>
> This is lockless design if only one read and one write. It likes kfifo.
>
> Assume CPU1 run xmit.  CPU2 run napi fec_enet_tx
>
> (1)       if (fep->cur_tx == fep->dirty_tx)
> (2)                 netif_stop_queue(ndev);
>
> if CPU2 update fep->dirty_tx before CPU1 run (1).  the condition is false.
> if CPU2 update fep->dirty_tx after (1) before (2),   netif_stop_queue
> will be called.  There are not problem at this time even though queue
> is not full here.  queue is not empty for sure here. fec_enet_tx will
> be called again when NAPI trigger by one frame finished transfer,
> fec_enet_tx will wake up send queue.
>
> Most worse case is waste one BD script.

I just send out a patch to try to fix your problem.
Can you help test it in your sit?

>
>>
>> --
>> Ueimor

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Francois Romieu]

Frank Li <lznuaa@gmail.com> :
[...]
> I just send out a patch to try to fix your problem.
> Can you help test it in your sit?

I don't have the required hardware.

But I can find bugs while the napi-is-too-hard-please-do-proper-project-mgmt
squad watches elsewhere. :o)

-- 
Ueimor

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Frank Li]

2013/4/28 Francois Romieu <romieu@fr.zoreil.com>:
> Frank Li <lznuaa@gmail.com> :
> [...]
>> I just send out a patch to try to fix your problem.
>> Can you help test it in your sit?
>
> I don't have the required hardware.
>
> But I can find bugs while the napi-is-too-hard-please-do-proper-project-mgmt
> squad watches elsewhere. :o)

Sorry, what's your means?

>
> --
> Ueimor

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Francois Romieu]

Frank Li <lznuaa@gmail.com> :
[...]
> This is lockless design if only one read and one write. It likes kfifo.
> 
> Assume CPU1 run xmit.  CPU2 run napi fec_enet_tx
> 
> (1)       if (fep->cur_tx == fep->dirty_tx)
> (2)                 netif_stop_queue(ndev);
> 
> if CPU2 update fep->dirty_tx before CPU1 run (1).  the condition is false.
> if CPU2 update fep->dirty_tx after (1) before (2),   netif_stop_queue
> will be called.  There are not problem at this time even though queue
> is not full here.  queue is not empty for sure here. fec_enet_tx will
> be called again when NAPI trigger by one frame finished transfer,
> fec_enet_tx will wake up send queue.

"before" and "after" may not work as expected between different CPU without
explicit synchronization (or barrier). It won't oops but it would be careful
to envision something like drivers/net/ethernet/broadcom/tg3.c::tg3_tx.

-- 
Ueimor

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Frank Li]

2013/4/28 Francois Romieu <romieu@fr.zoreil.com>:
> Frank Li <lznuaa@gmail.com> :
> [...]
>> This is lockless design if only one read and one write. It likes kfifo.
>>
>> Assume CPU1 run xmit.  CPU2 run napi fec_enet_tx
>>
>> (1)       if (fep->cur_tx == fep->dirty_tx)
>> (2)                 netif_stop_queue(ndev);
>>
>> if CPU2 update fep->dirty_tx before CPU1 run (1).  the condition is false.
>> if CPU2 update fep->dirty_tx after (1) before (2),   netif_stop_queue
>> will be called.  There are not problem at this time even though queue
>> is not full here.  queue is not empty for sure here. fec_enet_tx will
>> be called again when NAPI trigger by one frame finished transfer,
>> fec_enet_tx will wake up send queue.
>
> "before" and "after" may not work as expected between different CPU without
> explicit synchronization (or barrier). It won't oops but it would be careful
> to envision something like drivers/net/ethernet/broadcom/tg3.c::tg3_tx.

tg3_tx is safe method but need more lock.
But I think it is not related with this issue. I can change later.

>
> --
> Ueimor

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Francois Romieu]

Frank Li <lznuaa@gmail.com> :
[...]
> tg3_tx is safe method but need more lock.

It's the same design.

> But I think it is not related with this issue. I can change later.

It won't oops, sure. It will simply stop under high load or proper
timing.

-- 
Ueimor

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Frank Li]

2013/4/29, Francois Romieu <romieu@fr.zoreil.com>:
> Frank Li <lznuaa@gmail.com> :
> [...]
>> tg3_tx is safe method but need more lock.
>
> It's the same design.
>
>> But I think it is not related with this issue. I can change later.
>
> It won't oops, sure. It will simply stop under high load or proper
> timing.

It is difference problem.
It should be new patch.

Do you have test step to reproduce the problem what you said?

>
> --
> Ueimor
>

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Lucas Stach]

Am Samstag, den 27.04.2013, 18:26 +0800 schrieb Frank Li:
> >
> > - remove locking in a way that memory is freed which is in use
> 
> The purpose of remove lock is that fix dead lock.

Could you please elaborate? What is the exact situation which caused a
deadlock here? I tried to see what's happening from your changelogs, but
I'm not sure I can follow here. Wasn't the problem more of the used
spinlocks not being IRQ-save and thus potentially creating a deadlock?

Regards,
Lucas
-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Frank Li]

2013/4/29 Lucas Stach <l.stach@pengutronix.de>:
> What is the exact situation which caused a
> deadlock here? I tried to see what's happening from your changelogs, but
> I'm not sure I can follow here. Wasn't the problem more of the used
> spinlocks not being IRQ-save and thus potentially creating a deadlock?

You can view below email thread.
http://www.spinics.net/lists/netdev/msg225240.html

The main problem network API use spin_lock_bh only now. So it does not
prefer use irq to handle skb process.

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[David Miller]

From: Robert Schwebel <r.schwebel@pengutronix.de>
Date: Sat, 27 Apr 2013 11:05:45 +0200

> Please consider to merge the reverts. Otherwhise FEC will be broken
> in 3.9.

The amount of effort you put into this email, and complaining,
far exceed that necessary to help Frank write a fix.

I've already told you how I want to see this resolved, and you
haven't given me any new information, so my point of view has
not changed.

Re: [PATCH resend 1/3] Revert "net: fec: fix missing napi_disable call"

[Robert Schwebel]

On Sun, Apr 28, 2013 at 01:11:44AM -0400, David Miller wrote:
> > Please consider to merge the reverts. Otherwhise FEC will be broken
> > in 3.9.
>
> The amount of effort you put into this email, and complaining,
> far exceed that necessary to help Frank write a fix.
>
> I've already told you how I want to see this resolved, and you
> haven't given me any new information, so my point of view has
> not changed.

Ok, so 3.9 will be broken - it's your decision. We'll have a lot of time
to fix things properly then; Lucas will test & comment the new patches
next week then.

rsc
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |