netdev - Re: [PATCH v2] 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]

Message-ID: <83b83fb5-bd4a-4830-902e-8ac255d77021@intel.com>
Date: Thu, 16 May 2024 10:29:40 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
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>
Subject: Re: [PATCH v2] e1000e: move force SMBUS near the end of enable_ulp
 function



On 5/8/2024 5:06 AM, 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 the referred 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@...onical.com>
> Acked-by: Vitaly Lifshits <vitaly.lifshits@...el.com>
> Tested-by: Naama Meir <naamax.meir@...ux.intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> ---

You didn't add a tag for which tree to target, typically for patches
picked up through Intel Wired LAN, this would be "iwl-net" or
"iwl-next". I presume since this is a fix that this should go to iwl-net
and will apply it as appropriate.

Thanks,
Jake

> In the v2:
>  Change "this commit" to "the referred commit" in the commit header
>  Fix a potential infinite loop if ret_val is not zero
>  
>  drivers/net/ethernet/intel/e1000e/ich8lan.c | 22 +++++++++++++++++++++
>  drivers/net/ethernet/intel/e1000e/netdev.c  | 18 -----------------
>  2 files changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> index f9e94be36e97..2e98a2a0bead 100644
> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
> @@ -1225,6 +1225,28 @@ 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 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