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

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

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 <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Excellent points. Thanks Ben!

-PJ

From: Peter P Waskiewicz Jr <[email protected]>
Date: Sat, 12 Dec 2009 23:09:24 -0800

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 :-)

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

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

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

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