diff mbox series

[v2] e1000e: move force SMBUS near the end of enable_ulp function

Message ID 20240508120604.233166-1-hui.wang@canonical.com
State Superseded
Delegated to: Anthony Nguyen
Headers show
Series [v2] e1000e: move force SMBUS near the end of enable_ulp function | expand

Commit Message

Hui Wang May 8, 2024, 12:06 p.m. UTC
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@canonical.com>
Acked-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
Tested-by: Naama Meir <naamax.meir@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
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(-)

Comments

Simon Horman May 10, 2024, 11:02 a.m. UTC | #1
On Wed, May 08, 2024 at 08:06:04PM +0800, 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@canonical.com>
> Acked-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> Tested-by: Naama Meir <naamax.meir@linux.intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
> 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

Reviewed-by: Simon Horman <horms@kernel.org>
Jacob Keller May 16, 2024, 5:29 p.m. UTC | #2
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@canonical.com>
> Acked-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> Tested-by: Naama Meir <naamax.meir@linux.intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.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
Paul Menzel May 17, 2024, 5:45 a.m. UTC | #3
Dear Hui,


Thank you for your patch.

Am 08.05.24 um 14:06 schrieb Hui Wang:
> 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

*P*CH

> 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):

1.  s/Link/link/
2.  “couldn’t work” means the reduced bandwidth?
3.  Please add a blank line and maybe indent the past with four spaces.

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

immediate*l*?

> longest settling time as possible for interface in this function and
> doesn't change the original code logic.

Re-ordering code to achieve some waiting time sounds like, it’s not 100 
% sure, that the problem won’t occur again?

Could you please document your test system?

Just a side note: Booting Linux 6.9-rc5+ *with kexec* on Supermicro 
Super Server/X13SAE, BIOS 2.0 10/17/2022 with the network device below, 
it also came up only with 10 Mbps and Ethernet did not work, for example 
`ping`. I conjectured though, that the non-working part was due to the 
switch configuration not allowing 10 Mbps.

     00:1f.6 Ethernet controller [0200]: Intel Corporation Ethernet 
Connection (17) I219-LM [8086:1a1c] (rev 11)

I didn’t find the time to further analyze and report the issue.

Also could this also be related to the regression reported by the kernel 
test robot [1]?

     00:19.0 Ethernet controller: Intel Corporation Ethernet Connection 
(3) I218-V (rev 03)


> Fixes: 861e8086029e ("e1000e: move force SMBUS from enable ulp function to avoid PHY loss issue")
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> Acked-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> Tested-by: Naama Meir <naamax.meir@linux.intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
> 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


Kind regards,

Paul


[1]: 
https://lore.kernel.org/intel-wired-lan/202405150942.f9b873b1-oliver.sang@intel.com/
Hui Wang May 17, 2024, 9:45 a.m. UTC | #4
On 5/17/24 13:45, Paul Menzel wrote:
> Dear Hui,
>
>
> Thank you for your patch.
>
> Am 08.05.24 um 14:06 schrieb Hui Wang:
>> 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
>
> *P*CH
>
>> 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):
>
> 1.  s/Link/link/
> 2.  “couldn’t work” means the reduced bandwidth?
On my side, once the link changes to 10Mbps, I couldn't ping the machine 
anymore. And as you said, it probably has sth to do with switch/router 
configuration.
> 3. Please add a blank line and maybe indent the past with four spaces.
>
>> [   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
>
> immediate*l*?
>
>> longest settling time as possible for interface in this function and
>> doesn't change the original code logic.
>
> Re-ordering code to achieve some waiting time sounds like, it’s not 
> 100 % sure, that the problem won’t occur again?

Actually this patch not only adds the waiting time, but also restore the 
original code logic:

  original: On a machine with the CSME, the SMBUS will not be forced, 
accordingly the SMBUS will not be unforced after resume.

  wrong: On a machine with the CSME, the SMBUS is forced, but the SMBUS 
is not unforced after resume, there is an unbalance. My patch could fix 
this case.


>
> Could you please document your test system?
Lenovo Thinkpad P16Gen2 with ethernet card:

00:1f.6 Ethernet controller [0200]: Intel Corporation Device [8086:550a] (rev 20)

>
> Just a side note: Booting Linux 6.9-rc5+ *with kexec* on Supermicro 
> Super Server/X13SAE, BIOS 2.0 10/17/2022 with the network device 
> below, it also came up only with 10 Mbps and Ethernet did not work, 
> for example `ping`. I conjectured though, that the non-working part 
> was due to the switch configuration not allowing 10 Mbps.
>
>     00:1f.6 Ethernet controller [0200]: Intel Corporation Ethernet 
> Connection (17) I219-LM [8086:1a1c] (rev 11)
>
My test and result are same as yours.

Thanks.

> I didn’t find the time to further analyze and report the issue.
>
> Also could this also be related to the regression reported by the 
> kernel test robot [1]?
>
>     00:19.0 Ethernet controller: Intel Corporation Ethernet Connection 
> (3) I218-V (rev 03)
>
>
>> Fixes: 861e8086029e ("e1000e: move force SMBUS from enable ulp 
>> function to avoid PHY loss issue")
>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>> Acked-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>> Tested-by: Naama Meir <naamax.meir@linux.intel.com>
>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>> ---
>> 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
>
>
> Kind regards,
>
> Paul
>
>
> [1]: 
> https://lore.kernel.org/intel-wired-lan/202405150942.f9b873b1-oliver.sang@intel.com/
Paul Menzel May 17, 2024, 10:07 a.m. UTC | #5
Dear Hui,


Thank you for your response.


Am 17.05.24 um 11:45 schrieb Hui Wang:
> 
> On 5/17/24 13:45, Paul Menzel wrote:

>> Am 08.05.24 um 14:06 schrieb Hui Wang:
>>> 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
>>
>> *P*CH
>>
>>> 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):
>>
>> 1.  s/Link/link/
>> 2.  “couldn’t work” means the reduced bandwidth?
> 
> On my side, once the link changes to 10Mbps, I couldn't ping the machine 
> anymore. And as you said, it probably has sth to do with switch/router 
> configuration.
> 
>> 3. Please add a blank line and maybe indent the past with four spaces.
>>
>>> [   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
>>
>> immediate*l*?

Sorry, I meant immediate*ly*.

>>> longest settling time as possible for interface in this function and
>>> doesn't change the original code logic.
>>
>> Re-ordering code to achieve some waiting time sounds like, it’s not 
>> 100 % sure, that the problem won’t occur again?
> 
> Actually this patch not only adds the waiting time, but also restore the 
> original code logic:
> 
>   original: On a machine with the CSME, the SMBUS will not be forced, 
> accordingly the SMBUS will not be unforced after resume.
> 
>   wrong: On a machine with the CSME, the SMBUS is forced, but the SMBUS 
> is not unforced after resume, there is an unbalance. My patch could fix 
> this case.

Thank you for elaborating. In my opinion, then two commits would be 
better. One revert with a description of the problem and documentation 
of the test systems. Then the second patch with the fix.

>> Could you please document your test system?
> Lenovo Thinkpad P16Gen2 with ethernet card:
> 
> 00:1f.6 Ethernet controller [0200]: Intel Corporation Device [8086:550a] (rev 20)
> 
>> Just a side note: Booting Linux 6.9-rc5+ *with kexec* on Supermicro 
>> Super Server/X13SAE, BIOS 2.0 10/17/2022 with the network device 
>> below, it also came up only with 10 Mbps and Ethernet did not work, 
>> for example `ping`. I conjectured though, that the non-working part 
>> was due to the switch configuration not allowing 10 Mbps.
>>
>>     00:1f.6 Ethernet controller [0200]: Intel Corporation Ethernet Connection (17) I219-LM [8086:1a1c] (rev 11)
>>
> My test and result are same as yours.
> 
> Thanks.

Thank you for the confirmation.

>> I didn’t find the time to further analyze and report the issue.
>>
>> Also could this also be related to the regression reported by the 
>> kernel test robot [1]?
>>
>>     00:19.0 Ethernet controller: Intel Corporation Ethernet Connection (3) I218-V (rev 03)
>>
>>> Fixes: 861e8086029e ("e1000e: move force SMBUS from enable ulp 
>>> function to avoid PHY loss issue")
>>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>>> Acked-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>>> Tested-by: Naama Meir <naamax.meir@linux.intel.com>
>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>>> ---
>>> 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


Kind regards,

Paul


>> [1]: https://lore.kernel.org/intel-wired-lan/202405150942.f9b873b1-oliver.sang@intel.com/
Hui Wang May 17, 2024, 1 p.m. UTC | #6
On 5/17/24 18:07, Paul Menzel wrote:
> Dear Hui,
>
>
> Thank you for your response.
>
>
> Am 17.05.24 um 11:45 schrieb Hui Wang:
>>
>> On 5/17/24 13:45, Paul Menzel wrote:
>
>>> Am 08.05.24 um 14:06 schrieb Hui Wang:
>>>> 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
>>>
>>> *P*CH
>>>
>>>> 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):
>>>
>>> 1.  s/Link/link/
>>> 2.  “couldn’t work” means the reduced bandwidth?
>>
>> On my side, once the link changes to 10Mbps, I couldn't ping the 
>> machine anymore. And as you said, it probably has sth to do with 
>> switch/router configuration.
>>
>>> 3. Please add a blank line and maybe indent the past with four spaces.
>>>
>>>> [   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
>>>
>>> immediate*l*?
>
> Sorry, I meant immediate*ly*.
Got it.
>
>>>> longest settling time as possible for interface in this function and
>>>> doesn't change the original code logic.
>>>
>>> Re-ordering code to achieve some waiting time sounds like, it’s not 
>>> 100 % sure, that the problem won’t occur again?
>>
>> Actually this patch not only adds the waiting time, but also restore 
>> the original code logic:
>>
>>   original: On a machine with the CSME, the SMBUS will not be forced, 
>> accordingly the SMBUS will not be unforced after resume.
>>
>>   wrong: On a machine with the CSME, the SMBUS is forced, but the 
>> SMBUS is not unforced after resume, there is an unbalance. My patch 
>> could fix this case.
>
> Thank you for elaborating. In my opinion, then two commits would be 
> better. One revert with a description of the problem and documentation 
> of the test systems. Then the second patch with the fix.

What you said makes sense. But for this particular case, I think it is 
not necessary since the patch is not that complicated and explanation is 
already in the commit header.

Anyway, I will address the rest comment of you and send a v3 patch.

Thanks,

Hui.

>
>>> Could you please document your test system?
>> Lenovo Thinkpad P16Gen2 with ethernet card:
>>
>> 00:1f.6 Ethernet controller [0200]: Intel Corporation Device 
>> [8086:550a] (rev 20)
>>
>>> Just a side note: Booting Linux 6.9-rc5+ *with kexec* on Supermicro 
>>> Super Server/X13SAE, BIOS 2.0 10/17/2022 with the network device 
>>> below, it also came up only with 10 Mbps and Ethernet did not work, 
>>> for example `ping`. I conjectured though, that the non-working part 
>>> was due to the switch configuration not allowing 10 Mbps.
>>>
>>>     00:1f.6 Ethernet controller [0200]: Intel Corporation Ethernet 
>>> Connection (17) I219-LM [8086:1a1c] (rev 11)
>>>
>> My test and result are same as yours.
>>
>> Thanks.
>
> Thank you for the confirmation.
>
>>> I didn’t find the time to further analyze and report the issue.
>>>
>>> Also could this also be related to the regression reported by the 
>>> kernel test robot [1]?
>>>
>>>     00:19.0 Ethernet controller: Intel Corporation Ethernet 
>>> Connection (3) I218-V (rev 03)
>>>
>>>> Fixes: 861e8086029e ("e1000e: move force SMBUS from enable ulp 
>>>> function to avoid PHY loss issue")
>>>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>>>> Acked-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>>>> Tested-by: Naama Meir <naamax.meir@linux.intel.com>
>>>> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
>>>> ---
>>>> 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
>
>
> Kind regards,
>
> Paul
>
>
>>> [1]: 
>>> https://lore.kernel.org/intel-wired-lan/202405150942.f9b873b1-oliver.sang@intel.com/
diff mbox series

Patch

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