* [PATCH v4] ethtool: Add direct access to ops->get_sset_count
@ 2010-03-04 8:51 Jeff Kirsher
2010-03-04 13:23 ` Jeff Garzik
2010-03-04 14:26 ` Ben Hutchings
0 siblings, 2 replies; 6+ messages in thread
From: Jeff Kirsher @ 2010-03-04 8:51 UTC (permalink / raw)
To: davem; +Cc: jeff, netdev, gospo
From: Jeff Garzik <jgarzik@redhat.com>
This patch is an alternative approach for accessing string
counts, vs. the drvinfo indirect approach. This way the drvinfo
space doesn't run out, and we don't break ABI later.
Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
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 | 17 +++++++++--
net/core/ethtool.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 86 insertions(+), 3 deletions(-)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index cca1c3d..f6f961f 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -253,6 +253,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 */
@@ -606,9 +617,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 0f2f821..70075c4 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -214,6 +214,10 @@ 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;
@@ -240,6 +244,71 @@ 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;
@@ -1471,6 +1540,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;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4] ethtool: Add direct access to ops->get_sset_count
2010-03-04 8:51 [PATCH v4] ethtool: Add direct access to ops->get_sset_count Jeff Kirsher
@ 2010-03-04 13:23 ` Jeff Garzik
2010-03-05 22:00 ` David Miller
2010-03-04 14:26 ` Ben Hutchings
1 sibling, 1 reply; 6+ messages in thread
From: Jeff Garzik @ 2010-03-04 13:23 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: davem, netdev, gospo
On 03/04/2010 03:51 AM, Jeff Kirsher wrote:
> From: Jeff Garzik<jgarzik@redhat.com>
>
> This patch is an alternative approach for accessing string
> counts, vs. the drvinfo indirect approach. This way the drvinfo
> space doesn't run out, and we don't break ABI later.
>
> Signed-off-by: Jeff Garzik<jgarzik@redhat.com>
> 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 | 17 +++++++++--
> net/core/ethtool.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 86 insertions(+), 3 deletions(-)
Both patches look good to me. There is a cosmetic issue of needing to
sync up userspace and kernel ethtool.h WRT whitespace and deleted
constants, but I can do that after DaveM applies this patch.
Waiting for upstream application, or other objections...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4] ethtool: Add direct access to ops->get_sset_count
2010-03-04 8:51 [PATCH v4] ethtool: Add direct access to ops->get_sset_count Jeff Kirsher
2010-03-04 13:23 ` Jeff Garzik
@ 2010-03-04 14:26 ` Ben Hutchings
2010-03-04 18:21 ` [PATCH] " Jeff Garzik
1 sibling, 1 reply; 6+ messages in thread
From: Ben Hutchings @ 2010-03-04 14:26 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: davem, jeff, netdev, gospo
On Thu, 2010-03-04 at 00:51 -0800, Jeff Kirsher wrote:
> From: Jeff Garzik <jgarzik@redhat.com>
>
> This patch is an alternative approach for accessing string
> counts, vs. the drvinfo indirect approach. This way the drvinfo
> space doesn't run out, and we don't break ABI later.
[...]
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -214,6 +214,10 @@ 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
> + */
This comment belongs on the interface (ethtool.h) not the
implementation.
[...]
> +static noinline int ethtool_get_sset_info(struct net_device *dev,
> + void __user *useraddr)
> +{
[...]
> + /* calculate size of return buffer */
> + for (i = 0; i < 64; i++)
> + if (sset_mask & (1ULL << i))
> + n_bits++;
[...]
We have a function for this:
n_bits = hweight64(sset_mask);
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Re: [PATCH v4] ethtool: Add direct access to ops->get_sset_count
2010-03-04 14:26 ` Ben Hutchings
@ 2010-03-04 18:21 ` Jeff Garzik
2010-03-05 22:00 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Garzik @ 2010-03-04 18:21 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Jeff Kirsher, davem, netdev, gospo
[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]
On 03/04/2010 09:26 AM, Ben Hutchings wrote:
> On Thu, 2010-03-04 at 00:51 -0800, Jeff Kirsher wrote:
>> From: Jeff Garzik<jgarzik@redhat.com>
>>
>> This patch is an alternative approach for accessing string
>> counts, vs. the drvinfo indirect approach. This way the drvinfo
>> space doesn't run out, and we don't break ABI later.
> [...]
>> --- a/net/core/ethtool.c
>> +++ b/net/core/ethtool.c
>> @@ -214,6 +214,10 @@ 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
>> + */
>
> This comment belongs on the interface (ethtool.h) not the
> implementation.
Debatable -- the current comment is located at the callsite of
ops->get_sset_count(), which is where an implementor might think to add
a new call. Not all the numeric fields in ethtool_drvinfo are obtained
from ->get_sset_count().
Hence the "some" in the attached patch to include/linux/ethtool.h,
addressing your comment.
> [...]
>> +static noinline int ethtool_get_sset_info(struct net_device *dev,
>> + void __user *useraddr)
>> +{
> [...]
>> + /* calculate size of return buffer */
>> + for (i = 0; i< 64; i++)
>> + if (sset_mask& (1ULL<< i))
>> + n_bits++;
> [...]
>
> We have a function for this:
>
> n_bits = hweight64(sset_mask);
Agreed.
I've attached a follow-up patch, which should enable my/Jeff's kernel
patch to be applied, followed by this one.
Jeff
[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1755 bytes --]
Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
---
include/linux/ethtool.h | 7 +++++++
net/core/ethtool.c | 7 +++----
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index f6f961f..b33f316 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -61,6 +61,13 @@ struct ethtool_drvinfo {
/* For PCI devices, use pci_name(pci_dev). */
char reserved1[32];
char reserved2[12];
+ /*
+ * Some struct members below are filled in
+ * using ops->get_sset_count(). Obtaining
+ * this info from ethtool_drvinfo is now
+ * deprecated; Use ETHTOOL_GSSET_INFO
+ * instead.
+ */
__u32 n_priv_flags; /* number of flags valid in ETHTOOL_GPFLAGS */
__u32 n_stats; /* number of u64's from ETHTOOL_GSTATS */
__u32 testinfo_len;
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 70075c4..33d2ded 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -17,6 +17,7 @@
#include <linux/errno.h>
#include <linux/ethtool.h>
#include <linux/netdevice.h>
+#include <linux/bitops.h>
#include <asm/uaccess.h>
/*
@@ -216,7 +217,7 @@ static noinline int ethtool_get_drvinfo(struct net_device *dev, void __user *use
/*
* this method of obtaining string set info is deprecated;
- * consider using ETHTOOL_GSSET_INFO instead
+ * Use ETHTOOL_GSSET_INFO instead.
*/
if (ops->get_sset_count) {
int rc;
@@ -265,9 +266,7 @@ static noinline int ethtool_get_sset_info(struct net_device *dev,
return 0;
/* calculate size of return buffer */
- for (i = 0; i < 64; i++)
- if (sset_mask & (1ULL << i))
- n_bits++;
+ n_bits = hweight64(sset_mask);
memset(&info, 0, sizeof(info));
info.cmd = ETHTOOL_GSSET_INFO;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4] ethtool: Add direct access to ops->get_sset_count
2010-03-04 13:23 ` Jeff Garzik
@ 2010-03-05 22:00 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2010-03-05 22:00 UTC (permalink / raw)
To: jeff; +Cc: jeffrey.t.kirsher, netdev, gospo
From: Jeff Garzik <jeff@garzik.org>
Date: Thu, 04 Mar 2010 08:23:05 -0500
> On 03/04/2010 03:51 AM, Jeff Kirsher wrote:
>> From: Jeff Garzik<jgarzik@redhat.com>
>>
>> This patch is an alternative approach for accessing string
>> counts, vs. the drvinfo indirect approach. This way the drvinfo
>> space doesn't run out, and we don't break ABI later.
>>
>> Signed-off-by: Jeff Garzik<jgarzik@redhat.com>
>> 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 | 17 +++++++++--
>> net/core/ethtool.c | 72
>> +++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 86 insertions(+), 3 deletions(-)
>
> Both patches look good to me. There is a cosmetic issue of needing to
> sync up userspace and kernel ethtool.h WRT whitespace and deleted
> constants, but I can do that after DaveM applies this patch.
>
> Waiting for upstream application, or other objections...
Applied, thanks guys.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Re: [PATCH v4] ethtool: Add direct access to ops->get_sset_count
2010-03-04 18:21 ` [PATCH] " Jeff Garzik
@ 2010-03-05 22:00 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2010-03-05 22:00 UTC (permalink / raw)
To: jeff; +Cc: bhutchings, jeffrey.t.kirsher, netdev, gospo
From: Jeff Garzik <jeff@garzik.org>
Date: Thu, 04 Mar 2010 13:21:53 -0500
> I've attached a follow-up patch, which should enable my/Jeff's kernel
> patch to be applied, followed by this one.
Applied, thanks Jeff.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-03-05 22:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-03-04 8:51 [PATCH v4] ethtool: Add direct access to ops->get_sset_count Jeff Kirsher
2010-03-04 13:23 ` Jeff Garzik
2010-03-05 22:00 ` David Miller
2010-03-04 14:26 ` Ben Hutchings
2010-03-04 18:21 ` [PATCH] " Jeff Garzik
2010-03-05 22:00 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).