netdev - Re: [PATCH net-next v3 2/3] ethtool: Add generic options for tunables

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1408749965.4672.29.camel@deadeye.wl.decadent.org.uk>
Date:	Fri, 22 Aug 2014 16:26:05 -0700
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Govindarajulu Varadarajan <_govind@....com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	stephen@...workplumber.org, ssujith@...co.com, benve@...co.com
Subject: Re: [PATCH net-next v3 2/3] ethtool: Add generic options for
 tunables

On Thu, 2014-08-14 at 14:59 +0530, Govindarajulu Varadarajan wrote:
> 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 <_govind@....com>
> ---
>  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.

> +	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.

>  
> 
>  };
> 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.

Download attachment "signature.asc" of type "application/pgp-signature" (812 bytes)