[Intel-wired-lan] [PATCH iwl-net v1 1/1] e1000e: avoid failing the system during pm_suspend

[Intel-wired-lan] [PATCH iwl-net v1 1/1] e1000e: avoid failing the system during pm_suspend

[Vitaly Lifshits]

Occasionally when the system goes into pm_suspend, the suspend might fail
due to a PHY access error on the network adapter. Previously, this would
have caused the whole system to fail to go to a low power state.
An example of this was reported in the following Bugzilla:
https://bugzilla.kernel.org/show_bug.cgi?id=205015

[ 1663.694828] e1000e 0000:00:19.0 eth0: Failed to disable ULP
[ 1664.731040] asix 2-3:1.0 eth1: link up, 100Mbps, full-duplex, lpa 0xC1E1
[ 1665.093513] e1000e 0000:00:19.0 eth0: Hardware Error
[ 1665.596760] e1000e 0000:00:19.0: pci_pm_resume+0x0/0x80 returned 0 after 2975399 usecs
and then the system never recovers from it, and all the following suspend failed due to this
[22909.393854] PM: pci_pm_suspend(): e1000e_pm_suspend+0x0/0x760 [e1000e] returns -2
[22909.393858] PM: dpm_run_callback(): pci_pm_suspend+0x0/0x160 returns -2
[22909.393861] PM: Device 0000:00:1f.6 failed to suspend async: error -2

This can be avoided by changing the return values of __e1000_shutdown and
e1000e_pm_suspend functions so that they always return 0 (success). This
is consistent with what other drivers do.
If the e1000e deiver encounters a hardware error during suspend, potential
side effects include slightly higher power draw or non-working wake on
LAN. This is preferred to a system-level suspend failure, and a warning
message is written to the system log, so that the user can be aware that
the LAN controller experienced a problem during suspend.

Suggested-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=205015
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 360ee26557f7..f103249b12fa 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -6671,8 +6671,10 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime)
 		if (adapter->flags2 & FLAG2_HAS_PHY_WAKEUP) {
 			/* enable wakeup by the PHY */
 			retval = e1000_init_phy_wakeup(adapter, wufc);
-			if (retval)
-				return retval;
+			if (retval) {
+				e_err("Failed to enable wakeup\n");
+				goto skip_phy_configurations;
+			}
 		} else {
 			/* enable wakeup by the MAC */
 			ew32(WUFC, wufc);
@@ -6693,8 +6695,10 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime)
 			 * or broadcast.
 			 */
 			retval = e1000_enable_ulp_lpt_lp(hw, !runtime);
-			if (retval)
-				return retval;
+			if (retval) {
+				e_err("Failed to enable ULP\n");
+				goto skip_phy_configurations;
+			}
 		}
 	}
 
@@ -6726,6 +6730,7 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime)
 		hw->phy.ops.release(hw);
 	}
 
+skip_phy_configurations:
 	/* Release control of h/w to f/w.  If f/w is AMT enabled, this
 	 * would have already happened in close and is redundant.
 	 */
@@ -6968,15 +6973,13 @@ static int e1000e_pm_suspend(struct device *dev)
 	e1000e_pm_freeze(dev);
 
 	rc = __e1000_shutdown(pdev, false);
-	if (rc) {
-		e1000e_pm_thaw(dev);
-	} else {
+	if (!rc) {
 		/* Introduce S0ix implementation */
 		if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS)
 			e1000e_s0ix_entry_flow(adapter);
 	}
 
-	return rc;
+	return 0;
 }
 
 static int e1000e_pm_resume(struct device *dev)
-- 
2.34.1

Re: [Intel-wired-lan] [PATCH iwl-net v1 1/1] e1000e: avoid failing the system during pm_suspend

[Tony Nguyen]

On 8/6/2024 6:23 AM, Vitaly Lifshits wrote:
> Occasionally when the system goes into pm_suspend, the suspend might fail
> due to a PHY access error on the network adapter. Previously, this would
> have caused the whole system to fail to go to a low power state.
> An example of this was reported in the following Bugzilla:
> https://bugzilla.kernel.org/show_bug.cgi?id=205015
> 
> [ 1663.694828] e1000e 0000:00:19.0 eth0: Failed to disable ULP
> [ 1664.731040] asix 2-3:1.0 eth1: link up, 100Mbps, full-duplex, lpa 0xC1E1
> [ 1665.093513] e1000e 0000:00:19.0 eth0: Hardware Error
> [ 1665.596760] e1000e 0000:00:19.0: pci_pm_resume+0x0/0x80 returned 0 after 2975399 usecs
> and then the system never recovers from it, and all the following suspend failed due to this
> [22909.393854] PM: pci_pm_suspend(): e1000e_pm_suspend+0x0/0x760 [e1000e] returns -2
> [22909.393858] PM: dpm_run_callback(): pci_pm_suspend+0x0/0x160 returns -2
> [22909.393861] PM: Device 0000:00:1f.6 failed to suspend async: error -2
> 
> This can be avoided by changing the return values of __e1000_shutdown and
> e1000e_pm_suspend functions so that they always return 0 (success). This
> is consistent with what other drivers do.
> If the e1000e deiver encounters a hardware error during suspend, potential
> side effects include slightly higher power draw or non-working wake on
> LAN. This is preferred to a system-level suspend failure, and a warning
> message is written to the system log, so that the user can be aware that
> the LAN controller experienced a problem during suspend.
> 

Could you please provide a Fixes:

Thanks,
Tony

> Suggested-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=205015
> ---
>   drivers/net/ethernet/intel/e1000e/netdev.c | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 360ee26557f7..f103249b12fa 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -6671,8 +6671,10 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime)
>   		if (adapter->flags2 & FLAG2_HAS_PHY_WAKEUP) {
>   			/* enable wakeup by the PHY */
>   			retval = e1000_init_phy_wakeup(adapter, wufc);
> -			if (retval)
> -				return retval;
> +			if (retval) {
> +				e_err("Failed to enable wakeup\n");
> +				goto skip_phy_configurations;
> +			}
>   		} else {
>   			/* enable wakeup by the MAC */
>   			ew32(WUFC, wufc);
> @@ -6693,8 +6695,10 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime)
>   			 * or broadcast.
>   			 */
>   			retval = e1000_enable_ulp_lpt_lp(hw, !runtime);
> -			if (retval)
> -				return retval;
> +			if (retval) {
> +				e_err("Failed to enable ULP\n");
> +				goto skip_phy_configurations;
> +			}
>   		}
>   	}
>   
> @@ -6726,6 +6730,7 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool runtime)
>   		hw->phy.ops.release(hw);
>   	}
>   
> +skip_phy_configurations:
>   	/* Release control of h/w to f/w.  If f/w is AMT enabled, this
>   	 * would have already happened in close and is redundant.
>   	 */
> @@ -6968,15 +6973,13 @@ static int e1000e_pm_suspend(struct device *dev)
>   	e1000e_pm_freeze(dev);
>   
>   	rc = __e1000_shutdown(pdev, false);
> -	if (rc) {
> -		e1000e_pm_thaw(dev);
> -	} else {
> +	if (!rc) {
>   		/* Introduce S0ix implementation */
>   		if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS)
>   			e1000e_s0ix_entry_flow(adapter);
>   	}
>   
> -	return rc;
> +	return 0;
>   }
>   
>   static int e1000e_pm_resume(struct device *dev)

Re: [Intel-wired-lan] [PATCH iwl-net v1 1/1] e1000e: avoid failing the system during pm_suspend

[Mor Bar-Gabay]

On 06/08/2024 16:23, Vitaly Lifshits wrote:
> Occasionally when the system goes into pm_suspend, the suspend might fail
> due to a PHY access error on the network adapter. Previously, this would
> have caused the whole system to fail to go to a low power state.
> An example of this was reported in the following Bugzilla:
> https://bugzilla.kernel.org/show_bug.cgi?id=205015
> 
> [ 1663.694828] e1000e 0000:00:19.0 eth0: Failed to disable ULP
> [ 1664.731040] asix 2-3:1.0 eth1: link up, 100Mbps, full-duplex, lpa 0xC1E1
> [ 1665.093513] e1000e 0000:00:19.0 eth0: Hardware Error
> [ 1665.596760] e1000e 0000:00:19.0: pci_pm_resume+0x0/0x80 returned 0 after 2975399 usecs
> and then the system never recovers from it, and all the following suspend failed due to this
> [22909.393854] PM: pci_pm_suspend(): e1000e_pm_suspend+0x0/0x760 [e1000e] returns -2
> [22909.393858] PM: dpm_run_callback(): pci_pm_suspend+0x0/0x160 returns -2
> [22909.393861] PM: Device 0000:00:1f.6 failed to suspend async: error -2
> 
> This can be avoided by changing the return values of __e1000_shutdown and
> e1000e_pm_suspend functions so that they always return 0 (success). This
> is consistent with what other drivers do.
> If the e1000e deiver encounters a hardware error during suspend, potential
> side effects include slightly higher power draw or non-working wake on
> LAN. This is preferred to a system-level suspend failure, and a warning
> message is written to the system log, so that the user can be aware that
> the LAN controller experienced a problem during suspend.
> 
> Suggested-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=205015
> ---
>   drivers/net/ethernet/intel/e1000e/netdev.c | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)
> 
Tested-by: Mor Bar-Gabay <morx.bar.gabay@intel.com>