| Message ID | 20240413092743.1548310-1-hui.wang@canonical.com |
|---|---|
| State | Under Review |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | e1000e: move force SMBUS near the end of enable_ulp function | expand |
On 4/13/2024 12:27 PM, Hui Wang wrote: > The commit 861e8086029e ("e1000e: move force SMBUS from enable ulp > function to avoid PHY loss issue") introduces a regression on > CH_MTP_I219_LM18 (PCIID: 0x8086550A). Without this commit, the > ethernet works well after suspend and resume, but after applying the > commit, the ethernet couldn't work anymore after the resume and the > dmesg shows that the NIC Link changes to 10Mbps (1000Mbps originally): > [ 43.305084] e1000e 0000:00:1f.6 enp0s31f6: NIC Link is Up 10 Mbps Full Duplex, Flow Control: Rx/Tx > > Without the commit, the force SMBUS code will not be executed if > "return 0" or "goto out" is executed in the enable_ulp(), and in my > case, the "goto out" is executed since FWSM_FW_VALID is set. But after > applying the commit, the force SMBUS code will be ran unconditionally. > > Here move the force SMBUS code back to enable_ulp() and put it > immediate ahead of hw->phy.ops.release(hw), this could allow the > longest settling time as possible for interface in this function and > doesn't change the original code logic. > > Fixes: 861e8086029e ("e1000e: move force SMBUS from enable ulp function to avoid PHY loss issue") > Signed-off-by: Hui Wang <hui.wang@canonical.com> > --- > drivers/net/ethernet/intel/e1000e/ich8lan.c | 19 +++++++++++++++++++ > drivers/net/ethernet/intel/e1000e/netdev.c | 18 ------------------ > 2 files changed, 19 insertions(+), 18 deletions(-) > > diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c > index f9e94be36e97..dd670cd87df2 100644 > --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c > +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c > @@ -1225,6 +1225,25 @@ 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) > + goto release; > + 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 3692fce20195..cc8c531ec3df 100644 > --- a/drivers/net/ethernet/intel/e1000e/netdev.c > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c > @@ -6623,7 +6623,6 @@ 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) > @@ -6697,23 +6696,6 @@ 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 Dear Hui, Thank you for this patch and this observation. I think that you found a real misbehaviour in the original patch. However, I still think that forcing SMBUS functionality shouldn't be part of the ULP enabling flow, since they are two independent configurations. I will soon submit a patch where I wrap forcing SMBUS in e1000_shutdown with an if that checks if the FWSM_FW_VALID bit it set.
On Mon, 15 Apr 2024 13:15:41 +0300 Lifshits, Vitaly wrote: > Thank you for this patch and this observation. > I think that you found a real misbehaviour in the original patch. > However, I still think that forcing SMBUS functionality shouldn't be > part of the ULP enabling flow, since they are two independent > configurations. > > I will soon submit a patch where I wrap forcing SMBUS in e1000_shutdown > with an if that checks if the FWSM_FW_VALID bit it set. Why are you submitting a patch instead of asking the author to change theirs? This is not how code reviews work.
On 4/13/2024 12:27 PM, Hui Wang wrote: > The commit 861e8086029e ("e1000e: move force SMBUS from enable ulp > function to avoid PHY loss issue") introduces a regression on > CH_MTP_I219_LM18 (PCIID: 0x8086550A). Without this commit, the > ethernet works well after suspend and resume, but after applying the > commit, the ethernet couldn't work anymore after the resume and the > dmesg shows that the NIC Link changes to 10Mbps (1000Mbps originally): > [ 43.305084] e1000e 0000:00:1f.6 enp0s31f6: NIC Link is Up 10 Mbps Full Duplex, Flow Control: Rx/Tx > > Without the commit, the force SMBUS code will not be executed if > "return 0" or "goto out" is executed in the enable_ulp(), and in my > case, the "goto out" is executed since FWSM_FW_VALID is set. But after > applying the commit, the force SMBUS code will be ran unconditionally. > > Here move the force SMBUS code back to enable_ulp() and put it > immediate ahead of hw->phy.ops.release(hw), this could allow the > longest settling time as possible for interface in this function and > doesn't change the original code logic. > > Fixes: 861e8086029e ("e1000e: move force SMBUS from enable ulp function to avoid PHY loss issue") > Signed-off-by: Hui Wang <hui.wang@canonical.com> After discussing this patch offline with Hui this solution is the safest, therefore, I suggest to accept it. Acked-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c index f9e94be36e97..dd670cd87df2 100644 --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c @@ -1225,6 +1225,25 @@ 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) + goto release; + 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 3692fce20195..cc8c531ec3df 100644 --- a/drivers/net/ethernet/intel/e1000e/netdev.c +++ b/drivers/net/ethernet/intel/e1000e/netdev.c @@ -6623,7 +6623,6 @@ 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) @@ -6697,23 +6696,6 @@ 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
The commit 861e8086029e ("e1000e: move force SMBUS from enable ulp function to avoid PHY loss issue") introduces a regression on CH_MTP_I219_LM18 (PCIID: 0x8086550A). Without this commit, the ethernet works well after suspend and resume, but after applying the commit, the ethernet couldn't work anymore after the resume and the dmesg shows that the NIC Link changes to 10Mbps (1000Mbps originally): [ 43.305084] e1000e 0000:00:1f.6 enp0s31f6: NIC Link is Up 10 Mbps Full Duplex, Flow Control: Rx/Tx Without the commit, the force SMBUS code will not be executed if "return 0" or "goto out" is executed in the enable_ulp(), and in my case, the "goto out" is executed since FWSM_FW_VALID is set. But after applying the commit, the force SMBUS code will be ran unconditionally. Here move the force SMBUS code back to enable_ulp() and put it immediate ahead of hw->phy.ops.release(hw), this could allow the longest settling time as possible for interface in this function and doesn't change the original code logic. Fixes: 861e8086029e ("e1000e: move force SMBUS from enable ulp function to avoid PHY loss issue") Signed-off-by: Hui Wang <hui.wang@canonical.com> --- drivers/net/ethernet/intel/e1000e/ich8lan.c | 19 +++++++++++++++++++ drivers/net/ethernet/intel/e1000e/netdev.c | 18 ------------------ 2 files changed, 19 insertions(+), 18 deletions(-)