[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4DFB73B2.10506@candelatech.com>
Date: Fri, 17 Jun 2011 08:33:06 -0700
From: Ben Greear <greearb@...delatech.com>
To: Ben Hutchings <bhutchings@...arflare.com>
CC: netdev@...r.kernel.org
Subject: Re: [RFC 1/2] net: Support getting/setting RX-FCS in drivers.
On 06/17/2011 05:20 AM, Ben Hutchings wrote:
> On Thu, 2011-06-16 at 21:30 -0700, greearb@...delatech.com wrote:
>> From: Ben Greear<greearb@...delatech.com>
>>
>> This will allow us to enable/disable having the Ethernet
>> frame checksum appended to the skb. Enabling this is
>> useful when sniffing packets.
>>
>> In particular, this can be used to test logic that allows
>> a NIC to receive all frames, even ones with bad checksums.
>>
>> Signed-off-by: Ben Greear<greearb@...delatech.com>
>> ---
>> :100644 100644 439b173... e5e8747... M include/linux/ethtool.h
>> :100644 100644 fd14116... b36bac7... M net/core/ethtool.c
>> include/linux/ethtool.h | 5 +++++
>> net/core/ethtool.c | 38 ++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 43 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
>> index 439b173..e5e8747 100644
>> --- a/include/linux/ethtool.h
>> +++ b/include/linux/ethtool.h
>> @@ -955,6 +955,8 @@ struct ethtool_ops {
>> int (*get_dump_data)(struct net_device *,
>> struct ethtool_dump *, void *);
>> int (*set_dump)(struct net_device *, struct ethtool_dump *);
>> + int (*set_save_fcs)(struct net_device *, u32);
>> + int (*get_save_fcs)(struct net_device *, u32 *);
>
> These need to be described in the kernel-doc.
>
> Why do these function names use 'save_fcs' whereas the command names use
> 'RXFCS'?
>
>> };
>> #endif /* __KERNEL__ */
>> @@ -1029,6 +1031,9 @@ struct ethtool_ops {
>> #define ETHTOOL_SET_DUMP 0x0000003e /* Set dump settings */
>> #define ETHTOOL_GET_DUMP_FLAG 0x0000003f /* Get dump settings */
>> #define ETHTOOL_GET_DUMP_DATA 0x00000040 /* Get dump data */
>> +#define ETHTOOL_GETRXFCS 0x00000041 /* Get RX Save Frame Checksum */
>> +#define ETHTOOL_SETRXFCS 0x00000042 /* Set RX Save Frame Checksum */
>> +
>>
>> /* compatibility with older code */
>> #define SPARC_ETH_GSET ETHTOOL_GSET
>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>> index fd14116..b36bac7 100644
>> --- a/net/core/ethtool.c
>> +++ b/net/core/ethtool.c
>> @@ -1927,6 +1927,38 @@ out:
>> return ret;
>> }
>>
>> +static int ethtool_get_rx_fcs(struct net_device *dev, void __user *useraddr)
>> +{
>> + struct ethtool_value edata;
>> + int rv = 0;
>> +
>> + if (!dev->ethtool_ops->get_save_fcs)
>> + return -EOPNOTSUPP;
>> +
>> + rv = dev->ethtool_ops->get_save_fcs(dev,&edata.data);
>> + if (rv< 0)
>> + return rv;
>> +
>> + if (copy_to_user(useraddr,&edata, sizeof(edata)))
>> + return -EFAULT;
>> + return 0;
>> +}
>> +
>> +
>> +static int ethtool_set_rx_fcs(struct net_device *dev, void __user *useraddr)
>> +{
>> + struct ethtool_value id;
>> +
>> + if (!dev->ethtool_ops->set_save_fcs)
>> + return -EOPNOTSUPP;
>> +
>> + if (copy_from_user(&id, useraddr, sizeof(id)))
>> + return -EFAULT;
>> +
>> + return dev->ethtool_ops->set_save_fcs(dev, id.data);
>> +}
>> +
>> +
>> /* The main entry point in this file. Called from net/core/dev.c */
>>
>> int dev_ethtool(struct net *net, struct ifreq *ifr)
>> @@ -2152,6 +2184,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
>> case ETHTOOL_GET_DUMP_DATA:
>> rc = ethtool_get_dump_data(dev, useraddr);
>> break;
>> + case ETHTOOL_SETRXFCS:
>> + rc = ethtool_set_rx_fcs(dev, useraddr);
>> + break;
>> + case ETHTOOL_GETRXFCS:
>> + rc = ethtool_get_rx_fcs(dev, useraddr);
>> + break;
>
> You can use ethtool_{get,set}_value() rather than adding new trivial
> functions.
I'll look at that.
>
> And as Michal says, this could reasonably be a feature not an entirely
> separate flag. I'm not sure it's that important to have debugging flags
> in features, but I also don't want to have 2 commands per flag...
I'm not sure it really counts as a feature, and I've no desire to tangle
with the effort to extend features beyond 32 bits. Maybe we could have a new
ethtool command that took a struct with two args, so we can "set flag-foo val"
instead of 'enable-flag-foo' and 'disable-flag-foo'?
That should be only one additional method for the drivers to implement, and
can be used for the other patches I have planned as well.
Thanks,
Ben
--
Ben Greear <greearb@...delatech.com>
Candela Technologies Inc http://www.candelatech.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists