Patchwork net/fec: call netif_carrier_off when not having link

login
register
mail settings
Submitter Uwe Kleine-König
Date July 25, 2013, 1:27 p.m.
Message ID <1374758875-7926-1-git-send-email-u.kleine-koenig@pengutronix.de>
Download mbox | patch
Permalink /patch/261694/
State Superseded
Delegated to: David Miller
Headers show

Comments

Uwe Kleine-König - July 25, 2013, 1:27 p.m.
Without this patch I see a machine running an -rt patched Linux being
stuck in sch_direct_xmit when it looses link while there is still a
packet to be send. In this case the fec_enet_start_xmit routine returns
NETDEV_TX_BUSY which makes the network stack reschedule the packet and
so sch_direct_xmit calls fec_enet_start_xmit again.

The right fix is to tell the network stack that the link was lost (using
netif_carrier_off) so that it doesn't even try to get rid of the pending
packets.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

this resembles a problem described on the linux-rt mailing list some
time ago:

	http://thread.gmane.org/gmane.linux.rt.user/7815

So I Cc:d the people that participated in that thread.

IMHO this is 3.11 material (assuming I did it right) and is even worth
backporting as I think the problem exists in mainline, too, just harder
to trigger.

Best regards
Uwe

 drivers/net/ethernet/freescale/fec_main.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
stephen hemminger - July 25, 2013, 4:03 p.m.
On Thu, 25 Jul 2013 15:27:55 +0200
Uwe Kleine-König  <u.kleine-koenig@pengutronix.de> wrote:

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 0642006..631bd5a 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -280,11 +280,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	unsigned short	status;
>  	unsigned int index;
>  
> -	if (!fep->link) {
> -		/* Link is down or auto-negotiation is in progress. */
> -		return NETDEV_TX_BUSY;
> -	}
> -

That is a bug anyway. Since it would cause spin loop in transmit
code (even without -rt). If the driver cared to test it (most
drivers just let hardware deal with this situation), then it should
free packet and return TX_OK.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Duan Fugang-B38611 - July 26, 2013, 9:35 a.m.
On Fir, 26 Jul 2013 12:04, Stephen Hemminger wrote:
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c 
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 0642006..631bd5a 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -280,11 +280,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>>  	unsigned short	status;
>>  	unsigned int index;
>>  
>> -	if (!fep->link) {
>> -		/* Link is down or auto-negotiation is in progress. */
>> -		return NETDEV_TX_BUSY;
>> -	}
>> -
>
>That is a bug anyway. Since it would cause spin loop in transmit code (even without -rt).
>If the driver cared to test it (most drivers just let hardware deal with this situation),
>then it should free packet and return TX_OK.

When link is down, the logic is
	- call netif_stop_queue() to stop queue
	- and then notify there have no link using netif_carrier_off().

But the flow must be handled in fec_enet_adjust_link() function, not in xmit().


Thanks,
Andy

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Fainelli - July 26, 2013, 10:06 a.m.
Hello,

2013/7/26 Duan Fugang-B38611 <B38611@freescale.com>:
> On Fir, 26 Jul 2013 12:04, Stephen Hemminger wrote:
>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>> b/drivers/net/ethernet/freescale/fec_main.c
>>> index 0642006..631bd5a 100644
>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>> @@ -280,11 +280,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>>>      unsigned short  status;
>>>      unsigned int index;
>>>
>>> -    if (!fep->link) {
>>> -            /* Link is down or auto-negotiation is in progress. */
>>> -            return NETDEV_TX_BUSY;
>>> -    }
>>> -
>>
>>That is a bug anyway. Since it would cause spin loop in transmit code (even without -rt).
>>If the driver cared to test it (most drivers just let hardware deal with this situation),
>>then it should free packet and return TX_OK.
>
> When link is down, the logic is
>         - call netif_stop_queue() to stop queue
>         - and then notify there have no link using netif_carrier_off().

netif_carrier_off() is handled by the PHY library before calling the
adjust_link callback in FEC.

>
> But the flow must be handled in fec_enet_adjust_link() function, not in xmit().

What is special about this hardware that fec_restart() needs to do all
that work? Can't you just update the duplex/speed/pause settings
directly without doing a full hardware re-init? Also the comment above
it is wrong, fec_restart() is not just called when the duplex changes,
it is called when the link/speed/duplex settings changed and when the
workqueue is scheduled from fec_timeout().
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sascha Hauer - July 26, 2013, 10:52 a.m.
On Fri, Jul 26, 2013 at 11:06:06AM +0100, Florian Fainelli wrote:
> Hello,
> 
> 2013/7/26 Duan Fugang-B38611 <B38611@freescale.com>:
> > On Fir, 26 Jul 2013 12:04, Stephen Hemminger wrote:
> >>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> >>> b/drivers/net/ethernet/freescale/fec_main.c
> >>> index 0642006..631bd5a 100644
> >>> --- a/drivers/net/ethernet/freescale/fec_main.c
> >>> +++ b/drivers/net/ethernet/freescale/fec_main.c
> >>> @@ -280,11 +280,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> >>>      unsigned short  status;
> >>>      unsigned int index;
> >>>
> >>> -    if (!fep->link) {
> >>> -            /* Link is down or auto-negotiation is in progress. */
> >>> -            return NETDEV_TX_BUSY;
> >>> -    }
> >>> -
> >>
> >>That is a bug anyway. Since it would cause spin loop in transmit code (even without -rt).
> >>If the driver cared to test it (most drivers just let hardware deal with this situation),
> >>then it should free packet and return TX_OK.
> >
> > When link is down, the logic is
> >         - call netif_stop_queue() to stop queue
> >         - and then notify there have no link using netif_carrier_off().
> 
> netif_carrier_off() is handled by the PHY library before calling the
> adjust_link callback in FEC.
> 
> >
> > But the flow must be handled in fec_enet_adjust_link() function, not in xmit().
> 
> What is special about this hardware that fec_restart() needs to do all
> that work?

The special thing is that the driver is written in a braindamaged way.
fec_restart is kind of a suicide-and-resurrect function which is called
whenever something changes.

> Can't you just update the duplex/speed/pause settings
> directly without doing a full hardware re-init?

Yes, that's possible. I made some initial patches for this which we'll
probably send soon.

Sascha
Ben Hutchings - July 26, 2013, 3:31 p.m.
On Fri, 2013-07-26 at 09:35 +0000, Duan Fugang-B38611 wrote:
> On Fir, 26 Jul 2013 12:04, Stephen Hemminger wrote:
> >> diff --git a/drivers/net/ethernet/freescale/fec_main.c 
> >> b/drivers/net/ethernet/freescale/fec_main.c
> >> index 0642006..631bd5a 100644
> >> --- a/drivers/net/ethernet/freescale/fec_main.c
> >> +++ b/drivers/net/ethernet/freescale/fec_main.c
> >> @@ -280,11 +280,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> >>  	unsigned short	status;
> >>  	unsigned int index;
> >>  
> >> -	if (!fep->link) {
> >> -		/* Link is down or auto-negotiation is in progress. */
> >> -		return NETDEV_TX_BUSY;
> >> -	}
> >> -
> >
> >That is a bug anyway. Since it would cause spin loop in transmit code (even without -rt).
> >If the driver cared to test it (most drivers just let hardware deal with this situation),
> >then it should free packet and return TX_OK.
> 
> When link is down, the logic is
> 	- call netif_stop_queue() to stop queue
> 	- and then notify there have no link using netif_carrier_off().

netif_stop_queue() *must not* be called before netif_carrier_off(),
otherwise the TX watchdog can fire immediately.  The TX watchdog only
knows when the last packet was passed to the driver, not when the queue
was stopped.  The last packet could have been added an arbitrarily long
time before the link went down, therefore it may appear that the timeout
has already expired..

Although it is safe to call netif_stop_queue() after
netif_carrier_off(), it is not useful.  netif_stop_queue() should only
be called from your ndo_start_xmit operation and only because the queue
is full.  Any other reason to stop should be communicated to the kernel
using netif_carrier_off() or netif_device_detach().

Ben.

> But the flow must be handled in fec_enet_adjust_link() function, not in xmit().
Duan Fugang-B38611 - July 29, 2013, 2:10 a.m.
>netif_stop_queue() *must not* be called before netif_carrier_off(), otherwise the TX watchdog can fire immediately.

>The TX watchdog only knows when the last packet was passed to the driver, not when the queue was stopped.

>The last packet could have been added an arbitrarily long time before the link went down, therefore it may appear that the timeout has already expired..

>

>Although it is safe to call netif_stop_queue() after netif_carrier_off(), it is not useful.

>netif_stop_queue() should only be called from your ndo_start_xmit operation and only because the queue is full. 

>Any other reason to stop should be communicated to the kernel using netif_carrier_off() or netif_device_detach().

>

>Ben.


Agree.
I remember you said:
The watchdog fires when the software queue has been stopped *and* the link has been reported as up for over dev->watchdog_timeo ticks.
The software queue should be stopped if the hardware queue is full or nearly full.  If the software queue remains stopped and the link is
still reported up, then one of these things is happening:

1. The link went down but the driver didn't notice, or sent a transmit packet which never completes
2. TX completions are not being indicated or handled correctly
3. The hardware TX path has locked up
4. The link is stalled by excessive pause frames or collisions
5. Timeout is too low and/or low watermark is too high
(there may be other explanations)

The watchdog is primarily meant to deal with case 3, though all of cases 1-3 may be worked around by resetting the hardware.


Thanks,
Andy

-----Original Message-----
From: Ben Hutchings [mailto:bhutchings@solarflare.com] 

Sent: Friday, July 26, 2013 11:32 PM
To: Duan Fugang-B38611
Cc: Stephen Hemminger; Uwe Kleine-König; netdev@vger.kernel.org; David S. Miller; Estevam Fabio-R49496; Li Frank-B20596; Shawn Guo; kernel@pengutronix.de; Hector Palacios; Tim Sander; Steven Rostedt; Thomas Gleixner
Subject: Re: [PATCH] net/fec: call netif_carrier_off when not having link

On Fri, 2013-07-26 at 09:35 +0000, Duan Fugang-B38611 wrote:
> On Fir, 26 Jul 2013 12:04, Stephen Hemminger wrote:

> >> diff --git a/drivers/net/ethernet/freescale/fec_main.c

> >> b/drivers/net/ethernet/freescale/fec_main.c

> >> index 0642006..631bd5a 100644

> >> --- a/drivers/net/ethernet/freescale/fec_main.c

> >> +++ b/drivers/net/ethernet/freescale/fec_main.c

> >> @@ -280,11 +280,6 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)

> >>  	unsigned short	status;

> >>  	unsigned int index;

> >>  

> >> -	if (!fep->link) {

> >> -		/* Link is down or auto-negotiation is in progress. */

> >> -		return NETDEV_TX_BUSY;

> >> -	}

> >> -

> >

> >That is a bug anyway. Since it would cause spin loop in transmit code (even without -rt).

> >If the driver cared to test it (most drivers just let hardware deal 

> >with this situation), then it should free packet and return TX_OK.

> 

> When link is down, the logic is

> 	- call netif_stop_queue() to stop queue

> 	- and then notify there have no link using netif_carrier_off().


netif_stop_queue() *must not* be called before netif_carrier_off(), otherwise the TX watchdog can fire immediately.  The TX watchdog only knows when the last packet was passed to the driver, not when the queue was stopped.  The last packet could have been added an arbitrarily long time before the link went down, therefore it may appear that the timeout has already expired..

Although it is safe to call netif_stop_queue() after netif_carrier_off(), it is not useful.  netif_stop_queue() should only be called from your ndo_start_xmit operation and only because the queue is full.  Any other reason to stop should be communicated to the kernel using netif_carrier_off() or netif_device_detach().

Ben.

> But the flow must be handled in fec_enet_adjust_link() function, not in xmit().


--
Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Patch

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 0642006..631bd5a 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -280,11 +280,6 @@  fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	unsigned short	status;
 	unsigned int index;
 
-	if (!fep->link) {
-		/* Link is down or auto-negotiation is in progress. */
-		return NETDEV_TX_BUSY;
-	}
-
 	/* Fill in a Tx ring entry */
 	bdp = fep->cur_tx;
 
@@ -459,6 +454,11 @@  fec_restart(struct net_device *ndev, int duplex)
 		netif_tx_lock_bh(ndev);
 	}
 
+	if (!fep->link)
+		netif_carrier_off(ndev);
+	else
+		netif_carrier_on(ndev);
+
 	/* Whack a reset.  We should wait for this. */
 	writel(1, fep->hwp + FEC_ECNTRL);
 	udelay(10);