RE: [PATCH 2/2] e1000e: Fix link check race condition
> From: netdev-ow...@vger.kernel.org [mailto:netdev- > ow...@vger.kernel.org] On Behalf Of Benjamin Poirier > Sent: Monday, March 5, 2018 5:56 PM > To: Kirsher, Jeffrey T <jeffrey.t.kirs...@intel.com> > Cc: Alexander Duyck <alexander.du...@gmail.com>; Lennart Sorensen > <lsore...@csclub.uwaterloo.ca>; intel-wired-...@lists.osuosl.org; > net...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: [PATCH 2/2] e1000e: Fix link check race condition > > Alex reported the following race condition: > > /* link goes up... interrupt... schedule watchdog */ > \ e1000_watchdog_task > \ e1000e_has_link > \ hw->mac.ops.check_for_link() === > e1000e_check_for_copper_link > \ e1000e_phy_has_link_generic(..., ) > link = true > >/* link goes down... interrupt */ >\ e1000_msix_other >hw->mac.get_link_status = > true > > /* link is up */ > mac->get_link_status = false > > link_active = true > /* link_active is true, wrongly, and stays so because >* get_link_status is false */ > > Avoid this problem by making sure that we don't set get_link_status = false > after having checked the link. > > It seems this problem has been present since the introduction of e1000e. > > Link: https://lkml.org/lkml/2018/1/29/338 > Reported-by: Alexander Duyck <alexander.du...@gmail.com> > Signed-off-by: Benjamin Poirier <bpoir...@suse.com> > --- > drivers/net/ethernet/intel/e1000e/ich8lan.c | 31 --- > -- > drivers/net/ethernet/intel/e1000e/mac.c | 14 ++--- > 2 files changed, 24 insertions(+), 21 deletions(-) Tested-by: Aaron Brown <aaron.f.br...@intel.com>
RE: [PATCH 2/2] e1000e: Fix link check race condition
> From: netdev-ow...@vger.kernel.org [mailto:netdev- > ow...@vger.kernel.org] On Behalf Of Benjamin Poirier > Sent: Monday, March 5, 2018 5:56 PM > To: Kirsher, Jeffrey T > Cc: Alexander Duyck ; Lennart Sorensen > ; intel-wired-...@lists.osuosl.org; > net...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: [PATCH 2/2] e1000e: Fix link check race condition > > Alex reported the following race condition: > > /* link goes up... interrupt... schedule watchdog */ > \ e1000_watchdog_task > \ e1000e_has_link > \ hw->mac.ops.check_for_link() === > e1000e_check_for_copper_link > \ e1000e_phy_has_link_generic(..., ) > link = true > >/* link goes down... interrupt */ >\ e1000_msix_other >hw->mac.get_link_status = > true > > /* link is up */ > mac->get_link_status = false > > link_active = true > /* link_active is true, wrongly, and stays so because >* get_link_status is false */ > > Avoid this problem by making sure that we don't set get_link_status = false > after having checked the link. > > It seems this problem has been present since the introduction of e1000e. > > Link: https://lkml.org/lkml/2018/1/29/338 > Reported-by: Alexander Duyck > Signed-off-by: Benjamin Poirier > --- > drivers/net/ethernet/intel/e1000e/ich8lan.c | 31 --- > -- > drivers/net/ethernet/intel/e1000e/mac.c | 14 ++--- > 2 files changed, 24 insertions(+), 21 deletions(-) Tested-by: Aaron Brown
Re: [PATCH 2/2] e1000e: Fix link check race condition
On Mon, Mar 5, 2018 at 5:55 PM, Benjamin Poirierwrote: > Alex reported the following race condition: > > /* link goes up... interrupt... schedule watchdog */ > \ e1000_watchdog_task > \ e1000e_has_link > \ hw->mac.ops.check_for_link() === > e1000e_check_for_copper_link > \ e1000e_phy_has_link_generic(..., ) > link = true > > /* link goes down... interrupt */ > \ e1000_msix_other > hw->mac.get_link_status = > true > > /* link is up */ > mac->get_link_status = false > > link_active = true > /* link_active is true, wrongly, and stays so because > * get_link_status is false */ > > Avoid this problem by making sure that we don't set get_link_status = false > after having checked the link. > > It seems this problem has been present since the introduction of e1000e. > > Link: https://lkml.org/lkml/2018/1/29/338 > Reported-by: Alexander Duyck > Signed-off-by: Benjamin Poirier Looks good. Thanks. Acked-by: Alexander Duyck
Re: [PATCH 2/2] e1000e: Fix link check race condition
On Mon, Mar 5, 2018 at 5:55 PM, Benjamin Poirier wrote: > Alex reported the following race condition: > > /* link goes up... interrupt... schedule watchdog */ > \ e1000_watchdog_task > \ e1000e_has_link > \ hw->mac.ops.check_for_link() === > e1000e_check_for_copper_link > \ e1000e_phy_has_link_generic(..., ) > link = true > > /* link goes down... interrupt */ > \ e1000_msix_other > hw->mac.get_link_status = > true > > /* link is up */ > mac->get_link_status = false > > link_active = true > /* link_active is true, wrongly, and stays so because > * get_link_status is false */ > > Avoid this problem by making sure that we don't set get_link_status = false > after having checked the link. > > It seems this problem has been present since the introduction of e1000e. > > Link: https://lkml.org/lkml/2018/1/29/338 > Reported-by: Alexander Duyck > Signed-off-by: Benjamin Poirier Looks good. Thanks. Acked-by: Alexander Duyck
[PATCH 2/2] e1000e: Fix link check race condition
Alex reported the following race condition: /* link goes up... interrupt... schedule watchdog */ \ e1000_watchdog_task \ e1000e_has_link \ hw->mac.ops.check_for_link() === e1000e_check_for_copper_link \ e1000e_phy_has_link_generic(..., ) link = true /* link goes down... interrupt */ \ e1000_msix_other hw->mac.get_link_status = true /* link is up */ mac->get_link_status = false link_active = true /* link_active is true, wrongly, and stays so because * get_link_status is false */ Avoid this problem by making sure that we don't set get_link_status = false after having checked the link. It seems this problem has been present since the introduction of e1000e. Link: https://lkml.org/lkml/2018/1/29/338 Reported-by: Alexander DuyckSigned-off-by: Benjamin Poirier --- drivers/net/ethernet/intel/e1000e/ich8lan.c | 31 - drivers/net/ethernet/intel/e1000e/mac.c | 14 ++--- 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c index d6d4ed7acf03..1dddfb7b2de6 100644 --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c @@ -1383,6 +1383,7 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw) */ if (!mac->get_link_status) return 0; + mac->get_link_status = false; /* First we want to see if the MII Status Register reports * link. If so, then we want to get the current speed/duplex @@ -1390,12 +1391,12 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw) */ ret_val = e1000e_phy_has_link_generic(hw, 1, 0, ); if (ret_val) - return ret_val; + goto out; if (hw->mac.type == e1000_pchlan) { ret_val = e1000_k1_gig_workaround_hv(hw, link); if (ret_val) - return ret_val; + goto out; } /* When connected at 10Mbps half-duplex, some parts are excessively @@ -1428,7 +1429,7 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw) ret_val = hw->phy.ops.acquire(hw); if (ret_val) - return ret_val; + goto out; if (hw->mac.type == e1000_pch2lan) emi_addr = I82579_RX_CONFIG; @@ -1450,7 +1451,7 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw) hw->phy.ops.release(hw); if (ret_val) - return ret_val; + goto out; if (hw->mac.type >= e1000_pch_spt) { u16 data; @@ -1459,14 +1460,14 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw) if (speed == SPEED_1000) { ret_val = hw->phy.ops.acquire(hw); if (ret_val) - return ret_val; + goto out; ret_val = e1e_rphy_locked(hw, PHY_REG(776, 20), ); if (ret_val) { hw->phy.ops.release(hw); - return ret_val; + goto out; } ptr_gap = (data & (0x3FF << 2)) >> 2; @@ -1480,18 +1481,18 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw) } hw->phy.ops.release(hw); if (ret_val) - return ret_val; + goto out; } else { ret_val = hw->phy.ops.acquire(hw); if (ret_val) - return ret_val; + goto out; ret_val = e1e_wphy_locked(hw, PHY_REG(776, 20), 0xC023); hw->phy.ops.release(hw); if (ret_val) - return ret_val; +
[PATCH 2/2] e1000e: Fix link check race condition
Alex reported the following race condition: /* link goes up... interrupt... schedule watchdog */ \ e1000_watchdog_task \ e1000e_has_link \ hw->mac.ops.check_for_link() === e1000e_check_for_copper_link \ e1000e_phy_has_link_generic(..., ) link = true /* link goes down... interrupt */ \ e1000_msix_other hw->mac.get_link_status = true /* link is up */ mac->get_link_status = false link_active = true /* link_active is true, wrongly, and stays so because * get_link_status is false */ Avoid this problem by making sure that we don't set get_link_status = false after having checked the link. It seems this problem has been present since the introduction of e1000e. Link: https://lkml.org/lkml/2018/1/29/338 Reported-by: Alexander Duyck Signed-off-by: Benjamin Poirier --- drivers/net/ethernet/intel/e1000e/ich8lan.c | 31 - drivers/net/ethernet/intel/e1000e/mac.c | 14 ++--- 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c index d6d4ed7acf03..1dddfb7b2de6 100644 --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c @@ -1383,6 +1383,7 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw) */ if (!mac->get_link_status) return 0; + mac->get_link_status = false; /* First we want to see if the MII Status Register reports * link. If so, then we want to get the current speed/duplex @@ -1390,12 +1391,12 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw) */ ret_val = e1000e_phy_has_link_generic(hw, 1, 0, ); if (ret_val) - return ret_val; + goto out; if (hw->mac.type == e1000_pchlan) { ret_val = e1000_k1_gig_workaround_hv(hw, link); if (ret_val) - return ret_val; + goto out; } /* When connected at 10Mbps half-duplex, some parts are excessively @@ -1428,7 +1429,7 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw) ret_val = hw->phy.ops.acquire(hw); if (ret_val) - return ret_val; + goto out; if (hw->mac.type == e1000_pch2lan) emi_addr = I82579_RX_CONFIG; @@ -1450,7 +1451,7 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw) hw->phy.ops.release(hw); if (ret_val) - return ret_val; + goto out; if (hw->mac.type >= e1000_pch_spt) { u16 data; @@ -1459,14 +1460,14 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw) if (speed == SPEED_1000) { ret_val = hw->phy.ops.acquire(hw); if (ret_val) - return ret_val; + goto out; ret_val = e1e_rphy_locked(hw, PHY_REG(776, 20), ); if (ret_val) { hw->phy.ops.release(hw); - return ret_val; + goto out; } ptr_gap = (data & (0x3FF << 2)) >> 2; @@ -1480,18 +1481,18 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw) } hw->phy.ops.release(hw); if (ret_val) - return ret_val; + goto out; } else { ret_val = hw->phy.ops.acquire(hw); if (ret_val) - return ret_val; + goto out; ret_val = e1e_wphy_locked(hw, PHY_REG(776, 20), 0xC023); hw->phy.ops.release(hw); if (ret_val) - return ret_val; + goto out;