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

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 :

quoted

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 [off-list ref]

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.