netdev - Re: [iwl-net][PATCH] Revert "e1000e: move force SMBUS near the end of enable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 10 Jun 2024 08:36:00 +0200
From: Paul Menzel <pmenzel@...gen.mpg.de>
To: Hui Wang <hui.wang@...onical.com>, intel-wired-lan@...ts.osuosl.org,
 netdev@...r.kernel.org, kuba@...nel.org, anthony.l.nguyen@...el.com,
 vitaly.lifshits@...el.com, dima.ruinskiy@...el.com, davem@...emloft.net,
 pabeni@...hat.com, edumazet@...gle.com, sasha.neftin@...el.com,
 naamax.meir@...ux.intel.com
Cc: todd.e.brandt@...el.com, dmummenschanz@....de, rui.zhang@...el.com,
 jacob.e.keller@...el.com, horms@...nel.org, regressions@...ts.linux.dev,
 Naama Meir <naamax.meir@...ux.intel.com>
Subject: Re: [iwl-net][PATCH] Revert "e1000e: move force SMBUS near the end of
 enable_ulp function"

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@...el.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218940
> Reported-by: Dieter Mummenschanz <dmummenschanz@....de>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218936
> Signed-off-by: Hui Wang <hui.wang@...onical.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@...gen.mpg.de>


Kind regards,

Paul