netdev - Re: [iwl-net][PATCH v3] 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] [day] [month] [year] [list]

Date: Tue, 4 Jun 2024 15:51:18 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
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: [iwl-net][PATCH v3] e1000e: move force SMBUS near the end of
 enable_ulp function

On 17/05/2024 15:50, Hui Wang wrote:
> The commit 861e8086029e ("e1000e: move force SMBUS from enable ulp
> function to avoid PHY loss issue") introduces a regression on
> PCH_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
> immediately 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.
> 
> The issue was found on a Lenovo laptop with the ethernet hw as below:
> 00:1f.6 Ethernet controller [0200]: Intel Corporation Device [8086:550a]
> (rev 20).
> 
> And this patch is verified (cable plug and unplug, system suspend
> and resume) on Lenovo laptops with ethernet hw: [8086:550a],
> [8086:550b], [8086:15bb], [8086:15be], [8086:1a1f], [8086:1a1c] and
> [8086:0dc7].
> 
> 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>
> Reviewed-by: Simon Horman <horms@...nel.org>
> Reviewed-by: Paul Menzel <pmenzel@...gen.mpg.de>

Where did you receive Paul's tag? Please point to the lore link
documenting it.

In other patchsets tags were made up without real review, thus I have
doubts also here.

Best regards,
Krzysztof