[PATCH] fix for initial link state in 2.6.28

[PATCH] fix for initial link state in 2.6.28

[Michael Marineau]

[-- Attachment #1: Type: text/plain, Size: 2066 bytes --]

Greetings,

Commit b47300168e770b60ab96c8924854c3b0eb4260eb "Do not fire linkwatch
events until the device is registered." was made as a workaround for
drivers that call netif_carrier_off before registering the device.
Unfortunately this causes these drivers to incorrectly report their
link status as IF_OPER_UNKNOWN which can falsely set the IFF_RUNNING
flag when the interface is first brought up. This issues was
previously pointed out[1] but was dismissed saying that IFF_RUNNING is
not related to the link status. From my digging IFF_RUNNING, as
reported to userspace, is based on the link state. It is set based on
__LINK_STATE_START and IF_OPER_UP or IF_OPER_UNKNOWN. See [2], [3],
and [4]. (Whether or not the kernel has IFF_RUNNING set in flags is
not reported to user space so it may well be independent of the link,
I don't know if and when it may get set.)

The end result depends slightly depending on the driver. The the two I
tested were e1000e and b44. With e1000e if the system is booted
without a network cable attached the interface will falsely report
RUNNING when it is brought up causing NetworkManager to attempt to
start it and eventually time out. With b44 when the system is booted
with a network cable attached and brought up with dhcpcd it will time
out the first time.

The attached patch that will still set the operstate variable
correctly to IF_OPER_UP/DOWN/etc when linkwatch_fire_event is called
but then return rather than skipping the linkwatch_fire_event call
entirely as the previous fix did. (sorry it isn't inline, I don't have
a patch friendly email client at the moment)

What ever the final fix for this is, it should probably be pushed into
the stable tree since this is a minor but annoying regression in
2.6.28.

[1] http://marc.info/?l=linux-netdev&m=122922534630000&w=2
[2] http://lxr.linux.no/linux+v2.6.28/net/core/dev.c#L3349
[3] http://lxr.linux.no/linux+v2.6.28/include/linux/netdevice.h#L1147
[4] http://lxr.linux.no/linux+v2.6.28/include/linux/netdevice.h#L1376

Cheers,
-- 
Michael Marineau
mike@marineau.org

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: operstate-fix.patch --]
[-- Type: text/x-patch; name=operstate-fix.patch, Size: 1493 bytes --]

diff --git a/net/core/link_watch.c b/net/core/link_watch.c
index 183d180..1e401e1 100644
--- a/net/core/link_watch.c
+++ b/net/core/link_watch.c
@@ -179,7 +178,6 @@ static void __linkwatch_run_queue(int urgent_only)
 		 */
 		clear_bit(__LINK_STATE_LINKWATCH_PENDING, &dev->state);
 
-		rfc2863_policy(dev);
 		if (dev->flags & IFF_UP) {
 			if (netif_carrier_ok(dev))
 				dev_activate(dev);
@@ -216,6 +214,12 @@ void linkwatch_fire_event(struct net_device *dev)
 {
 	bool urgent = linkwatch_urgent_event(dev);
 
+	rfc2863_policy(dev);
+
+	/* Some drivers call netif_carrier_off early */
+	if (dev->reg_state == NETREG_UNINITIALIZED)
+		return;
+
 	if (!test_and_set_bit(__LINK_STATE_LINKWATCH_PENDING, &dev->state)) {
 		dev_hold(dev);
 
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index cdcd16f..3ea6741 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -270,8 +270,6 @@ static void dev_watchdog_down(struct net_device *dev)
 void netif_carrier_on(struct net_device *dev)
 {
 	if (test_and_clear_bit(__LINK_STATE_NOCARRIER, &dev->state)) {
-		if (dev->reg_state == NETREG_UNINITIALIZED)
-			return;
 		linkwatch_fire_event(dev);
 		if (netif_running(dev))
 			__netdev_watchdog_up(dev);
@@ -288,8 +286,6 @@ EXPORT_SYMBOL(netif_carrier_on);
 void netif_carrier_off(struct net_device *dev)
 {
 	if (!test_and_set_bit(__LINK_STATE_NOCARRIER, &dev->state)) {
-		if (dev->reg_state == NETREG_UNINITIALIZED)
-			return;
 		linkwatch_fire_event(dev);
 	}
 }

Re: [PATCH] fix for initial link state in 2.6.28

[David Miller]

I find it mysterious how someone can write such an excellent
patch and such a well written commit message, then in the
end not give a proper signoff for their changes :-/

Applied, and queued up for -stable, but please provide proper
signoffs in the future.

Re: [PATCH] fix for initial link state in 2.6.28

[Michael Marineau]

On Sun, Jan 4, 2009 at 8:18 PM, David Miller <davem@davemloft.net> wrote:
>
> I find it mysterious how someone can write such an excellent
> patch and such a well written commit message, then in the
> end not give a proper signoff for their changes :-/

Being completely unfamiliar with network drivers I was a bit fried
after tracing though enough code to finally understand what was going
on so it slipped my mind. :-)

>
> Applied, and queued up for -stable, but please provide proper
> signoffs in the future.

Noted. Thanks for queuing it up!
-- 
Michael Marineau
mike@marineau.org