On 8/22/2025 12:13 AM, Timo Teras wrote:
Sorry for accidental sending this before finished. Comments amended.
On Fri, 22 Aug 2025 00:01:09 +0300
Timo Teras <[email protected]> wrote:
Few more observations inline.
On Thu, 21 Aug 2025 20:12:29 +0300
Vitaly Lifshits <[email protected]> wrote:
The K1 state reduces power consumption on ICH family network
controllers during idle periods, similarly to L1 state on PCI
Express NICs. Therefore, it is recommended and enabled by default.
However, on some systems it has been observed to have adverse side
effects, such as packet loss. It has been established through debug
that the problem may be due to firmware misconfiguration of specific
systems, interoperability with certain link partners, or marginal
electrical conditions of specific units.
These problems typically cannot be fixed in the field, and generic
workarounds to resolve the side effects on all systems, while
keeping K1 enabled, were found infeasible.
Therefore, add the option for users to globally disable K1 idle
state on the adapter.
Additionally, disable K1 by default for MTL and later platforms, due
to issues reported with the current configuration.
Link:
https://lore.kernel.org/intel-wired-lan/camqyjg3lvqfgqmctxeapur_jq0oqh7ggdxruvtrx_6tth2m...@mail.gmail.com/
Link:
https://lore.kernel.org/intel-wired-lan/[email protected]/
Link:
https://lore.kernel.org/intel-wired-lan/Z_z9EjcKtwHCQcZR@mail-itl/
Link: https://github.com/QubesOS/qubes-issues/issues/9896 Link:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2115393
Signed-off-by: Vitaly Lifshits <[email protected]>
---
v2: Add a missing semaphore acquiring and disable K1 on MTL by
default ---
drivers/net/ethernet/intel/e1000e/e1000.h | 1 +
drivers/net/ethernet/intel/e1000e/ethtool.c | 29 ++++++++++++++--
drivers/net/ethernet/intel/e1000e/hw.h | 1 +
drivers/net/ethernet/intel/e1000e/ich8lan.c | 38
+++++++++++---------- drivers/net/ethernet/intel/e1000e/ich8lan.h |
2 ++ drivers/net/ethernet/intel/e1000e/netdev.c | 10 ++++++
6 files changed, 60 insertions(+), 21 deletions(-)
[snip]
a/drivers/net/ethernet/intel/e1000e/netdev.c +++
b/drivers/net/ethernet/intel/e1000e/netdev.c @@ -5267,6 +5267,13 @@
static void e1000_watchdog_task(struct work_struct *work)
&adapter->link_duplex); e1000_print_link_info(adapter);
+ if (adapter->flags2 & FLAG2_DISABLE_K1) {
+ hw->phy.ops.acquire(hw);
+
adapter->hw.dev_spec.ich8lan.disable_k1 = true;
+
e1000_reconfigure_k1_params(&adapter->hw);
+ hw->phy.ops.release(hw);
+ }
+
This also seems to now calling on every whatdog run the k1-params
again and again if FLAG2_DISABLE_K1 is set. Is this intended? I
suspect this should also change id disable_k1 != flags2 &
FLAG2_DISABLE_K1.
Also disable_k1 is never set to false.
Does this mean that if disable-k1 was enabled, but later disabled.
Things don't work as the rest of the code paths check disable_k1 which
does not get updated.
/* check if SmartSpeed worked */
e1000e_check_downshift(hw);
if (phy->speed_downgraded)
@@ -7675,6 +7682,9 @@ static int e1000_probe(struct pci_dev *pdev,
const struct pci_device_id *ent) /* init PTP hardware clock */
e1000e_ptp_init(adapter);
+ if (hw->mac.type >= e1000_pch_mtp)
+ adapter->flags2 |= FLAG2_DISABLE_K1;
+
Is this sufficient?
This was intended here because the flags2 does not propagate to
ich8lan.disable_k1 until the watch dog executes with the link up.
Thus most of the checks for disable_k1 do nothing until the link goes
up. Is this the intended behaviour?
/* reset the hardware with the new settings */
e1000e_reset(adapter);
Hi Timo,
Thank you again for reviewing this patch.
You raised great points, so I revisited the code and made a few changes
that I'll send in v3, these will address your concerns.
