Message-ID: <20250730152848.GJ1877762@horms.kernel.org>
Date: Wed, 30 Jul 2025 16:28:48 +0100
From: Simon Horman <horms@...nel.org>
To: "Lifshits, Vitaly" <vitaly.lifshits@...el.com>
Cc: "Ruinskiy, Dima" <dima.ruinskiy@...el.com>,
	"Keller, Jacob E" <jacob.e.keller@...el.com>,
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
Subject: Re: [RFC net-next v1 1/1] e1000e: Introduce private flag and module
 param to disable K1

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@...nel.org>
> > > > Sent: Monday, July 14, 2025 9:55 AM
> > > > To: Lifshits, Vitaly <vitaly.lifshits@...el.com>
> > > > Cc: andrew+netdev@...n.ch; davem@...emloft.net; edumazet@...gle.com;
> > > > kuba@...nel.org; pabeni@...hat.com; netdev@...r.kernel.org;
> > > > Ruinskiy, Dima
> > > > <dima.ruinskiy@...el.com>; Nguyen, Anthony L
> > > > <anthony.l.nguyen@...el.com>;
> > > > Keller, Jacob E <jacob.e.keller@...el.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@...l.gmail.
> > > > com/
> > > > > Link: https://lore.kernel.org/intel-wired-
> > > > lan/20250626153544.1853d106@...x.my.domain/
> > > > > Link:
> > > > > https://lore.kernel.org/intel-wired-lan/Z_z9EjcKtwHCQcZR@mail-
> > > > > itl/
> > > > > 
> > > > > Signed-off-by: Vitaly Lifshits <vitaly.lifshits@...el.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!

Powered by blists - more mailing lists