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] [day] [month] [year] [list]

Message-ID: <1409122764.30127.26.camel@deadeye.wl.decadent.org.uk>
Date:	Tue, 26 Aug 2014 23:59:24 -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 Wed, 2014-08-27 at 03:03 +0530, Govindarajulu Varadarajan wrote:
> On Fri, 22 Aug 2014, Ben Hutchings wrote:
> > On Thu, 2014-08-14 at 14:59 +0530, Govindarajulu Varadarajan wrote:
> >> @@ -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.
> 
> >> +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.

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

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