You cannot select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
91 lines
3.0 KiB
Diff
91 lines
3.0 KiB
Diff
From patchwork Fri Jul 21 18:36:26 2017
|
|
Content-Type: text/plain; charset="utf-8"
|
|
MIME-Version: 1.0
|
|
Content-Transfer-Encoding: 7bit
|
|
Subject: [4/5] e1000e: Separate signaling for link check/link up
|
|
From: Benjamin Poirier <bpoirier@suse.com>
|
|
X-Patchwork-Id: 9857491
|
|
Message-Id: <20170721183627.13373-4-bpoirier@suse.com>
|
|
To: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
|
|
Cc: Lennart Sorensen <lsorense@csclub.uwaterloo.ca>,
|
|
intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
|
|
linux-kernel@vger.kernel.org
|
|
Date: Fri, 21 Jul 2017 11:36:26 -0700
|
|
|
|
Lennart reported the following race condition:
|
|
|
|
\ e1000_watchdog_task
|
|
\ e1000e_has_link
|
|
\ hw->mac.ops.check_for_link() === e1000e_check_for_copper_link
|
|
/* link is up */
|
|
mac->get_link_status = false;
|
|
|
|
/* interrupt */
|
|
\ e1000_msix_other
|
|
hw->mac.get_link_status = true;
|
|
|
|
link_active = !hw->mac.get_link_status
|
|
/* link_active is false, wrongly */
|
|
|
|
This problem arises because the single flag get_link_status is used to
|
|
signal two different states: link status needs checking and link status is
|
|
down.
|
|
|
|
Avoid the problem by using the return value of .check_for_link to signal
|
|
the link status to e1000e_has_link().
|
|
|
|
Reported-by: Lennart Sorensen <lsorense@csclub.uwaterloo.ca>
|
|
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
|
|
---
|
|
drivers/net/ethernet/intel/e1000e/mac.c | 11 ++++++++---
|
|
drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-
|
|
2 files changed, 9 insertions(+), 4 deletions(-)
|
|
|
|
--- a/drivers/net/ethernet/intel/e1000e/mac.c
|
|
+++ b/drivers/net/ethernet/intel/e1000e/mac.c
|
|
@@ -410,6 +410,9 @@ void e1000e_clear_hw_cntrs_base(struct e
|
|
* Checks to see of the link status of the hardware has changed. If a
|
|
* change in link status has been detected, then we read the PHY registers
|
|
* to get the current speed/duplex if link exists.
|
|
+ *
|
|
+ * Returns a negative error code (-E1000_ERR_*) or 0 (link down) or 1 (link
|
|
+ * up).
|
|
**/
|
|
s32 e1000e_check_for_copper_link(struct e1000_hw *hw)
|
|
{
|
|
@@ -423,7 +426,7 @@ s32 e1000e_check_for_copper_link(struct
|
|
* Change or Rx Sequence Error interrupt.
|
|
*/
|
|
if (!mac->get_link_status)
|
|
- return 0;
|
|
+ return 1;
|
|
|
|
/* First we want to see if the MII Status Register reports
|
|
* link. If so, then we want to get the current speed/duplex
|
|
@@ -461,10 +464,12 @@ s32 e1000e_check_for_copper_link(struct
|
|
* different link partner.
|
|
*/
|
|
ret_val = e1000e_config_fc_after_link_up(hw);
|
|
- if (ret_val)
|
|
+ if (ret_val) {
|
|
e_dbg("Error configuring flow control\n");
|
|
+ return ret_val;
|
|
+ }
|
|
|
|
- return ret_val;
|
|
+ return 1;
|
|
}
|
|
|
|
/**
|
|
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
|
|
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
|
|
@@ -5056,7 +5056,7 @@ static bool e1000e_has_link(struct e1000
|
|
case e1000_media_type_copper:
|
|
if (hw->mac.get_link_status) {
|
|
ret_val = hw->mac.ops.check_for_link(hw);
|
|
- link_active = !hw->mac.get_link_status;
|
|
+ link_active = ret_val > 0;
|
|
} else {
|
|
link_active = true;
|
|
}
|