Re: [PATCH net-next v3 2/3] ethtool: Add generic options for tunables
From: Ben Hutchings <hidden>
Date: 2014-08-22 23:26:22
On Thu, 2014-08-14 at 14:59 +0530, Govindarajulu Varadarajan wrote:
quoted hunk ↗ jump to hunk
This patch adds new ethtool cmd, ETHTOOL_GTUNABLE & ETHTOOL_STUNABLE for getting tunable values from driver. Add ethtool_tunable_ops array to ethtool_ops. The array index will be the tunable tcmd as defined in enum tunable_cmd. ethtool_tunable_ops has two function pointers set & get. This is set by the driver for getting/setting particular tunable. Signed-off-by: Govindarajulu Varadarajan <redacted> --- include/linux/ethtool.h | 6 ++++++ include/uapi/linux/ethtool.h | 17 +++++++++++++++ net/core/ethtool.c | 51 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+)diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index e658229..33bff94 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h@@ -77,6 +77,11 @@ static inline u32 ethtool_rxfh_indir_default(u32 index, u32 n_rx_rings) return index % n_rx_rings; } +struct ethtool_tunable_ops { + int (*set)(struct net_device *, struct ethtool_tunable *);
The second pointer type should be const-qualified.
quoted hunk ↗ jump to hunk
+ int (*get)(struct net_device *, struct ethtool_tunable *); +}; + /** * struct ethtool_ops - optional netdev operations * @get_settings: Get various device settings including Ethernet link@@ -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.
quoted hunk ↗ jump to hunk
};diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index e3c7a71..99e43ca 100644 --- a/include/uapi/linux/ethtool.h +++ b/include/uapi/linux/ethtool.h@@ -209,6 +209,21 @@ struct ethtool_value { __u32 data; }; +enum tunable_cmd { + ETHTOOL_RX_COPYBREAK = 0, + ETHTOOL_TUNABLE_MAX, +}; + +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).
By the way, you must use double-underscore prefixes on fixed-width
integer type names in UAPI headers.
Ben.
--
Ben Hutchings
Lowery's Law:
If it jams, force it. If it breaks, it needed replacing anyway. Attachments
- signature.asc [application/pgp-signature] 811 bytes