On 02/26/2010 06:49 PM, Peter P Waskiewicz Jr wrote:

On Fri, 2010-02-26 at 05:56 -0800, Jeff Garzik wrote:

quoted

On 02/26/2010 06:54 AM, Jeff Kirsher wrote:

quoted

From: Peter Waskiewicz<redacted>

The drvinfo struct should include the number of strings that
get_rx_ntuple will return.  It will be variable if an underlying
driver implements its own get_rx_ntuple routine, so userspace
needs to know how much data is coming.

Signed-off-by: Peter P Waskiewicz Jr<redacted>
Signed-off-by: Jeff Kirsher<redacted>
---

   include/linux/ethtool.h |    1 +
   net/core/ethtool.c      |    3 +++
   2 files changed, 4 insertions(+), 0 deletions(-)

(resending reply, standard patch-sending box is having trouble sending
to vger)


As noted in the other email, your patch breaks ABI.  The proper path is
to decrease the size of reserved struct member, and NOT shift the offset
of other members.



However, perhaps consider the following patch for returning n-tuple
count, for four reasons:

1) space in ethtool_drvinfo is limited

2) the patch below permits trivial string set addition, without
     ABI changes beyond adding a new ETH_SS_xxx constant.

3) the patch below permits direct access to ops->get_sset_count(),
     rather than implicit access via ethtool_drvinfo

4) ethtool_drvinfo interface does not permit indication of
     ops->get_sset_count() failure, versus returning zero value.  The
     patch below does so, via output sset_mask.

WARNING: this patch is compile-tested only.

NOTE: I added a cosmetic fix to ETHTOOL_[GS]RXNTUPLE constants, making
their indentation consistent with the rest of the list of constants.

Signed-off-by: Jeff Garzik<redacted>

I'm updating your patch since I found an issue.  The mask is passing in
the ETH_SS_* flags, but then they're treated as bits, not enumerated
flags.  I'm thinking of the best non-intrusive way to correct it.

As Ben noted, you cannot change those enumerated values, as they are 
already part of the ABI.

For ETHTOOL_GSSET_INFO, you initialize the sset_mask like this:

	info.sset_mask = (1ULL << ETH_SS_NTUPLE_FILTERS);

A multiple initialization would look like this:

	info.sset_mask = (1ULL << ETH_SS_NTUPLE_FILTERS) |
			(1ULL << ETH_SS_STATS) |
			(1ULL << ETH_SS_PRIV_FLAGS);

Do you still see an issue in my suggested code, now that sset_mask 
confusion is cleared up?

Regards,

	Jeff