netdev - Re: [PATCH net-next v2 2/3] ethtool: Add support for DMA buffer settings

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: <1406987782.30258.43.camel@deadeye.wl.decadent.org.uk>
Date:	Sat, 02 Aug 2014 14:56:22 +0100
From:	Ben Hutchings <ben@...adent.org.uk>
To:	David Miller <davem@...emloft.net>
Cc:	_govind@....com, netdev@...r.kernel.org, ssujith@...co.com,
	benve@...co.com
Subject: Re: [PATCH net-next v2 2/3] ethtool: Add support for DMA buffer
 settings

On Wed, 2014-07-30 at 17:53 -0700, David Miller wrote:
> From: Govindarajulu Varadarajan <_govind@....com>
> Date: Tue, 29 Jul 2014 17:10:38 +0530
> 
> > @@ -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.

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