On Wed, 2014-08-27 at 03:03 +0530, Govindarajulu Varadarajan wrote:

On Fri, 22 Aug 2014, Ben Hutchings wrote:

quoted

On Thu, 2014-08-14 at 14:59 +0530, Govindarajulu Varadarajan wrote:

quoted

@@ -257,6 +262,7 @@ struct ethtool_ops {
 				     struct ethtool_eeprom *, u8 *);
 	int	(*get_eee)(struct net_device *, struct ethtool_eee *);
 	int	(*set_eee)(struct net_device *, struct ethtool_eee *);
+	struct ethtool_tunable_ops tunable_ops[ETHTOOL_TUNABLE_MAX];

This is OK but if we add a lot of tunables then it bloats up each driver
with a (probably quite sparse) array of function pointers.

Will fix with David's suggestion.

quoted

quoted

+struct ethtool_tunable {
+	u32 cmd;
+	u32 tcmd;
+	u32 len;
+	union {
+		u32 rx_copybreak;
+		u32 data[48];
+	} data;

[...]

This is not at all generic.  If tunables don't all have the same type,
then the type - not just the length - should be explicit.

I also think that if the length of a value can vary then we should not
declare a data member at all.  So we would have something like:

struct ethtool_tunable {
	__u32	cmd;
	__u32	id;
	__u32	type_id;
	__u32	len;
	__u8	data[0];
};

Then the value buffer would be passed to the driver functions separately
(as for other variable-length command structures).

OK. id show be the tunable command type right? Like RX_COPYBREAK, TX_COPYBREAK
etc. What is the type_id?

A number that identifies the type of the tunable (it might be u32, u64,
link-layer address, text, etc.).

Ben.

quoted

By the way, you must use double-underscore prefixes on fixed-width
integer type names in UAPI headers.

will fix it.

Thanks

-- 
Ben Hutchings
No political challenge can be met by shopping. - George Monbiot