On Wed, 2014-07-30 at 17:53 -0700, David Miller wrote:

From: Govindarajulu Varadarajan <redacted>
Date: Tue, 29 Jul 2014 17:10:38 +0530

quoted

@@ -440,6 +440,20 @@ struct ethtool_ringparam {
 };
 
 /**
+ * struct ethtool_buffparam - DMA buffer parameters
+ * @rx_copybreak_cur: current receive DMA buff rx_copybreak.
+ * @rx_copybreak_min: min rx_copybreak set by driver.
+ * @rx_copybreak_max: Max rx_copybreak set by driver.
+ * @reserved: reserve room for future use.
+ */
+struct ethtool_buffparam {
+	__u32	cmd;
+	__u32	rx_copybreak_cur;
+	__u32	rx_copybreak_max;
+	__u8	reserved[84];
+};

I don't understand the reasoning behind this reserved field.

You can't use it to add more fields later, because right now
we'll let the user put any garbage there and thus if you add
more fields that garbage from older apps would be interpreted
as one of the new values.

That's OK, we can test that they're all zero.  Or add flags.

Largely we have not been adding reserved fields to new ethtool
structures, and this is the primary reason I guess.

Yes we have, but not consistently.

It's a shame that when we want to add a new 32-bit knob we have
to add an entire new struct.

We have ethtool_value, but that's only good for one knob at a time and
we have to allocate an entire new ethtool command value for each such
knob.

Two commands and two function pointers!

I really don't know what the recommend here.

However I wonder what value that "max" thing has, I think setting the
value to infinity is just fine, it just means every packet will be
copied.

So if we remove the 'max', we just have the copybreak itself, and you
can therefore use ethtool_value and allocate the ethtool command
numbers.

What do you think?

How about adding a generic operation for independent tunables:

struct ethtool_get_tunable {
	u32	cmd;
	u32	id;
	u64	value, min, max;
};

struct ethtool_set_tunable {
	u32	cmd;
	u32	id;
	u64	value;
};

	int (*get_tunable)(struct net_device *, struct ethtool_get_tunable *);
	int (*set_tunable)(struct net_device *, const struct ethtool_set_tunable *);

The id to name mapping could be provided either through a stringset or
macros in <uapi/linux/ethtool.h>.  And perhaps we could split the id
space to allow for driver-specific tunables (while strongly discouraging
those for in-tree drivers).

Ben.

-- 
Ben Hutchings
Reality is just a crutch for people who can't handle science fiction.