[RFC 0/2] Allow NICs to pass Frame Checksum up the stack.

[RFC 0/2] Allow NICs to pass Frame Checksum up the stack.

[greearb]

From: Ben Greear <greearb@candelatech.com>

This series provides ethtool support to set and get the rx-checksum
flag, and adds support to the e100 driver.

Assuming this series is acceptable, I would then propose a series of patches
to allow receiving all frames, even ones with bad checksums, etc.

And if that is acceptable, a third series would allow user-space to generate
packets with custom ethernet frame checksums (ie, bad ones if desired).

And finally, more drivers, such as e1000 and hopefully e100e can
be supported.

Ben Greear (2):
  net:  Support getting/setting RX-FCS in drivers.
  e100:  Support receiving Ethernet FCS.

 drivers/net/e100.c      |   39 ++++++++++++++++++++++++++++++++++++---
 include/linux/ethtool.h |    5 +++++
 net/core/ethtool.c      |   38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 3 deletions(-)

-- 
1.7.3.4

[RFC 1/2] net: Support getting/setting RX-FCS in drivers.

[greearb]

From: Ben Greear <greearb@candelatech.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@candelatech.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 *);
 
 };
 #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;
 	default:
 		rc = -EOPNOTSUPP;
 	}
-- 
1.7.3.4

[RFC 2/2] e100: Support receiving Ethernet FCS.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Helps when sniffing packets.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 e336c79... 80fcadf... M	drivers/net/e100.c
 drivers/net/e100.c |   39 ++++++++++++++++++++++++++++++++++++---
 1 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/net/e100.c b/drivers/net/e100.c
index e336c79..80fcadf 100644
--- a/drivers/net/e100.c
+++ b/drivers/net/e100.c
@@ -587,6 +587,7 @@ struct nic {
 		multicast_all      = (1 << 2),
 		wol_magic          = (1 << 3),
 		ich_10h_workaround = (1 << 4),
+		save_fcs           = (1 << 5),
 	} flags					____cacheline_aligned;
 
 	enum mac mac;
@@ -1130,6 +1131,9 @@ static void e100_configure(struct nic *nic, struct cb *cb, struct sk_buff *skb)
 		config->promiscuous_mode = 0x1;		/* 1=on, 0=off */
 	}
 
+	if (nic->flags & save_fcs)
+		config->rx_crc_transfer = 0x1;	/* 1=save, 0=discard */
+
 	if (nic->flags & multicast_all)
 		config->multicast_all = 0x1;		/* 1=accept, 0=no */
 
@@ -1917,6 +1921,7 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,
 	struct sk_buff *skb = rx->skb;
 	struct rfd *rfd = (struct rfd *)skb->data;
 	u16 rfd_status, actual_size;
+	u16 fcs_pad = 0;
 
 	if (unlikely(work_done && *work_done >= work_to_do))
 		return -EAGAIN;
@@ -1949,9 +1954,11 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,
 	}
 
 	/* Get actual data size */
+	if (nic->flags & save_fcs)
+		fcs_pad = 4;
 	actual_size = le16_to_cpu(rfd->actual_size) & 0x3FFF;
-	if (unlikely(actual_size > RFD_BUF_LEN - sizeof(struct rfd)))
-		actual_size = RFD_BUF_LEN - sizeof(struct rfd);
+	if (unlikely(actual_size > RFD_BUF_LEN + fcs_pad - sizeof(struct rfd)))
+		actual_size = RFD_BUF_LEN + fcs_pad - sizeof(struct rfd);
 
 	/* Get data */
 	pci_unmap_single(nic->pdev, rx->dma_addr,
@@ -1978,7 +1985,7 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,
 	if (unlikely(!(rfd_status & cb_ok))) {
 		/* Don't indicate if hardware indicates errors */
 		dev_kfree_skb_any(skb);
-	} else if (actual_size > ETH_DATA_LEN + VLAN_ETH_HLEN) {
+	} else if (actual_size > ETH_DATA_LEN + VLAN_ETH_HLEN + fcs_pad) {
 		/* Don't indicate oversized frames */
 		nic->rx_over_length_errors++;
 		dev_kfree_skb_any(skb);
@@ -2370,6 +2377,30 @@ static int e100_set_settings(struct net_device *netdev, struct ethtool_cmd *cmd)
 	return err;
 }
 
+static int e100_set_save_fcs(struct net_device *netdev, u32 data)
+{
+	struct nic *nic = netdev_priv(netdev);
+	if (data)
+		nic->flags |= save_fcs;
+	else
+		nic->flags &= ~save_fcs;
+
+	e100_exec_cb(nic, NULL, e100_configure);
+
+	return 0;
+}
+
+static int e100_get_save_fcs(struct net_device *netdev, u32* data)
+{
+	struct nic *nic = netdev_priv(netdev);
+	if (nic->flags & save_fcs)
+		*data = 1;
+	else
+		*data = 0;
+
+	return 0;
+}
+
 static void e100_get_drvinfo(struct net_device *netdev,
 	struct ethtool_drvinfo *info)
 {
@@ -2689,6 +2720,8 @@ static const struct ethtool_ops e100_ethtool_ops = {
 	.set_phys_id		= e100_set_phys_id,
 	.get_ethtool_stats	= e100_get_ethtool_stats,
 	.get_sset_count		= e100_get_sset_count,
+	.set_save_fcs           = e100_set_save_fcs,
+	.get_save_fcs           = e100_get_save_fcs,
 };
 
 static int e100_do_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
-- 
1.7.3.4

Re: [RFC 1/2] net: Support getting/setting RX-FCS in drivers.

[Michał Mirosław]

2011/6/17  <greearb@candelatech.com>:
> From: Ben Greear <greearb@candelatech.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.

You want to silently run away from extending dev->features field? ;-)

Best Regards,
Michał Mirosław

Re: [RFC 1/2] net: Support getting/setting RX-FCS in drivers.

[Ben Hutchings]

On Thu, 2011-06-16 at 21:30 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.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@candelatech.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.

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

Ben.

>  	default:
>  		rc = -EOPNOTSUPP;
>  	}

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [RFC 1/2] net: Support getting/setting RX-FCS in drivers.

[Ben Greear]

On 06/17/2011 05:20 AM, Ben Hutchings wrote:
> On Thu, 2011-06-16 at 21:30 -0700, greearb@candelatech.com wrote:
>> From: Ben Greear<greearb@candelatech.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@candelatech.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@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 1/2] net: Support getting/setting RX-FCS in drivers.

[Ben Hutchings]

On Fri, 2011-06-17 at 08:33 -0700, Ben Greear wrote:
> On 06/17/2011 05:20 AM, Ben Hutchings wrote:
> > On Thu, 2011-06-16 at 21:30 -0700, greearb@candelatech.com wrote:
> >> From: Ben Greear<greearb@candelatech.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.
[...]
> > 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.

There are several people wanting to add new flags, so between you I'm
sure you can manage it.

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

Whereas implementing an extra feature toggle now requires zero extra
methods.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.