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

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

[Hector Palacios]

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 <b32542@freescale.com>
Signed-off-by: Frank Li <Frank.Li@freescale.com>
Signed-off-by: Hector Palacios <hector.palacios@digi.com>
---
 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);
 		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;
 
 	/* 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;
 		}
 	}
-- 
1.7.1

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

[Eric Dumazet]

Le lundi 06 février 2012 à 10:33 +0100, Hector Palacios a écrit :
> 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 <b32542@freescale.com>
> Signed-off-by: Frank Li <Frank.Li@freescale.com>
> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> ---
>  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.

>  		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 ?

>  
>  	/* 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;
>  		}
>  	}

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

[Hector Palacios]

On 02/06/2012 10:56 AM, Eric Dumazet wrote:
> Le lundi 06 février 2012 à 10:33 +0100, Hector Palacios a écrit :
>> 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<b32542@freescale.com>
>> Signed-off-by: Frank Li<Frank.Li@freescale.com>
>> Signed-off-by: Hector Palacios<hector.palacios@digi.com>
>> ---
>>   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.
>
>>   		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 ?
>
>>
>>   	/* 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

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

[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.

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

[Steven Rostedt]

On Mon, 2012-02-06 at 12:03 +0100, Hector Palacios wrote:

> >>
> >> @@ -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 ?
> >

> 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

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

[Eric Dumazet]

Le lundi 06 février 2012 à 08:25 -0500, Steven Rostedt a écrit :
> On Mon, 2012-02-06 at 12:03 +0100, Hector Palacios wrote:
> 
> > >>
> > >> @@ -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 ?
> > >
> 
> > 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.

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

[Tim Sander]

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.

[PATCH] fec: fix tx bounce handling

[Eric Dumazet]

fec_enet_alloc_buffers() doesnt check for allocation
failures for tx bounce buffers, and assumes kmalloc() returns aligned
blocks of memory. This is not always true.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
Compile tested only.

 drivers/net/ethernet/freescale/fec.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 7b25e9c..f831d0d 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -319,8 +319,8 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	if (((unsigned long) bufaddr) & FEC_ALIGNMENT) {
 		unsigned int index;
 		index = bdp - fep->tx_bd_base;
-		memcpy(fep->tx_bounce[index], skb->data, skb->len);
-		bufaddr = fep->tx_bounce[index];
+		bufaddr = PTR_ALIGN(fep->tx_bounce[index], FEC_ALIGNMENT + 1);
+		memcpy(bufaddr, skb->data, skb->len);
 	}
 
 	/*
@@ -1196,7 +1196,6 @@ static void fec_enet_free_buffers(struct net_device *ndev)
 		bdp++;
 	}
 
-	bdp = fep->tx_bd_base;
 	for (i = 0; i < TX_RING_SIZE; i++)
 		kfree(fep->tx_bounce[i]);
 }
@@ -1210,7 +1209,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 +1229,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;

Re: [PATCH] fec: fix tx bounce handling

[Tim Sander]

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:

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 336edd7..74d5865 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -284,11 +284,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;
@@ -319,8 +314,8 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
        if (((unsigned long) bufaddr) & FEC_ALIGNMENT) {
                unsigned int index;
                index = bdp - fep->tx_bd_base;
-               memcpy(fep->tx_bounce[index], skb->data, skb->len);
-               bufaddr = fep->tx_bounce[index];
+               bufaddr = PTR_ALIGN(fep->tx_bounce[index], FEC_ALIGNMENT + 1);
+               memcpy(bufaddr, skb->data, skb->len);
        }
 
        /*
@@ -542,6 +537,9 @@ fec_stop(struct net_device *ndev)
                writel(2, fep->hwp + FEC_ECNTRL);
                writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
        }
+
+       netif_stop_queue(ndev);
+       fep->link=0;
 }
 
 
@@ -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.

Re: [PATCH] fec: fix tx bounce handling

[Eric Dumazet]

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.

> Tim
> 
> Heres my patch for 3.3:
> 
> diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> index 336edd7..74d5865 100644
> --- a/drivers/net/ethernet/freescale/fec.c
> +++ b/drivers/net/ethernet/freescale/fec.c
> @@ -284,11 +284,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>         unsigned short  status;
>         unsigned long flags;
>  
> -       if (!fep->link) {



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.

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

[Tim Sander]

Hi
Am Montag 06 Februar 2012, 14:39:59 schrieb Eric Dumazet:
> Le lundi 06 février 2012 à 08:25 -0500, Steven Rostedt a écrit :
> > On Mon, 2012-02-06 at 12:03 +0100, Hector Palacios wrote:
> > > >> @@ -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 ?
> > > 
> > > 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

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 885d8ba..140cb13 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -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);
                return NETDEV_TX_BUSY;
        }
 
@@ -470,6 +471,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 +791,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 +1123,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;