diff mbox

[v2,03/10] e1000e: Support RXFCS feature flag.

Message ID 1328730885-10941-4-git-send-email-greearb@candelatech.com
State Awaiting Upstream, archived
Delegated to: David Miller
Headers show

Commit Message

Ben Greear Feb. 8, 2012, 7:54 p.m. UTC 
From: Ben Greear <greearb@candelatech.com>

This enables enabling/disabling reception of the Ethernet
FCS.  This can be useful when sniffing packets.

For e1000e, enabling RXFCS can change the default
behaviour for how the NIC handles CRC.  Disabling RXFCS
will take the NIC back to defaults, which can be configured
as part of the module options.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 45e5ae8... 04a21fc... M	drivers/net/ethernet/intel/e1000e/e1000.h
:100644 100644 293a760... e3eefda... M	drivers/net/ethernet/intel/e1000e/netdev.c
:100644 100644 9c6a56d... ff796e4... M	drivers/net/ethernet/intel/e1000e/param.c
 drivers/net/ethernet/intel/e1000e/e1000.h  |    1 +
 drivers/net/ethernet/intel/e1000e/netdev.c |   41 +++++++++++++++++++++++-----
 drivers/net/ethernet/intel/e1000e/param.c  |    5 +++-
 3 files changed, 39 insertions(+), 8 deletions(-)

Comments

=?ISO-8859-2?Q?Micha=B3_Miros=B3aw?= Feb. 10, 2012, 10:46 p.m. UTC | #1 
2012/2/8  <greearb@candelatech.com>:
> From: Ben Greear <greearb@candelatech.com>
>
> This enables enabling/disabling reception of the Ethernet
> FCS.  This can be useful when sniffing packets.
>
> For e1000e, enabling RXFCS can change the default
> behaviour for how the NIC handles CRC.  Disabling RXFCS
> will take the NIC back to defaults, which can be configured
> as part of the module options.
[...]

This is not how I would expect the features to behave. Default value
should be set on probe() time, and when you disable RXFCS it should
just get disabled.

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Greear Feb. 10, 2012, 10:57 p.m. UTC | #2 
On 02/10/2012 02:46 PM, Michał Mirosław wrote:
> 2012/2/8<greearb@candelatech.com>:
>> From: Ben Greear<greearb@candelatech.com>
>>
>> This enables enabling/disabling reception of the Ethernet
>> FCS.  This can be useful when sniffing packets.
>>
>> For e1000e, enabling RXFCS can change the default
>> behaviour for how the NIC handles CRC.  Disabling RXFCS
>> will take the NIC back to defaults, which can be configured
>> as part of the module options.
> [...]
>
> This is not how I would expect the features to behave. Default value
> should be set on probe() time, and when you disable RXFCS it should
> just get disabled.

The NIC itself may still receive the FCS, but it will be removed
before the pkt is sent up the stack once you disable the 'rx-fcs' flag.

My goal was to make sure that if you enabled and then disabled the
new rx-fcs flag, then you would be back at the original behaviour.

I think that if the "rx-fcs off" logic is to change the default
behaviour, then the Intel folks probably need to make those changes:
It seems that there are some tricky work-arounds regarding fcs and
segmented packets for at least some versions of the e1000e chipsets.

Thanks,
Ben
=?ISO-8859-2?Q?Micha=B3_Miros=B3aw?= Feb. 10, 2012, 11:09 p.m. UTC | #3 
W dniu 10 lutego 2012 23:57 użytkownik Ben Greear
<greearb@candelatech.com> napisał:
> On 02/10/2012 02:46 PM, Michał Mirosław wrote:
>>
>> 2012/2/8<greearb@candelatech.com>:
>>>
>>> From: Ben Greear<greearb@candelatech.com>
>>>
>>> This enables enabling/disabling reception of the Ethernet
>>> FCS.  This can be useful when sniffing packets.
>>>
>>> For e1000e, enabling RXFCS can change the default
>>> behaviour for how the NIC handles CRC.  Disabling RXFCS
>>> will take the NIC back to defaults, which can be configured
>>> as part of the module options.
>>
>> [...]
>>
>> This is not how I would expect the features to behave. Default value
>> should be set on probe() time, and when you disable RXFCS it should
>> just get disabled.
> The NIC itself may still receive the FCS, but it will be removed
> before the pkt is sent up the stack once you disable the 'rx-fcs' flag.
>
> My goal was to make sure that if you enabled and then disabled the
> new rx-fcs flag, then you would be back at the original behaviour.
>
> I think that if the "rx-fcs off" logic is to change the default
> behaviour, then the Intel folks probably need to make those changes:
> It seems that there are some tricky work-arounds regarding fcs and
> segmented packets for at least some versions of the e1000e chipsets.

That makes sense. The flag FLAG2_DFLT_CRC_STRIPPING should be called
FLAG2_FORCE_CRC_STRIPPING_OFF or something if that's the case.
"Default" doesn't tell if that's a strong preference or just because.

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Greear Feb. 11, 2012, 12:06 a.m. UTC | #4 
On 02/10/2012 03:09 PM, Michał Mirosław wrote:
> W dniu 10 lutego 2012 23:57 użytkownik Ben Greear
> <greearb@candelatech.com>  napisał:
>> On 02/10/2012 02:46 PM, Michał Mirosław wrote:
>>>
>>> 2012/2/8<greearb@candelatech.com>:
>>>>
>>>> From: Ben Greear<greearb@candelatech.com>
>>>>
>>>> This enables enabling/disabling reception of the Ethernet
>>>> FCS.  This can be useful when sniffing packets.
>>>>
>>>> For e1000e, enabling RXFCS can change the default
>>>> behaviour for how the NIC handles CRC.  Disabling RXFCS
>>>> will take the NIC back to defaults, which can be configured
>>>> as part of the module options.
>>>
>>> [...]
>>>
>>> This is not how I would expect the features to behave. Default value
>>> should be set on probe() time, and when you disable RXFCS it should
>>> just get disabled.
>> The NIC itself may still receive the FCS, but it will be removed
>> before the pkt is sent up the stack once you disable the 'rx-fcs' flag.
>>
>> My goal was to make sure that if you enabled and then disabled the
>> new rx-fcs flag, then you would be back at the original behaviour.
>>
>> I think that if the "rx-fcs off" logic is to change the default
>> behaviour, then the Intel folks probably need to make those changes:
>> It seems that there are some tricky work-arounds regarding fcs and
>> segmented packets for at least some versions of the e1000e chipsets.
>
> That makes sense. The flag FLAG2_DFLT_CRC_STRIPPING should be called
> FLAG2_FORCE_CRC_STRIPPING_OFF or something if that's the case.
> "Default" doesn't tell if that's a strong preference or just because.

The new flag is just to store the original crc-stripping preference
configured by the module options.  It's not really forcing anything,
it is just the 'default' value.

So, I don't think it should be changed.  However, I will defer to the
driver maintainers...if they think I should change it, then I will,
otherwise, I'm going to leave it as is.

Thanks,
Ben
diff mbox

Patch

diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index 45e5ae8..04a21fc 100644
--- a/drivers/net/ethernet/intel/e1000e/e1000.h
+++ b/drivers/net/ethernet/intel/e1000e/e1000.h
@@ -461,6 +461,7 @@  struct e1000_info {
 #define FLAG2_CHECK_PHY_HANG              (1 << 9)
 #define FLAG2_NO_DISABLE_RX               (1 << 10)
 #define FLAG2_PCIM2PCI_ARBITER_WA         (1 << 11)
+#define FLAG2_DFLT_CRC_STRIPPING          (1 << 12)
 
 #define E1000_RX_DESC_PS(R, i)	    \
 	(&(((union e1000_rx_desc_packet_split *)((R).desc))[i]))
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 293a760..e3eefda 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -936,8 +936,16 @@  static bool e1000_clean_rx_irq(struct e1000_ring *rx_ring, int *work_done,
 		}
 
 		/* adjust length to remove Ethernet CRC */
-		if (!(adapter->flags2 & FLAG2_CRC_STRIPPING))
-			length -= 4;
+		if (!(adapter->flags2 & FLAG2_CRC_STRIPPING)) {
+			/* If configured to store CRC, don't subtract FCS,
+			 * but keep the FCS bytes out of the total_rx_bytes
+			 * counter
+			 */
+			if (netdev->features & NETIF_F_RXFCS)
+				total_rx_bytes -= 4;
+			else
+				length -= 4;
+		}
 
 		total_rx_bytes += length;
 		total_rx_packets++;
@@ -1304,8 +1312,10 @@  static bool e1000_clean_rx_irq_ps(struct e1000_ring *rx_ring, int *work_done,
 							   DMA_FROM_DEVICE);
 
 				/* remove the CRC */
-				if (!(adapter->flags2 & FLAG2_CRC_STRIPPING))
-					l1 -= 4;
+				if (!(adapter->flags2 & FLAG2_CRC_STRIPPING)) {
+					if (!(netdev->features & NETIF_F_RXFCS))
+						l1 -= 4;
+				}
 
 				skb_put(skb, l1);
 				goto copydone;
@@ -1331,8 +1341,10 @@  static bool e1000_clean_rx_irq_ps(struct e1000_ring *rx_ring, int *work_done,
 		/* strip the ethernet crc, problem is we're using pages now so
 		 * this whole operation can get a little cpu intensive
 		 */
-		if (!(adapter->flags2 & FLAG2_CRC_STRIPPING))
-			pskb_trim(skb, skb->len - 4);
+		if (!(adapter->flags2 & FLAG2_CRC_STRIPPING)) {
+			if (!(netdev->features & NETIF_F_RXFCS))
+				pskb_trim(skb, skb->len - 4);
+		}
 
 copydone:
 		total_rx_bytes += skb->len;
@@ -5987,7 +5999,7 @@  static int e1000_set_features(struct net_device *netdev,
 		adapter->flags |= FLAG_TSO_FORCE;
 
 	if (!(changed & (NETIF_F_HW_VLAN_RX | NETIF_F_HW_VLAN_TX |
-			 NETIF_F_RXCSUM | NETIF_F_RXHASH)))
+			 NETIF_F_RXCSUM | NETIF_F_RXHASH | NETIF_F_RXFCS)))
 		return 0;
 
 	/*
@@ -6001,6 +6013,20 @@  static int e1000_set_features(struct net_device *netdev,
 		return -EINVAL;
 	}
 
+	if (changed & NETIF_F_RXFCS) {
+		if (features & NETIF_F_RXFCS) {
+			adapter->flags2 &= ~FLAG2_CRC_STRIPPING;
+		} else {
+			/* We need to take it back to defaults, which might mean
+			 * stripping is still disabled at the adapter level.
+			 */
+			if (adapter->flags2 & FLAG2_DFLT_CRC_STRIPPING)
+				adapter->flags2 |= FLAG2_CRC_STRIPPING;
+			else
+				adapter->flags2 &= ~FLAG2_CRC_STRIPPING;
+		}
+	}
+
 	netdev->features = features;
 
 	if (netif_running(netdev))
@@ -6199,6 +6225,7 @@  static int __devinit e1000_probe(struct pci_dev *pdev,
 
 	/* Set user-changeable features (subset of all device features) */
 	netdev->hw_features = netdev->features;
+	netdev->hw_features |= NETIF_F_RXFCS;
 
 	if (adapter->flags & FLAG_HAS_HW_VLAN_FILTER)
 		netdev->features |= NETIF_F_HW_VLAN_FILTER;
diff --git a/drivers/net/ethernet/intel/e1000e/param.c b/drivers/net/ethernet/intel/e1000e/param.c
index 9c6a56d..ff796e4 100644
--- a/drivers/net/ethernet/intel/e1000e/param.c
+++ b/drivers/net/ethernet/intel/e1000e/param.c
@@ -463,10 +463,13 @@  void __devinit e1000e_check_options(struct e1000_adapter *adapter)
 		if (num_CrcStripping > bd) {
 			unsigned int crc_stripping = CrcStripping[bd];
 			e1000_validate_option(&crc_stripping, &opt, adapter);
-			if (crc_stripping == OPTION_ENABLED)
+			if (crc_stripping == OPTION_ENABLED) {
 				adapter->flags2 |= FLAG2_CRC_STRIPPING;
+				adapter->flags2 |= FLAG2_DFLT_CRC_STRIPPING;
+			}
 		} else {
 			adapter->flags2 |= FLAG2_CRC_STRIPPING;
+			adapter->flags2 |= FLAG2_DFLT_CRC_STRIPPING;
 		}
 	}
 	{ /* Kumeran Lock Loss Workaround */