Date:   Tue, 1 Dec 2020 03:02:51 +0000
From:   "Limonciello, Mario" <Mario.Limonciello@...l.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
CC:     Tony Nguyen <anthony.l.nguyen@...el.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Netdev <netdev@...r.kernel.org>,
        Stefan Assmann <sassmann@...hat.com>,
        "Neftin, Sasha" <sasha.neftin@...el.com>,
        Aaron Brown <aaron.f.brown@...el.com>
Subject: RE: [net-next 1/4] e1000e: allow turning s0ix flows on for systems
 with ME

 
> On Mon, Nov 30, 2020 at 2:16 PM Limonciello, Mario
> <Mario.Limonciello@...l.com> wrote:
> >
> > >
> > > Generally the use of module parameters and sysfs usage are frowned
> > > upon.
> >
> > I was trying to build on the existing module parameters that existed
> > already for e1000e.  So I guess I would ask, why are those not done in
> > ethtool?  Should those parameters go away and they migrate to ethtool
> > for the same reasons as this?
> 
> What it comes down to is that the existing module parameters are
> grandfathered in and we should not break things by removing them. New
> drivers aren't allowed to add them, and we are not supposed to add to
> them.
> 
> > > Based on the configuration isn't this something that could just
> > > be controlled via an ethtool priv flag? Couldn't you just have this
> > > default to whatever the heuristics decide at probe on and then support
> > > enabling/disabling it via the priv flag? You could look at
> > > igb_get_priv_flags/igb_set_priv_flags for an example of how to do what
> > > I am proposing.
> >
> > I don't disagree this solution would work, but it adds a new dependency
> > on having ethtool and the kernel move together to enable it.
> 
> Actually ethtool wouldn't have to change. The priv-flags are passed as
> strings to ethtool from the driver and set as a u32 bit flag array if
> I recall correctly.

Ah thanks, yeah I see that.  So should this just be passing in and out
priv->flags shifted and ORed with priv->flags2?  IIRC both of those are 16 bits.

And like my suggested change a new bit in priv->flags2.

> 
> > One advantage of the way this is done it allows shipping a system with an
> > older Linux kernel that isn't yet recognized in the kernel heuristics to
> > turn on by default with a small udev rule or kernel command line change.
> >
> > For example systems that aren't yet released could have this documented on
> > RHEL certification pages at release time for older RHEL releases before a
> > patch to add to the heuristics has been backported.
> 
> I suggest taking a look at the priv-flags interface. I am not
> suggesting adding a new interface to ethtool. It is an existing
> interface designed to allow for one-off features to be
> enabled/disabled on a given port.

Yes, this makes more sense to me now, thanks.

> 
> > >
> > > I think it would simplify this quite a bit since you wouldn't have to
> > > implement sysfs show/store operations for the value and would instead
> > > be allowing for reading/setting via the get_priv_flags/set_priv_flags
> > > operations. In addition you could leave the code for checking what
> > > supports this in place and have it set a flag that can be read or
> > > overwritten.
> >
> > If the consensus is to move in this direction, yes I'll redo the patch
> > series and modify ethtool as well.
> 
> No changes needed to ethtool. The flags are driver specific which is
> why this would work, or are you saying this change will be needed for
> other drivers? If so then yes I would recommend coming up with a
> standard interface we can use for those drivers as well.

I don't expect this to be needed in any other drivers right now.

Powered by blists - more mailing lists