[RFC net-next v1 1/1] e1000e: Introduce private flag and module param to disable K1

[RFC net-next v1 1/1] e1000e: Introduce private flag and module param to disable K1

[Vitaly Lifshits]

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 system administrators to globally disable
K1 idle state on the adapter.

Link: https://lore.kernel.org/intel-wired-lan/CAMqyJG3LVqfgqMcTxeaPur_Jq0oQH7GgdxRuVtRX_6TTH2mX5Q@mail.gmail.com/
Link: https://lore.kernel.org/intel-wired-lan/20250626153544.1853d106@onyx.my.domain/
Link: https://lore.kernel.org/intel-wired-lan/Z_z9EjcKtwHCQcZR@mail-itl/

Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
---
 drivers/net/ethernet/intel/e1000e/e1000.h   |  1 +
 drivers/net/ethernet/intel/e1000e/ethtool.c | 16 ++++++--
 drivers/net/ethernet/intel/e1000e/hw.h      |  1 +
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 44 +++++++++++----------
 drivers/net/ethernet/intel/e1000e/ich8lan.h |  2 +
 drivers/net/ethernet/intel/e1000e/netdev.c  |  7 ++++
 drivers/net/ethernet/intel/e1000e/param.c   | 27 +++++++++++++
 7 files changed, 74 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index 952898151565..5c284468de74 100644
--- a/drivers/net/ethernet/intel/e1000e/e1000.h
+++ b/drivers/net/ethernet/intel/e1000e/e1000.h
@@ -461,6 +461,7 @@ s32 e1000e_get_base_timinca(struct e1000_adapter *adapter, u32 *timinca);
 #define FLAG2_CHECK_RX_HWTSTAMP           BIT(13)
 #define FLAG2_CHECK_SYSTIM_OVERFLOW       BIT(14)
 #define FLAG2_ENABLE_S0IX_FLOWS           BIT(15)
+#define FLAG2_DISABLE_K1		   BIT(16)
 
 #define E1000_RX_DESC_PS(R, i)	    \
 	(&(((union e1000_rx_desc_packet_split *)((R).desc))[i]))
diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
index 9364bc2b4eb1..cc1ea2daa324 100644
--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
@@ -26,6 +26,8 @@ struct e1000_stats {
 static const char e1000e_priv_flags_strings[][ETH_GSTRING_LEN] = {
 #define E1000E_PRIV_FLAGS_S0IX_ENABLED	BIT(0)
 	"s0ix-enabled",
+#define E1000E_PRIV_FLAGS_DISABLE_K1	BIT(1)
+	"disable-k1",
 };
 
 #define E1000E_PRIV_FLAGS_STR_LEN ARRAY_SIZE(e1000e_priv_flags_strings)
@@ -2304,23 +2306,31 @@ static u32 e1000e_get_priv_flags(struct net_device *netdev)
 	if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS)
 		priv_flags |= E1000E_PRIV_FLAGS_S0IX_ENABLED;
 
+	if (adapter->flags2 & FLAG2_DISABLE_K1)
+		priv_flags |= E1000E_PRIV_FLAGS_DISABLE_K1;
+
 	return priv_flags;
 }
 
 static int e1000e_set_priv_flags(struct net_device *netdev, u32 priv_flags)
 {
 	struct e1000_adapter *adapter = netdev_priv(netdev);
+	struct e1000_hw *hw = &adapter->hw;
 	unsigned int flags2 = adapter->flags2;
 
-	flags2 &= ~FLAG2_ENABLE_S0IX_FLOWS;
+	flags2 &= ~(FLAG2_ENABLE_S0IX_FLOWS | FLAG2_DISABLE_K1);
 	if (priv_flags & E1000E_PRIV_FLAGS_S0IX_ENABLED) {
-		struct e1000_hw *hw = &adapter->hw;
-
 		if (hw->mac.type < e1000_pch_cnp)
 			return -EINVAL;
 		flags2 |= FLAG2_ENABLE_S0IX_FLOWS;
 	}
 
+	if (priv_flags & E1000E_PRIV_FLAGS_DISABLE_K1) {
+		if (hw->mac.type < e1000_ich8lan)
+			return -EINVAL;
+		flags2 |= FLAG2_DISABLE_K1;
+	}
+
 	if (flags2 != adapter->flags2)
 		adapter->flags2 = flags2;
 
diff --git a/drivers/net/ethernet/intel/e1000e/hw.h b/drivers/net/ethernet/intel/e1000e/hw.h
index fc8ed38aa095..655872c99a9b 100644
--- a/drivers/net/ethernet/intel/e1000e/hw.h
+++ b/drivers/net/ethernet/intel/e1000e/hw.h
@@ -709,6 +709,7 @@ struct e1000_dev_spec_ich8lan {
 	struct e1000_shadow_ram shadow_ram[E1000_ICH8_SHADOW_RAM_WORDS];
 	bool nvm_k1_enabled;
 	bool eee_disable;
+	bool disable_k1;
 	u16 eee_lp_ability;
 	enum e1000_ulp_state ulp_state;
 };
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index df4e7d781cb1..bdb6fa97e108 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -286,21 +286,23 @@ static void e1000_toggle_lanphypc_pch_lpt(struct e1000_hw *hw)
 }
 
 /**
- * e1000_reconfigure_k1_exit_timeout - reconfigure K1 exit timeout to
- * align to MTP and later platform requirements.
+ * e1000_reconfigure_k1_params - reconfigure Kumeran K1 parameters.
  * @hw: pointer to the HW structure
  *
  * Context: PHY semaphore must be held by caller.
  * Return: 0 on success, negative on failure
  */
-static s32 e1000_reconfigure_k1_exit_timeout(struct e1000_hw *hw)
+s32 e1000_reconfigure_k1_params(struct e1000_hw *hw)
 {
 	u16 phy_timeout;
 	u32 fextnvm12;
 	s32 ret_val;
 
-	if (hw->mac.type < e1000_pch_mtp)
+	if (hw->mac.type < e1000_pch_mtp) {
+		if (hw->dev_spec.ich8lan.disable_k1)
+			return e1000_configure_k1_ich8lan(hw, false);
 		return 0;
+	}
 
 	/* Change Kumeran K1 power down state from P0s to P1 */
 	fextnvm12 = er32(FEXTNVM12);
@@ -310,9 +312,12 @@ static s32 e1000_reconfigure_k1_exit_timeout(struct e1000_hw *hw)
 
 	/* Wait for the interface the settle */
 	usleep_range(1000, 1100);
+	if (hw->dev_spec.ich8lan.disable_k1)
+		return e1000_configure_k1_ich8lan(hw, false);
 
 	/* Change K1 exit timeout */
-	ret_val = e1e_rphy_locked(hw, I217_PHY_TIMEOUTS_REG,
+	ret_val = e1e_rphy_locked(hw,
+				  I217_PHY_TIMEOUTS_REG,
 				  &phy_timeout);
 	if (ret_val)
 		return ret_val;
@@ -320,8 +325,7 @@ static s32 e1000_reconfigure_k1_exit_timeout(struct e1000_hw *hw)
 	phy_timeout &= ~I217_PHY_TIMEOUTS_K1_EXIT_TO_MASK;
 	phy_timeout |= 0xF00;
 
-	return e1e_wphy_locked(hw, I217_PHY_TIMEOUTS_REG,
-				  phy_timeout);
+	return e1e_wphy_locked(hw, I217_PHY_TIMEOUTS_REG, phy_timeout);
 }
 
 /**
@@ -373,8 +377,8 @@ static s32 e1000_init_phy_workarounds_pchlan(struct e1000_hw *hw)
 		/* At this point the PHY might be inaccessible so don't
 		 * propagate the failure
 		 */
-		if (e1000_reconfigure_k1_exit_timeout(hw))
-			e_dbg("Failed to reconfigure K1 exit timeout\n");
+		if (e1000_reconfigure_k1_params(hw))
+			e_dbg("Failed to reconfigure K1 parameters\n");
 
 		fallthrough;
 	case e1000_pch_lpt:
@@ -473,10 +477,10 @@ static s32 e1000_init_phy_workarounds_pchlan(struct e1000_hw *hw)
 		if (hw->mac.type >= e1000_pch_mtp) {
 			ret_val = hw->phy.ops.acquire(hw);
 			if (ret_val) {
-				e_err("Failed to reconfigure K1 exit timeout\n");
+				e_err("Failed to reconfigure K1 parameters\n");
 				goto out;
 			}
-			ret_val = e1000_reconfigure_k1_exit_timeout(hw);
+			ret_val = e1000_reconfigure_k1_params(hw);
 			hw->phy.ops.release(hw);
 		}
 	}
@@ -4948,17 +4952,15 @@ static s32 e1000_init_hw_ich8lan(struct e1000_hw *hw)
 	u16 i;
 
 	e1000_initialize_hw_bits_ich8lan(hw);
-	if (hw->mac.type >= e1000_pch_mtp) {
-		ret_val = hw->phy.ops.acquire(hw);
-		if (ret_val)
-			return ret_val;
+	ret_val = hw->phy.ops.acquire(hw);
+	if (ret_val)
+		return ret_val;
 
-		ret_val = e1000_reconfigure_k1_exit_timeout(hw);
-		hw->phy.ops.release(hw);
-		if (ret_val) {
-			e_dbg("Error failed to reconfigure K1 exit timeout\n");
-			return ret_val;
-		}
+	ret_val = e1000_reconfigure_k1_params(hw);
+	hw->phy.ops.release(hw);
+	if (ret_val) {
+		e_dbg("Error failed to reconfigure K1 parameters\n");
+		return ret_val;
 	}
 
 	/* Initialize identification LED */
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.h b/drivers/net/ethernet/intel/e1000e/ich8lan.h
index 5feb589a9b5f..97ceb6fa763b 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.h
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.h
@@ -310,4 +310,6 @@ s32 e1000_read_emi_reg_locked(struct e1000_hw *hw, u16 addr, u16 *data);
 s32 e1000_write_emi_reg_locked(struct e1000_hw *hw, u16 addr, u16 data);
 s32 e1000_set_eee_pchlan(struct e1000_hw *hw);
 s32 e1000_enable_ulp_lpt_lp(struct e1000_hw *hw, bool to_sx);
+s32 e1000_reconfigure_k1_params(struct e1000_hw *hw);
+
 #endif /* _E1000E_ICH8LAN_H_ */
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 7719e15813ee..03c3c238bbc7 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -5267,6 +5267,11 @@ static void e1000_watchdog_task(struct work_struct *work)
 						  &adapter->link_duplex);
 			e1000_print_link_info(adapter);
 
+			if (adapter->flags2 & FLAG2_DISABLE_K1) {
+				adapter->hw.dev_spec.ich8lan.disable_k1 = true;
+				e1000_reconfigure_k1_params(&adapter->hw);
+			}
+
 			/* check if SmartSpeed worked */
 			e1000e_check_downshift(hw);
 			if (phy->speed_downgraded)
@@ -7474,6 +7479,8 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	adapter->bd_number = cards_found++;
 
 	e1000e_check_options(adapter);
+	if (adapter->flags2 & FLAG2_DISABLE_K1)
+		adapter->hw.dev_spec.ich8lan.disable_k1 = true;
 
 	/* setup adapter struct */
 	err = e1000_sw_init(adapter);
diff --git a/drivers/net/ethernet/intel/e1000e/param.c b/drivers/net/ethernet/intel/e1000e/param.c
index 3132d8f2f207..28d78a42a06f 100644
--- a/drivers/net/ethernet/intel/e1000e/param.c
+++ b/drivers/net/ethernet/intel/e1000e/param.c
@@ -136,6 +136,15 @@ E1000_PARAM(WriteProtectNVM,
 E1000_PARAM(CrcStripping,
 	    "Enable CRC Stripping, disable if your BMC needs the CRC");
 
+/* Enable K1
+ *
+ * Valid Range: 0, 1
+ *
+ * Default Value: 1 (enabled)
+ */
+E1000_PARAM(EnableK1,
+	    "Enable Kumeran K1 state [WARNING: Disabling K1 may cause increased power consumption]");
+
 struct e1000_option {
 	enum { enable_option, range_option, list_option } type;
 	const char *name;
@@ -524,4 +533,22 @@ void e1000e_check_options(struct e1000_adapter *adapter)
 			}
 		}
 	}
+	/* Enable K1 */
+	{
+		static const struct e1000_option opt = {
+			.type = enable_option,
+			.name = "Kumeran K1 State",
+			.err = "defaulting to Enabled",
+			.def = OPTION_ENABLED
+		};
+
+		if (num_EnableK1 > bd) {
+			unsigned int enable_k1 = EnableK1[bd];
+			e1000_validate_option(&enable_k1, &opt, adapter);
+
+			if (hw->mac.type >= e1000_ich8lan)
+				if (!enable_k1)
+					adapter->flags2 |= FLAG2_DISABLE_K1;
+		}
+	}
 }
-- 
2.34.1

Re: [RFC net-next v1 1/1] e1000e: Introduce private flag and module param to disable K1

[Simon Horman]

On Thu, Jul 10, 2025 at 12:24:55PM +0300, Vitaly Lifshits 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 system administrators to globally disable
> K1 idle state on the adapter.
> 
> Link: https://lore.kernel.org/intel-wired-lan/CAMqyJG3LVqfgqMcTxeaPur_Jq0oQH7GgdxRuVtRX_6TTH2mX5Q@mail.gmail.com/
> Link: https://lore.kernel.org/intel-wired-lan/20250626153544.1853d106@onyx.my.domain/
> Link: https://lore.kernel.org/intel-wired-lan/Z_z9EjcKtwHCQcZR@mail-itl/
> 
> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>

Hi Vitaly,

If I understand things correctly, this patch adds a new module parameter
to the e1000 driver. As adding new module parameters to networking driver
is discouraged I'd like to ask if another mechanism can be found.
E.g. devlink.

RE: [RFC net-next v1 1/1] e1000e: Introduce private flag and module param to disable K1

[Keller, Jacob E]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Monday, July 14, 2025 9:55 AM
> To: Lifshits, Vitaly <vitaly.lifshits@intel.com>
> Cc: andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; Ruinskiy, Dima
> <dima.ruinskiy@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
> Keller, Jacob E <jacob.e.keller@intel.com>
> Subject: Re: [RFC net-next v1 1/1] e1000e: Introduce private flag and module
> param to disable K1
> 
> On Thu, Jul 10, 2025 at 12:24:55PM +0300, Vitaly Lifshits 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 system administrators to globally disable
> > K1 idle state on the adapter.
> >
> > Link: https://lore.kernel.org/intel-wired-
> lan/CAMqyJG3LVqfgqMcTxeaPur_Jq0oQH7GgdxRuVtRX_6TTH2mX5Q@mail.gmail.
> com/
> > Link: https://lore.kernel.org/intel-wired-
> lan/20250626153544.1853d106@onyx.my.domain/
> > Link: https://lore.kernel.org/intel-wired-lan/Z_z9EjcKtwHCQcZR@mail-itl/
> >
> > Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> 
> Hi Vitaly,
> 
> If I understand things correctly, this patch adds a new module parameter
> to the e1000 driver. As adding new module parameters to networking driver
> is discouraged I'd like to ask if another mechanism can be found.
> E.g. devlink.

One motivation for the module parameter is that it is simple to set it "permanently" by setting the module parameter to be loaded by default. I don't think any distro has something equivalent for devlink or ethtool flags. Of course that’s not really the kernel's fault.

I agree that new module parameters are generally discouraged from being added. A devlink parameter could work, but it does require administrator to script setting the parameter at boot on affected systems. This also will require a bit more work to implement because the e1000e driver does not expose devlink.

Would an ethtool private flag on its own be sufficient/accepted..? I know those are also generally discouraged because of past attempts to avoid implementing generic interfaces.. However I don't think there is a "generic" interface for this, at least based on my understanding. It appears to be a low power state for the embedded device on a platform, which is quite specific to this device and hardware design ☹

Re: [RFC net-next v1 1/1] e1000e: Introduce private flag and module param to disable K1

[Ruinskiy, Dima]

On 15/07/2025 0:30, Keller, Jacob E wrote:
> 
> 
>> -----Original Message-----
>> From: Simon Horman <horms@kernel.org>
>> Sent: Monday, July 14, 2025 9:55 AM
>> To: Lifshits, Vitaly <vitaly.lifshits@intel.com>
>> Cc: andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
>> kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; Ruinskiy, Dima
>> <dima.ruinskiy@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
>> Keller, Jacob E <jacob.e.keller@intel.com>
>> Subject: Re: [RFC net-next v1 1/1] e1000e: Introduce private flag and module
>> param to disable K1
>>
>> On Thu, Jul 10, 2025 at 12:24:55PM +0300, Vitaly Lifshits 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 system administrators to globally disable
>>> K1 idle state on the adapter.
>>>
>>> Link: https://lore.kernel.org/intel-wired-
>> lan/CAMqyJG3LVqfgqMcTxeaPur_Jq0oQH7GgdxRuVtRX_6TTH2mX5Q@mail.gmail.
>> com/
>>> Link: https://lore.kernel.org/intel-wired-
>> lan/20250626153544.1853d106@onyx.my.domain/
>>> Link: https://lore.kernel.org/intel-wired-lan/Z_z9EjcKtwHCQcZR@mail-itl/
>>>
>>> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>>
>> Hi Vitaly,
>>
>> If I understand things correctly, this patch adds a new module parameter
>> to the e1000 driver. As adding new module parameters to networking driver
>> is discouraged I'd like to ask if another mechanism can be found.
>> E.g. devlink.
> 
> One motivation for the module parameter is that it is simple to set it "permanently" by setting the module parameter to be loaded by default. I don't think any distro has something equivalent for devlink or ethtool flags. Of course that’s not really the kernel's fault.
> 
> I agree that new module parameters are generally discouraged from being added. A devlink parameter could work, but it does require administrator to script setting the parameter at boot on affected systems. This also will require a bit more work to implement because the e1000e driver does not expose devlink.
> 
> Would an ethtool private flag on its own be sufficient/accepted..? I know those are also generally discouraged because of past attempts to avoid implementing generic interfaces.. However I don't think there is a "generic" interface for this, at least based on my understanding. It appears to be a low power state for the embedded device on a platform, which is quite specific to this device and hardware design ☹

Basically what we are looking for here is, as Jake mentioned, a way for 
a system administrator / "power-user" to "permanently" set the driver 
option in order to mask the issue on specific systems suffering from it.

As it can sometimes manifest during early hardware initialization 
stages, I'm concerned that just an ethtool private flag is insufficient, 
as it may be 'too late' to set it after 'probe'.

Not being familiar enough with devlink, I do not understand if it can be 
active already as early as 'probe', but given the fact that e1000e 
currently does not implement any devlink stuff, this would require a 
bigger (and riskier?) change to the code. The module parameter is fairly 
trivial, since e1000e already supports a number of these.

I do not know the history and why module parameters are discouraged, but 
it seems that there has to be some standardized way to pass user 
configuration to kernel modules, which takes effect as soon as the 
module is loaded. I always thought module parameters were that 
interface; if things have evolved, I would be happy to learn. :)

--Dima

Re: [RFC net-next v1 1/1] e1000e: Introduce private flag and module param to disable K1

[Lifshits, Vitaly]

On 7/16/2025 1:25 PM, Ruinskiy, Dima wrote:
> On 15/07/2025 0:30, Keller, Jacob E wrote:
>>
>>
>>> -----Original Message-----
>>> From: Simon Horman <horms@kernel.org>
>>> Sent: Monday, July 14, 2025 9:55 AM
>>> To: Lifshits, Vitaly <vitaly.lifshits@intel.com>
>>> Cc: andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
>>> kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org; Ruinskiy, 
>>> Dima
>>> <dima.ruinskiy@intel.com>; Nguyen, Anthony L 
>>> <anthony.l.nguyen@intel.com>;
>>> Keller, Jacob E <jacob.e.keller@intel.com>
>>> Subject: Re: [RFC net-next v1 1/1] e1000e: Introduce private flag and 
>>> module
>>> param to disable K1
>>>
>>> On Thu, Jul 10, 2025 at 12:24:55PM +0300, Vitaly Lifshits 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 system administrators to globally disable
>>>> K1 idle state on the adapter.
>>>>
>>>> Link: https://lore.kernel.org/intel-wired-
>>> lan/CAMqyJG3LVqfgqMcTxeaPur_Jq0oQH7GgdxRuVtRX_6TTH2mX5Q@mail.gmail.
>>> com/
>>>> Link: https://lore.kernel.org/intel-wired-
>>> lan/20250626153544.1853d106@onyx.my.domain/
>>>> Link: https://lore.kernel.org/intel-wired-lan/Z_z9EjcKtwHCQcZR@mail- 
>>>> itl/
>>>>
>>>> Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>>>
>>> Hi Vitaly,
>>>
>>> If I understand things correctly, this patch adds a new module parameter
>>> to the e1000 driver. As adding new module parameters to networking 
>>> driver
>>> is discouraged I'd like to ask if another mechanism can be found.
>>> E.g. devlink.
>>
>> One motivation for the module parameter is that it is simple to set it 
>> "permanently" by setting the module parameter to be loaded by default. 
>> I don't think any distro has something equivalent for devlink or 
>> ethtool flags. Of course that’s not really the kernel's fault.
>>
>> I agree that new module parameters are generally discouraged from 
>> being added. A devlink parameter could work, but it does require 
>> administrator to script setting the parameter at boot on affected 
>> systems. This also will require a bit more work to implement because 
>> the e1000e driver does not expose devlink.
>>
>> Would an ethtool private flag on its own be sufficient/accepted..? I 
>> know those are also generally discouraged because of past attempts to 
>> avoid implementing generic interfaces.. However I don't think there is 
>> a "generic" interface for this, at least based on my understanding. It 
>> appears to be a low power state for the embedded device on a platform, 
>> which is quite specific to this device and hardware design ☹
> 
> Basically what we are looking for here is, as Jake mentioned, a way for 
> a system administrator / "power-user" to "permanently" set the driver 
> option in order to mask the issue on specific systems suffering from it.
> 
> As it can sometimes manifest during early hardware initialization 
> stages, I'm concerned that just an ethtool private flag is insufficient, 
> as it may be 'too late' to set it after 'probe'.
> 
> Not being familiar enough with devlink, I do not understand if it can be 
> active already as early as 'probe', but given the fact that e1000e 
> currently does not implement any devlink stuff, this would require a 
> bigger (and riskier?) change to the code. The module parameter is fairly 
> trivial, since e1000e already supports a number of these.
> 
> I do not know the history and why module parameters are discouraged, but 
> it seems that there has to be some standardized way to pass user 
> configuration to kernel modules, which takes effect as soon as the 
> module is loaded. I always thought module parameters were that 
> interface; if things have evolved, I would be happy to learn. :)
> 
> --Dima

While I understand that module params are generally discouraged—as
Jacob Keller pointed out—implementing the same functionality via devlink
presents some challenges. Although it may be technically feasible, it
would likely complicate configuration for sysadmins who need to disable
K1 on affected systems.

In my view, extending an existing interface in an older driver is a 
safer and more pragmatic approach than introducing a new one, especially 
given the legacy nature of the devices involved. These systems are often 
beyond the scope of our current test coverage, and minimizing the risk 
of regressions is critical.

Regarding the ethtool private flag: while it may not address all 
potential link issues, it does help mitigate certain packet loss 
scenarios. My motivation for proposing it was to offer end-users a 
straightforward workaround—by setting the flag and retriggering 
auto-negotiation, they may resolve issues without needing to unload and 
reload the e1000e module.

Re: [RFC net-next v1 1/1] e1000e: Introduce private flag and module param to disable K1

[Simon Horman]

On Wed, Jul 30, 2025 at 05:11:20PM +0300, Lifshits, Vitaly wrote:
> 
> 
> On 7/16/2025 1:25 PM, Ruinskiy, Dima wrote:
> > On 15/07/2025 0:30, Keller, Jacob E wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Simon Horman <horms@kernel.org>
> > > > Sent: Monday, July 14, 2025 9:55 AM
> > > > To: Lifshits, Vitaly <vitaly.lifshits@intel.com>
> > > > Cc: andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com;
> > > > kuba@kernel.org; pabeni@redhat.com; netdev@vger.kernel.org;
> > > > Ruinskiy, Dima
> > > > <dima.ruinskiy@intel.com>; Nguyen, Anthony L
> > > > <anthony.l.nguyen@intel.com>;
> > > > Keller, Jacob E <jacob.e.keller@intel.com>
> > > > Subject: Re: [RFC net-next v1 1/1] e1000e: Introduce private
> > > > flag and module
> > > > param to disable K1
> > > > 
> > > > On Thu, Jul 10, 2025 at 12:24:55PM +0300, Vitaly Lifshits 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 system administrators to globally disable
> > > > > K1 idle state on the adapter.
> > > > > 
> > > > > Link: https://lore.kernel.org/intel-wired-
> > > > lan/CAMqyJG3LVqfgqMcTxeaPur_Jq0oQH7GgdxRuVtRX_6TTH2mX5Q@mail.gmail.
> > > > com/
> > > > > Link: https://lore.kernel.org/intel-wired-
> > > > lan/20250626153544.1853d106@onyx.my.domain/
> > > > > Link:
> > > > > https://lore.kernel.org/intel-wired-lan/Z_z9EjcKtwHCQcZR@mail-
> > > > > itl/
> > > > > 
> > > > > Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
> > > > 
> > > > Hi Vitaly,
> > > > 
> > > > If I understand things correctly, this patch adds a new module parameter
> > > > to the e1000 driver. As adding new module parameters to
> > > > networking driver
> > > > is discouraged I'd like to ask if another mechanism can be found.
> > > > E.g. devlink.
> > > 
> > > One motivation for the module parameter is that it is simple to set
> > > it "permanently" by setting the module parameter to be loaded by
> > > default. I don't think any distro has something equivalent for
> > > devlink or ethtool flags. Of course that’s not really the kernel's
> > > fault.
> > > 
> > > I agree that new module parameters are generally discouraged from
> > > being added. A devlink parameter could work, but it does require
> > > administrator to script setting the parameter at boot on affected
> > > systems. This also will require a bit more work to implement because
> > > the e1000e driver does not expose devlink.
> > > 
> > > Would an ethtool private flag on its own be sufficient/accepted..? I
> > > know those are also generally discouraged because of past attempts
> > > to avoid implementing generic interfaces.. However I don't think
> > > there is a "generic" interface for this, at least based on my
> > > understanding. It appears to be a low power state for the embedded
> > > device on a platform, which is quite specific to this device and
> > > hardware design ☹
> > 
> > Basically what we are looking for here is, as Jake mentioned, a way for
> > a system administrator / "power-user" to "permanently" set the driver
> > option in order to mask the issue on specific systems suffering from it.
> > 
> > As it can sometimes manifest during early hardware initialization
> > stages, I'm concerned that just an ethtool private flag is insufficient,
> > as it may be 'too late' to set it after 'probe'.
> > 
> > Not being familiar enough with devlink, I do not understand if it can be
> > active already as early as 'probe', but given the fact that e1000e
> > currently does not implement any devlink stuff, this would require a
> > bigger (and riskier?) change to the code. The module parameter is fairly
> > trivial, since e1000e already supports a number of these.
> > 
> > I do not know the history and why module parameters are discouraged, but
> > it seems that there has to be some standardized way to pass user
> > configuration to kernel modules, which takes effect as soon as the
> > module is loaded. I always thought module parameters were that
> > interface; if things have evolved, I would be happy to learn. :)
> > 
> > --Dima
> 
> While I understand that module params are generally discouraged—as
> Jacob Keller pointed out—implementing the same functionality via devlink
> presents some challenges. Although it may be technically feasible, it
> would likely complicate configuration for sysadmins who need to disable
> K1 on affected systems.
> 
> In my view, extending an existing interface in an older driver is a safer
> and more pragmatic approach than introducing a new one, especially given the
> legacy nature of the devices involved. These systems are often beyond the
> scope of our current test coverage, and minimizing the risk of regressions
> is critical.
> 
> Regarding the ethtool private flag: while it may not address all potential
> link issues, it does help mitigate certain packet loss scenarios. My
> motivation for proposing it was to offer end-users a straightforward
> workaround—by setting the flag and retriggering auto-negotiation, they may
> resolve issues without needing to unload and reload the e1000e module.

Thanks Vitaly,

My opinion is that devlink is the correct way to solve this problem.
However, I do understand from the responses above (3) that this is somewhat
non-trivial to implement and thus comes with some risks. And I do accept
your argument that for old drivers, which already use module parameters,
some pragmatism seems appropriate.

IOW, I drop my objection to using a module parameter in this case.

What I would suggest is that some consideration is given to adding devlink
support to this driver. And thus modernising it in that respect. Doing so
may provide better options for users in future.

Thanks!

Re: [RFC net-next v1 1/1] e1000e: Introduce private flag and module param to disable K1

[Jakub Kicinski]

On Wed, 30 Jul 2025 16:28:48 +0100 Simon Horman wrote:
> My opinion is that devlink is the correct way to solve this problem.
> However, I do understand from the responses above (3) that this is somewhat
> non-trivial to implement and thus comes with some risks. And I do accept
> your argument that for old drivers, which already use module parameters,
> some pragmatism seems appropriate.
> 
> IOW, I drop my objection to using a module parameter in this case.
> 
> What I would suggest is that some consideration is given to adding devlink
> support to this driver. And thus modernising it in that respect. Doing so
> may provide better options for users in future.

FWIW I will still object. The ethtool priv flag is fine, personally 
I don't have a strong preference on devlink vs ethtool priv flags.
But if you a module param you'd need a very strong justification..

Re: [RFC net-next v1 1/1] e1000e: Introduce private flag and module param to disable K1

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 1816 bytes --]



On 7/30/2025 1:42 PM, Jakub Kicinski wrote:
> On Wed, 30 Jul 2025 16:28:48 +0100 Simon Horman wrote:
>> My opinion is that devlink is the correct way to solve this problem.
>> However, I do understand from the responses above (3) that this is somewhat
>> non-trivial to implement and thus comes with some risks. And I do accept
>> your argument that for old drivers, which already use module parameters,
>> some pragmatism seems appropriate.
>>
>> IOW, I drop my objection to using a module parameter in this case.
>>
>> What I would suggest is that some consideration is given to adding devlink
>> support to this driver. And thus modernising it in that respect. Doing so
>> may provide better options for users in future.
> 
> FWIW I will still object. The ethtool priv flag is fine, personally 
> I don't have a strong preference on devlink vs ethtool priv flags.
> But if you a module param you'd need a very strong justification..

I think just the ethtool private flag is sufficient. The primary
downside appears to be the "inability" to easily set the flag at boot,
but...

At a minimum you can create a systemd service file set as a one shot
script which executes at boot, and issues the command. Alternatively it
may be possible to get NetworkManager or systemd-networkd to issue the
command automatically.

Being targeted at the device is a lot better semantics than being
targeted at every device owned by e1000e.

I also have no objection to setting it as a devlink parameter. If the
device had permanent NVM storage for the setting, that would be even
better.

I understand the motivation for not wanting to enable devlink due to the
extra development cost to integrate it into the e1000e driver. I'll
leave that decision up to Vitaly and team.

Thanks,
Jake

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [RFC net-next v1 1/1] e1000e: Introduce private flag and module param to disable K1

[Jakub Kicinski]

On Wed, 30 Jul 2025 15:10:45 -0700 Jacob Keller wrote:
> > FWIW I will still object. The ethtool priv flag is fine, personally 
> > I don't have a strong preference on devlink vs ethtool priv flags.
> > But if you a module param you'd need a very strong justification..  
> 
> I think just the ethtool private flag is sufficient. The primary
> downside appears to be the "inability" to easily set the flag at boot,
> but...

I haven't played with udev in a while but it used to have the ability
to run a command / script when device appears. So that'd be my first
choice if how to hook the setting in when device is probed.

Re: [RFC net-next v1 1/1] e1000e: Introduce private flag and module param to disable K1

[Ruinskiy, Dima]

On 31/07/2025 3:06, Jakub Kicinski wrote:
> On Wed, 30 Jul 2025 15:10:45 -0700 Jacob Keller wrote:
>>> FWIW I will still object. The ethtool priv flag is fine, personally
>>> I don't have a strong preference on devlink vs ethtool priv flags.
>>> But if you a module param you'd need a very strong justification..
>>
>> I think just the ethtool private flag is sufficient. The primary
>> downside appears to be the "inability" to easily set the flag at boot,
>> but...
> 
> I haven't played with udev in a while but it used to have the ability
> to run a command / script when device appears. So that'd be my first
> choice if how to hook the setting in when device is probed.

My concern here is not as much as how to set the private flag 
automatically at each boot (I leave this to the system administrator).

The concern is whether it can be set early enough during probe() to be 
effective. There is a good deal of HW access that happens during 
probe(). If it takes place before the flag is set, the HW can enter a 
bad state and changing K1 behavior later on does not always recover it.

With the module parameter, adapter->flags2 |= FLAG2_DISABLE_K1 gets set 
inside e1000e_check_options(), which is before any HW access takes 
place. If the private flag method can give similar guarantees, then it 
would be sufficient.

--Dima

Re: [RFC net-next v1 1/1] e1000e: Introduce private flag and module param to disable K1

[Jakub Kicinski]

On Thu, 31 Jul 2025 10:00:44 +0300 Ruinskiy, Dima wrote:
> My concern here is not as much as how to set the private flag 
> automatically at each boot (I leave this to the system administrator).
> 
> The concern is whether it can be set early enough during probe() to be 
> effective. There is a good deal of HW access that happens during 
> probe(). If it takes place before the flag is set, the HW can enter a 
> bad state and changing K1 behavior later on does not always recover it.
> 
> With the module parameter, adapter->flags2 |= FLAG2_DISABLE_K1 gets set 
> inside e1000e_check_options(), which is before any HW access takes 
> place. If the private flag method can give similar guarantees, then it 
> would be sufficient.

Presumably you are going to detect all the bad SKUs in the driver to
the best of your ability. So we're talking about a workaround that lets
the user tweak things until a relevant patch reaches stable..

Re: [RFC net-next v1 1/1] e1000e: Introduce private flag and module param to disable K1

[Jacob Keller]

[-- Attachment #1.1: Type: text/plain, Size: 1209 bytes --]



On 7/31/2025 7:19 AM, Jakub Kicinski wrote:
> On Thu, 31 Jul 2025 10:00:44 +0300 Ruinskiy, Dima wrote:
>> My concern here is not as much as how to set the private flag 
>> automatically at each boot (I leave this to the system administrator).
>>
>> The concern is whether it can be set early enough during probe() to be 
>> effective. There is a good deal of HW access that happens during 
>> probe(). If it takes place before the flag is set, the HW can enter a 
>> bad state and changing K1 behavior later on does not always recover it.
>>
>> With the module parameter, adapter->flags2 |= FLAG2_DISABLE_K1 gets set 
>> inside e1000e_check_options(), which is before any HW access takes 
>> place. If the private flag method can give similar guarantees, then it 
>> would be sufficient.
> 
> Presumably you are going to detect all the bad SKUs in the driver to
> the best of your ability. So we're talking about a workaround that lets
> the user tweak things until a relevant patch reaches stable..
> 

I think you could just default to K1 disabled, and have the parameter
for turning it on/off available. Ideally you'd default to disabled only
on known SKUs that are problematic?

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [RFC net-next v1 1/1] e1000e: Introduce private flag and module param to disable K1

[Lifshits, Vitaly]

On 7/31/2025 6:51 PM, Jacob Keller wrote:
> 
> 
> On 7/31/2025 7:19 AM, Jakub Kicinski wrote:
>> On Thu, 31 Jul 2025 10:00:44 +0300 Ruinskiy, Dima wrote:
>>> My concern here is not as much as how to set the private flag
>>> automatically at each boot (I leave this to the system administrator).
>>>
>>> The concern is whether it can be set early enough during probe() to be
>>> effective. There is a good deal of HW access that happens during
>>> probe(). If it takes place before the flag is set, the HW can enter a
>>> bad state and changing K1 behavior later on does not always recover it.
>>>
>>> With the module parameter, adapter->flags2 |= FLAG2_DISABLE_K1 gets set
>>> inside e1000e_check_options(), which is before any HW access takes
>>> place. If the private flag method can give similar guarantees, then it
>>> would be sufficient.

This was precisely the intention behind introducing the module parameter 
initially. The private flag isn't a comprehensive solution—it's more of 
a mechanism to allow configuration changes without unloading the e1000e 
module.

>>
>> Presumably you are going to detect all the bad SKUs in the driver to
>> the best of your ability. So we're talking about a workaround that lets
>> the user tweak things until a relevant patch reaches stable..

Regarding the SKUs: the issues we've encountered aren't tied to specific 
SKUs. Instead, they stem from broader environmental configurations that 
the driver cannot address directly. For instance, misconfigurations in 
the BIOS can only be resolved by the BIOS vendor, assuming they choose 
to do so. Until such fixes are available to end users, the module 
parameter provides a practical workaround for these system firmware issues.
>>
> 
> I think you could just default to K1 disabled, and have the parameter
> for turning it on/off available. Ideally you'd default to disabled only
> on known SKUs that are problematic?

As mentioned earlier, defaulting to K1 disabled isn't ideal. While it 
might help avoid certain issues on specific units, it would negatively 
impact the device's power consumption across all systems, the 
overwhelming majority of which would never experience any problem. 
Therefore, it's preferable to keep K1 enabled by default and allow users 
to disable it only when necessary.

Re: [RFC net-next v1 1/1] e1000e: Introduce private flag and module param to disable K1

[En-Wei WU]

> Regarding the SKUs: the issues we've encountered aren't tied to specific SKUs

From our side, the issue is only specific to an SKU. If that's also
the case from other sides, I'm wondering if it's possible to add a
quirk table for enabling/disabling K1 configuration.
The upside would be:
1. There is no need for a user to bother giving the module parameter
2. Let the kernel problem leave in the kernel

Thanks for your time. Please let me know your concern.

On Sun, 3 Aug 2025 at 20:39, Lifshits, Vitaly <vitaly.lifshits@intel.com> wrote:
>
>
>
> On 7/31/2025 6:51 PM, Jacob Keller wrote:
> >
> >
> > On 7/31/2025 7:19 AM, Jakub Kicinski wrote:
> >> On Thu, 31 Jul 2025 10:00:44 +0300 Ruinskiy, Dima wrote:
> >>> My concern here is not as much as how to set the private flag
> >>> automatically at each boot (I leave this to the system administrator).
> >>>
> >>> The concern is whether it can be set early enough during probe() to be
> >>> effective. There is a good deal of HW access that happens during
> >>> probe(). If it takes place before the flag is set, the HW can enter a
> >>> bad state and changing K1 behavior later on does not always recover it.
> >>>
> >>> With the module parameter, adapter->flags2 |= FLAG2_DISABLE_K1 gets set
> >>> inside e1000e_check_options(), which is before any HW access takes
> >>> place. If the private flag method can give similar guarantees, then it
> >>> would be sufficient.
>
> This was precisely the intention behind introducing the module parameter
> initially. The private flag isn't a comprehensive solution—it's more of
> a mechanism to allow configuration changes without unloading the e1000e
> module.
>
> >>
> >> Presumably you are going to detect all the bad SKUs in the driver to
> >> the best of your ability. So we're talking about a workaround that lets
> >> the user tweak things until a relevant patch reaches stable..
>
> Regarding the SKUs: the issues we've encountered aren't tied to specific
> SKUs. Instead, they stem from broader environmental configurations that
> the driver cannot address directly. For instance, misconfigurations in
> the BIOS can only be resolved by the BIOS vendor, assuming they choose
> to do so. Until such fixes are available to end users, the module
> parameter provides a practical workaround for these system firmware issues.
> >>
> >
> > I think you could just default to K1 disabled, and have the parameter
> > for turning it on/off available. Ideally you'd default to disabled only
> > on known SKUs that are problematic?
>
> As mentioned earlier, defaulting to K1 disabled isn't ideal. While it
> might help avoid certain issues on specific units, it would negatively
> impact the device's power consumption across all systems, the
> overwhelming majority of which would never experience any problem.
> Therefore, it's preferable to keep K1 enabled by default and allow users
> to disable it only when necessary.
>