Re: [PATCH] net/fec: infinite spin on sirq-net-tx on real-time

On 02/06/2012 10:56 AM, Eric Dumazet wrote:

Le lundi 06 février 2012 à 10:33 +0100, Hector Palacios a écrit :

quoted

If softirqd is a real time task, an inifinite spin is hit
if the FEC driver tries to send a packet before the autonegotiation
with the PHY has completed.

This was seen when booting the platform with DHCP on. The driver
sends the DHCP request before the PHY has completed autonegotiation.
As a consequence, the driver's dev_hard_start_xmit returns NETDEV_TX_BUSY.
NETDEV_TX_BUSY is part of NET_TX_MASK thus the packet is requeued (the
skb->next = nskb) in dev_hard_start_xmit(). And the NETDEV_TX_BUSY is
passed back to sch_derect_xmit() which calls dev_requeue_skb() which
then calls __netif_schedule(q) which will call __netif_reschedule(q)
which will then do raise_softirq_irqoff(NET_TX_SOFTIRQ).

Thus, as soon as ksoftirq exits this routine, it will restart the
process over again. As the fec driver never finished with its
negotiations, the process starts over again and we never move forward.

Newsgroup reference: linux-rt-users http://www.spinics.net/lists/linux-rt-users/msg07551.html

Signed-off-by: Zeng Zhaoming<redacted>
Signed-off-by: Frank Li<redacted>
Signed-off-by: Hector Palacios<redacted>
---
  drivers/net/ethernet/freescale/fec.c |    3 +++
  1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index c136230..3fa7d3b 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c

@@ -284,6 +284,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)

  	if (!fep->link) {
  		/* Link is down or autonegotiation is in progress. */
+		netif_stop_queue(dev);

This seems odd.

IMHO, NETDEV_TX_BUSY should be avoided as much as possible.

Its part of the old driver API.

If queue was stopped, you would not have to test fep->link at all in
fast path, and Qdisc would not have to requeue a packet eventually.

quoted

  		return NETDEV_TX_BUSY;
  	}

@@ -530,6 +531,7 @@ fec_stop(struct net_device *ndev)
  	udelay(10);
  	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
  	writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
+	fep->link = 0;

Why not call netif_stop_queue(dev) here ?

quoted

  	/* We have to keep ENET enabled to have MII interrupt stay working */
  	if (id_entry->driver_data&  FEC_QUIRK_ENET_MAC)

@@ -866,6 +868,7 @@ static void fec_enet_adjust_link(struct net_device *ndev)
  	if (phy_dev->link) {
  		if (fep->full_duplex != phy_dev->duplex) {
  			fec_restart(ndev, phy_dev->duplex);
+			netif_wake_queue(dev);
  			status_change = 1;
  		}
  	}

I'm no network driver expert so I'll leave it up to others to comment. I just forward 
ported a patch I came across in Freescale's BSP which solves the problem in mainline 
and in RT.
-- 
Héctor Palacios