[{"id":1725791,"web_url":"http://patchwork.ozlabs.org/comment/1725791/","msgid":"<20170721185002.GV18556@csclub.uwaterloo.ca>","list_archive_url":null,"date":"2017-07-21T18:50:02","subject":"Re: [Intel-wired-lan] [PATCH 4/5] e1000e: Separate signaling for\n\tlink check/link up","submitter":{"id":936,"url":"http://patchwork.ozlabs.org/api/people/936/","name":"Lennart Sorensen","email":"lsorense@csclub.uwaterloo.ca"},"content":"On Fri, Jul 21, 2017 at 11:36:26AM -0700, Benjamin Poirier wrote:\n> Lennart reported the following race condition:\n> \n> \\ e1000_watchdog_task\n> \\ e1000e_has_link\n> \\ hw->mac.ops.check_for_link() === e1000e_check_for_copper_link\n> /* link is up */\n> mac->get_link_status = false;\n> \n> /* interrupt */\n> \\ e1000_msix_other\n> hw->mac.get_link_status = true;\n> \n> link_active = !hw->mac.get_link_status\n> /* link_active is false, wrongly */\n> \n> This problem arises because the single flag get_link_status is used to\n> signal two different states: link status needs checking and link status is\n> down.\n> \n> Avoid the problem by using the return value of .check_for_link to signal\n> the link status to e1000e_has_link().\n> \n> Reported-by: Lennart Sorensen \n> Signed-off-by: Benjamin Poirier \n\nThis too seems potentially -stable worthy, although with patch 5, the\nproblem becomes much much less likely to occur.","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","intel-wired-lan@lists.osuosl.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","intel-wired-lan@lists.osuosl.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=osuosl.org\n\t(client-ip=140.211.166.133; helo=hemlock.osuosl.org;\n\tenvelope-from=intel-wired-lan-bounces@osuosl.org;\n\treceiver=)","Received":["from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xDfwD49ncz9s4s\n\tfor ;\n\tSat, 22 Jul 2017 04:50:20 +1000 (AEST)","from localhost (localhost [127.0.0.1])\n\tby hemlock.osuosl.org (Postfix) with ESMTP id 1CF688A1DC;\n\tFri, 21 Jul 2017 18:50:19 +0000 (UTC)","from hemlock.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id PFM2dUDE6-Qx; Fri, 21 Jul 2017 18:50:15 +0000 (UTC)","from ash.osuosl.org (ash.osuosl.org [140.211.166.34])\n\tby hemlock.osuosl.org (Postfix) with ESMTP id D493B8A17B;\n\tFri, 21 Jul 2017 18:50:15 +0000 (UTC)","from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138])\n\tby ash.osuosl.org (Postfix) with ESMTP id 2E0B01C0964\n\tfor ;\n\tFri, 21 Jul 2017 18:50:12 +0000 (UTC)","from localhost (localhost [127.0.0.1])\n\tby whitealder.osuosl.org (Postfix) with ESMTP id F1C7181FF6\n\tfor ;\n\tFri, 21 Jul 2017 18:50:11 +0000 (UTC)","from whitealder.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id xByXdaqdue3A for ;\n\tFri, 21 Jul 2017 18:50:07 +0000 (UTC)","from caffeine.csclub.uwaterloo.ca (caffeine.csclub.uwaterloo.ca\n\t[129.97.134.17])\n\tby whitealder.osuosl.org (Postfix) with ESMTPS id AE6D781EB7\n\tfor ;\n\tFri, 21 Jul 2017 18:50:04 +0000 (UTC)","by caffeine.csclub.uwaterloo.ca (Postfix, from userid 20367)\n\tid EB32FC006D; Fri, 21 Jul 2017 14:50:02 -0400 (EDT)"],"X-Virus-Scanned":["amavisd-new at osuosl.org","amavisd-new at osuosl.org"],"X-Greylist":"from auto-whitelisted by SQLgrey-1.7.6","Date":"Fri, 21 Jul 2017 14:50:02 -0400","To":"Benjamin Poirier ","Message-ID":"<20170721185002.GV18556@csclub.uwaterloo.ca>","References":"<20170721160937.GA22632@csclub.uwaterloo.ca>\n\t<20170721183627.13373-1-bpoirier@suse.com>\n\t<20170721183627.13373-4-bpoirier@suse.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20170721183627.13373-4-bpoirier@suse.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","From":"lsorense@csclub.uwaterloo.ca (Lennart Sorensen)","Cc":"netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,\n\tlinux-kernel@vger.kernel.org","Subject":"Re: [Intel-wired-lan] [PATCH 4/5] e1000e: Separate signaling for\n\tlink check/link up","X-BeenThere":"intel-wired-lan@osuosl.org","X-Mailman-Version":"2.1.18-1","Precedence":"list","List-Id":"Intel Wired Ethernet Linux Kernel Driver Development\n\t","List-Unsubscribe":", \n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":", \n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"intel-wired-lan-bounces@osuosl.org","Sender":"\"Intel-wired-lan\" "}},{"id":1735909,"web_url":"http://patchwork.ozlabs.org/comment/1735909/","msgid":"<14acedf3-e5d9-31e8-9ff6-fabc2127c021@intel.com>","list_archive_url":null,"date":"2017-08-02T11:28:07","subject":"Re: [Intel-wired-lan] [PATCH 4/5] e1000e: Separate signaling for\n\tlink check/link up","submitter":{"id":69860,"url":"http://patchwork.ozlabs.org/api/people/69860/","name":"Sasha Neftin","email":"sasha.neftin@intel.com"},"content":"On 7/21/2017 21:36, Benjamin Poirier wrote:\n> Lennart reported the following race condition:\n>\n> \\ e1000_watchdog_task\n> \\ e1000e_has_link\n> \\ hw->mac.ops.check_for_link() === e1000e_check_for_copper_link\n> /* link is up */\n> mac->get_link_status = false;\n>\n> /* interrupt */\n> \\ e1000_msix_other\n> hw->mac.get_link_status = true;\n>\n> link_active = !hw->mac.get_link_status\n> /* link_active is false, wrongly */\n>\n> This problem arises because the single flag get_link_status is used to\n> signal two different states: link status needs checking and link status is\n> down.\n>\n> Avoid the problem by using the return value of .check_for_link to signal\n> the link status to e1000e_has_link().\n>\n> Reported-by: Lennart Sorensen \n> Signed-off-by: Benjamin Poirier \n> ---\n> drivers/net/ethernet/intel/e1000e/mac.c | 11 ++++++++---\n> drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-\n> 2 files changed, 9 insertions(+), 4 deletions(-)\n>\n> diff --git a/drivers/net/ethernet/intel/e1000e/mac.c b/drivers/net/ethernet/intel/e1000e/mac.c\n> index b322011ec282..f457c5703d0c 100644\n> --- a/drivers/net/ethernet/intel/e1000e/mac.c\n> +++ b/drivers/net/ethernet/intel/e1000e/mac.c\n> @@ -410,6 +410,9 @@ void e1000e_clear_hw_cntrs_base(struct e1000_hw *hw)\n> * Checks to see of the link status of the hardware has changed. If a\n> * change in link status has been detected, then we read the PHY registers\n> * to get the current speed/duplex if link exists.\n> + *\n> + * Returns a negative error code (-E1000_ERR_*) or 0 (link down) or 1 (link\n> + * up).\n> **/\n> s32 e1000e_check_for_copper_link(struct e1000_hw *hw)\n> {\n> @@ -423,7 +426,7 @@ s32 e1000e_check_for_copper_link(struct e1000_hw *hw)\n> \t * Change or Rx Sequence Error interrupt.\n> \t */\n> \tif (!mac->get_link_status)\n> -\t\treturn 0;\n> +\t\treturn 1;\n> \n> \t/* First we want to see if the MII Status Register reports\n> \t * link. If so, then we want to get the current speed/duplex\n> @@ -461,10 +464,12 @@ s32 e1000e_check_for_copper_link(struct e1000_hw *hw)\n> \t * different link partner.\n> \t */\n> \tret_val = e1000e_config_fc_after_link_up(hw);\n> -\tif (ret_val)\n> +\tif (ret_val) {\n> \t\te_dbg(\"Error configuring flow control\\n\");\n> +\t\treturn ret_val;\n> +\t}\n> \n> -\treturn ret_val;\n> +\treturn 1;\n> }\n> \n> /**\n> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c\n> index fc6a1d9999b2..5a8ab1136566 100644\n> --- a/drivers/net/ethernet/intel/e1000e/netdev.c\n> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c\n> @@ -5081,7 +5081,7 @@ static bool e1000e_has_link(struct e1000_adapter *adapter)\n> \tcase e1000_media_type_copper:\n> \t\tif (hw->mac.get_link_status) {\n> \t\t\tret_val = hw->mac.ops.check_for_link(hw);\n> -\t\t\tlink_active = !hw->mac.get_link_status;\n> +\t\t\tlink_active = ret_val > 0;\n> \t\t} else {\n> \t\t\tlink_active = true;\n> \t\t}\n\nHello Benjamin,\n\nWill this patch fix any serious problem with link indication? Is it \nnecessary? Can we consider your patch series without 4/5 part?","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","intel-wired-lan@lists.osuosl.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","intel-wired-lan@lists.osuosl.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=osuosl.org\n\t(client-ip=140.211.166.136; helo=silver.osuosl.org;\n\tenvelope-from=intel-wired-lan-bounces@osuosl.org;\n\treceiver=)","Received":["from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xMrXj2LCQz9t24\n\tfor ;\n\tWed, 2 Aug 2017 21:28:20 +1000 (AEST)","from localhost (localhost [127.0.0.1])\n\tby silver.osuosl.org (Postfix) with ESMTP id 4C19330BE9;\n\tWed, 2 Aug 2017 11:28:19 +0000 (UTC)","from silver.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id gdQq9V9wBybr; Wed, 2 Aug 2017 11:28:16 +0000 (UTC)","from ash.osuosl.org (ash.osuosl.org [140.211.166.34])\n\tby silver.osuosl.org (Postfix) with ESMTP id 0E867301AD;\n\tWed, 2 Aug 2017 11:28:16 +0000 (UTC)","from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138])\n\tby ash.osuosl.org (Postfix) with ESMTP id 8A2691C0B59\n\tfor ;\n\tWed, 2 Aug 2017 11:28:14 +0000 (UTC)","from localhost (localhost [127.0.0.1])\n\tby whitealder.osuosl.org (Postfix) with ESMTP id 5E04389093\n\tfor ;\n\tWed, 2 Aug 2017 11:28:14 +0000 (UTC)","from whitealder.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id IHqEQzPZBz5n for ;\n\tWed, 2 Aug 2017 11:28:11 +0000 (UTC)","from mga03.intel.com (mga03.intel.com [134.134.136.65])\n\tby whitealder.osuosl.org (Postfix) with ESMTPS id 28B778881C\n\tfor ;\n\tWed, 2 Aug 2017 11:28:11 +0000 (UTC)","from orsmga005.jf.intel.com ([10.7.209.41])\n\tby orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t02 Aug 2017 04:28:10 -0700","from unknown (HELO [10.185.202.57]) ([10.185.202.57])\n\tby orsmga005.jf.intel.com with ESMTP; 02 Aug 2017 04:28:08 -0700"],"X-Virus-Scanned":["amavisd-new at osuosl.org","amavisd-new at osuosl.org"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.41,311,1498546800\"; d=\"scan'208\";a=\"132487450\"","To":"Benjamin Poirier ,\n\tJeff Kirsher ","References":"<20170721160937.GA22632@csclub.uwaterloo.ca>\n\t<20170721183627.13373-1-bpoirier@suse.com>\n\t<20170721183627.13373-4-bpoirier@suse.com>","From":"\"Neftin, Sasha\" ","Message-ID":"<14acedf3-e5d9-31e8-9ff6-fabc2127c021@intel.com>","Date":"Wed, 2 Aug 2017 14:28:07 +0300","User-Agent":"Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101\n\tThunderbird/45.6.0","MIME-Version":"1.0","In-Reply-To":"<20170721183627.13373-4-bpoirier@suse.com>","Cc":"netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,\n\tlinux-kernel@vger.kernel.org,\n\tLennart Sorensen ","Subject":"Re: [Intel-wired-lan] [PATCH 4/5] e1000e: Separate signaling for\n\tlink check/link up","X-BeenThere":"intel-wired-lan@osuosl.org","X-Mailman-Version":"2.1.18-1","Precedence":"list","List-Id":"Intel Wired Ethernet Linux Kernel Driver Development\n\t","List-Unsubscribe":", \n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":", \n\t","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"intel-wired-lan-bounces@osuosl.org","Sender":"\"Intel-wired-lan\" "}},{"id":1736095,"web_url":"http://patchwork.ozlabs.org/comment/1736095/","msgid":"<20170802143437.ggdlmsszqinuvcmc@csclub.uwaterloo.ca>","list_archive_url":null,"date":"2017-08-02T14:34:37","subject":"Re: [Intel-wired-lan] [PATCH 4/5] e1000e: Separate signaling for\n\tlink check/link up","submitter":{"id":936,"url":"http://patchwork.ozlabs.org/api/people/936/","name":"Lennart Sorensen","email":"lsorense@csclub.uwaterloo.ca"},"content":"On Wed, Aug 02, 2017 at 02:28:07PM +0300, Neftin, Sasha wrote:\n> On 7/21/2017 21:36, Benjamin Poirier wrote:\n> > Lennart reported the following race condition:\n> > \n> > \\ e1000_watchdog_task\n> > \\ e1000e_has_link\n> > \\ hw->mac.ops.check_for_link() === e1000e_check_for_copper_link\n> > /* link is up */\n> > mac->get_link_status = false;\n> > \n> > /* interrupt */\n> > \\ e1000_msix_other\n> > hw->mac.get_link_status = true;\n> > \n> > link_active = !hw->mac.get_link_status\n> > /* link_active is false, wrongly */\n> > \n> > This problem arises because the single flag get_link_status is used to\n> > signal two different states: link status needs checking and link status is\n> > down.\n> > \n> > Avoid the problem by using the return value of .check_for_link to signal\n> > the link status to e1000e_has_link().\n> > \n> > Reported-by: Lennart Sorensen \n> > Signed-off-by: Benjamin Poirier \n> > ---\n> > drivers/net/ethernet/intel/e1000e/mac.c | 11 ++++++++---\n> > drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-\n> > 2 files changed, 9 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/drivers/net/ethernet/intel/e1000e/mac.c b/drivers/net/ethernet/intel/e1000e/mac.c\n> > index b322011ec282..f457c5703d0c 100644\n> > --- a/drivers/net/ethernet/intel/e1000e/mac.c\n> > +++ b/drivers/net/ethernet/intel/e1000e/mac.c\n> > @@ -410,6 +410,9 @@ void e1000e_clear_hw_cntrs_base(struct e1000_hw *hw)\n> > * Checks to see of the link status of the hardware has changed. If a\n> > * change in link status has been detected, then we read the PHY registers\n> > * to get the current speed/duplex if link exists.\n> > + *\n> > + * Returns a negative error code (-E1000_ERR_*) or 0 (link down) or 1 (link\n> > + * up).\n> > **/\n> > s32 e1000e_check_for_copper_link(struct e1000_hw *hw)\n> > {\n> > @@ -423,7 +426,7 @@ s32 e1000e_check_for_copper_link(struct e1000_hw *hw)\n> > \t * Change or Rx Sequence Error interrupt.\n> > \t */\n> > \tif (!mac->get_link_status)\n> > -\t\treturn 0;\n> > +\t\treturn 1;\n> > \t/* First we want to see if the MII Status Register reports\n> > \t * link. If so, then we want to get the current speed/duplex\n> > @@ -461,10 +464,12 @@ s32 e1000e_check_for_copper_link(struct e1000_hw *hw)\n> > \t * different link partner.\n> > \t */\n> > \tret_val = e1000e_config_fc_after_link_up(hw);\n> > -\tif (ret_val)\n> > +\tif (ret_val) {\n> > \t\te_dbg(\"Error configuring flow control\\n\");\n> > +\t\treturn ret_val;\n> > +\t}\n> > -\treturn ret_val;\n> > +\treturn 1;\n> > }\n> > /**\n> > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c\n> > index fc6a1d9999b2..5a8ab1136566 100644\n> > --- a/drivers/net/ethernet/intel/e1000e/netdev.c\n> > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c\n> > @@ -5081,7 +5081,7 @@ static bool e1000e_has_link(struct e1000_adapter *adapter)\n> > \tcase e1000_media_type_copper:\n> > \t\tif (hw->mac.get_link_status) {\n> > \t\t\tret_val = hw->mac.ops.check_for_link(hw);\n> > -\t\t\tlink_active = !hw->mac.get_link_status;\n> > +\t\t\tlink_active = ret_val > 0;\n> > \t\t} else {\n> > \t\t\tlink_active = true;\n> > \t\t}\n> \n> Hello Benjamin,\n> \n> Will this patch fix any serious problem with link indication? Is it\n> necessary? Can we consider your patch series without 4/5 part?\n\nWithout this patch, you have the race condition that can make the\nwatchdog_task mistakenly think the link is down when it isn't, and then\nit resets the adapter, which does make the link go down.\n\nSo it is rather catastrophic for the interface.\n\nThe other patch to the interrupt handling should make it never get hit,\nbut the issue does still exist if not fixed and I wouldn't rule out that\nit could possibly still happen even with the other fix in place.","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","intel-wired-lan@lists.osuosl.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","intel-wired-lan@lists.osuosl.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=osuosl.org\n\t(client-ip=140.211.166.138; helo=whitealder.osuosl.org;\n\tenvelope-from=intel-wired-lan-bounces@osuosl.org;\n\treceiver=)","Received":["from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xMwgs6fvWz9t2Q\n\tfor ;\n\tThu, 3 Aug 2017 00:34:49 +1000 (AEST)","from localhost (localhost [127.0.0.1])\n\tby whitealder.osuosl.org (Postfix) with ESMTP id 1E9A189055;\n\tWed, 2 Aug 2017 14:34:46 +0000 (UTC)","from whitealder.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id Z3K1TY0C1AKK; Wed, 2 Aug 2017 14:34:42 +0000 (UTC)","from ash.osuosl.org (ash.osuosl.org [140.211.166.34])\n\tby whitealder.osuosl.org (Postfix) with ESMTP id 9FFC28908F;\n\tWed, 2 Aug 2017 14:34:42 +0000 (UTC)","from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136])\n\tby ash.osuosl.org (Postfix) with ESMTP id 358B81C09DE\n\tfor ;\n\tWed, 2 Aug 2017 14:34:41 +0000 (UTC)","from localhost (localhost [127.0.0.1])\n\tby silver.osuosl.org (Postfix) with ESMTP id 2D90930C4E\n\tfor ;\n\tWed, 2 Aug 2017 14:34:41 +0000 (UTC)","from silver.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id L676qzNjuljK for ;\n\tWed, 2 Aug 2017 14:34:40 +0000 (UTC)","from caffeine.csclub.uwaterloo.ca (caffeine.csclub.uwaterloo.ca\n\t[129.97.134.17])\n\tby silver.osuosl.org (Postfix) with ESMTPS id D66982F6A2\n\tfor ;\n\tWed, 2 Aug 2017 14:34:39 +0000 (UTC)","by caffeine.csclub.uwaterloo.ca (Postfix, from userid 20367)\n\tid D078AC10D7; Wed, 2 Aug 2017 10:34:37 -0400 (EDT)"],"X-Virus-Scanned":["amavisd-new at osuosl.org","amavisd-new at osuosl.org"],"X-Greylist":"from auto-whitelisted by SQLgrey-1.7.6","Date":"Wed, 2 Aug 2017 10:34:37 -0400","To":"\"Neftin, Sasha\" ","Message-ID":"<20170802143437.ggdlmsszqinuvcmc@csclub.uwaterloo.ca>","References":"<20170721160937.GA22632@csclub.uwaterloo.ca>\n\t<20170721183627.13373-1-bpoirier@suse.com>\n\t<20170721183627.13373-4-bpoirier@suse.com>\n\t<14acedf3-e5d9-31e8-9ff6-fabc2127c021@intel.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<14acedf3-e5d9-31e8-9ff6-fabc2127c021@intel.com>","User-Agent":"NeoMutt/20170113 (1.7.2)","From":"lsorense@csclub.uwaterloo.ca (Lennart Sorensen)","Cc":"Benjamin Poirier , netdev@vger.kernel.org,\n\tintel-wired-lan@lists.osuosl.org, linux-kernel@vger.kernel.org","Subject":"Re: [Intel-wired-lan] [PATCH 4/5] e1000e: Separate signaling for\n\tlink check/link up","X-BeenThere":"intel-wired-lan@osuosl.org","X-Mailman-Version":"2.1.18-1","Precedence":"list","List-Id":"Intel Wired Ethernet Linux Kernel Driver Development\n\t","List-Unsubscribe":", \n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":", \n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"intel-wired-lan-bounces@osuosl.org","Sender":"\"Intel-wired-lan\" "}},{"id":1736108,"web_url":"http://patchwork.ozlabs.org/comment/1736108/","msgid":"<20170802144922.txmee23d35o4r7mh@f1.synalogic.ca>","list_archive_url":null,"date":"2017-08-02T14:49:22","subject":"Re: [Intel-wired-lan] [PATCH 4/5] e1000e: Separate signaling for\n\tlink check/link up","submitter":{"id":67008,"url":"http://patchwork.ozlabs.org/api/people/67008/","name":"Benjamin Poirier","email":"bpoirier@suse.com"},"content":"On 2017/08/02 10:34, Lennart Sorensen wrote:\n> On Wed, Aug 02, 2017 at 02:28:07PM +0300, Neftin, Sasha wrote:\n> > On 7/21/2017 21:36, Benjamin Poirier wrote:\n> > > Lennart reported the following race condition:\n> > > \n> > > \\ e1000_watchdog_task\n> > > \\ e1000e_has_link\n> > > \\ hw->mac.ops.check_for_link() === e1000e_check_for_copper_link\n> > > /* link is up */\n> > > mac->get_link_status = false;\n> > > \n> > > /* interrupt */\n> > > \\ e1000_msix_other\n> > > hw->mac.get_link_status = true;\n> > > \n> > > link_active = !hw->mac.get_link_status\n> > > /* link_active is false, wrongly */\n> > > \n> > > This problem arises because the single flag get_link_status is used to\n> > > signal two different states: link status needs checking and link status is\n> > > down.\n> > > \n> > > Avoid the problem by using the return value of .check_for_link to signal\n> > > the link status to e1000e_has_link().\n> > > \n> > > Reported-by: Lennart Sorensen \n> > > Signed-off-by: Benjamin Poirier \n> > > ---\n> > > drivers/net/ethernet/intel/e1000e/mac.c | 11 ++++++++---\n> > > drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-\n> > > 2 files changed, 9 insertions(+), 4 deletions(-)\n> > > \n> > > diff --git a/drivers/net/ethernet/intel/e1000e/mac.c b/drivers/net/ethernet/intel/e1000e/mac.c\n> > > index b322011ec282..f457c5703d0c 100644\n> > > --- a/drivers/net/ethernet/intel/e1000e/mac.c\n> > > +++ b/drivers/net/ethernet/intel/e1000e/mac.c\n> > > @@ -410,6 +410,9 @@ void e1000e_clear_hw_cntrs_base(struct e1000_hw *hw)\n> > > * Checks to see of the link status of the hardware has changed. If a\n> > > * change in link status has been detected, then we read the PHY registers\n> > > * to get the current speed/duplex if link exists.\n> > > + *\n> > > + * Returns a negative error code (-E1000_ERR_*) or 0 (link down) or 1 (link\n> > > + * up).\n> > > **/\n> > > s32 e1000e_check_for_copper_link(struct e1000_hw *hw)\n> > > {\n> > > @@ -423,7 +426,7 @@ s32 e1000e_check_for_copper_link(struct e1000_hw *hw)\n> > > \t * Change or Rx Sequence Error interrupt.\n> > > \t */\n> > > \tif (!mac->get_link_status)\n> > > -\t\treturn 0;\n> > > +\t\treturn 1;\n> > > \t/* First we want to see if the MII Status Register reports\n> > > \t * link. If so, then we want to get the current speed/duplex\n> > > @@ -461,10 +464,12 @@ s32 e1000e_check_for_copper_link(struct e1000_hw *hw)\n> > > \t * different link partner.\n> > > \t */\n> > > \tret_val = e1000e_config_fc_after_link_up(hw);\n> > > -\tif (ret_val)\n> > > +\tif (ret_val) {\n> > > \t\te_dbg(\"Error configuring flow control\\n\");\n> > > +\t\treturn ret_val;\n> > > +\t}\n> > > -\treturn ret_val;\n> > > +\treturn 1;\n> > > }\n> > > /**\n> > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c\n> > > index fc6a1d9999b2..5a8ab1136566 100644\n> > > --- a/drivers/net/ethernet/intel/e1000e/netdev.c\n> > > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c\n> > > @@ -5081,7 +5081,7 @@ static bool e1000e_has_link(struct e1000_adapter *adapter)\n> > > \tcase e1000_media_type_copper:\n> > > \t\tif (hw->mac.get_link_status) {\n> > > \t\t\tret_val = hw->mac.ops.check_for_link(hw);\n> > > -\t\t\tlink_active = !hw->mac.get_link_status;\n> > > +\t\t\tlink_active = ret_val > 0;\n> > > \t\t} else {\n> > > \t\t\tlink_active = true;\n> > > \t\t}\n> > \n> > Hello Benjamin,\n> > \n> > Will this patch fix any serious problem with link indication? Is it\n> > necessary? Can we consider your patch series without 4/5 part?\n> \n> Without this patch, you have the race condition that can make the\n> watchdog_task mistakenly think the link is down when it isn't, and then\n> it resets the adapter, which does make the link go down.\n> \n> So it is rather catastrophic for the interface.\n> \n> The other patch to the interrupt handling should make it never get hit,\n> but the issue does still exist if not fixed and I wouldn't rule out that\n> it could possibly still happen even with the other fix in place.\n\nExactly. I wouldn't have explained it better, thanks.","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","intel-wired-lan@lists.osuosl.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","intel-wired-lan@lists.osuosl.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=osuosl.org\n\t(client-ip=140.211.166.137; helo=fraxinus.osuosl.org;\n\tenvelope-from=intel-wired-lan-bounces@osuosl.org;\n\treceiver=)","Received":["from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xMx102fMXz9t0F\n\tfor ;\n\tThu, 3 Aug 2017 00:49:39 +1000 (AEST)","from localhost (localhost [127.0.0.1])\n\tby fraxinus.osuosl.org (Postfix) with ESMTP id 9CB7C86C9D;\n\tWed, 2 Aug 2017 14:49:37 +0000 (UTC)","from fraxinus.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id lUcJLTXgjeLw; Wed, 2 Aug 2017 14:49:35 +0000 (UTC)","from ash.osuosl.org (ash.osuosl.org [140.211.166.34])\n\tby fraxinus.osuosl.org (Postfix) with ESMTP id DA1D986DCD;\n\tWed, 2 Aug 2017 14:49:35 +0000 (UTC)","from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133])\n\tby ash.osuosl.org (Postfix) with ESMTP id A1BD41C09DE\n\tfor ;\n\tWed, 2 Aug 2017 14:49:34 +0000 (UTC)","from localhost (localhost [127.0.0.1])\n\tby hemlock.osuosl.org (Postfix) with ESMTP id 9A0008A2C7\n\tfor ;\n\tWed, 2 Aug 2017 14:49:34 +0000 (UTC)","from hemlock.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id 21S2R+7fSZY5 for ;\n\tWed, 2 Aug 2017 14:49:33 +0000 (UTC)","from mx1.suse.de (mx2.suse.de [195.135.220.15])\n\tby hemlock.osuosl.org (Postfix) with ESMTPS id 6F4E08A20A\n\tfor ;\n\tWed, 2 Aug 2017 14:49:33 +0000 (UTC)","from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254])\n\tby mx1.suse.de (Postfix) with ESMTP id D1FB3AEEB;\n\tWed, 2 Aug 2017 14:49:30 +0000 (UTC)"],"X-Virus-Scanned":["amavisd-new at osuosl.org","amavisd-new at osuosl.org","by amavisd-new at test-mx.suse.de"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","Date":"Wed, 2 Aug 2017 07:49:22 -0700","From":"Benjamin Poirier ","To":"Lennart Sorensen ","Message-ID":"<20170802144922.txmee23d35o4r7mh@f1.synalogic.ca>","References":"<20170721160937.GA22632@csclub.uwaterloo.ca>\n\t<20170721183627.13373-1-bpoirier@suse.com>\n\t<20170721183627.13373-4-bpoirier@suse.com>\n\t<14acedf3-e5d9-31e8-9ff6-fabc2127c021@intel.com>\n\t<20170802143437.ggdlmsszqinuvcmc@csclub.uwaterloo.ca>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20170802143437.ggdlmsszqinuvcmc@csclub.uwaterloo.ca>","User-Agent":"NeoMutt/20170421 (1.8.2)","Cc":"netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,\n\tlinux-kernel@vger.kernel.org","Subject":"Re: [Intel-wired-lan] [PATCH 4/5] e1000e: Separate signaling for\n\tlink check/link up","X-BeenThere":"intel-wired-lan@osuosl.org","X-Mailman-Version":"2.1.18-1","Precedence":"list","List-Id":"Intel Wired Ethernet Linux Kernel Driver Development\n\t","List-Unsubscribe":", \n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":", \n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"intel-wired-lan-bounces@osuosl.org","Sender":"\"Intel-wired-lan\" "}},{"id":1768884,"web_url":"http://patchwork.ozlabs.org/comment/1768884/","msgid":"<309B89C4C689E141A5FF6A0C5FB2118B8C6A05C9@ORSMSX101.amr.corp.intel.com>","list_archive_url":null,"date":"2017-09-15T00:27:46","subject":"Re: [Intel-wired-lan] [PATCH 4/5] e1000e: Separate signaling for\n\tlink check/link up","submitter":{"id":37025,"url":"http://patchwork.ozlabs.org/api/people/37025/","name":"Brown, Aaron F","email":"aaron.f.brown@intel.com"},"content":"On 7/21/2017 21:36, Benjamin Poirier wrote:\n> Lennart reported the following race condition:\n>\n> \\ e1000_watchdog_task\n> \\ e1000e_has_link\n> \\ hw->mac.ops.check_for_link() === e1000e_check_for_copper_link\n> /* link is up */\n> mac->get_link_status = false;\n>\n> /* interrupt */\n> \\ e1000_msix_other\n> hw->mac.get_link_status = true;\n>\n> link_active = !hw->mac.get_link_status\n> /* link_active is false, wrongly */\n>\n> This problem arises because the single flag get_link_status is used to\n> signal two different states: link status needs checking and link status is\n> down.\n>\n> Avoid the problem by using the return value of .check_for_link to signal\n> the link status to e1000e_has_link().\n>\n> Reported-by: Lennart Sorensen \n> Signed-off-by: Benjamin Poirier \n> ---\n> drivers/net/ethernet/intel/e1000e/mac.c | 11 ++++++++---\n> drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-\n> 2 files changed, 9 insertions(+), 4 deletions(-)\n\nTested-by: Aaron Brown ","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","intel-wired-lan@lists.osuosl.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","intel-wired-lan@lists.osuosl.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=osuosl.org\n\t(client-ip=140.211.166.133; helo=hemlock.osuosl.org;\n\tenvelope-from=intel-wired-lan-bounces@osuosl.org;\n\treceiver=)","Received":["from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xtbpL5JHrz9sxR\n\tfor ;\n\tFri, 15 Sep 2017 10:27:54 +1000 (AEST)","from localhost (localhost [127.0.0.1])\n\tby hemlock.osuosl.org (Postfix) with ESMTP id AB5348AE26;\n\tFri, 15 Sep 2017 00:27:52 +0000 (UTC)","from hemlock.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id 4uuZN6i2l558; Fri, 15 Sep 2017 00:27:51 +0000 (UTC)","from ash.osuosl.org (ash.osuosl.org [140.211.166.34])\n\tby hemlock.osuosl.org (Postfix) with ESMTP id 9C9CF8AE22;\n\tFri, 15 Sep 2017 00:27:51 +0000 (UTC)","from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137])\n\tby ash.osuosl.org (Postfix) with ESMTP id 70CF21C26AA\n\tfor ;\n\tFri, 15 Sep 2017 00:27:50 +0000 (UTC)","from localhost (localhost [127.0.0.1])\n\tby fraxinus.osuosl.org (Postfix) with ESMTP id 686B588FEC\n\tfor ;\n\tFri, 15 Sep 2017 00:27:50 +0000 (UTC)","from fraxinus.osuosl.org ([127.0.0.1])\n\tby localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id rbxfalDUA8qc for ;\n\tFri, 15 Sep 2017 00:27:49 +0000 (UTC)","from mga01.intel.com (mga01.intel.com [192.55.52.88])\n\tby fraxinus.osuosl.org (Postfix) with ESMTPS id 9E49B88FE9\n\tfor ;\n\tFri, 15 Sep 2017 00:27:49 +0000 (UTC)","from orsmga005.jf.intel.com ([10.7.209.41])\n\tby fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t14 Sep 2017 17:27:48 -0700","from orsmsx106.amr.corp.intel.com ([10.22.225.133])\n\tby orsmga005.jf.intel.com with ESMTP; 14 Sep 2017 17:27:48 -0700","from orsmsx160.amr.corp.intel.com (10.22.226.43) by\n\tORSMSX106.amr.corp.intel.com (10.22.225.133) with Microsoft SMTP\n\tServer (TLS) id 14.3.319.2; Thu, 14 Sep 2017 17:27:47 -0700","from orsmsx101.amr.corp.intel.com ([169.254.8.156]) by\n\tORSMSX160.amr.corp.intel.com ([169.254.13.61]) with mapi id\n\t14.03.0319.002; Thu, 14 Sep 2017 17:27:47 -0700"],"X-Virus-Scanned":["amavisd-new at osuosl.org","amavisd-new at osuosl.org"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.42,395,1500966000\"; d=\"scan'208\";a=\"149382344\"","From":"\"Brown, Aaron F\" ","To":"Benjamin Poirier ","Thread-Topic":"[Intel-wired-lan] [PATCH 4/5] e1000e: Separate signaling for\n\tlink check/link up","Thread-Index":"AQHTAlBoyKFCWZq1akqEK1QqfoE4T6JxdYuAgAA0G4CAAAQfAIBDv+WQ","Date":"Fri, 15 Sep 2017 00:27:46 +0000","Message-ID":"<309B89C4C689E141A5FF6A0C5FB2118B8C6A05C9@ORSMSX101.amr.corp.intel.com>","References":"<20170721160937.GA22632@csclub.uwaterloo.ca>\n\t<20170721183627.13373-1-bpoirier@suse.com>\n\t<20170721183627.13373-4-bpoirier@suse.com>\n\t<14acedf3-e5d9-31e8-9ff6-fabc2127c021@intel.com>\n\t<20170802143437.ggdlmsszqinuvcmc@csclub.uwaterloo.ca>\n\t<20170802144922.txmee23d35o4r7mh@f1.synalogic.ca>","In-Reply-To":"<20170802144922.txmee23d35o4r7mh@f1.synalogic.ca>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","dlp-product":"dlpe-windows","dlp-version":"11.0.0.116","dlp-reaction":"no-action","x-originating-ip":"[10.22.254.139]","MIME-Version":"1.0","Cc":"\"netdev@vger.kernel.org\" ,\n\t\"intel-wired-lan@lists.osuosl.org\" ,\n\t\"linux-kernel@vger.kernel.org\" , Lennart\n\tSorensen ","Subject":"Re: [Intel-wired-lan] [PATCH 4/5] e1000e: Separate signaling for\n\tlink check/link up","X-BeenThere":"intel-wired-lan@osuosl.org","X-Mailman-Version":"2.1.18-1","Precedence":"list","List-Id":"Intel Wired Ethernet Linux Kernel Driver Development\n\t","List-Unsubscribe":", \n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":", \n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"intel-wired-lan-bounces@osuosl.org","Sender":"\"Intel-wired-lan\" "}}]