| Message ID | 20190625143911.37284-1-vitaly.lifshits@intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Jeff Kirsher |
| Headers | show |
| Series | [v3] e1000e: PCIm function state support | expand |
On 6/25/2019 17:39, Vitaly Lifshits wrote: > Due to commit: 5d8682588605 ("[misc] mei: me: allow runtime > pm for platform with D0i3") > When disconnecting the cable and reconnecting it the NIC > enters DMoff state. This caused wrong link indication > and duplex mismatch. This bug is described in: > https://bugzilla.redhat.com/show_bug.cgi?id=1689436 > > Checking PCIm function state and performing PHY reset after a > timeout in watchdog task solves this issue. > > Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com> > --- > > V2: Fixed typos in commit massage > V3: Fixed minor typo > --- > drivers/net/ethernet/intel/e1000e/defines.h | 3 +++ > drivers/net/ethernet/intel/e1000e/netdev.c | 18 +++++++++++++++++- > 2 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/e1000e/defines.h b/drivers/net/ethernet/intel/e1000e/defines.h > index fd550dee4982..13877fe300f1 100644 > --- a/drivers/net/ethernet/intel/e1000e/defines.h > +++ b/drivers/net/ethernet/intel/e1000e/defines.h > @@ -222,6 +222,9 @@ > #define E1000_STATUS_PHYRA 0x00000400 /* PHY Reset Asserted */ > #define E1000_STATUS_GIO_MASTER_ENABLE 0x00080000 /* Master Req status */ > > +/* PCIm function state */ > +#define E1000_STATUS_PCIM_STATE 0x40000000 > + > #define HALF_DUPLEX 1 > #define FULL_DUPLEX 2 > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c > index b081a1ef6859..c6a10fd30e4e 100644 > --- a/drivers/net/ethernet/intel/e1000e/netdev.c > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c > @@ -5173,8 +5173,9 @@ static void e1000_watchdog_task(struct work_struct *work) > struct e1000_mac_info *mac = &adapter->hw.mac; > struct e1000_phy_info *phy = &adapter->hw.phy; > struct e1000_ring *tx_ring = adapter->tx_ring; > + u32 dmoff_exit_timeout = 100, tries = 0; > struct e1000_hw *hw = &adapter->hw; > - u32 link, tctl; > + u32 link, tctl, pcim_state; > > if (test_bit(__E1000_DOWN, &adapter->state)) > return; > @@ -5199,6 +5200,21 @@ static void e1000_watchdog_task(struct work_struct *work) > /* Cancel scheduled suspend requests. */ > pm_runtime_resume(netdev->dev.parent); > > + /* Checking if MAC is in DMoff state*/ > + pcim_state = er32(STATUS); > + while (pcim_state & E1000_STATUS_PCIM_STATE) { > + if (tries++ == dmoff_exit_timeout) { > + e_dbg("Error in exiting dmoff\n"); > + break; > + } > + usleep_range(10000, 20000); > + pcim_state = er32(STATUS); > + > + /* Checking if MAC exited DMoff state */ > + if (!(pcim_state & E1000_STATUS_PCIM_STATE)) > + e1000_phy_hw_reset(&adapter->hw); > + } > + > /* update snapshot of PHY registers on LSC */ > e1000_phy_read_status(adapter); > mac->ops.get_link_up_info(&adapter->hw, > Thanks Vitalik Acked-by: Sasha Neftin <sasha.neftin@intel.com>
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@osuosl.org] On Behalf Of > Neftin, Sasha > Sent: Tuesday, June 25, 2019 8:15 AM > To: Lifshits, Vitaly <vitaly.lifshits@intel.com>; intel-wired-lan@lists.osuosl.org > Subject: Re: [Intel-wired-lan] [PATCH v3] e1000e: PCIm function state support > > On 6/25/2019 17:39, Vitaly Lifshits wrote: > > Due to commit: 5d8682588605 ("[misc] mei: me: allow runtime > > pm for platform with D0i3") > > When disconnecting the cable and reconnecting it the NIC > > enters DMoff state. This caused wrong link indication > > and duplex mismatch. This bug is described in: > > https://bugzilla.redhat.com/show_bug.cgi?id=1689436 > > > > Checking PCIm function state and performing PHY reset after a > > timeout in watchdog task solves this issue. > > > > Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com> > > --- > > > > V2: Fixed typos in commit massage > > V3: Fixed minor typo > > --- > > drivers/net/ethernet/intel/e1000e/defines.h | 3 +++ > > drivers/net/ethernet/intel/e1000e/netdev.c | 18 +++++++++++++++++- > > 2 files changed, 20 insertions(+), 1 deletion(-) Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Dear Vitaly, Thank you for the patch. Some suggestions below. On 06/25/19 16:39, Vitaly Lifshits wrote: > Due to commit: 5d8682588605 ("[misc] mei: me: allow runtime > pm for platform with D0i3") Do not indent it but integrate it into the line. > When disconnecting the cable and reconnecting it the NIC s/When/when/ > enters DMoff state. This caused wrong link indication > and duplex mismatch. This bug is described in: > https://bugzilla.redhat.com/show_bug.cgi?id=1689436 Isn’t there a tag Link: or Bugzilla: to mention these URLs? Maybe add it below? (See `git log --grep bugzilla` for how this is used.) > Checking PCIm function state and performing PHY reset after a > timeout in watchdog task solves this issue. In what data sheet is the function state described? > Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com> > --- > > V2: Fixed typos in commit massage > V3: Fixed minor typo > --- > drivers/net/ethernet/intel/e1000e/defines.h | 3 +++ > drivers/net/ethernet/intel/e1000e/netdev.c | 18 +++++++++++++++++- > 2 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/e1000e/defines.h b/drivers/net/ethernet/intel/e1000e/defines.h > index fd550dee4982..13877fe300f1 100644 > --- a/drivers/net/ethernet/intel/e1000e/defines.h > +++ b/drivers/net/ethernet/intel/e1000e/defines.h > @@ -222,6 +222,9 @@ > #define E1000_STATUS_PHYRA 0x00000400 /* PHY Reset Asserted */ > #define E1000_STATUS_GIO_MASTER_ENABLE 0x00080000 /* Master Req status */ > > +/* PCIm function state */ > +#define E1000_STATUS_PCIM_STATE 0x40000000 Shouldn’t the name be something E1000_STATUS_PCIM_STATE_DMOFF? > + > #define HALF_DUPLEX 1 > #define FULL_DUPLEX 2 > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c > index b081a1ef6859..c6a10fd30e4e 100644 > --- a/drivers/net/ethernet/intel/e1000e/netdev.c > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c > @@ -5173,8 +5173,9 @@ static void e1000_watchdog_task(struct work_struct *work) > struct e1000_mac_info *mac = &adapter->hw.mac; > struct e1000_phy_info *phy = &adapter->hw.phy; > struct e1000_ring *tx_ring = adapter->tx_ring; > + u32 dmoff_exit_timeout = 100, tries = 0; Shouldn’t a macro be used for the time-out value? > struct e1000_hw *hw = &adapter->hw; > - u32 link, tctl; > + u32 link, tctl, pcim_state; > > if (test_bit(__E1000_DOWN, &adapter->state)) > return; > @@ -5199,6 +5200,21 @@ static void e1000_watchdog_task(struct work_struct *work) > /* Cancel scheduled suspend requests. */ > pm_runtime_resume(netdev->dev.parent); > > + /* Checking if MAC is in DMoff state*/ > + pcim_state = er32(STATUS); > + while (pcim_state & E1000_STATUS_PCIM_STATE) { > + if (tries++ == dmoff_exit_timeout) { > + e_dbg("Error in exiting dmoff\n"); Shouldn’t this be a user visible error message? If so, please elaborate and mention the time-out. > Couldn’t exit DMoff after %i s. Your card might not work correctly, > TIMEOUTMACRONAME > + break; > + } > + usleep_range(10000, 20000); > + pcim_state = er32(STATUS); > + > + /* Checking if MAC exited DMoff state */ > + if (!(pcim_state & E1000_STATUS_PCIM_STATE)) If the macro name is more specific, the comment could be removed. If not, the comment should use imperative mood (as below): Check if …. Also can the while loop and if condition be refactored, as the condition check if the same? > + e1000_phy_hw_reset(&adapter->hw); > + } > + > /* update snapshot of PHY registers on LSC */ > e1000_phy_read_status(adapter); > mac->ops.get_link_up_info(&adapter->hw, Kind regards, Paul
On 6/28/2019 11:57, Paul Menzel wrote: > Dear Vitaly, > > > Thank you for the patch. Some suggestions below. > > On 06/25/19 16:39, Vitaly Lifshits wrote: >> Due to commit: 5d8682588605 ("[misc] mei: me: allow runtime >> pm for platform with D0i3") > > Do not indent it but integrate it into the line. > >> When disconnecting the cable and reconnecting it the NIC > > s/When/when/ > >> enters DMoff state. This caused wrong link indication >> and duplex mismatch. This bug is described in: >> https://bugzilla.redhat.com/show_bug.cgi?id=1689436 > > Isn’t there a tag Link: or Bugzilla: to mention these URLs? > Maybe add it below? (See `git log --grep bugzilla` for how this > is used.) > >> Checking PCIm function state and performing PHY reset after a >> timeout in watchdog task solves this issue. > > In what data sheet is the function state described? > >> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com> >> --- >> >> V2: Fixed typos in commit massage >> V3: Fixed minor typo >> --- >> drivers/net/ethernet/intel/e1000e/defines.h | 3 +++ >> drivers/net/ethernet/intel/e1000e/netdev.c | 18 +++++++++++++++++- >> 2 files changed, 20 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/intel/e1000e/defines.h b/drivers/net/ethernet/intel/e1000e/defines.h >> index fd550dee4982..13877fe300f1 100644 >> --- a/drivers/net/ethernet/intel/e1000e/defines.h >> +++ b/drivers/net/ethernet/intel/e1000e/defines.h >> @@ -222,6 +222,9 @@ >> #define E1000_STATUS_PHYRA 0x00000400 /* PHY Reset Asserted */ >> #define E1000_STATUS_GIO_MASTER_ENABLE 0x00080000 /* Master Req status */ >> >> +/* PCIm function state */ >> +#define E1000_STATUS_PCIM_STATE 0x40000000 > > Shouldn’t the name be something E1000_STATUS_PCIM_STATE_DMOFF? > >> + >> #define HALF_DUPLEX 1 >> #define FULL_DUPLEX 2 >> >> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c >> index b081a1ef6859..c6a10fd30e4e 100644 >> --- a/drivers/net/ethernet/intel/e1000e/netdev.c >> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c >> @@ -5173,8 +5173,9 @@ static void e1000_watchdog_task(struct work_struct *work) >> struct e1000_mac_info *mac = &adapter->hw.mac; >> struct e1000_phy_info *phy = &adapter->hw.phy; >> struct e1000_ring *tx_ring = adapter->tx_ring; >> + u32 dmoff_exit_timeout = 100, tries = 0; > > Shouldn’t a macro be used for the time-out value? > >> struct e1000_hw *hw = &adapter->hw; >> - u32 link, tctl; >> + u32 link, tctl, pcim_state; >> >> if (test_bit(__E1000_DOWN, &adapter->state)) >> return; >> @@ -5199,6 +5200,21 @@ static void e1000_watchdog_task(struct work_struct *work) >> /* Cancel scheduled suspend requests. */ >> pm_runtime_resume(netdev->dev.parent); >> >> + /* Checking if MAC is in DMoff state*/ >> + pcim_state = er32(STATUS); >> + while (pcim_state & E1000_STATUS_PCIM_STATE) { >> + if (tries++ == dmoff_exit_timeout) { >> + e_dbg("Error in exiting dmoff\n"); > > Shouldn’t this be a user visible error message? If so, please elaborate and > mention the time-out. > >> Couldn’t exit DMoff after %i s. Your card might not work correctly, >> TIMEOUTMACRONAME > >> + break; >> + } >> + usleep_range(10000, 20000); >> + pcim_state = er32(STATUS); >> + >> + /* Checking if MAC exited DMoff state */ >> + if (!(pcim_state & E1000_STATUS_PCIM_STATE)) > > If the macro name is more specific, the comment could be removed. If not, > the comment should use imperative mood (as below): Check if …. > > Also can the while loop and if condition be refactored, as the condition > check if the same? > >> + e1000_phy_hw_reset(&adapter->hw); >> + } >> + >> /* update snapshot of PHY registers on LSC */ >> e1000_phy_read_status(adapter); >> mac->ops.get_link_up_info(&adapter->hw, > > > Kind regards, > > Paul > > > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan > Paul, thanks for your comments. I worked with Vitalik on this - we will address your suggestions and re-submit the patch.
Dear Vitaly, I am adding the list back, so that the Linux kernel experts can chime and correct my answers/suggestions. On 7/16/19 1:28 PM, Lifshits, Vitaly wrote: > On 6/28/2019 11:57, Paul Menzel wrote: >> On 06/25/19 16:39, Vitaly Lifshits wrote: >>> Due to commit: 5d8682588605 ("[misc] mei: me: allow runtime >>> pm for platform with D0i3") >> Do not indent it but integrate it into the line. >> >>> When disconnecting the cable and reconnecting it the NIC >> s/When/when/ >> >>> enters DMoff state. This caused wrong link indication >>> and duplex mismatch. This bug is described in: >>> https://bugzilla.redhat.com/show_bug.cgi?id=1689436 >> Isn’t there a tag Link: or Bugzilla: to mention these URLs? >> Maybe add it below? (See `git log --grep bugzilla` for how this >> is used.) >> >>> Checking PCIm function state and performing PHY reset after a >>> timeout in watchdog task solves this issue. >> In what data sheet is the function state described? > PCIm function state isn't mentioned in the I219 data sheet. It should be updated then. ;-) > However the DMoff state (which is a pcim state) is mentioned in it. > In I218 data sheet this state is mentioned. Thanks. I found the data sheet. https://datasheet.octopart.com/WGI218LM-S-LK3B-Intel-datasheet-76215422.pdf >>> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com> >>> --- >>> >>> V2: Fixed typos in commit massage >>> V3: Fixed minor typo >>> --- >>> drivers/net/ethernet/intel/e1000e/defines.h | 3 +++ >>> drivers/net/ethernet/intel/e1000e/netdev.c | 18 +++++++++++++++++- >>> 2 files changed, 20 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/ethernet/intel/e1000e/defines.h b/drivers/net/ethernet/intel/e1000e/defines.h >>> index fd550dee4982..13877fe300f1 100644 >>> --- a/drivers/net/ethernet/intel/e1000e/defines.h >>> +++ b/drivers/net/ethernet/intel/e1000e/defines.h >>> @@ -222,6 +222,9 @@ >>> #define E1000_STATUS_PHYRA 0x00000400 /* PHY Reset Asserted */ >>> #define E1000_STATUS_GIO_MASTER_ENABLE 0x00080000 /* Master Req status */ >>> +/* PCIm function state */ >>> +#define E1000_STATUS_PCIM_STATE 0x40000000 >> Shouldn’t the name be something E1000_STATUS_PCIM_STATE_DMOFF? > This bit indicates the pcim state if it is set then the device is in > DMoff state. Indeed. Shouldn’t the macro name be changed to my suggestion to better describe it’s meaning? >>> + >>> #define HALF_DUPLEX 1 >>> #define FULL_DUPLEX 2 >>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c >>> index b081a1ef6859..c6a10fd30e4e 100644 >>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c >>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c >>> @@ -5173,8 +5173,9 @@ static void e1000_watchdog_task(struct work_struct *work) >>> struct e1000_mac_info *mac = &adapter->hw.mac; >>> struct e1000_phy_info *phy = &adapter->hw.phy; >>> struct e1000_ring *tx_ring = adapter->tx_ring; >>> + u32 dmoff_exit_timeout = 100, tries = 0; >> Shouldn’t a macro be used for the time-out value? > Just to make sure I understand you correctly, did you mean that I > should set a defined value like DMOFF_EXIT_TIMEOUT 100? Yes, that is what I meant. But I am no C or Linux expert, so I do not know, if macros are wanted seeing that they do not have a type. If you go with a C variable, it should be `unsigned int` and `const`? I heard enums are an alternative to macros. >>> struct e1000_hw *hw = &adapter->hw; >>> - u32 link, tctl; >>> + u32 link, tctl, pcim_state; >>> if (test_bit(__E1000_DOWN, &adapter->state)) >>> return; >>> @@ -5199,6 +5200,21 @@ static void e1000_watchdog_task(struct work_struct *work) >>> /* Cancel scheduled suspend requests. */ >>> pm_runtime_resume(netdev->dev.parent); >>> + /* Checking if MAC is in DMoff state*/ >>> + pcim_state = er32(STATUS); >>> + while (pcim_state & E1000_STATUS_PCIM_STATE) { >>> + if (tries++ == dmoff_exit_timeout) { >>> + e_dbg("Error in exiting dmoff\n"); >> Shouldn’t this be a user visible error message? If so, please elaborate and >> mention the time-out. >> >>> Couldn’t exit DMoff after %i s. Your card might not work correctly, >>> TIMEOUTMACRONAME >>> + break; >>> + } >>> + usleep_range(10000, 20000); >>> + pcim_state = er32(STATUS); >>> + >>> + /* Checking if MAC exited DMoff state */ >>> + if (!(pcim_state & E1000_STATUS_PCIM_STATE)) >> If the macro name is more specific, the comment could be removed. If not, >> the comment should use imperative mood (as below): Check if …. >> >> Also can the while loop and if condition be refactored, as the condition >> check if the same? > The thing is that I don't want to perform phy_hw_reset if the device wasn't in this state at all. > Does removing the if from here and using another one after the loop is a good solution for this? > (By using tries variable) If the if statement condition `!(pcim_state & E1000_STATUS_PCIM_STATE)` is true, then the while loop condition is false, right? So the if statement can at least be moved *outside* the loop (the compiler probably does it already). Couldn’t you write it like below? 1. do-while-loop: Saves one `pcim_state` assignment, but has a mandatory delay of `usleep_range`. do { if (tries++ == dmoff_exit_timeout) { e_dbg("Error in exiting dmoff\n"); break; } pcim_state = er32(STATUS); usleep_range(10000, 20000); } while (pcim_state & E1000_STATUS_PCIM_STATE) if (!(pcim_state & E1000_STATUS_PCIM_STATE)) e1000_phy_hw_reset(&adapter->hw); 2. while-loop: Has an additional pcim_state assignment before the loop. pcim_state = er32(STATUS); while (pcim_state & E1000_STATUS_PCIM_STATE) { if (tries++ == dmoff_exit_timeout) { e_dbg("Error in exiting dmoff\n"); break; } pcim_state = er32(STATUS); usleep_range(10000, 20000); } if (!(pcim_state & E1000_STATUS_PCIM_STATE)) e1000_phy_hw_reset(&adapter->hw); (I used your macro name. Should you decide to update it, it needs to be updated of course.) >>> + e1000_phy_hw_reset(&adapter->hw); >>> + } >>> + >>> /* update snapshot of PHY registers on LSC */ >>> e1000_phy_read_status(adapter); >>> mac->ops.get_link_up_info(&adapter->hw, Kind regards, Paul
diff --git a/drivers/net/ethernet/intel/e1000e/defines.h b/drivers/net/ethernet/intel/e1000e/defines.h index fd550dee4982..13877fe300f1 100644 --- a/drivers/net/ethernet/intel/e1000e/defines.h +++ b/drivers/net/ethernet/intel/e1000e/defines.h @@ -222,6 +222,9 @@ #define E1000_STATUS_PHYRA 0x00000400 /* PHY Reset Asserted */ #define E1000_STATUS_GIO_MASTER_ENABLE 0x00080000 /* Master Req status */ +/* PCIm function state */ +#define E1000_STATUS_PCIM_STATE 0x40000000 + #define HALF_DUPLEX 1 #define FULL_DUPLEX 2 diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c index b081a1ef6859..c6a10fd30e4e 100644 --- a/drivers/net/ethernet/intel/e1000e/netdev.c +++ b/drivers/net/ethernet/intel/e1000e/netdev.c @@ -5173,8 +5173,9 @@ static void e1000_watchdog_task(struct work_struct *work) struct e1000_mac_info *mac = &adapter->hw.mac; struct e1000_phy_info *phy = &adapter->hw.phy; struct e1000_ring *tx_ring = adapter->tx_ring; + u32 dmoff_exit_timeout = 100, tries = 0; struct e1000_hw *hw = &adapter->hw; - u32 link, tctl; + u32 link, tctl, pcim_state; if (test_bit(__E1000_DOWN, &adapter->state)) return; @@ -5199,6 +5200,21 @@ static void e1000_watchdog_task(struct work_struct *work) /* Cancel scheduled suspend requests. */ pm_runtime_resume(netdev->dev.parent); + /* Checking if MAC is in DMoff state*/ + pcim_state = er32(STATUS); + while (pcim_state & E1000_STATUS_PCIM_STATE) { + if (tries++ == dmoff_exit_timeout) { + e_dbg("Error in exiting dmoff\n"); + break; + } + usleep_range(10000, 20000); + pcim_state = er32(STATUS); + + /* Checking if MAC exited DMoff state */ + if (!(pcim_state & E1000_STATUS_PCIM_STATE)) + e1000_phy_hw_reset(&adapter->hw); + } + /* update snapshot of PHY registers on LSC */ e1000_phy_read_status(adapter); mac->ops.get_link_up_info(&adapter->hw,
Due to commit: 5d8682588605 ("[misc] mei: me: allow runtime pm for platform with D0i3") When disconnecting the cable and reconnecting it the NIC enters DMoff state. This caused wrong link indication and duplex mismatch. This bug is described in: https://bugzilla.redhat.com/show_bug.cgi?id=1689436 Checking PCIm function state and performing PHY reset after a timeout in watchdog task solves this issue. Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com> --- V2: Fixed typos in commit massage V3: Fixed minor typo --- drivers/net/ethernet/intel/e1000e/defines.h | 3 +++ drivers/net/ethernet/intel/e1000e/netdev.c | 18 +++++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-)