ethtool: Add direct access to ops->get_sset_count

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.

Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index f6f961f..b33f316 100644 (file)
--- 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 (file)
--- 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;