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

From: Ben Greear <redacted>

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

From: Ben Greear <redacted>

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 <redacted>
---
: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

2011/6/17  [off-list ref]:

From: Ben Greear <redacted>

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

On Thu, 2011-06-16 at 21:30 -0700, greearb@candelatech.com wrote:

These need to be described in the kernel-doc.

Why do these function names use 'save_fcs' whereas the command names use
'RXFCS'?

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.

On 06/17/2011 05:20 AM, Ben Hutchings wrote:

On Thu, 2011-06-16 at 21:30 -0700, greearb@candelatech.com wrote:

quoted

From: Ben Greear<redacted>

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<redacted>
---
: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'?

quoted

  };
  #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 [off-list ref]
Candela Technologies Inc  http://www.candelatech.com

On Fri, 2011-06-17 at 08:33 -0700, Ben Greear wrote:

On 06/17/2011 05:20 AM, Ben Hutchings wrote:

quoted

On Thu, 2011-06-16 at 21:30 -0700, greearb@candelatech.com wrote:

quoted

From: Ben Greear<redacted>

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.

[...]

quoted

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.

From: Ben Greear <redacted>

Helps when sniffing packets.

Signed-off-by: Ben Greear <redacted>
---
: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