RFC: ethtool: add device-specific feature support in a generic fashion

RFC: ethtool: add device-specific feature support in a generic fashion

[Peter P Waskiewicz Jr]

This is a follow-up to my first RFC, getting opinions where the best
place to put device-specific feature toggling would be.  The feedback I
received was to look at doing this in ethtool.  Below is a high-level
design of what I'd like to do, and wanted to vet this with the community
to see if this is aligned with what people would like to see.

The general idea is to have a generic framework in ethtool to enumerate
device-specific commands.  A sample structure that would represent each
of these commands is:

	enum oem_cmds {
		OEM_CMD_0 = 0,
		OEM_CMD_1,
		OEM_CMD_2,
	...
	etc.
	...
	};

	struct oem_feature_cmd {
		/* Description of the feature */
		char *description;

		/* Does the feature toggling requires a device reset */
		u8 require_reset;

		/* The command-line name for the command */
		char *oem_cmd_name;

		/* The command number assigned to this */
		u32 oem_cmd;

		/* value for the command */
		u32 oem_cmd_val;
	};

Now underlying drivers would implement as many of these commands as
needed, and assign strings to each.  These strings, in oem_cmd_name,
would be used by ethtool in userspace to parse the command line
properly.

The reason I included a field that indicates whether a reset is required
or not is to support features that require multiple commands to be
synchronously programmed.  An example is ixgbe's Flow Director.  FDIR
has a number of knobs, specifically what filtering mode to use (hash or
perfect match), number of filters to use, and how often to sample Tx
packets (assumes hash mode).  This way a user could set all three
options, then issue "ethtool -r ethX" to apply them.

An example of features ixgbe could implement, using the above framework,
would be:

	- oem_cmd = OEM_CMD1
		- oem_cmd_name = "fdir_filter_mode"
		- require_reset = 1
		- description = "Set filtering mode for Flow Director. 0 = off, 1 =
hash (default), 2 = perfect"

	- oem_cmd = OEM_CMD2
		- oem_cmd_name = "fdir_num_filters"
		- require_reset = 1
		- description = "How many filters to support: 0 = 2k perfect/8k hash
(default), 1 = 4k perfect/16k hash, 2 = 8k perfect/32k hash"

	- oem_cmd = OEM_CMD3
		- oem_cmd_name = "num_vfs"
		- require_reset = 1
		- description = "Number of Virtual Functions to enable.  This only
takes effect when SR-IOV is enabled."

	- oem_cmd = OEM_CMD4
		- oem_cmd_name = "enable_sriov"
		- require_reset = 0
		- description = "Enables SR-IOV in the device."

The general flow would be ethtool would read all commands the underlying
device supports, then would use that to parse the command line.
Assuming the command parses correctly, the configuration would then be
sent through the ioctl() to the driver.

This framework is a basic substitute for module parameters, which our
upstream drivers do not include.  The advantage here is any commands
that a driver wishes to add, to enable a new feature in hardware, would
not require ethtool to change.  The command is simply added to the
driver, and this framework will pick up the new command.

Any comments, questions, or suggestions are much appreciated.

Cheers,
-PJ Waskiewicz

Re: RFC: ethtool: add device-specific feature support in a generic fashion

[Ben Greear]

On 12/12/2009 06:33 PM, Peter P Waskiewicz Jr wrote:
> This is a follow-up to my first RFC, getting opinions where the best
> place to put device-specific feature toggling would be.  The feedback I
> received was to look at doing this in ethtool.  Below is a high-level
> design of what I'd like to do, and wanted to vet this with the community
> to see if this is aligned with what people would like to see.
>
> The general idea is to have a generic framework in ethtool to enumerate
> device-specific commands.  A sample structure that would represent each
> of these commands is:
>
> 	enum oem_cmds {
> 		OEM_CMD_0 = 0,
> 		OEM_CMD_1,
> 		OEM_CMD_2,
> 	...
> 	etc.
> 	...
> 	};
>
> 	struct oem_feature_cmd {
> 		/* Description of the feature */
> 		char *description;
>
> 		/* Does the feature toggling requires a device reset */
> 		u8 require_reset;
>
> 		/* The command-line name for the command */
> 		char *oem_cmd_name;
>
> 		/* The command number assigned to this */
> 		u32 oem_cmd;
>
> 		/* value for the command */
> 		u32 oem_cmd_val;
> 	};

I'd add a 32-bit field for flags, with require_reset being one of them.  I'd also
align it so that it has no holes on 32/64 bit (put char*'s next to each other, maybe
pad with another 32-bit 'spare' field.  Maybe even use uint64 for the pointers so
that the struct size is same on 32-bit and 64-bit, to aid using 32-bit apps on
64-bit OS easily.

This way, as new uses are found for this, the structure remains binary compatible.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: RFC: ethtool: add device-specific feature support in a generic fashion

[Peter P Waskiewicz Jr]

On Sat, 2009-12-12 at 20:19 -0800, Ben Greear wrote:
> On 12/12/2009 06:33 PM, Peter P Waskiewicz Jr wrote:
> > This is a follow-up to my first RFC, getting opinions where the best
> > place to put device-specific feature toggling would be.  The feedback I
> > received was to look at doing this in ethtool.  Below is a high-level
> > design of what I'd like to do, and wanted to vet this with the community
> > to see if this is aligned with what people would like to see.
> >
> > The general idea is to have a generic framework in ethtool to enumerate
> > device-specific commands.  A sample structure that would represent each
> > of these commands is:
> >
> > 	enum oem_cmds {
> > 		OEM_CMD_0 = 0,
> > 		OEM_CMD_1,
> > 		OEM_CMD_2,
> > 	...
> > 	etc.
> > 	...
> > 	};
> >
> > 	struct oem_feature_cmd {
> > 		/* Description of the feature */
> > 		char *description;
> >
> > 		/* Does the feature toggling requires a device reset */
> > 		u8 require_reset;
> >
> > 		/* The command-line name for the command */
> > 		char *oem_cmd_name;
> >
> > 		/* The command number assigned to this */
> > 		u32 oem_cmd;
> >
> > 		/* value for the command */
> > 		u32 oem_cmd_val;
> > 	};
> 
> I'd add a 32-bit field for flags, with require_reset being one of them.  I'd also
> align it so that it has no holes on 32/64 bit (put char*'s next to each other, maybe
> pad with another 32-bit 'spare' field.  Maybe even use uint64 for the pointers so
> that the struct size is same on 32-bit and 64-bit, to aid using 32-bit apps on
> 64-bit OS easily.
> 
> This way, as new uses are found for this, the structure remains binary compatible.
> 

Excellent points.  Thanks Ben!

-PJ

Re: RFC: ethtool: add device-specific feature support in a generic fashion

[David Miller]

From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Date: Sat, 12 Dec 2009 23:09:24 -0800

> Excellent points.  Thanks Ben!

See ethtool_gstrings please.

There is no reason to put a "char *" in any of your interfaces, just
as we don't need to in the existing ethtool stats stuff.

And you can put the number of knobs available in struct
ethtool_drvinfo right before n_priv_flags where we have a reserved
area just for adding things like this.

You are specifying way too much new stuff in your datastructures,
unnecessary duplicating existing structures and facilities.

I pointed you at the ethtool private stats stuff because you
can reuse %85 of it's implementation for your purposes :-)

Re: RFC: ethtool: add device-specific feature support in a generic fashion

[Peter P Waskiewicz Jr]

On Sun, 2009-12-13 at 20:19 -0800, David Miller wrote:
> From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> Date: Sat, 12 Dec 2009 23:09:24 -0800
> 
> > Excellent points.  Thanks Ben!
> 
> See ethtool_gstrings please.
> 

Yes, I looked at this before, then started thinking about it harder than
I needed to.

> There is no reason to put a "char *" in any of your interfaces, just
> as we don't need to in the existing ethtool stats stuff.
> 

Ah, right.  Just a pointer to a chunk of data.  I see now.

> And you can put the number of knobs available in struct
> ethtool_drvinfo right before n_priv_flags where we have a reserved
> area just for adding things like this.
> 
> You are specifying way too much new stuff in your datastructures,
> unnecessary duplicating existing structures and facilities.

Yeah, I was making this too complicated.  Glad I sent the RFC on the
design first before writing any code.  :-)

> I pointed you at the ethtool private stats stuff because you
> can reuse %85 of it's implementation for your purposes :-)

Right!  In this case, I just needed a firmer bonk on the head to look in
the right direction.

On the right track now Dave, thanks.

-PJ