From patchwork Fri Apr 10 00:48:04 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jesse Brandeburg X-Patchwork-Id: 25821 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by ozlabs.org (Postfix) with ESMTP id 23A2EDE1FC for ; Fri, 10 Apr 2009 10:48:47 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935287AbZDJAsJ (ORCPT ); Thu, 9 Apr 2009 20:48:09 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S934825AbZDJAsI (ORCPT ); Thu, 9 Apr 2009 20:48:08 -0400 Received: from mga02.intel.com ([134.134.136.20]:29458 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1764816AbZDJAsF (ORCPT ); Thu, 9 Apr 2009 20:48:05 -0400 Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP; 09 Apr 2009 17:40:27 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.40,164,1239001200"; d="scan'208";a="505184266" Received: from plxs0284.pdx.intel.com ([10.7.76.148]) by orsmga001.jf.intel.com with ESMTP; 09 Apr 2009 17:47:32 -0700 Received: from jbrandeb-desk1.amr.corp.intel.com (jbrandeb-desk1.amr.corp.intel.com [134.134.3.173]) by plxs0284.pdx.intel.com (8.12.11.20060308/8.12.10/MailSET/Hub) with ESMTP id n3A0m4uJ019960; Thu, 9 Apr 2009 17:48:04 -0700 Date: Thu, 9 Apr 2009 17:48:04 -0700 (Pacific Daylight Time) From: "Brandeburg, Jesse" To: David Miller , Andrew Lutomirski cc: "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH 1/1] Tell linkwatch about new interfaces In-Reply-To: Message-ID: References: <20090404.170539.148727646.davem@davemloft.net> User-Agent: Alpine 2.00 (WNT 1167 2008-08-23) ReplyTo: "Brandeburg, Jesse" X-X-Sender: amrjbrandeb@imapmail.glb.intel.com MIME-Version: 1.0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Sat, 4 Apr 2009, Andrew Lutomirski wrote: > On Sat, Apr 4, 2009 at 8:05 PM, David Miller wrote: > > From: Andrew Lutomirski > > Date: Wed, 1 Apr 2009 11:40:06 -0400 > > > >> When a network driver registers a new interface, linkwatch will not notice, > >> and hence not set the rfc2863 operstate, until netif_carrier_on gets called. > >> If the new interface has no carrier when it is connected, then a status of > >> "unknown" is reported to userspace, which confuses various tools > >> (NetworkManager, for example). > >> > >> This fires a linkwatch event for all new interfaces, so that operstate > >> gets set reasonably quickly. > >> > >> Signed-off-by: Andrew Lutomirski > > > > The default assumed state for a freshly registered network > > device is that the link is up. > > > > If that disagrees from reality, the driver should make the > > appropriate netif_carrier_off() call. > > > > I'm sure you'll find that the e1000 driver is not doing this > > and that is what causes the bug you are seeing. > > Dave, if we move the netif_carrier_off call to our dev->open like tg3 has, do you think this is sufficient? I note that we were calling netif_tx_stop_all_queues here, but I'm curious if davem thinks we need to lock out our tx routine until dev->open completes or whether the starting state of the netdev is sufficient. > Nope. The end of e1000_probe (in e1000e) is: > > /* tell the stack to leave us alone until e1000_open() is called */ > netif_carrier_off(netdev); > netif_tx_stop_all_queues(netdev); > > strcpy(netdev->name, "eth%d"); > err = register_netdev(netdev); > if (err) > goto err_register; > > e1000_print_device_info(adapter); > > netif_carrier_off does: > > 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); > } > } > > So, either it should be illegal to call netif_carrier_off on an > unregistered net_device (and there should be a WARN() in > netif_carrier_off), or it should work correctly, and > register_netdevice should do the right thing when there is no carrier > (i.e. something like my patch). does this patch also fix the issue? Tested-by: Andy Lutomirski --- 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 ===== begin ===== e1000e: indicate link down at load From: Jesse Brandeburg same kind of patch as e1000, let driver explicitly push link state once link comes up. Signed-off-by: Jesse Brandeburg --- drivers/net/e1000e/netdev.c | 6 ++---- 1 files changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/net/e1000e/netdev.c b/drivers/net/e1000e/netdev.c index fb78278..6a0411e 100644 --- a/drivers/net/e1000e/netdev.c +++ b/drivers/net/e1000e/netdev.c @@ -3072,6 +3072,8 @@ static int e1000_open(struct net_device *netdev) if (test_bit(__E1000_TESTING, &adapter->state)) return -EBUSY; + netif_carrier_off(netdev); + /* allocate transmit descriptors */ err = e1000e_setup_tx_resources(adapter); if (err) @@ -5006,10 +5008,6 @@ static int __devinit e1000_probe(struct pci_dev *pdev, if (!(adapter->flags & FLAG_HAS_AMT)) e1000_get_hw_control(adapter); - /* tell the stack to leave us alone until e1000_open() is called */ - netif_carrier_off(netdev); - netif_tx_stop_all_queues(netdev); - strcpy(netdev->name, "eth%d"); err = register_netdev(netdev); if (err)