> On Mon, Nov 30, 2020 at 2:16 PM Limonciello, Mario > <mario.limoncie...@dell.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.
