[net-next-2.6 PATCH] ethtool: Add n-tuple string length to drvinfo and return it

[net-next-2.6 PATCH] ethtool: Add n-tuple string length to drvinfo and return it

[Jeff Kirsher]

From: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>

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 <peter.p.waskiewicz.jr@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

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

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index cca1c3d..f7992a2 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -63,6 +63,7 @@ struct ethtool_drvinfo {
 	char	reserved2[12];
 	__u32	n_priv_flags;	/* number of flags valid in ETHTOOL_GPFLAGS */
 	__u32	n_stats;	/* number of u64's from ETHTOOL_GSTATS */
+	__u32	n_ntuples;	/* number of n-tuple filters from GSTRINGS */
 	__u32	testinfo_len;
 	__u32	eedump_len;	/* Size of data from ETHTOOL_GEEPROM (bytes) */
 	__u32	regdump_len;	/* Size of data from ETHTOOL_GREGS (bytes) */
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 31b1edd..1c94f48 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -224,6 +224,9 @@ static noinline int ethtool_get_drvinfo(struct net_device *dev, void __user *use
 		rc = ops->get_sset_count(dev, ETH_SS_PRIV_FLAGS);
 		if (rc >= 0)
 			info.n_priv_flags = rc;
+		rc = ops->get_sset_count(dev, ETH_SS_NTUPLE_FILTERS);
+		if (rc >= 0)
+			info.n_ntuples = rc;
 	}
 	if (ops->get_regs_len)
 		info.regdump_len = ops->get_regs_len(dev);

Re: [net-next-2.6 PATCH] ethtool: Add n-tuple string length to drvinfo and return it

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Fri, 26 Feb 2010 03:54:20 -0800

> From: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
> 
> 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 <peter.p.waskiewicz.jr@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Applied.

Re: [net-next-2.6 PATCH] ethtool: Add n-tuple string length to drvinfo and return it

[Jeff Garzik]

On 02/26/2010 07:20 AM, David Miller wrote:
> From: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
> Date: Fri, 26 Feb 2010 03:54:20 -0800
>
>> From: Peter Waskiewicz<peter.p.waskiewicz.jr@intel.com>
>>
>> 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<peter.p.waskiewicz.jr@intel.com>
>> Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>
> Applied.

NAK.  Did you even read the patch?

We don't increase the size of struct ethtool_drvinfo, _especially_ by 
sticking struct members into the middle of the struct.

What do you think 'reserved' is for???

	Jeff

Re: [net-next-2.6 PATCH] ethtool: Add n-tuple string length to drvinfo and return it

[David Miller]

From: Jeff Garzik <jeff@garzik.org>
Date: Fri, 26 Feb 2010 08:05:19 -0500

> NAK.  Did you even read the patch?

I did, brain doesn't work sometimes :-)

> We don't increase the size of struct ethtool_drvinfo, _especially_ by
> sticking struct members into the middle of the struct.
> 
> What do you think 'reserved' is for???

Right, I'll revert, thanks.

[PATCH] Re: [net-next-2.6 PATCH] ethtool: Add n-tuple string length to drvinfo and return it

[Jeff Garzik]

On Fri, Feb 26, 2010 at 03:54:20AM -0800, Jeff Kirsher wrote:
> From: Peter Waskiewicz <peter.p.waskiewicz.jr@intel.com>
> 
> 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 <peter.p.waskiewicz.jr@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

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 <jgarzik@redhat.com>

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index f7992a2..04ecf18 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -254,6 +254,17 @@ struct ethtool_gstrings {
 	__u8	data[0];
 };
 
+struct ethtool_sset_info {
+	__u32	cmd;		/* ETHTOOL_GSSET_INFO */
+	__u32	reserved;
+	__u64	sset_mask;	/* input: each bit selects an sset to query */
+				/* output: each bit a returned sset */
+	__u32	data[0];	/* ETH_SS_xxx count, in order, based on bits
+				   in sset_mask.  One bit implies one
+				   __u32, two bits implies two
+				   __u32's, etc. */
+};
+
 enum ethtool_test_flags {
 	ETH_TEST_FL_OFFLINE	= (1 << 0),	/* online / offline */
 	ETH_TEST_FL_FAILED	= (1 << 1),	/* test passed / failed */
@@ -607,9 +618,9 @@ struct ethtool_ops {
 #define	ETHTOOL_SRXCLSRLINS	0x00000032 /* Insert RX classification rule */
 #define	ETHTOOL_FLASHDEV	0x00000033 /* Flash firmware to device */
 #define	ETHTOOL_RESET		0x00000034 /* Reset hardware */
-
-#define ETHTOOL_SRXNTUPLE	0x00000035 /* Add an n-tuple filter to device */
-#define ETHTOOL_GRXNTUPLE	0x00000036 /* Get n-tuple filters from device */
+#define	ETHTOOL_SRXNTUPLE	0x00000035 /* Add an n-tuple filter to device */
+#define	ETHTOOL_GRXNTUPLE	0x00000036 /* Get n-tuple filters from device */
+#define	ETHTOOL_GSSET_INFO	0x00000037 /* Get string set info */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 1c94f48..c019f92 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -241,6 +241,69 @@ static noinline int ethtool_get_drvinfo(struct net_device *dev, void __user *use
 /*
  * noinline attribute so that gcc doesnt use too much stack in dev_ethtool()
  */
+static noinline int ethtool_get_sset_info(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_sset_info info;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	u64 sset_mask;
+	int i, idx = 0, n_bits = 0, ret, rc;
+	u32 *info_buf = NULL;
+
+	if (!ops->get_sset_count)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&info, useraddr, sizeof(info)))
+		return -EFAULT;
+
+	/* store copy of mask, because we zero struct later on */
+	sset_mask = info.sset_mask;
+	if (!sset_mask)
+		return 0;
+
+	/* calculate size of return buffer */
+	for (i = 0; i < 64; i++)
+		if (sset_mask & (1ULL << i))
+			n_bits++;
+
+	memset(&info, 0, sizeof(info));
+	info.cmd = ETHTOOL_GSSET_INFO;
+
+	info_buf = kzalloc(n_bits * sizeof(u32), GFP_USER);
+	if (!info_buf)
+		return -ENOMEM;
+
+	/* fill return buffer based on input bitmask and successful 
+	 * get_sset_count return
+	 */
+	for (i = 0; i < 64; i++) {
+		if (!(sset_mask & (1ULL << i)))
+			continue;
+
+		rc = ops->get_sset_count(dev, i);
+		if (rc >= 0) {
+			info.sset_mask |= (1ULL << i);
+			info_buf[idx++] = rc;
+		}
+	}
+
+	ret = -EFAULT;
+	if (copy_to_user(useraddr, &info, sizeof(info)))
+		goto out;
+
+	useraddr += offsetof(struct ethtool_sset_info, data);
+	if (copy_to_user(useraddr, info_buf, idx * sizeof(u32)))
+		goto out;
+
+	ret = 0;
+
+out:
+	kfree(info_buf);
+	return ret;
+}
+
+/*
+ * noinline attribute so that gcc doesnt use too much stack in dev_ethtool()
+ */
 static noinline int ethtool_set_rxnfc(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_rxnfc cmd;
@@ -1472,6 +1535,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GRXNTUPLE:
 		rc = ethtool_get_rx_ntuple(dev, useraddr);
 		break;
+	case ETHTOOL_GSSET_INFO:
+		rc = ethtool_get_sset_info(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}

Re: [net-next-2.6 PATCH] ethtool: Add n-tuple string length to drvinfo and return it

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 1647 bytes --]

On 02/26/2010 06:54 AM, Jeff Kirsher wrote:
> From: Peter Waskiewicz<peter.p.waskiewicz.jr@intel.com>
>
> 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<peter.p.waskiewicz.jr@intel.com>
> Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
> ---
>
>   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 <jgarzik@redhat.com>



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 3582 bytes --]

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index f7992a2..04ecf18 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -254,6 +254,17 @@ struct ethtool_gstrings {
 	__u8	data[0];
 };
 
+struct ethtool_sset_info {
+	__u32	cmd;		/* ETHTOOL_GSSET_INFO */
+	__u32	reserved;
+	__u64	sset_mask;	/* input: each bit selects an sset to query */
+				/* output: each bit a returned sset */
+	__u32	data[0];	/* ETH_SS_xxx count, in order, based on bits
+				   in sset_mask.  One bit implies one
+				   __u32, two bits implies two
+				   __u32's, etc. */
+};
+
 enum ethtool_test_flags {
 	ETH_TEST_FL_OFFLINE	= (1 << 0),	/* online / offline */
 	ETH_TEST_FL_FAILED	= (1 << 1),	/* test passed / failed */
@@ -607,9 +618,9 @@ struct ethtool_ops {
 #define	ETHTOOL_SRXCLSRLINS	0x00000032 /* Insert RX classification rule */
 #define	ETHTOOL_FLASHDEV	0x00000033 /* Flash firmware to device */
 #define	ETHTOOL_RESET		0x00000034 /* Reset hardware */
-
-#define ETHTOOL_SRXNTUPLE	0x00000035 /* Add an n-tuple filter to device */
-#define ETHTOOL_GRXNTUPLE	0x00000036 /* Get n-tuple filters from device */
+#define	ETHTOOL_SRXNTUPLE	0x00000035 /* Add an n-tuple filter to device */
+#define	ETHTOOL_GRXNTUPLE	0x00000036 /* Get n-tuple filters from device */
+#define	ETHTOOL_GSSET_INFO	0x00000037 /* Get string set info */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 1c94f48..c019f92 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -241,6 +241,69 @@ static noinline int ethtool_get_drvinfo(struct net_device *dev, void __user *use
 /*
  * noinline attribute so that gcc doesnt use too much stack in dev_ethtool()
  */
+static noinline int ethtool_get_sset_info(struct net_device *dev, void __user *useraddr)
+{
+	struct ethtool_sset_info info;
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	u64 sset_mask;
+	int i, idx = 0, n_bits = 0, ret, rc;
+	u32 *info_buf = NULL;
+
+	if (!ops->get_sset_count)
+		return -EOPNOTSUPP;
+
+	if (copy_from_user(&info, useraddr, sizeof(info)))
+		return -EFAULT;
+
+	/* store copy of mask, because we zero struct later on */
+	sset_mask = info.sset_mask;
+	if (!sset_mask)
+		return 0;
+
+	/* calculate size of return buffer */
+	for (i = 0; i < 64; i++)
+		if (sset_mask & (1ULL << i))
+			n_bits++;
+
+	memset(&info, 0, sizeof(info));
+	info.cmd = ETHTOOL_GSSET_INFO;
+
+	info_buf = kzalloc(n_bits * sizeof(u32), GFP_USER);
+	if (!info_buf)
+		return -ENOMEM;
+
+	/* fill return buffer based on input bitmask and successful 
+	 * get_sset_count return
+	 */
+	for (i = 0; i < 64; i++) {
+		if (!(sset_mask & (1ULL << i)))
+			continue;
+
+		rc = ops->get_sset_count(dev, i);
+		if (rc >= 0) {
+			info.sset_mask |= (1ULL << i);
+			info_buf[idx++] = rc;
+		}
+	}
+
+	ret = -EFAULT;
+	if (copy_to_user(useraddr, &info, sizeof(info)))
+		goto out;
+
+	useraddr += offsetof(struct ethtool_sset_info, data);
+	if (copy_to_user(useraddr, info_buf, idx * sizeof(u32)))
+		goto out;
+
+	ret = 0;
+
+out:
+	kfree(info_buf);
+	return ret;
+}
+
+/*
+ * noinline attribute so that gcc doesnt use too much stack in dev_ethtool()
+ */
 static noinline int ethtool_set_rxnfc(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_rxnfc cmd;
@@ -1472,6 +1535,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GRXNTUPLE:
 		rc = ethtool_get_rx_ntuple(dev, useraddr);
 		break;
+	case ETHTOOL_GSSET_INFO:
+		rc = ethtool_get_sset_info(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}

Re: [net-next-2.6 PATCH] ethtool: Add n-tuple string length to drvinfo and return it

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 938 bytes --]

On 02/26/2010 08:56 AM, Jeff Garzik wrote:
> 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 <jgarzik@redhat.com>


With, perhaps, a note like the attached as a reminder to folks about 
future additions.


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 455 bytes --]

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 31b1edd..e376e8c 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -212,6 +212,9 @@ static noinline int ethtool_get_drvinfo(struct net_device *dev, void __user *use
 	info.cmd = ETHTOOL_GDRVINFO;
 	ops->get_drvinfo(dev, &info);
 
+	/* this method of obtaining string set info is deprecated;
+	 * consider using ETHTOOL_GSSET_INFO instead
+	 */
 	if (ops->get_sset_count) {
 		int rc;
 

Re: [net-next-2.6 PATCH] ethtool: Add n-tuple string length to drvinfo and return it

[Peter P Waskiewicz Jr]

On Fri, 2010-02-26 at 05:11 -0800, David Miller wrote:
> From: Jeff Garzik <jeff@garzik.org>
> Date: Fri, 26 Feb 2010 08:05:19 -0500
> 
> > NAK.  Did you even read the patch?
> 
> I did, brain doesn't work sometimes :-)
> 
> > We don't increase the size of struct ethtool_drvinfo, _especially_ by
> > sticking struct members into the middle of the struct.
> > 

Brain lapse.  When you mentioned ABI not being locked down until 2.6.34
is dropped, I didn't think.

I'm testing your proposed change, and once I have it working, I'll give
it to Jeff K. to get pushed along with a new userspace patch.

> > What do you think 'reserved' is for???
> 
> Right, I'll revert, thanks.

Re: [net-next-2.6 PATCH] ethtool: Add n-tuple string length to drvinfo and return it

[Peter P Waskiewicz Jr]

On Fri, 2010-02-26 at 05:56 -0800, Jeff Garzik wrote:
> On 02/26/2010 06:54 AM, Jeff Kirsher wrote:
> > From: Peter Waskiewicz<peter.p.waskiewicz.jr@intel.com>
> >
> > 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<peter.p.waskiewicz.jr@intel.com>
> > Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
> > ---
> >
> >   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 <jgarzik@redhat.com>

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.

-PJ

Re: [net-next-2.6 PATCH] ethtool: Add n-tuple string length to drvinfo and return it

[Jeff Garzik]

On 02/26/2010 06:49 PM, Peter P Waskiewicz Jr wrote:
> On Fri, 2010-02-26 at 05:56 -0800, Jeff Garzik wrote:
>> On 02/26/2010 06:54 AM, Jeff Kirsher wrote:
>>> From: Peter Waskiewicz<peter.p.waskiewicz.jr@intel.com>
>>>
>>> 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<peter.p.waskiewicz.jr@intel.com>
>>> Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>>> ---
>>>
>>>    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<jgarzik@redhat.com>
>
> 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

Re: [net-next-2.6 PATCH] ethtool: Add n-tuple string length to drvinfo and return it

[Jeff Garzik]

On 02/27/2010 01:31 AM, Jeff Garzik wrote:
> On 02/26/2010 06:49 PM, Peter P Waskiewicz Jr wrote:
>> On Fri, 2010-02-26 at 05:56 -0800, Jeff Garzik wrote:
>>> On 02/26/2010 06:54 AM, Jeff Kirsher wrote:
>>>> From: Peter Waskiewicz<peter.p.waskiewicz.jr@intel.com>
>>>>
>>>> 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<peter.p.waskiewicz.jr@intel.com>
>>>> Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>>>> ---
>>>>
>>>> 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<jgarzik@redhat.com>
>>
>> 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);

Additionally, upon ioctl(2) completion, info.sset_mask will contain 
(1<<ETH_SS_NTUPLE_FILTERS) if and only if the kernel 
ops->get_sset_count() function call returned successfully.

Thus, the absence of that bit in info.sset_mask indicates the driver 
returned failure.

This condition needs to be checked in your userspace ethtool patch.

	Jeff

Re: [net-next-2.6 PATCH] ethtool: Add n-tuple string length to drvinfo and return it

[Waskiewicz Jr, Peter P]

On Fri, 26 Feb 2010, Jeff Garzik wrote:

> On 02/26/2010 06:49 PM, Peter P Waskiewicz Jr wrote:
> > On Fri, 2010-02-26 at 05:56 -0800, Jeff Garzik wrote:
> >> On 02/26/2010 06:54 AM, Jeff Kirsher wrote:
> >>> From: Peter Waskiewicz<peter.p.waskiewicz.jr@intel.com>
> >>>
> >>> 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<peter.p.waskiewicz.jr@intel.com>
> >>> Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
> >>> ---
> >>>
> >>>    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<jgarzik@redhat.com>
> >
> > 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.

Apologies.  My outstanding IT support marked your and Ben's emails as 
spam, and moved it aside where I couldn't see them until this morning...

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

This sounds like it makes sense.  I'll make the changes (and the userspace 
change you suggested in another mail) and get new patches out.

> 
> Regards,
> 
> 	Jeff