RE: [PATCH 1/2] Ethtool: Add EEE support

On Wed, 2012-06-06 at 16:27 +0000, Yuval Mintz wrote:

Hi Ben,

I stand corrected; I'll supply a new version shortly. 

I've got 2 questions  though:

quoted

For now, use int for booleans.  At some point I would like to see a
thorough cleanup of ethtool to use bool where appropriate - but that's
independent of this

I've noticed that the 'do_generic_set' function assumes all fields are ints.

Yes, but that can and should be fixed.

Is this a convention we should stick to (using  __u32 in the ethtool structs)?

Yes.  It is generally preferable to use explicitly fixed-size types in
kernel-userland ABI, even though I wouldn't expect the size of int to
change for many decades to come.

I'm asking because I'm "wasting" fields in the ethtool_eee struct as I use
__u32 for boolean fields, simply because what seems to be the conventional
method won't work with smaller fields (corrupts the following fields).

It's slightly wasteful but not worth worrying about.

The seconds question - is there a dependency between your acceptance of
this patch series and Dave's acceptance of the kernel's ethtool modification?
I'm asking because changes in the ethtool header there should be applied in
this patch series as well (in ethtool-copy.h).

There is a dependency, yes.  However I generally update ethtool-copy.h
as a separate commit myself (as you can see in the git commit log for
it) so I won't demand that you refresh the version in your patch.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.