| Message ID | 20240502091215.13068-1-en-wei.wu@canonical.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | e1000e: fix link fluctuations problem | expand |
[Fix address jesse.brandeburg@intel.co*m*] Dear Ricky, Thank you for your patch. Am 02.05.24 um 11:12 schrieb Ricky Wu: > As described in https://bugzilla.kernel.org/show_bug.cgi?id=218642, > some e1000e NIC reports link up -> link down -> link up when hog-plugging Do you mean ho*t*-plugging? > the Ethernet cable. > > The problem is because the unstable behavior of Link Status bit in > PHY Status Register of some e1000e NIC. When we re-plug the cable, > the e1000e_phy_has_link_generic() (called after the Link-Status-Changed > interrupt) has read this bit with 1->0->1 (1=link up, 0=link down) > and e1000e reports it to net device layer respectively. Wow. I guess this was “fun” to debug. Could you please document, what NICs you saw this, and if it is documented in any datasheet/errata? > This patch solves the problem by passing polling delays on > e1000e_phy_has_link_generic() so that it will not get the unstable > states of Link Status bit. Does this have any downsides on systems with non-buggy hardware? > Also, the sleep codes in e1000e_phy_has_link_generic() only take > effect when error occurs reading the MII register. Moving these codes > forward to the beginning of the loop so that the polling delays passed > into this function can take effect on any situation. Could you please split this hunk into a separate patch? Should it Fixes: tag be added? Are there any other public bug reports and discussions you could reference? Link: https://bugzilla.kernel.org/show_bug.cgi?id=218642 > Signed-off-by: Ricky Wu <en-wei.wu@canonical.com> > --- > drivers/net/ethernet/intel/e1000e/ich8lan.c | 5 ++++- > drivers/net/ethernet/intel/e1000e/phy.c | 10 ++++++---- > 2 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c > index f9e94be36e97..c462aa6e6dee 100644 > --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c > +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c > @@ -1427,8 +1427,11 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw) > /* First we want to see if the MII Status Register reports > * link. If so, then we want to get the current speed/duplex > * of the PHY. > + * Some PHYs have link fluctuations with the instability of > + * Link Status bit (BMSR_LSTATUS) in MII Status Register. > + * Increase the iteration times and delay solves the problem. Increas*ing*? > */ > - ret_val = e1000e_phy_has_link_generic(hw, 1, 0, &link); > + ret_val = e1000e_phy_has_link_generic(hw, COPPER_LINK_UP_LIMIT, 100000, &link); Could you please document how 100000 was chosen? > if (ret_val) > goto out; > > diff --git a/drivers/net/ethernet/intel/e1000e/phy.c b/drivers/net/ethernet/intel/e1000e/phy.c > index 93544f1cc2a5..ef056363d721 100644 > --- a/drivers/net/ethernet/intel/e1000e/phy.c > +++ b/drivers/net/ethernet/intel/e1000e/phy.c > @@ -1776,7 +1776,13 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations, > u16 i, phy_status; > > *success = false; > + > for (i = 0; i < iterations; i++) { > + if (usec_interval >= 1000) > + msleep(usec_interval / 1000); > + else > + udelay(usec_interval); > + > /* Some PHYs require the MII_BMSR register to be read > * twice due to the link bit being sticky. No harm doing > * it across the board. > @@ -1799,10 +1805,6 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations, > *success = true; > break; > } > - if (usec_interval >= 1000) > - msleep(usec_interval / 1000); > - else > - udelay(usec_interval); > } > > return ret_val;
Dear Paul Menzel, Thank you for your quick response. > Do you mean ho*t*-plugging? > Increas*ing*? Yes, sorry about the misspelling. > Could you please document what NICs you saw this Yes. I saw this in Intel I219-LM. I haven't seen this bug on other NICs. > and if it is documented in any datasheet/errata? No, we couldn't find any datasheet/errata documenting this. > Does this have any downsides on systems with non-buggy hardware? No, I've tested other non-buggy hardwares (like I219-V) and it has no effect on them. >Could you please split this hunk into a separate patch? Sure! I'll send the v2 patchset soon. > Are there any other public bug reports and discussions you could reference? No. We have an internal private bug report but it cannot be exposed to the public. Thank you for your time. On Fri, 3 May 2024 at 13:34, Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > [Fix address jesse.brandeburg@intel.co*m*] > > > Dear Ricky, > > > Thank you for your patch. > > > Am 02.05.24 um 11:12 schrieb Ricky Wu: > > As described in https://bugzilla.kernel.org/show_bug.cgi?id=218642, > > some e1000e NIC reports link up -> link down -> link up when hog-plugging > > Do you mean ho*t*-plugging? > > > the Ethernet cable. > > > > The problem is because the unstable behavior of Link Status bit in > > PHY Status Register of some e1000e NIC. When we re-plug the cable, > > the e1000e_phy_has_link_generic() (called after the Link-Status-Changed > > interrupt) has read this bit with 1->0->1 (1=link up, 0=link down) > > and e1000e reports it to net device layer respectively. > > Wow. I guess this was “fun” to debug. Could you please document, what > NICs you saw this, and if it is documented in any datasheet/errata? > > > This patch solves the problem by passing polling delays on > > e1000e_phy_has_link_generic() so that it will not get the unstable > > states of Link Status bit. > > Does this have any downsides on systems with non-buggy hardware? > > > Also, the sleep codes in e1000e_phy_has_link_generic() only take > > effect when error occurs reading the MII register. Moving these codes > > forward to the beginning of the loop so that the polling delays passed > > into this function can take effect on any situation. > > Could you please split this hunk into a separate patch? > > Should it Fixes: tag be added? > > Are there any other public bug reports and discussions you could reference? > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=218642 > > > Signed-off-by: Ricky Wu <en-wei.wu@canonical.com> > > --- > > drivers/net/ethernet/intel/e1000e/ich8lan.c | 5 ++++- > > drivers/net/ethernet/intel/e1000e/phy.c | 10 ++++++---- > > 2 files changed, 10 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c > > index f9e94be36e97..c462aa6e6dee 100644 > > --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c > > +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c > > @@ -1427,8 +1427,11 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw) > > /* First we want to see if the MII Status Register reports > > * link. If so, then we want to get the current speed/duplex > > * of the PHY. > > + * Some PHYs have link fluctuations with the instability of > > + * Link Status bit (BMSR_LSTATUS) in MII Status Register. > > + * Increase the iteration times and delay solves the problem. > > Increas*ing*? > > > */ > > - ret_val = e1000e_phy_has_link_generic(hw, 1, 0, &link); > > + ret_val = e1000e_phy_has_link_generic(hw, COPPER_LINK_UP_LIMIT, 100000, &link); > > Could you please document how 100000 was chosen? > > > if (ret_val) > > goto out; > > > > diff --git a/drivers/net/ethernet/intel/e1000e/phy.c b/drivers/net/ethernet/intel/e1000e/phy.c > > index 93544f1cc2a5..ef056363d721 100644 > > --- a/drivers/net/ethernet/intel/e1000e/phy.c > > +++ b/drivers/net/ethernet/intel/e1000e/phy.c > > @@ -1776,7 +1776,13 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations, > > u16 i, phy_status; > > > > *success = false; > > + > > for (i = 0; i < iterations; i++) { > > + if (usec_interval >= 1000) > > + msleep(usec_interval / 1000); > > + else > > + udelay(usec_interval); > > + > > /* Some PHYs require the MII_BMSR register to be read > > * twice due to the link bit being sticky. No harm doing > > * it across the board. > > @@ -1799,10 +1805,6 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations, > > *success = true; > > break; > > } > > - if (usec_interval >= 1000) > > - msleep(usec_interval / 1000); > > - else > > - udelay(usec_interval); > > } > > > > return ret_val;
On Thu, May 02, 2024 at 05:12:15PM +0800, Ricky Wu wrote: > As described in https://bugzilla.kernel.org/show_bug.cgi?id=218642, > some e1000e NIC reports link up -> link down -> link up when hog-plugging > the Ethernet cable. > > The problem is because the unstable behavior of Link Status bit in > PHY Status Register of some e1000e NIC. Why PHY is this? It might be the PHY manufacture has an errata, since this is probably not the MAC causing the problem, but the PHY itself. Andrew
> Why PHY is this? It's the Intel I219-LM, and I haven't found any other device having the same issue. > It might be the PHY manufacture has an errata, since > this is probably not the MAC causing the problem, but the PHY itself. Yes. The problem seems to be a PHY problem. I'm wondering if doing a workaround on MAC like this patch is suitable. For further information, please check here: https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20240503101836.32755-1-en-wei.wu@canonical.com/ On Tue, 7 May 2024 at 03:39, Andrew Lunn <andrew@lunn.ch> wrote: > > On Thu, May 02, 2024 at 05:12:15PM +0800, Ricky Wu wrote: > > As described in https://bugzilla.kernel.org/show_bug.cgi?id=218642, > > some e1000e NIC reports link up -> link down -> link up when hog-plugging > > the Ethernet cable. > > > > The problem is because the unstable behavior of Link Status bit in > > PHY Status Register of some e1000e NIC. > > Why PHY is this? It might be the PHY manufacture has an errata, since > this is probably not the MAC causing the problem, but the PHY itself. > > Andrew
On Tue, May 07, 2024 at 11:24:05AM +0200, En-Wei WU wrote: > > Why PHY is this? > It's the Intel I219-LM, and I haven't found any other device having > the same issue. There is no Linux PHY driver for this device, only the code buried in the e1000e MAC driver. Sometimes Intel use Marvell PHYs, and there are a couple of work arounds in the Marvell PHY driver, which might of given you a clue. But not in this case... Andrew
On 07/05/2024 15:10, Andrew Lunn wrote: > On Tue, May 07, 2024 at 11:24:05AM +0200, En-Wei WU wrote: >>> Why PHY is this? >> It's the Intel I219-LM, and I haven't found any other device having >> the same issue. > > There is no Linux PHY driver for this device, only the code buried in > the e1000e MAC driver. Sometimes Intel use Marvell PHYs, and there are > a couple of work arounds in the Marvell PHY driver, which might of > given you a clue. But not in this case... In I219-* parts used LSI PHY. This PHY is compliant with the 802.3 IEEE standard if I recall correctly. Auto-negotiation and link establishment are processed following the IEEE standard and could vary from platform to platform but are not violent to the IEEE standard. En-Wei, My recommendation is not to accept these patches. If you think there is a HW/PHY problem - open a ticket on Intel PAE. Sasha > > Andrew
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c index f9e94be36e97..c462aa6e6dee 100644 --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c @@ -1427,8 +1427,11 @@ static s32 e1000_check_for_copper_link_ich8lan(struct e1000_hw *hw) /* First we want to see if the MII Status Register reports * link. If so, then we want to get the current speed/duplex * of the PHY. + * Some PHYs have link fluctuations with the instability of + * Link Status bit (BMSR_LSTATUS) in MII Status Register. + * Increase the iteration times and delay solves the problem. */ - ret_val = e1000e_phy_has_link_generic(hw, 1, 0, &link); + ret_val = e1000e_phy_has_link_generic(hw, COPPER_LINK_UP_LIMIT, 100000, &link); if (ret_val) goto out; diff --git a/drivers/net/ethernet/intel/e1000e/phy.c b/drivers/net/ethernet/intel/e1000e/phy.c index 93544f1cc2a5..ef056363d721 100644 --- a/drivers/net/ethernet/intel/e1000e/phy.c +++ b/drivers/net/ethernet/intel/e1000e/phy.c @@ -1776,7 +1776,13 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations, u16 i, phy_status; *success = false; + for (i = 0; i < iterations; i++) { + if (usec_interval >= 1000) + msleep(usec_interval / 1000); + else + udelay(usec_interval); + /* Some PHYs require the MII_BMSR register to be read * twice due to the link bit being sticky. No harm doing * it across the board. @@ -1799,10 +1805,6 @@ s32 e1000e_phy_has_link_generic(struct e1000_hw *hw, u32 iterations, *success = true; break; } - if (usec_interval >= 1000) - msleep(usec_interval / 1000); - else - udelay(usec_interval); } return ret_val;
As described in https://bugzilla.kernel.org/show_bug.cgi?id=218642, some e1000e NIC reports link up -> link down -> link up when hog-plugging the Ethernet cable. The problem is because the unstable behavior of Link Status bit in PHY Status Register of some e1000e NIC. When we re-plug the cable, the e1000e_phy_has_link_generic() (called after the Link-Status-Changed interrupt) has read this bit with 1->0->1 (1=link up, 0=link down) and e1000e reports it to net device layer respectively. This patch solves the problem by passing polling delays on e1000e_phy_has_link_generic() so that it will not get the unstable states of Link Status bit. Also, the sleep codes in e1000e_phy_has_link_generic() only take effect when error occurs reading the MII register. Moving these codes forward to the beginning of the loop so that the polling delays passed into this function can take effect on any situation. Signed-off-by: Ricky Wu <en-wei.wu@canonical.com> --- drivers/net/ethernet/intel/e1000e/ich8lan.c | 5 ++++- drivers/net/ethernet/intel/e1000e/phy.c | 10 ++++++---- 2 files changed, 10 insertions(+), 5 deletions(-)