From: Jeff Garzik <jeff@garzik•org>
To: Ben Hutchings <bhutchings@solarflare•com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel•com>,
davem@davemloft•net, netdev@vger•kernel.org, gospo@redhat•com
Subject: [PATCH] Re: [PATCH v4] ethtool: Add direct access to ops->get_sset_count
Date: Thu, 04 Mar 2010 13:21:53 -0500 [thread overview]
Message-ID: <4B8FFA41.2020700@garzik.org> (raw)
In-Reply-To: <1267712797.2819.149.camel@localhost>
[-- 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;
next prev parent reply other threads:[~2010-03-04 18:21 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 ` Jeff Garzik [this message]
2010-03-05 22:00 ` [PATCH] " David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4B8FFA41.2020700@garzik.org \
--to=jeff@garzik$(echo .)org \
--cc=bhutchings@solarflare$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=gospo@redhat$(echo .)com \
--cc=jeffrey.t.kirsher@intel$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox