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(-)
@@ -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);returnNETDEV_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;/* We have to keep ENET enabled to have MII interrupt stay working */if(id_entry->driver_data&FEC_QUIRK_ENET_MAC)
From: Eric Dumazet <hidden> Date: 2012-02-06 09:56:36
Le lundi 06 février 2012 à 10:33 +0100, Hector Palacios a écrit :
quoted hunk
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(-)
@@ -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.
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(-)
@@ -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.
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
From: Eric Dumazet <hidden> Date: 2012-02-06 12:55:28
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'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.
Hector,
Eric's suggestion may also work. Could your revert this patch and add
the netif_stop_queue(dev) there, and see if it fixes the problems in
both mainline and -rt?
Thanks!
-- Steve
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.
Hector,
Eric's suggestion may also work. Could your revert this patch and add
the netif_stop_queue(dev) there, and see if it fixes the problems in
both mainline and -rt?
Not sure it will be enough to call netif_stop_queue(dev) in fec_stop()
We probably need to start the device with its tx queue stopped, then
later when device is really ready wakeup the queue.
From: Tim Sander <hidden> Date: 2012-02-06 13:51:16
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
@@ -242,11 +242,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)unsignedshortstatus;unsignedlongflags;-if(!fep->link){-/* Link is down or autonegotiation is in progress. */-returnNETDEV_TX_BUSY;-}-spin_lock_irqsave(&fep->hw_lock,flags);/* Fill in a Tx ring entry */bdp=fep->cur_tx;
@@ -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.
From: Tim Sander <hidden> Date: 2012-02-06 16:10:26
Hi
I forward ported the patch i have for 3.0-rt (which was working on a quick test)
to the net-dev branch with the patch from Eric mixed in.
But a quick test revealed that dmesg is full of:
eth0: tx queue full!.
Not good! Any suggestions on this?
Tim
Heres my patch for 3.3:
@@ -284,11 +284,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)unsignedshortstatus;unsignedlongflags;-if(!fep->link){-/* Link is down or autonegotiation is in progress. */-returnNETDEV_TX_BUSY;-}-spin_lock_irqsave(&fep->hw_lock,flags);/* Fill in a Tx ring entry */bdp=fep->cur_tx;
@@ -1210,7 +1208,7 @@ static int fec_enet_alloc_buffers(struct net_device *ndev)bdp=fep->rx_bd_base;for(i=0;i<RX_RING_SIZE;i++){-skb=dev_alloc_skb(FEC_ENET_RX_FRSIZE);+skb=__dev_alloc_skb(FEC_ENET_RX_FRSIZE,GFP_KERNEL);if(!skb){fec_enet_free_buffers(ndev);return-ENOMEM;
@@ -1230,6 +1228,10 @@ 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]){+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.
From: Eric Dumazet <hidden> Date: 2012-02-06 16:25:13
Le lundi 06 février 2012 à 17:09 +0100, Tim Sander a écrit :
Hi
I forward ported the patch i have for 3.0-rt (which was working on a quick test)
to the net-dev branch with the patch from Eric mixed in.
But a quick test revealed that dmesg is full of:
eth0: tx queue full!.
Not good! Any suggestions on this?
Please dont mix things.
My patch has nothing to do with the TX ring handling.
We first must fix the driver before removing this work around.
- /* Link is down or autonegotiation is in progress. */
- return NETDEV_TX_BUSY;
- }
-
In fact, returning NETDEV_TX_BUSY here is proof driver is buggy.
We should not enter fec_enet_start_xmit() is device is not ready to send
frames.
There are missing netif_stop_queue(dev) in this driver.
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.
Hector,
Eric's suggestion may also work. Could your revert this patch and add
the netif_stop_queue(dev) there, and see if it fixes the problems in
both mainline and -rt?
Not sure it will be enough to call netif_stop_queue(dev) in fec_stop()
We probably need to start the device with its tx queue stopped, then
later when device is really ready wakeup the queue.
I am talking about 3.0-rt33 kernel since this is the one i can test the best.
Ok i just found out that removing:
if (!fep->link) {
/* Link is down or autonegotiation is in progress. */
netif_stop_queue(ndev);
return NETDEV_TX_BUSY;
}
does not seem to work. Also netif_stop_queue seems to be mandantory to get the driver working.
If this condition in the hotpath is removed i see the ksoftirq high cpuload problem again.
I am not sure if the patch for lines 470 if netif_stop_queue(ndev); is really nessary.
I also found an double cleanup in my earlier patch. So heres the next iteration of the
fec ksoftirq fix for preempt rt.
Tim
@@ -244,6 +244,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(ndev);returnNETDEV_TX_BUSY;}