On Mon, Nov 30, 2020 at 2:16 PM Limonciello, Mario
[off-list ref] wrote:

quoted

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

quoted

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.