[Intel-wired-lan] [iwl-net][PATCH] Revert "e1000e: move force SMBUS near the end of enable_ulp function"

[Intel-wired-lan] [iwl-net][PATCH] Revert "e1000e: move force SMBUS near the end of enable_ulp function"

[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
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(-)

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
-- 
2.34.1

Re: [Intel-wired-lan] [iwl-net][PATCH] Revert "e1000e: move force SMBUS near the end of enable_ulp function"

[Paul Menzel]

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

Re: [Intel-wired-lan] [iwl-net][PATCH] Revert "e1000e: move force SMBUS near the end of enable_ulp function"

[Zhang, Rui]

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

Re: [Intel-wired-lan] [iwl-net][PATCH] Revert "e1000e: move force SMBUS near the end of enable_ulp function"

[Hui Wang]

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

Re: [Intel-wired-lan] [iwl-net][PATCH] Revert "e1000e: move force SMBUS near the end of enable_ulp function"

[Hui Wang]

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.

>