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

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

v2:  Make naming more consistent, use get/set_value.

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

 drivers/net/e100.c      |   35 ++++++++++++++++++++++++++++++++---
 include/linux/ethtool.h |    8 ++++++++
 net/core/ethtool.c      |    8 ++++++++
 3 files changed, 48 insertions(+), 3 deletions(-)

-- 
1.7.3.4

[PATCH 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... 675ddc0... M	include/linux/ethtool.h
:100644 100644 fd14116... 0e01860... M	net/core/ethtool.c
 include/linux/ethtool.h |    8 ++++++++
 net/core/ethtool.c      |    8 ++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 439b173..675ddc0 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -877,6 +877,9 @@ bool ethtool_invalid_flags(struct net_device *dev, u32 data, u32 supported);
  * 		   and flag of the device.
  * @get_dump_data: Get dump data.
  * @set_dump: Set dump specific flags to the device.
+ * @set_save_rxfcs:  Set flag to save (1), or discard (0), the Ethernet
+ *	Frame Checksum for received packets.
+ * @get_save_rxfcs:  Get current value for Save RX-FCS flag.
  *
  * All operations are optional (i.e. the function pointer may be set
  * to %NULL) and callers must take this into account.  Callers must
@@ -955,6 +958,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_rxfcs)(struct net_device *, u32);
+	u32	(*get_save_rxfcs)(struct net_device *);
 
 };
 #endif /* __KERNEL__ */
@@ -1029,6 +1034,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_GET_SAVE_RXFCS	0x00000041 /* Get RX Save Frame Checksum */
+#define ETHTOOL_SET_SAVE_RXFCS	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..0e01860 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -2152,6 +2152,14 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GET_DUMP_DATA:
 		rc = ethtool_get_dump_data(dev, useraddr);
 		break;
+	case ETHTOOL_SET_SAVE_RXFCS:
+		rc = ethtool_set_value(dev, useraddr,
+				       dev->ethtool_ops->set_save_rxfcs);
+		break;
+	case ETHTOOL_GET_SAVE_RXFCS:
+		rc =  ethtool_get_value(dev, useraddr, ethcmd,
+					dev->ethtool_ops->get_save_rxfcs);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}
-- 
1.7.3.4

[PATCH 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... 647d8c6... M	drivers/net/e100.c
 drivers/net/e100.c |   35 ++++++++++++++++++++++++++++++++---
 1 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/net/e100.c b/drivers/net/e100.c
index e336c79..647d8c6 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_rxfcs         = (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_rxfcs)
+		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 rxfcs_pad = 0;
 
 	if (unlikely(work_done && *work_done >= work_to_do))
 		return -EAGAIN;
@@ -1949,9 +1954,12 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,
 	}
 
 	/* Get actual data size */
+	if (nic->flags & save_rxfcs)
+		rxfcs_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 + rxfcs_pad - sizeof(struct rfd)))
+		actual_size = RFD_BUF_LEN + rxfcs_pad - sizeof(struct rfd);
 
 	/* Get data */
 	pci_unmap_single(nic->pdev, rx->dma_addr,
@@ -1978,7 +1986,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 + rxfcs_pad) {
 		/* Don't indicate oversized frames */
 		nic->rx_over_length_errors++;
 		dev_kfree_skb_any(skb);
@@ -2370,6 +2378,25 @@ static int e100_set_settings(struct net_device *netdev, struct ethtool_cmd *cmd)
 	return err;
 }
 
+static int e100_set_save_rxfcs(struct net_device *netdev, u32 data)
+{
+	struct nic *nic = netdev_priv(netdev);
+	if (data)
+		nic->flags |= save_rxfcs;
+	else
+		nic->flags &= ~save_rxfcs;
+
+	e100_exec_cb(nic, NULL, e100_configure);
+
+	return 0;
+}
+
+static u32 e100_get_save_rxfcs(struct net_device *netdev)
+{
+	struct nic *nic = netdev_priv(netdev);
+	return !!(nic->flags & save_rxfcs);
+}
+
 static void e100_get_drvinfo(struct net_device *netdev,
 	struct ethtool_drvinfo *info)
 {
@@ -2689,6 +2716,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_rxfcs		= e100_set_save_rxfcs,
+	.get_save_rxfcs		= e100_get_save_rxfcs,
 };
 
 static int e100_do_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
-- 
1.7.3.4

Re: [PATCH 2/2] e100: Support receiving Ethernet FCS.

[Jeff Kirsher]

On Fri, Jun 17, 2011 at 12:40,  <greearb@candelatech.com> wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> Helps when sniffing packets.
>
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
> :100644 100644 e336c79... 647d8c6... M  drivers/net/e100.c
>  drivers/net/e100.c |   35 ++++++++++++++++++++++++++++++++---
>  1 files changed, 32 insertions(+), 3 deletions(-)
>

The patch looks fine to me.

Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

-- 
Cheers,
Jeff

Re: [PATCH 0/2] Allow NICs to pass Frame Checksum up the stack.

[Michał Mirosław]

2011/6/17  <greearb@candelatech.com>:
> 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.
[...]

If you want to test the new features approach, you can shave top bit
off NETIF_F_GSO_MASK (there are two unused bits there). The
introducing patch will be +2 lines (+1 dev.c, +1 ethtool.c),
implementation in e100: 20 lines less than patch you sent, ethtool
userspace changes: none (assuming
http://patchwork.ozlabs.org/patch/96374/ or equivalent applied).

Best Regards,
Michał Mirosław

Re: [PATCH 0/2] Allow NICs to pass Frame Checksum up the stack.

[Ben Greear]

On 06/17/2011 01:00 PM, Michał Mirosław wrote:
> 2011/6/17<greearb@candelatech.com>:
>> 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.
> [...]
>
> If you want to test the new features approach, you can shave top bit
> off NETIF_F_GSO_MASK (there are two unused bits there). The
> introducing patch will be +2 lines (+1 dev.c, +1 ethtool.c),
> implementation in e100: 20 lines less than patch you sent, ethtool
> userspace changes: none (assuming
> http://patchwork.ozlabs.org/patch/96374/ or equivalent applied).

Well, is that patch going in?

How do we get more bits if we need more than two?  Totally new
API?

Thanks,
Ben

>
> Best Regards,
> Michał Mirosław


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 0/2] Allow NICs to pass Frame Checksum up the stack.

[Ben Hutchings]

On Fri, 2011-06-17 at 13:48 -0700, Ben Greear wrote:
> On 06/17/2011 01:00 PM, Michał Mirosław wrote:
> > 2011/6/17<greearb@candelatech.com>:
> >> 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.
> > [...]
> >
> > If you want to test the new features approach, you can shave top bit
> > off NETIF_F_GSO_MASK (there are two unused bits there). The
> > introducing patch will be +2 lines (+1 dev.c, +1 ethtool.c),
> > implementation in e100: 20 lines less than patch you sent, ethtool
> > userspace changes: none (assuming
> > http://patchwork.ozlabs.org/patch/96374/ or equivalent applied).
> 
> Well, is that patch going in?
> 
> How do we get more bits if we need more than two?  Totally new
> API?

See this thread:
http://thread.gmane.org/gmane.linux.network/196684

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.

Re: [PATCH 0/2] Allow NICs to pass Frame Checksum up the stack.

[Ben Greear]

On 06/17/2011 01:57 PM, Ben Hutchings wrote:
> On Fri, 2011-06-17 at 13:48 -0700, Ben Greear wrote:
>> On 06/17/2011 01:00 PM, Michał Mirosław wrote:
>>> 2011/6/17<greearb@candelatech.com>:
>>>> 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.
>>> [...]
>>>
>>> If you want to test the new features approach, you can shave top bit
>>> off NETIF_F_GSO_MASK (there are two unused bits there). The
>>> introducing patch will be +2 lines (+1 dev.c, +1 ethtool.c),
>>> implementation in e100: 20 lines less than patch you sent, ethtool
>>> userspace changes: none (assuming
>>> http://patchwork.ozlabs.org/patch/96374/ or equivalent applied).
>>
>> Well, is that patch going in?
>>
>> How do we get more bits if we need more than two?  Totally new
>> API?
>
> See this thread:
> http://thread.gmane.org/gmane.linux.network/196684

It's idle for 11 days.  Seems dead in the water if you ask me.

Thanks,
Ben

>
> Ben.
>


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 0/2] Allow NICs to pass Frame Checksum up the stack.

[David Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Fri, 17 Jun 2011 13:58:55 -0700

> It's idle for 11 days.  Seems dead in the water if you ask me.

Because nobody wants to put in the effort to implement it properly.

It's not an excuse for polluting the kernel with extraneous ethtool
commands just because a fundamental issue is not being worked on.

If this is important to you, you have all the power in the world to
make it move forward.

Your choice.

Re: [PATCH 0/2] Allow NICs to pass Frame Checksum up the stack.

[Ben Greear]

On 06/19/2011 04:21 PM, David Miller wrote:
> From: Ben Greear<greearb@candelatech.com>
> Date: Fri, 17 Jun 2011 13:58:55 -0700
>
>> It's idle for 11 days.  Seems dead in the water if you ask me.
>
> Because nobody wants to put in the effort to implement it properly.
>
> It's not an excuse for polluting the kernel with extraneous ethtool
> commands just because a fundamental issue is not being worked on.
>
> If this is important to you, you have all the power in the world to
> make it move forward.
>
> Your choice.

The last email in the features thread that I saw was a question to you.

Is the original author's work worth trying to salvage, or should
the next person to attempt this ignore all that and start fresh?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 0/2] Allow NICs to pass Frame Checksum up the stack.

[David Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Sun, 19 Jun 2011 16:51:31 -0700

> The last email in the features thread that I saw was a question to
> you.

I'm frustrated with the author because he keeps submitting what
amounts to schemes that have the same problem or are incredibly ugly,
and it's becomming a waste of my time to continue discussing the
matter.

> Is the original author's work worth trying to salvage, or should
> the next person to attempt this ignore all that and start fresh?

The above should make that obvious.

Re: [PATCH 0/2] Allow NICs to pass Frame Checksum up the stack.

[Ben Greear]

On 06/19/2011 04:54 PM, David Miller wrote:
> From: Ben Greear<greearb@candelatech.com>
> Date: Sun, 19 Jun 2011 16:51:31 -0700
>
>> The last email in the features thread that I saw was a question to
>> you.
>
> I'm frustrated with the author because he keeps submitting what
> amounts to schemes that have the same problem or are incredibly ugly,
> and it's becomming a waste of my time to continue discussing the
> matter.
>
>> Is the original author's work worth trying to salvage, or should
>> the next person to attempt this ignore all that and start fresh?
>
> The above should make that obvious.

Well, do you have a suggested plan of attack for this?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 0/2] Allow NICs to pass Frame Checksum up the stack.

[David Miller]

From: greearb@candelatech.com
Date: Fri, 17 Jun 2011 12:40:13 -0700

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

Please don't create entire new ethtool ops just to set what amounts
precisely to a boolean flag, that's what ETHTOOL_{S,G}FLAGS is for.

The fact that we've exhausted 32-bits of flags in netdev->features
is not an acceptable reason to avoid using those interfaces.