| Message ID | 20240610013222.12082-1-hui.wang@canonical.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-net] Revert "e1000e: move force SMBUS near the end of enable_ulp function" | expand |
Dear Hui, Thank you for your patch. Am 10.06.24 um 03:32 schrieb Hui Wang: > This reverts commit bfd546a552e140b0a4c8a21527c39d6d21addb28 > > Commit bfd546a552e1 ("e1000e: move force SMBUS near the end of > enable_ulp function") introduces system suspend failure on some > ethernet cards, at the moment, the pciid of the affected ethernet > cards include [8086:15b8] and [8086:15bc]. > > About the regression the commit bfd546a552e1 ("e1000e: move force … regression introduced by commit … > SMBUS near the end of enable_ulp function") tried to fix, looks like > it is not trivial to fix, we need to find a better way to resolve it. Please send a revert for commit 861e8086029e (e1000e: move force SMBUS from enable ulp function to avoid PHY loss issue), present since Linux v6.9-rc3 and not containing enough information in the commit messsage, so we have a proper baseline. (That’s also why I originally suggested to split it into two commits (revert + your change).) > Reported-by: Todd Brandt <todd.e.brandt@intel.com> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218940 > Reported-by: Dieter Mummenschanz <dmummenschanz@web.de> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218936 > Signed-off-by: Hui Wang <hui.wang@canonical.com> > --- > drivers/net/ethernet/intel/e1000e/ich8lan.c | 22 --------------------- > drivers/net/ethernet/intel/e1000e/netdev.c | 18 +++++++++++++++++ > 2 files changed, 18 insertions(+), 22 deletions(-) > > diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c > index 2e98a2a0bead..f9e94be36e97 100644 > --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c > +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c > @@ -1225,28 +1225,6 @@ s32 e1000_enable_ulp_lpt_lp(struct e1000_hw *hw, bool to_sx) > } > > release: > - /* Switching PHY interface always returns MDI error > - * so disable retry mechanism to avoid wasting time > - */ > - e1000e_disable_phy_retry(hw); > - > - /* Force SMBus mode in PHY */ > - ret_val = e1000_read_phy_reg_hv_locked(hw, CV_SMB_CTRL, &phy_reg); > - if (ret_val) { > - e1000e_enable_phy_retry(hw); > - hw->phy.ops.release(hw); > - goto out; > - } > - phy_reg |= CV_SMB_CTRL_FORCE_SMBUS; > - e1000_write_phy_reg_hv_locked(hw, CV_SMB_CTRL, phy_reg); > - > - e1000e_enable_phy_retry(hw); > - > - /* Force SMBus mode in MAC */ > - mac_reg = er32(CTRL_EXT); > - mac_reg |= E1000_CTRL_EXT_FORCE_SMBUS; > - ew32(CTRL_EXT, mac_reg); > - > hw->phy.ops.release(hw); > out: > if (ret_val) > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c > index da5c59daf8ba..220d62fca55d 100644 > --- a/drivers/net/ethernet/intel/e1000e/netdev.c > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c > @@ -6623,6 +6623,7 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime) > struct e1000_hw *hw = &adapter->hw; > u32 ctrl, ctrl_ext, rctl, status, wufc; > int retval = 0; > + u16 smb_ctrl; > > /* Runtime suspend should only enable wakeup for link changes */ > if (runtime) > @@ -6696,6 +6697,23 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime) > if (retval) > return retval; > } > + > + /* Force SMBUS to allow WOL */ > + /* Switching PHY interface always returns MDI error > + * so disable retry mechanism to avoid wasting time > + */ > + e1000e_disable_phy_retry(hw); > + > + e1e_rphy(hw, CV_SMB_CTRL, &smb_ctrl); > + smb_ctrl |= CV_SMB_CTRL_FORCE_SMBUS; > + e1e_wphy(hw, CV_SMB_CTRL, smb_ctrl); > + > + e1000e_enable_phy_retry(hw); > + > + /* Force SMBus mode in MAC */ > + ctrl_ext = er32(CTRL_EXT); > + ctrl_ext |= E1000_CTRL_EXT_FORCE_SMBUS; > + ew32(CTRL_EXT, ctrl_ext); > } > > /* Ensure that the appropriate bits are set in LPI_CTRL Naama also added Tested-by lines two both commits in question. Could Intel’s test coverage please extended to the problem at hand? Acked-by: Paul Menzel <pmenzel@molgen.mpg.de> Kind regards, Paul
On Mon, 2024-06-10 at 08:36 +0200, Paul Menzel wrote: > Dear Hui, > > > Thank you for your patch. > > > Am 10.06.24 um 03:32 schrieb Hui Wang: > > This reverts commit bfd546a552e140b0a4c8a21527c39d6d21addb28 > > > > Commit bfd546a552e1 ("e1000e: move force SMBUS near the end of > > enable_ulp function") introduces system suspend failure on some > > ethernet cards, at the moment, the pciid of the affected ethernet > > cards include [8086:15b8] and [8086:15bc]. > > > > About the regression the commit bfd546a552e1 ("e1000e: move force > > … regression introduced by commit … > > > SMBUS near the end of enable_ulp function") tried to fix, looks > > like > > it is not trivial to fix, we need to find a better way to resolve > > it. > > Please send a revert for commit 861e8086029e (e1000e: move force > SMBUS > from enable ulp function to avoid PHY loss issue), present since > Linux > v6.9-rc3 and not containing enough information in the commit > messsage, > so we have a proper baseline. (That’s also why I originally suggested > to > split it into two commits (revert + your change).) > > > Reported-by: Todd Brandt <todd.e.brandt@intel.com> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218940 > > Reported-by: Dieter Mummenschanz <dmummenschanz@web.de> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218936 > > Signed-off-by: Hui Wang <hui.wang@canonical.com> > > --- > > drivers/net/ethernet/intel/e1000e/ich8lan.c | 22 ---------------- > > ----- > > drivers/net/ethernet/intel/e1000e/netdev.c | 18 > > +++++++++++++++++ > > 2 files changed, 18 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c > > b/drivers/net/ethernet/intel/e1000e/ich8lan.c > > index 2e98a2a0bead..f9e94be36e97 100644 > > --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c > > +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c > > @@ -1225,28 +1225,6 @@ s32 e1000_enable_ulp_lpt_lp(struct e1000_hw > > *hw, bool to_sx) > > } > > > > release: > > - /* Switching PHY interface always returns MDI error > > - * so disable retry mechanism to avoid wasting time > > - */ > > - e1000e_disable_phy_retry(hw); > > - > > - /* Force SMBus mode in PHY */ > > - ret_val = e1000_read_phy_reg_hv_locked(hw, CV_SMB_CTRL, > > &phy_reg); > > - if (ret_val) { > > - e1000e_enable_phy_retry(hw); > > - hw->phy.ops.release(hw); > > - goto out; > > - } > > - phy_reg |= CV_SMB_CTRL_FORCE_SMBUS; > > - e1000_write_phy_reg_hv_locked(hw, CV_SMB_CTRL, phy_reg); > > - > > - e1000e_enable_phy_retry(hw); > > - > > - /* Force SMBus mode in MAC */ > > - mac_reg = er32(CTRL_EXT); > > - mac_reg |= E1000_CTRL_EXT_FORCE_SMBUS; > > - ew32(CTRL_EXT, mac_reg); > > - > > hw->phy.ops.release(hw); > > out: > > if (ret_val) > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c > > b/drivers/net/ethernet/intel/e1000e/netdev.c > > index da5c59daf8ba..220d62fca55d 100644 > > --- a/drivers/net/ethernet/intel/e1000e/netdev.c > > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c > > @@ -6623,6 +6623,7 @@ static int __e1000_shutdown(struct pci_dev > > *pdev, bool runtime) > > struct e1000_hw *hw = &adapter->hw; > > u32 ctrl, ctrl_ext, rctl, status, wufc; > > int retval = 0; > > + u16 smb_ctrl; > > > > /* Runtime suspend should only enable wakeup for link > > changes */ > > if (runtime) > > @@ -6696,6 +6697,23 @@ static int __e1000_shutdown(struct pci_dev > > *pdev, bool runtime) > > if (retval) > > return retval; > > } > > + > > + /* Force SMBUS to allow WOL */ > > + /* Switching PHY interface always returns MDI error > > + * so disable retry mechanism to avoid wasting time > > + */ > > + e1000e_disable_phy_retry(hw); > > + > > + e1e_rphy(hw, CV_SMB_CTRL, &smb_ctrl); > > + smb_ctrl |= CV_SMB_CTRL_FORCE_SMBUS; > > + e1e_wphy(hw, CV_SMB_CTRL, smb_ctrl); > > + > > + e1000e_enable_phy_retry(hw); > > + > > + /* Force SMBus mode in MAC */ > > + ctrl_ext = er32(CTRL_EXT); > > + ctrl_ext |= E1000_CTRL_EXT_FORCE_SMBUS; > > + ew32(CTRL_EXT, ctrl_ext); > > } > > > > /* Ensure that the appropriate bits are set in LPI_CTRL > > Naama also added Tested-by lines two both commits in question. Could > Intel’s test coverage please extended to the problem at hand? > > Acked-by: Paul Menzel <pmenzel@molgen.mpg.de> Plus that, 1. Todd and I can test with upstream + this patch to confirm that a. the regression for Todd is gone. b. the s2idle failure for me is back 2. I can test with upstream + this patch + revert of commit 861e8086029e (e1000e: move force SMBUS from enable ulp function to avoid PHY loss issue) to confirm s2idle is working again. thanks, rui > > > Kind regards, > > Paul
On 6/10/24 14:36, Paul Menzel wrote: > Dear Hui, > > > Thank you for your patch. > > > Am 10.06.24 um 03:32 schrieb Hui Wang: >> This reverts commit bfd546a552e140b0a4c8a21527c39d6d21addb28 >> >> Commit bfd546a552e1 ("e1000e: move force SMBUS near the end of >> enable_ulp function") introduces system suspend failure on some >> ethernet cards, at the moment, the pciid of the affected ethernet >> cards include [8086:15b8] and [8086:15bc]. >> >> About the regression the commit bfd546a552e1 ("e1000e: move force > > … regression introduced by commit … Got it. > >> SMBUS near the end of enable_ulp function") tried to fix, looks like >> it is not trivial to fix, we need to find a better way to resolve it. > > Please send a revert for commit 861e8086029e (e1000e: move force SMBUS > from enable ulp function to avoid PHY loss issue), present since Linux > v6.9-rc3 and not containing enough information in the commit messsage, > so we have a proper baseline. (That’s also why I originally suggested > to split it into two commits (revert + your change).) In regards to reverting the commit 861e8086029e (e1000e: move force SMBUS from enable ulp function to avoid PHY loss issue), the author is Vitaly, let him evaluate how to act. Thanks. >
On 6/10/24 22:14, Zhang, Rui wrote: > On Mon, 2024-06-10 at 08:36 +0200, Paul Menzel wrote: >> Dear Hui, >> >> >> >> Naama also added Tested-by lines two both commits in question. Could >> Intel’s test coverage please extended to the problem at hand? >> >> Acked-by: Paul Menzel <pmenzel@molgen.mpg.de> > Plus that, > 1. Todd and I can test with upstream + this patch to confirm that > a. the regression for Todd is gone. > b. the s2idle failure for me is back > 2. I can test with upstream + this patch + revert of commit > 861e8086029e (e1000e: move force SMBUS from enable ulp function to > avoid PHY loss issue) to confirm s2idle is working again. > > thanks, > rui Thanks. > >> >> Kind regards, >> >> Paul
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c index 2e98a2a0bead..f9e94be36e97 100644 --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c @@ -1225,28 +1225,6 @@ s32 e1000_enable_ulp_lpt_lp(struct e1000_hw *hw, bool to_sx) } release: - /* Switching PHY interface always returns MDI error - * so disable retry mechanism to avoid wasting time - */ - e1000e_disable_phy_retry(hw); - - /* Force SMBus mode in PHY */ - ret_val = e1000_read_phy_reg_hv_locked(hw, CV_SMB_CTRL, &phy_reg); - if (ret_val) { - e1000e_enable_phy_retry(hw); - hw->phy.ops.release(hw); - goto out; - } - phy_reg |= CV_SMB_CTRL_FORCE_SMBUS; - e1000_write_phy_reg_hv_locked(hw, CV_SMB_CTRL, phy_reg); - - e1000e_enable_phy_retry(hw); - - /* Force SMBus mode in MAC */ - mac_reg = er32(CTRL_EXT); - mac_reg |= E1000_CTRL_EXT_FORCE_SMBUS; - ew32(CTRL_EXT, mac_reg); - hw->phy.ops.release(hw); out: if (ret_val) diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c index da5c59daf8ba..220d62fca55d 100644 --- a/drivers/net/ethernet/intel/e1000e/netdev.c +++ b/drivers/net/ethernet/intel/e1000e/netdev.c @@ -6623,6 +6623,7 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime) struct e1000_hw *hw = &adapter->hw; u32 ctrl, ctrl_ext, rctl, status, wufc; int retval = 0; + u16 smb_ctrl; /* Runtime suspend should only enable wakeup for link changes */ if (runtime) @@ -6696,6 +6697,23 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime) if (retval) return retval; } + + /* Force SMBUS to allow WOL */ + /* Switching PHY interface always returns MDI error + * so disable retry mechanism to avoid wasting time + */ + e1000e_disable_phy_retry(hw); + + e1e_rphy(hw, CV_SMB_CTRL, &smb_ctrl); + smb_ctrl |= CV_SMB_CTRL_FORCE_SMBUS; + e1e_wphy(hw, CV_SMB_CTRL, smb_ctrl); + + e1000e_enable_phy_retry(hw); + + /* Force SMBus mode in MAC */ + ctrl_ext = er32(CTRL_EXT); + ctrl_ext |= E1000_CTRL_EXT_FORCE_SMBUS; + ew32(CTRL_EXT, ctrl_ext); } /* Ensure that the appropriate bits are set in LPI_CTRL
This reverts commit bfd546a552e140b0a4c8a21527c39d6d21addb28 Commit bfd546a552e1 ("e1000e: move force SMBUS near the end of enable_ulp function") introduces system suspend failure on some ethernet cards, at the moment, the pciid of the affected ethernet cards include [8086:15b8] and [8086:15bc]. About the regression the commit bfd546a552e1 ("e1000e: move force SMBUS near the end of enable_ulp function") tried to fix, looks like it is not trivial to fix, we need to find a better way to resolve it. Reported-by: Todd Brandt <todd.e.brandt@intel.com> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218940 Reported-by: Dieter Mummenschanz <dmummenschanz@web.de> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218936 Signed-off-by: Hui Wang <hui.wang@canonical.com> --- drivers/net/ethernet/intel/e1000e/ich8lan.c | 22 --------------------- drivers/net/ethernet/intel/e1000e/netdev.c | 18 +++++++++++++++++ 2 files changed, 18 insertions(+), 22 deletions(-)