[PATCH] e1000e: continue to init phy even when failed to disable ULP

Subsystems: intel ethernet drivers, networking drivers, the rest

STALE2203d

10 messages, 5 authors, 2020-07-28 · open the first message on its own page

[PATCH] e1000e: continue to init phy even when failed to disable ULP

From: Aaron Ma <hidden>
Date: 2020-06-16 10:06:00 

After commit "e1000e: disable s0ix entry and exit flows for ME systems",
some ThinkPads always failed to disable ulp by ME.
commit "e1000e: Warn if disabling ULP failed" break out of init phy:

error log:
[   42.364753] e1000e 0000:00:1f.6 enp0s31f6: Failed to disable ULP
[   42.524626] e1000e 0000:00:1f.6 enp0s31f6: PHY Wakeup cause - Unicast Packet
[   42.822476] e1000e 0000:00:1f.6 enp0s31f6: Hardware Error

When disable s0ix, E1000_FWSM_ULP_CFG_DONE will never be 1.
If continue to init phy like before, it can work as before.
iperf test result good too.

Chnage e_warn to e_dbg, in case it confuses.

Signed-off-by: Aaron Ma <redacted>
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index f999cca37a8a..63405819eb83 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -302,8 +302,7 @@ static s32 e1000_init_phy_workarounds_pchlan(struct e1000_hw *hw)
 	hw->dev_spec.ich8lan.ulp_state = e1000_ulp_state_unknown;
 	ret_val = e1000_disable_ulp_lpt_lp(hw, true);
 	if (ret_val) {
-		e_warn("Failed to disable ULP\n");
-		goto out;
+		e_dbg("Failed to disable ULP\n");
 	}
 
 	ret_val = hw->phy.ops.acquire(hw);
-- 
2.26.2

Re: [Intel-wired-lan] [PATCH] e1000e: continue to init phy even when failed to disable ULP

From: Paul Menzel <hidden>
Date: 2020-06-16 10:20:33 

Dear Aaron,


Thank you for your patch.

(Rant: Some more fallout from the other patch, which nobody reverted.)

Am 16.06.20 um 12:05 schrieb Aaron Ma:
After commit "e1000e: disable s0ix entry and exit flows for ME systems",
some ThinkPads always failed to disable ulp by ME.
Please add the (short) commit hash from the master branch.

s/ulp/ULP/

Please list one ThinkPad as example.

commit "e1000e: Warn if disabling ULP failed" break out of init phy:
1.  Please add the closing quote ".
2.  Please add the commit hash.

error log:
[   42.364753] e1000e 0000:00:1f.6 enp0s31f6: Failed to disable ULP
[   42.524626] e1000e 0000:00:1f.6 enp0s31f6: PHY Wakeup cause - Unicast Packet
[   42.822476] e1000e 0000:00:1f.6 enp0s31f6: Hardware Error

When disable s0ix, E1000_FWSM_ULP_CFG_DONE will never be 1.
If continue to init phy like before, it can work as before.
iperf test result good too.

Chnage e_warn to e_dbg, in case it confuses.
s/Chnage/Change/

Please leave the level warning, and improve the warning message instead, 
so a user knows what is going on.

Could you please add a `Fixes:` tag and the URL to the bug report?
quoted hunk
Signed-off-by: Aaron Ma <redacted>
---
  drivers/net/ethernet/intel/e1000e/ich8lan.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index f999cca37a8a..63405819eb83 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -302,8 +302,7 @@ static s32 e1000_init_phy_workarounds_pchlan(struct e1000_hw *hw)
  	hw->dev_spec.ich8lan.ulp_state = e1000_ulp_state_unknown;
  	ret_val = e1000_disable_ulp_lpt_lp(hw, true);
  	if (ret_val) {
-		e_warn("Failed to disable ULP\n");
-		goto out;
+		e_dbg("Failed to disable ULP\n");
  	}
  
  	ret_val = hw->phy.ops.acquire(hw);
Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH] e1000e: continue to init phy even when failed to disable ULP

From: Aaron Ma <hidden>
Date: 2020-06-16 10:31:44 

On 6/16/20 6:20 PM, Paul Menzel wrote:
Dear Aaron,


Thank you for your patch.

(Rant: Some more fallout from the other patch, which nobody reverted.)
Would you like a revert?

Thanks,
Aaron

Am 16.06.20 um 12:05 schrieb Aaron Ma:
quoted
After commit "e1000e: disable s0ix entry and exit flows for ME systems",
some ThinkPads always failed to disable ulp by ME.
Please add the (short) commit hash from the master branch.

s/ulp/ULP/

Please list one ThinkPad as example.
quoted
commit "e1000e: Warn if disabling ULP failed" break out of init phy:
1.  Please add the closing quote ".
2.  Please add the commit hash.
quoted
error log:
[   42.364753] e1000e 0000:00:1f.6 enp0s31f6: Failed to disable ULP
[   42.524626] e1000e 0000:00:1f.6 enp0s31f6: PHY Wakeup cause - Unicast Packet
[   42.822476] e1000e 0000:00:1f.6 enp0s31f6: Hardware Error

When disable s0ix, E1000_FWSM_ULP_CFG_DONE will never be 1.
If continue to init phy like before, it can work as before.
iperf test result good too.

Chnage e_warn to e_dbg, in case it confuses.
s/Chnage/Change/

Please leave the level warning, and improve the warning message instead, so a user knows what is going on.

Could you please add a `Fixes:` tag and the URL to the bug report?
quoted
Signed-off-by: Aaron Ma <redacted>
---
  drivers/net/ethernet/intel/e1000e/ich8lan.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index f999cca37a8a..63405819eb83 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -302,8 +302,7 @@ static s32 e1000_init_phy_workarounds_pchlan(struct e1000_hw *hw)
      hw->dev_spec.ich8lan.ulp_state = e1000_ulp_state_unknown;
      ret_val = e1000_disable_ulp_lpt_lp(hw, true);
      if (ret_val) {
-        e_warn("Failed to disable ULP\n");
-        goto out;
+        e_dbg("Failed to disable ULP\n");
      }
        ret_val = hw->phy.ops.acquire(hw);
Kind regards,

Paul

Re: [PATCH] e1000e: continue to init phy even when failed to disable ULP

From: Kai-Heng Feng <hidden>
Date: 2020-06-16 11:24:05 

quoted hunk
On Jun 16, 2020, at 18:05, Aaron Ma [off-list ref] wrote:

After commit "e1000e: disable s0ix entry and exit flows for ME systems",
some ThinkPads always failed to disable ulp by ME.
commit "e1000e: Warn if disabling ULP failed" break out of init phy:

error log:
[   42.364753] e1000e 0000:00:1f.6 enp0s31f6: Failed to disable ULP
[   42.524626] e1000e 0000:00:1f.6 enp0s31f6: PHY Wakeup cause - Unicast Packet
[   42.822476] e1000e 0000:00:1f.6 enp0s31f6: Hardware Error

When disable s0ix, E1000_FWSM_ULP_CFG_DONE will never be 1.
If continue to init phy like before, it can work as before.
iperf test result good too.

Chnage e_warn to e_dbg, in case it confuses.

Signed-off-by: Aaron Ma <redacted>
---
drivers/net/ethernet/intel/e1000e/ich8lan.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index f999cca37a8a..63405819eb83 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -302,8 +302,7 @@ static s32 e1000_init_phy_workarounds_pchlan(struct e1000_hw *hw)
	hw->dev_spec.ich8lan.ulp_state = e1000_ulp_state_unknown;
	ret_val = e1000_disable_ulp_lpt_lp(hw, true);
If si0x entry isn't enabled, maybe skip calling e1000_disable_ulp_lpt_lp() altogether?
We can use e1000e_check_me() to check that.

	if (ret_val) {
-		e_warn("Failed to disable ULP\n");
-		goto out;
+		e_dbg("Failed to disable ULP\n");
	}
The change of "e1000e: Warn if disabling ULP failed" is intentional to catch bugs like this.

Kai-Heng

	ret_val = hw->phy.ops.acquire(hw);
-- 
2.26.2





  

    
Re: [PATCH] e1000e: continue to init phy even when failed to disable ULP

    

      From: Aaron Ma <hidden>

      Date: 2020-06-16 11:26:37 
    

  

  

On 6/16/20 7:23 PM, Kai-Heng Feng wrote:
quoted
On Jun 16, 2020, at 18:05, Aaron Ma [off-list ref] wrote:

After commit "e1000e: disable s0ix entry and exit flows for ME systems",
some ThinkPads always failed to disable ulp by ME.
commit "e1000e: Warn if disabling ULP failed" break out of init phy:

error log:
[   42.364753] e1000e 0000:00:1f.6 enp0s31f6: Failed to disable ULP
[   42.524626] e1000e 0000:00:1f.6 enp0s31f6: PHY Wakeup cause - Unicast Packet
[   42.822476] e1000e 0000:00:1f.6 enp0s31f6: Hardware Error

When disable s0ix, E1000_FWSM_ULP_CFG_DONE will never be 1.
If continue to init phy like before, it can work as before.
iperf test result good too.

Chnage e_warn to e_dbg, in case it confuses.

Signed-off-by: Aaron Ma <redacted>
---
drivers/net/ethernet/intel/e1000e/ich8lan.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index f999cca37a8a..63405819eb83 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -302,8 +302,7 @@ static s32 e1000_init_phy_workarounds_pchlan(struct e1000_hw *hw)
	hw->dev_spec.ich8lan.ulp_state = e1000_ulp_state_unknown;
	ret_val = e1000_disable_ulp_lpt_lp(hw, true);
If si0x entry isn't enabled, maybe skip calling e1000_disable_ulp_lpt_lp() altogether?
We can use e1000e_check_me() to check that.
No, s0ix is different feature with ULP mode.

Aaron

quoted
	if (ret_val) {
-		e_warn("Failed to disable ULP\n");
-		goto out;
+		e_dbg("Failed to disable ULP\n");
	}
The change of "e1000e: Warn if disabling ULP failed" is intentional to catch bugs like this.

Kai-Heng

quoted
	ret_val = hw->phy.ops.acquire(hw);
-- 
2.26.2





  

    
[v2][PATCH] e1000e: continue to init phy even when failed to disable ULP

    

      From: Aaron Ma <hidden>

      Date: 2020-06-17 11:13:10 
    

  

  
After commit: e086ba2fccd ("e1000e: disable s0ix entry and exit flows
 for ME systems").
ThinkPad P14s always failed to disable ULP by ME.
commit: 0c80cdbf33 ("e1000e: Warn if disabling ULP failed")
break out of init phy:

error log:
[   42.364753] e1000e 0000:00:1f.6 enp0s31f6: Failed to disable ULP
[   42.524626] e1000e 0000:00:1f.6 enp0s31f6: PHY Wakeup cause - Unicast Packet
[   42.822476] e1000e 0000:00:1f.6 enp0s31f6: Hardware Error

When disable s0ix, E1000_FWSM_ULP_CFG_DONE will never be 1.
If continue to init phy like before, it can work as before.
iperf test result good too.

Fixes: 0c80cdbf33 ("e1000e: Warn if disabling ULP failed")
Signed-off-by: Aaron Ma <redacted>
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index f999cca37a8a..be7475c5529d 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -303,7 +303,6 @@ static s32 e1000_init_phy_workarounds_pchlan(struct e1000_hw *hw)
 	ret_val = e1000_disable_ulp_lpt_lp(hw, true);
 	if (ret_val) {
 		e_warn("Failed to disable ULP\n");
-		goto out;
 	}
 
 	ret_val = hw->phy.ops.acquire(hw);
-- 
2.26.2






  

    
Re: [v2][PATCH] e1000e: continue to init phy even when failed to disable ULP

    

      From: Jakub Kicinski <kuba@kernel.org>

      Date: 2020-06-17 16:28:58 
    

  

  
On Wed, 17 Jun 2020 19:12:48 +0800 Aaron Ma wrote:
After commit: e086ba2fccd ("e1000e: disable s0ix entry and exit flows
 for ME systems").
ThinkPad P14s always failed to disable ULP by ME.
commit: 0c80cdbf33 ("e1000e: Warn if disabling ULP failed")
break out of init phy:

error log:
[   42.364753] e1000e 0000:00:1f.6 enp0s31f6: Failed to disable ULP
[   42.524626] e1000e 0000:00:1f.6 enp0s31f6: PHY Wakeup cause - Unicast Packet
[   42.822476] e1000e 0000:00:1f.6 enp0s31f6: Hardware Error

When disable s0ix, E1000_FWSM_ULP_CFG_DONE will never be 1.
If continue to init phy like before, it can work as before.
iperf test result good too.

Fixes: 0c80cdbf33 ("e1000e: Warn if disabling ULP failed")
Signed-off-by: Aaron Ma <redacted>
Fixes tag: Fixes: 0c80cdbf33 ("e1000e: Warn if disabling ULP failed")
Has these problem(s):
	- SHA1 should be at least 12 digits long
	  Can be fixed by setting core.abbrev to 12 (or more) or (for git v2.11
	  or later) just making sure it is not set (or set to "auto").





  

    
[v3][PATCH] e1000e: continue to init phy even when failed to disable ULP

    

      From: Aaron Ma <hidden>

      Date: 2020-06-18 06:45:28 
    

  

  
After commit: e086ba2fccda4 ("e1000e: disable s0ix entry and exit flows
 for ME systems").
ThinkPad P14s always failed to disable ULP by ME.
commit: 0c80cdbf3320 ("e1000e: Warn if disabling ULP failed")
break out of init phy:

error log:
[   42.364753] e1000e 0000:00:1f.6 enp0s31f6: Failed to disable ULP
[   42.524626] e1000e 0000:00:1f.6 enp0s31f6: PHY Wakeup cause - Unicast Packet
[   42.822476] e1000e 0000:00:1f.6 enp0s31f6: Hardware Error

When disable s0ix, E1000_FWSM_ULP_CFG_DONE will never be 1.
If continue to init phy like before, it can work as before.
iperf test result good too.

Fixes: 0c80cdbf3320 ("e1000e: Warn if disabling ULP failed")
Signed-off-by: Aaron Ma <redacted>
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index f999cca37a8a..be7475c5529d 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -303,7 +303,6 @@ static s32 e1000_init_phy_workarounds_pchlan(struct e1000_hw *hw)
 	ret_val = e1000_disable_ulp_lpt_lp(hw, true);
 	if (ret_val) {
 		e_warn("Failed to disable ULP\n");
-		goto out;
 	}
 
 	ret_val = hw->phy.ops.acquire(hw);
-- 
2.26.2






  

    
[v4][PATCH] e1000e: continue to init phy even when failed to disable ULP

    

      From: Aaron Ma <hidden>

      Date: 2020-06-18 06:55:11 
    

  

  
After 'commit e086ba2fccda4 ("e1000e: disable s0ix entry and exit flows
 for ME systems")',
ThinkPad P14s always failed to disable ULP by ME.
'commit 0c80cdbf3320 ("e1000e: Warn if disabling ULP failed")'
break out of init phy:

error log:
[   42.364753] e1000e 0000:00:1f.6 enp0s31f6: Failed to disable ULP
[   42.524626] e1000e 0000:00:1f.6 enp0s31f6: PHY Wakeup cause - Unicast Packet
[   42.822476] e1000e 0000:00:1f.6 enp0s31f6: Hardware Error

When disable s0ix, E1000_FWSM_ULP_CFG_DONE will never be 1.
If continue to init phy like before, it can work as before.
iperf test result good too.

Fixes: 0c80cdbf3320 ("e1000e: Warn if disabling ULP failed")
Signed-off-by: Aaron Ma <redacted>
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index f999cca37a8a..be7475c5529d 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -303,7 +303,6 @@ static s32 e1000_init_phy_workarounds_pchlan(struct e1000_hw *hw)
 	ret_val = e1000_disable_ulp_lpt_lp(hw, true);
 	if (ret_val) {
 		e_warn("Failed to disable ULP\n");
-		goto out;
 	}
 
 	ret_val = hw->phy.ops.acquire(hw);
-- 
2.26.2






  

    
RE: [Intel-wired-lan] [v4][PATCH] e1000e: continue to init phy even when failed to disable ULP

    

      From: Brown, Aaron F <hidden>

      Date: 2020-07-28 20:36:08 
    

  

  
From: Intel-wired-lan <redacted> On Behalf Of
Aaron Ma
Sent: Wednesday, June 17, 2020 11:55 PM
To: kuba@kernel.org; Kirsher, Jeffrey T <redacted>;
davem@davemloft.net; intel-wired-lan@lists.osuosl.org;
netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Lifshits, Vitaly
[off-list ref]; kai.heng.feng@canonical.com; Neftin, Sasha
[off-list ref]
Subject: [Intel-wired-lan] [v4][PATCH] e1000e: continue to init phy even when
failed to disable ULP

After 'commit e086ba2fccda4 ("e1000e: disable s0ix entry and exit flows
 for ME systems")',
ThinkPad P14s always failed to disable ULP by ME.
'commit 0c80cdbf3320 ("e1000e: Warn if disabling ULP failed")'
break out of init phy:

error log:
[   42.364753] e1000e 0000:00:1f.6 enp0s31f6: Failed to disable ULP
[   42.524626] e1000e 0000:00:1f.6 enp0s31f6: PHY Wakeup cause - Unicast
Packet
[   42.822476] e1000e 0000:00:1f.6 enp0s31f6: Hardware Error

When disable s0ix, E1000_FWSM_ULP_CFG_DONE will never be 1.
If continue to init phy like before, it can work as before.
iperf test result good too.

Fixes: 0c80cdbf3320 ("e1000e: Warn if disabling ULP failed")
Signed-off-by: Aaron Ma <redacted>
---
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 1 -
 1 file changed, 1 deletion(-)
I never did find a system that triggered the initial problem, but from a compatibility with the set of systems I do have working...
Tested-by: Aaron Brown <redacted>




    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help