From: Hui Wang <hui.wang@canonical.com>
To: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
kuba@kernel.org, anthony.l.nguyen@intel.com,
vitaly.lifshits@intel.com, dima.ruinskiy@intel.com,
davem@davemloft.net, pabeni@redhat.com, edumazet@google.com,
sasha.neftin@intel.com, naamax.meir@linux.intel.com,
Jacob Keller <jacob.e.keller@intel.com>,
Oliver Sang <oliver.sang@intel.com>,
Zhang Rui <rui.zhang@intel.com>,
regressions@lists.linux.dev
Subject: Re: [Intel-wired-lan] [PATCH v2] e1000e: move force SMBUS near the end of enable_ulp function
Date: Fri, 17 May 2024 21:00:46 +0800 [thread overview]
Message-ID: <2493e4f7-7043-4408-be99-9ee329cb3db9@canonical.com> (raw)
In-Reply-To: <8bc68316-fef4-47c0-ad08-ad93f6a4fb30@molgen.mpg.de>
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/
prev parent reply other threads:[~2024-05-17 13:00 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-08 12:06 [PATCH v2] e1000e: move force SMBUS near the end of enable_ulp function Hui Wang
2024-05-10 11:02 ` Simon Horman
2024-05-16 17:29 ` Jacob Keller
2024-05-17 5:45 ` [Intel-wired-lan] " Paul Menzel
[not found] ` <7b0a36a5-4265-41e7-b66a-2cd1f01aed42@canonical.com>
2024-05-17 10:07 ` Paul Menzel
2024-05-17 13:00 ` Hui Wang [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2493e4f7-7043-4408-be99-9ee329cb3db9@canonical.com \
--to=hui.wang@canonical.com \
--cc=anthony.l.nguyen@intel.com \
--cc=davem@davemloft.net \
--cc=dima.ruinskiy@intel.com \
--cc=edumazet@google.com \
--cc=intel-wired-lan@lists.osuosl.org \
--cc=jacob.e.keller@intel.com \
--cc=kuba@kernel.org \
--cc=naamax.meir@linux.intel.com \
--cc=netdev@vger.kernel.org \
--cc=oliver.sang@intel.com \
--cc=pabeni@redhat.com \
--cc=pmenzel@molgen.mpg.de \
--cc=regressions@lists.linux.dev \
--cc=rui.zhang@intel.com \
--cc=sasha.neftin@intel.com \
--cc=vitaly.lifshits@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).