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