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

["Tim Sander" <tim.sander@hbm.com>]

Hi 

I just reworked the driver according to Eric's suggestions.
It passes first smoke tests. I don't know how to test the out of memory path.

Am Montag, 6. Februar 2012, 13:55:24 schrieb Eric Dumazet:
> Le lundi 06 février 2012 à 12:03 +0100, Hector Palacios a écrit :
> > 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.
> 
> I understood you didnt write the patch alone, and my question was
> addressed to all people involved, not only to you.
> 
> FEC driver needs some bugfixes, before diverging too much from the state
> of the art.
> 
> For example, fec_enet_alloc_buffers()  doesnt check for allocation
> failures :
> 
> fep->tx_bounce[i] = kmalloc(FEC_ENET_TX_FRSIZE, GFP_KERNEL);
> 
> NULL dereferences are then possible later in fec_enet_start_xmit()
> 
> By the way, I am not even sure kmalloc(2048) has a guarantee on
> alignement of the result, depending on the slub/slab debugging options.
I have not taken care of the alignment issue mentioned.

> 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-of-by: Tim Sander <tim.sander@hbm.com>

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 885d8ba..f69d95f 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -242,11 +242,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
        unsigned short  status;
        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;
@@ -470,6 +465,9 @@ 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);
+
+       netif_stop_queue(ndev);
+       fep->link = 0;
 }
 
 
@@ -787,6 +785,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(ndev);
                        status_change = 1;
                }
        }
@@ -1118,6 +1117,13 @@ static int fec_enet_alloc_buffers(struct net_device *ndev)
        bdp = fep->tx_bd_base;
        for (i = 0; i < TX_RING_SIZE; i++) {
                fep->tx_bounce[i] = kmalloc(FEC_ENET_TX_FRSIZE, GFP_KERNEL);
+               if(!fep->tx_bounce[i])  {
+                       for(j = 0; j < i; j++) {
+                               kfree(fep->tx_bounce[j]);
+                       }
+                       fec_enet_free_buffers(ndev);
+                       return -ENOMEM;
+               }
 
                bdp->cbd_sc = 0;
                bdp->cbd_bufaddr = 0;


Hottinger Baldwin Messtechnik GmbH, Im Tiefen See 45, 64293 Darmstadt, Germany | www.hbm.com 

Registered as GmbH (German limited liability corporation) in the commercial register at the local court of Darmstadt, HRB 1147  
Company domiciled in Darmstadt | CEO: Andreas Huellhorst | Chairman of the board: James Charles Webster

Als Gesellschaft mit beschraenkter Haftung eingetragen im Handelsregister des Amtsgerichts Darmstadt unter HRB 1147 
Sitz der Gesellschaft: Darmstadt | Geschaeftsfuehrung: Andreas Huellhorst | Aufsichtsratsvorsitzender: James Charles Webster

The information in this email is confidential. It is intended solely for the addressee. If you are not the intended recipient, please let me know and delete this email.

Die in dieser E-Mail enthaltene Information ist vertraulich und lediglich für den Empfaenger bestimmt. Sollten Sie nicht der eigentliche Empfaenger sein, informieren Sie mich bitte kurz und loeschen diese E-Mail.